-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: getLastSynchronizedGlobalSnapshot support for L0NodeContext #962
base: feature/allow-spends
Are you sure you want to change the base?
Conversation
e81eff0
to
442fb80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but I'm not able to verify the logic if it's correct. @ryle-goehausen please review the PR before merging
@ryle-goehausen @AlexBrandes this implementation looks like non deterministic, because it depends on the local view of the latest global snapshot. it will be different for each node, so each node would choose a different cutoff ordinal. |
Yep. If N is substracted from the latest CIS then it will get desynchronized if latest CIS is also desynchronized. Of course there are cases when multiple CIS are in single GIS but very likely fixed-window will cause forks anyway. That's why I think we need to switch to consensus-based solution. According to diagram:
But some nodes are C+3 but some may be already C+4 which may result in C-3 that is in GLO = G-1 instead of G-2. |
The merge-base changed after approval.
442fb80
to
19c4c0f
Compare
Based on the "fixed-window" solution the following algorithm is implemented:
It needs to be tested on the cluster and may have some edge cases, so I would treat it as a proof of concept (PoC).