Skip to content
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

Open
wants to merge 1 commit into
base: feature/allow-spends
Choose a base branch
from

Conversation

kpudlik
Copy link
Contributor

@kpudlik kpudlik commented Oct 25, 2024

Based on the "fixed-window" solution the following algorithm is implemented:

  1. Confirm pending sent binaries (just as it was working before).
  2. Take the highest confirmed binary index and subtract the fixed window size from it to get the highest Currency Snapshot (binary) that is still within the window. Remove all confirmed binaries that are older, as these are out of the fixed window bounds.
  3. Take the global snapshot in which the fixed window's Currency Snapshot (binary) was confirmed and provide it to the Data Application. Remove all stored global snapshots that are older, as these will no longer be returned.

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).

IPadawans
IPadawans previously approved these changes Oct 25, 2024
Copy link
Member

@IPadawans IPadawans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

marcinwadon
marcinwadon previously approved these changes Oct 25, 2024
Copy link
Member

@marcinwadon marcinwadon left a 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

@marcinwadon
Copy link
Member

@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.

@kpudlik
Copy link
Contributor Author

kpudlik commented Oct 25, 2024

@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:

Fixed Window (C+3)- 5 = (C-2) -> Find(GL0 = G-2)

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.

@kpudlik kpudlik dismissed stale reviews from marcinwadon and IPadawans October 25, 2024 20:13

The merge-base changed after approval.

@kpudlik kpudlik force-pushed the PROT-864-fixed-window-pruning branch from 442fb80 to 19c4c0f Compare October 25, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants