Fix/5193 stackerdb decoherence #5197
Merged
Conversation
This file contains 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
…te machine are using
… on irrecoverable error
…as to verify that connection pinning prevents decoherence
jferrant
previously approved these changes
Sep 17, 2024
kantai
reviewed
Sep 17, 2024
obycode
previously approved these changes
Sep 17, 2024
…ignore flag set Signed-off-by: Jacinta Ferrant <[email protected]>
…ting for unnecessary signatures Signed-off-by: Jacinta Ferrant <[email protected]>
This allows us to avoid hitting block 240, which is when the stackers get unstacked and the chain stalls, making `partial_tenure_fork` less flaky
Merged
|
I am testing this on mainnet along with my other in-flight PRs, and I think I'm getting OOM'ed. I need to confirm first. |
will also run this branch to see if i can reproduce |
Co-authored-by: Brice Dobry <[email protected]>
obycode
approved these changes
Sep 23, 2024
kantai
approved these changes
Sep 23, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 23, 2024
…erence Fix/5193 stackerdb decoherence
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This fixes #5193 by having all p2p state machines (namely, both epoch 2.x and Nakamoto inv sync and StackerDB) track and report their pinned connections to the peer network, so they won't be pruned. The cause of the decoherency seems to have been that once a peer's outbound neighbor count exceeded
[connection_opts].soft_max_neighbors_per_orgor one of the other similar limits, the pruner would simply close the newer connections until the number of connections was brought down. This would often happen during StackerDB sync (and would also happen in inv sync), which would have the effect of a node with many neighbors failing to synchronize their StackerDB replicas.This I suspect was also the cause of the decoherence we would see with larger Nakamoto testnets, where the soft limits on the number of neighbors were exceeded.
You can see the effect of this PR in
/v2/neighbors-- inbound and outbound peer entries now report anage(in seconds), which should rarely be reset due to the pinning. Before, neighbors would come and go very quickly as state machines connected to them and the pruner immediately disconnected them.Leaving as a draft for now so I can test this live with the Nakamoto testnet signers.