Redesign RecordStore API / interaction

[shamil-gadelshin]

Description

Currently, RecordStore trait returns Result only for put and add_provider operations. However, custom implementations (for example DB-based) could return errors on each operation.

I wonder whether it's possible to change this trait and add Result on each operation to make the trait more consistent with use cases.

Motivation

This change will allow handling errors for implementations in an idiomatic way.

Current Implementation

    /// Gets a record from the store, given its key.
    fn get(&'a self, k: &Key) -> Option<Cow<'_, Record>>;

    /// Puts a record into the store.
    fn put(&'a mut self, r: Record) -> Result<()>;

    /// Removes the record with the given key from the store.
    fn remove(&'a mut self, k: &Key);

    /// Gets an iterator over all (value-) records currently stored.
    fn records(&'a self) -> Self::RecordsIter;

.....

Are you planning to do it yourself in a pull request?

Not this time. This change could affect inner Kademlia error handling.

[shamil-gadelshin]

@mxinden This issue I mentioned on the community call.

There was also a question about splitting RecordStore trait into separate traits for records and providers.

[rklaehn]

Yes, I noticed the same thing when trying to implement a persistent record store for iroh. I just had to unwrap the errors to be able to conform to the interface, which is not nice.

#3021 is also somewhat related. There is not much point in having a persistent record store for a large number of records if the behaviour pulls it all into one giant Vec<ProviderRecord>.

[mxinden]

I would suggest we move forward here and write a first proof-of-concept. Anyone volunteering?

[shamil-gadelshin]

It's easy to change an interface and alter all usages for it. I could do that. However, adding correct handling for an operation result in some cases could be too invasive in the current architecture.

For example - the call of the store.remove in the PutRecordJob.poll() doesn't assume returning (or some other handling) of an error. I could probably add log::debug or log::error in such cases but I'm not sure you will accept that because it won't propagate the error to the upper layers. Will logging errors be enough when it's not possible to return an upper-level result as in kad::Behaviour::remove_record()?

[mxinden]

I don't think we should log errors, but instead bubble them up to the user. Why is that not an option?

[shamil-gadelshin]

Ok. I will create a PR with one operation and show you.

[shamil-gadelshin]

Please, have a look when you have time: #3076 @mxinden

[mxinden]

An alternative approach to the one explored in #3076 is to move the RecordStore outside of libp2p-kad. Instead of the Kademlia NetworkBehaviour implementation having direct (synchronous) access to the RecordStore implementation, the Kademlia NetworkBehaviour implementation emits events like RecordRequested which the user responds to by retrieving the record from a DB (potentially async) and providing the record to the Kademlia NetworkBehaviour implementation via a method on Kademlia.

[nazar-pc]

That would be a very libp2p way of doing it and consistent with the rest of APIs.

[jq-rs]

I have an use case similar to #2024 where a key is updated by several nodes asynchronously and the value-content should converge eventually. Happy to test any implementation that would allow this.