Conversation
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
I went through the first couple of commits, with a fine-toothed comb seeing that this is your first contribution. Don't worry we won't be as pedantic going forward :-)
I'll finish the review first thing tomorrow, but thought you might need the feedback early, so here's an incomplete review :-)
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
Very nice PR, I like it a lot. I think we might have had a small misunderstanding with the tombstone, in that I had in mind using the last valid version number to block any future updates on it, and sending that back and forth intrinsically making updates fail.
Also I'm not sure the conflicts can even happen, and what I think you were marking as conflict, is actually a normal thing to happen, i.e., a No-op following an old cached value being returned.
| #[derive(Debug, Default)] | ||
| pub struct SafeMergeResult { | ||
| pub changes: Vec<(String, Option<u64>, u64)>, | ||
| pub conflict_count: usize, |
There was a problem hiding this comment.
No, you are counting overwrites with older versions as conflict, when really its not. The reason is that we may have multiple requests in flight, request A changes field A, and request B changes field B, if the signer B handles request B it'll return the A with the same version as before, which you would consider a conflict, but it is just stale, and merging will just pick the newer value. The staging takes care of ensuring that state is updated correctly, please copy those semantics where possible.
libs/gl-plugin/src/node/mod.rs
Outdated
| trace!("hsmd hsm_id={} request processor started", hsm_id); | ||
| let mut last_sent_sketch = StateSketch::new(); | ||
| let mut last_seen_sync_epoch = state_sync_epoch.load(Ordering::SeqCst); | ||
| enum SyncMode { |
There was a problem hiding this comment.
Do we really need a full sync mode at all? As mentioned before the conflicts you identified are in fact no-ops, and therefore we don't end up falling back to full sync. The Diff mode will still do a full sync when the connection is first established simply because its sketch for what the signer already has is empty, so all kvv tuples are new from its PoV.
ihordiachenko
changed the title
cdecker
left a comment
cdecker
left a comment
There was a problem hiding this comment.
Excellent PR, thank you @ihordiachenko 🙏
ShahanaFarooqui
force-pushed
the
main
branch
2 times, most recently
from
7185343 to
2fafe20
Compare
2 participants
Implements #745
Key design considerations