Create BackupWriter and BackupReader traits for async unit backup

[joschisan]

This pr is reproposing the original design in https://github.com/Cardinal-Cryptography/AlephBFT/pull/399 before it was switched to using AsyncRead and AsyncWrite. The initial design proposed here is currently implemented in our fork and allows a very clean integration with our async database abstraction over RocksDB. Please find the integration into Fedimint here https://github.com/fedimint/fedimint/blob/master/fedimint-server/src/atomic_broadcast/backup.rs

The current API using AsyncWrite is quite low level and therefore quite unpractical for us, however I did not notice at the time that the api design in the original pr was changed otherwise I would have brought it up at the time. My apologies for the unnecessary back and forth this caused you; I should have caught this at the time.

I am reopening the issue now as we intend to switch from our fork back to upstream.

Please note that reading the backup was never really the issue and I am proposing it with two traits here for symmetry which makes it a little cleaner imo. We could therefore also stick to AsyncRead instead of defining a new trait or just take a byte vector / boxed slice directly since we are reading it all into a vector at once anyway.

I am aware that there were concerns around introducing new traits for this, however, after implementing both sides integrating via these traits it seems to me that this interface is the cleanest on both sides so it seems worth it.

[github-actions[bot]]

Please make sure the following happened

• [ ] Appropriate tests created
• [ ] Infrastructure updated accordingly
• [ ] Updated existing documentation
• [ ] New documentation created
• [ ] Version bumped if breaking changes

[timorleph]

We would strongly prefer not to change the API again, and our reasons for preferring the already existing traits mentioned in #399 still hold. You should be able to implement the Async* traits for your Backup* structs with no major problems, although maybe parts of them will look a tad awkward. The awkwardness is somewhat intentional, since the backup is supposed to be more akin to a transaction log from a DB, which should be lower level than a DB.

[dpc]

You should be able to implement the Async* traits for your Backup* structs with no major problems, although maybe parts of them will look a tad awkward

"tad awkward" is an understatement.

It's such a complication for no good reason. AsyncWrite/Read are just a ill suited (too low level, overly generic, forcing completely pointless complexity on the implementation) abstraction used here for the wrong reasons.

The items here are not going to be 16MBs (will they?), to justify needing streaming API that AsyncWrite/Read. The alepbhft is already (and most probably forever) only ever going to call write_all + flush, because most probably fundamentally it needs to read the whole thing in memory to validate it, doesn't have a need to deal with streaming of very large sets of data, so it will always have whole thing contiguous in memory and will just write everything all at once. So what could have been a single async fn save_item(&[u8]) -> io::Result<()> turns into implementing way more complex https://docs.rs/futures-io/latest/futures_io/trait.AsyncWrite.html unloved nightmare because theoretically alephbft code could change in the future and start calling multiple writes (it won't, but the interface here is ill suited and more generic). The whole thing will probably break in the future, because the whole async io traits are not really stable and figured out yet (that's why they are in 3rd party crate, no `std), AFAIR.

To sum up "tad awkward":

• extra dependency
• extra gnarly (and possibly buggy) code
• extra performance hit
• extra breaking change when the IO-traits dust settles

Anyway @joschisan it is definitely possible to implement impl AsyncWrite for SomeOurStructDoingTheJob that does a whole gathering (and copying, wasting the performance) in poll_write dance, and write to db on flush&close. We either need to find some helper, or write that raw-polling-async code ourselves. Not much point debating here. @maan2003 You know your async ecosystem. Do you know of a good existing wrapper like that?

[maan2003]

this poll dance for async write doesn't look trivial. I don't think it is possible to convert async fn save(&self, &mut [u8]) into async write, because caller can just poll_write with a different buffer next time.

I don't know of any such wrappers.

[dpc]

this poll dance for async write doesn't look trivial. I don't think it is possible to convert async fn save(&self, &mut [u8]) into async write, because caller can just poll_write with a different buffer next time.

The implementation needs to copy bytes into its own buffer that gets cleared on flush/close, no way around it. If that's what you mean.

[maan2003]

oh right, you can just make all async stuff happen on flush

[kostekIV]

oh right, you can just make all async stuff happen on flush

or on write making flush a noop – this is relatively common.

Anyway, we are closing this.