refactor: remove steps from StateSync interface

[tomyrd]

This version is still a WIP.

The idea is to remove the sync steps from the interface of the StateSync so that the user doesn't have to deal with them and the sync is shown as a single step process.

After some bug fixing, the rust tests are passing so this version works! It's a bit messy and the documentation is now outdated. Will be working on this next but the main changes can be discussed:

Main changes

The main change with this new functionality is that we relied on store to persist the updates on each step but if we remove the steps, we need to store the intermediate states of notes on each step and update them accordingly without storing them. The changes made in state_sync.rs can be summarized as:

• NoteUpdates: Instead of being 4 vectors of notes, now it's 2 maps (one for input and another for output notes). This means that it is better for retrieving the state of a specific note. Additionally, a new insert_or_ignore is used to add the state in the store (before starting the sync process) to the note update, if the note is not tracked in the update it is inserted but if it's tracked (meaning that it was modified in a previous step) then it is ignored.
• Callbacks: Each callback now receives a NoteUpdates with the state of changed notes since the start of the sync process. The callbacks use the insert_or_ignore to add notes if needed and then apply the state changes to the NoteUpdates to update it with the new event. The updated NoteUpdates is returned.
  • Why not just give a mutable reference for the NoteUpdates? This was my first approach but there are problems with the lifetimes of the closures and the received references. In theory it wouldn't be a problem as both lifetimes are the same but the rust compiler can be sure of this and will doesn't like the references in the closure.
• BlockUpdates: Now a SyncStateUpdate represents an update after multiple steps, so it may contain multiple new block headers. All of the data related to each new block is stored in a new BlockUpdates struct.
• New OnBlockReceived callback: This callback was added to deal with the arrival of each new block. It is used to have a store access to check the relevance of each block received. The component in general still doesn't have access to the store so this callback is needed.

[bobbinth]

Not a review (I haven't really looked at the code) - but to make a few comments:

Callbacks: Each callback now receives a NoteUpdates with the state of changed notes since the start of the sync process. The callbacks use the insert_or_ignore to add notes if needed and then apply the state changes to the NoteUpdates to update it with the new event. The updated NoteUpdates is returned.

Why is this necessary? Could the StateSync struct not manage all intermediate updates internally? For example, say we get a note A from the node, we pass it to the OnNoteReceived handler and it gives us back NoteUpdates. From that point on, if we receive any additional info about this note (e.g., we get a nullifier for it), we should make the update internally within the StateSync struct, no?

New OnBlockReceived callback: This callback was added to deal with the arrival of each new block. It is used to have a store access to check the relevance of each block received. The component in general still doesn't have access to the store so this callback is needed.

What exactly do we need to check in the store? Can this info be pre-loaded into the StateSync struct?

[tomyrd]

From that point on, if we receive any additional info about this note (e.g., we get a nullifier for it), we should make the update internally within the StateSync struct, no?

This can be done. It would move the responsibility of dealing with intermediate updates from the callback to the component itself. The only issue I see with this is that the callback wouldn't be called for every event, just those for which the component doesn't already have the internal version and need to be queried from the store. If we wanted users to be able to react to certain events with the callbacks, this would limit that ability, as it's not called for every instance.

Also, this would create some code duplication but I don't think it would be very much as some of it can be moved to helper functions.

What exactly do we need to check in the store? Can this info be pre-loaded into the StateSync struct?

The NoteChecker uses the store to check for consumability of known notes. After checking, the store is internally used to get every tracked account ID, which is something we already have inside SyncState. I'll change the note checker so that it can accept the list instead of getting it from the store.

[tomyrd]

Regarding this part:

Why not just give a mutable reference for the NoteUpdates? This was my first approach but there are problems with the lifetimes of the closures and the received references. In theory it wouldn't be a problem as both lifetimes are the same but the rust compiler can be sure of this and will doesn't like the references in the closure.

@igamigo mentioned we could use an Arc<RefCell> for this which would also simplify the interface as we can just mutate the original NoteUpdates instead of transfering ownership. This may cause problems for the Sync+Send client which we also want, but I'm not entirely sure (the SyncState component is instantiated on every sync and then destroyed, it's not part of the Client)

[tomyrd]

An update on some of the ideas mentioned earlier today:

we could use an Arc<RefCell> for this which would also simplify

I wasn't able to use RefCell because it isn't send+sync so I ended up using a RwLock instead. The chages for this are isolated in commit 174c995 so it can be reverted if we don't like it.

I'll change the note checker so that it can accept the list instead of getting it from the store.

The note checker also uses the store to check that the account contains the necessary assets to consume a swap note, we don't have access to this information inside the SyncState as we only have the headers. We would need to pass every full Account object if we want to pre-load it, maybe it's worthwhile compromise.

[bobbinth]

From that point on, if we receive any additional info about this note (e.g., we get a nullifier for it), we should make the update internally within the StateSync struct, no?

This can be done. It would move the responsibility of dealing with intermediate updates from the callback to the component itself. The only issue I see with this is that the callback wouldn't be called for every event, just those for which the component doesn't already have the internal version and need to be queried from the store. If we wanted users to be able to react to certain events with the callbacks, this would limit that ability, as it's not called for every instance.

I think we should try to go this route. There are only two things that can happen during state sync: we get a committed note or a note gets nullifies. I think state transitions for these are pretty straight forward (unless I'm missing something, of course).

The NoteChecker uses the store to check for consumability of known notes. After checking, the store is internally used to get every tracked account ID, which is something we already have inside SyncState. I'll change the note checker so that it can accept the list instead of getting it from the store.

Shouldn't the note checker be run inside the note handling callback?

[tomyrd]

Shouldn't the note checker be run inside the note handling callback?

I think it would be possible, I can move the block relevance check to the OnNoteCommitted and the committed transactions check to OnNullifierReceived so we end up with just those 2 callbacks

[tomyrd]

Update: in c1fe5fa I ended up merging the callbacks as mentioned above. The callback interfaces changed a bit because they now need some extra information about the sync response to decide the correct state changes. I think it looks cleaner now which is good!

Tomorrow, I'll look into changing the way we pass the entire NoteUpdates to each callback. Maybe there's a better way.