-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Statement-store: defer statement protocol connections during major sync #11487
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
Open
DenzelPenzel
wants to merge
38
commits into
master
Choose a base branch
from
denzelpenzel/statement-store-loss-during-major-sync
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 29 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
7d0c13d
statement-store: defer statement protocol connections during major sync
DenzelPenzel c8a9ed7
statement-store: major sync tests
DenzelPenzel a382525
Update from github-actions[bot] running command 'fmt'
github-actions[bot] 7cedec4
statement-store: clippy
DenzelPenzel 5a8e3f1
statement-store: fix spawn_network to support single-collator tests,
DenzelPenzel b87cd17
Update from github-actions[bot] running command 'prdoc --audience nod…
github-actions[bot] 8db0b1a
statement-store: use BTreeMap for deferred_peers
DenzelPenzel 2120c5d
statement-store: clippy
DenzelPenzel 038e8e5
Merge branch 'master' into denzelpenzel/statement-store-loss-during-m…
DenzelPenzel 0576365
statement-store: fix deferred peer race and extract sync transition c…
DenzelPenzel b0a6db0
Merge remote-tracking branch 'origin/master' into denzelpenzel/statem…
DenzelPenzel 2934407
statement-store: address review feedback
DenzelPenzel acf6280
Update from github-actions[bot] running command 'fmt'
github-actions[bot] 6e62495
statement-store: reconnect peers after major sync for statement recovery
DenzelPenzel 20a4386
statement-store: update prdoc to reflect simplified approach
DenzelPenzel 037cc4a
Merge remote-tracking branch 'origin/master' into denzelpenzel/statem…
DenzelPenzel 6ffe8e7
Merge remote-tracking branch 'origin/master' into denzelpenzel/statem…
DenzelPenzel 39dc866
statement-store: fix conflicts
DenzelPenzel 4903081
Update from github-actions[bot] running command 'fmt'
github-actions[bot] 85aecdc
statement-store: added the new ci matrix test
DenzelPenzel 07e6433
Merge remote-tracking branch 'origin/denzelpenzel/statement-store-los…
DenzelPenzel fe16cee
statement-store: improve reconnect_statement_peers robustness and tes…
DenzelPenzel 79890d7
statement-store: recover statements lost during major sync
DenzelPenzel e2118dd
statement-store: check statements
DenzelPenzel a8d7939
Update from github-actions[bot] running command 'fmt'
github-actions[bot] b69e515
statement-store: incr timeout
DenzelPenzel b076543
Merge branch 'denzelpenzel/statement-store-loss-during-major-sync' of…
DenzelPenzel fc46117
Update from github-actions[bot] running command 'fmt'
github-actions[bot] 2af5257
statement-store: improve naming
DenzelPenzel c96de87
statement-store: buffer peers during major sync instead of remove+add…
DenzelPenzel 922309e
Merge remote-tracking branch 'origin/master' into denzelpenzel/statem…
DenzelPenzel 637f1b0
statement-store: unit test with_syncing
DenzelPenzel 1803dab
statement-store: update prdoc
DenzelPenzel e09bd4c
Update from github-actions[bot] running command 'fmt'
github-actions[bot] 0a86fbf
statement-store: recover statements dropped during major sync
DenzelPenzel 76f2487
Merge remote-tracking branch 'origin/denzelpenzel/statement-store-los…
DenzelPenzel 6e86175
statement-store: improve recovery integration test coverage
DenzelPenzel 6a90c6b
Update from github-actions[bot] running command 'fmt'
github-actions[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| title: 'statement-store: reconnect peers after major sync for statement recovery' | ||
|
DenzelPenzel marked this conversation as resolved.
Outdated
|
||
| doc: | ||
| - audience: Node Dev | ||
| description: |- | ||
| # Description | ||
|
|
||
| Nodes drop all statements received from peers during major sync because of the `is_major_syncing()` guard. | ||
| After major sync completes, those statements are permanently lost because peers will not re-send them. | ||
|
|
||
| This fix detects when major sync ends and reconnects all statement protocol peers by closing and | ||
| reopening their notification substreams. This triggers bidirectional initial sync on both sides, | ||
| recovering any statements that were missed during the sync period. | ||
|
|
||
| The approach is minimal: only reacts to the sync-end transition with a single `was_major_syncing` | ||
| flag. No peer tracking or debounce needed since the reconnect only closes notification substreams | ||
| (not TCP connections) and is inexpensive. | ||
| crates: | ||
| - name: sc-network-statement | ||
| bump: patch | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do you have anything to proove 3 happens ?
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.
Yes, this cond confirm it
polkadot-sdk/cumulus/zombienet/zombienet-sdk/tests/zombie_ci/statement_store/integration.rs
Line 495 in fc46117
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.
But ... this confirms just that you had a major sync happening not that, you received statements while you were in major sync.