Fix the deadlock during statements gossiping #9868
Merged
+21
−13
Conversation
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
gui1117
reviewed
Sep 29, 2025
Comment on lines
765
to
772
| let keys: Vec<_> = { | ||
| let index = self.index.read(); | ||
| index.entries.keys().cloned().collect() | ||
| }; | ||
|
|
||
| let mut result = Vec::with_capacity(keys.len()); | ||
| for h in keys { | ||
| let encoded = self.db.get(col::STATEMENTS, &h).map_err(|e| Error::Db(e.to_string()))?; |
There was a problem hiding this comment.
Note that by the time we read into the db the statement might have been removed.
So this operation doesn't return the view of the statement store at one point in time. Instead it returns most statements. It could be that a statement was removed and another added in the meantime.
But it can be good and more efficient than before as well.
But for instance if a user always. Override one statement in one channel. While the store always have one statement for this user, this function might return none sometimes. But again it can be good.
There was a problem hiding this comment.
But it can be good and more efficient than before as well.
It could be good, but benchmarking revealed a regression. I suppose without the lock, concurrent access to the DB slows things down
AndreiEres
force-pushed
the
AndreiEres/fix-statement-store-deadlock
branch
2 times, most recently
from
c2e4e5d to
faa4bf0
Compare
AndreiEres
commented
Sep 30, 2025
| Block::Hash: From<BlockHash>, | ||
| Client: ProvideRuntimeApi<Block> | ||
| + HeaderBackend<Block> | ||
| + sc_client_api::ExecutorProvider<Block> |
There was a problem hiding this comment.
Not related to the subject, but should be removed as we don't need this trait. It only complicates the test setup.
AndreiEres
commented
Sep 30, 2025
| } | ||
|
|
||
| /// Perform periodic store maintenance | ||
| pub fn maintain(&self) { |
There was a problem hiding this comment.
Not related to the current deadlock, but better to remove unnecessary reads. It makes the log more precise as the change of index between maintenance and logging is possible. Keeping the write lock during the DB commit is not necessary.
|
/cmd prdoc --audience node_dev --bump patch |
georgepisaltu
approved these changes
Sep 30, 2025
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
AndreiEres
force-pushed
the
AndreiEres/fix-statement-store-deadlock
branch
from
26918d9 to
4bfda69
Compare
gui1117
approved these changes
Sep 30, 2025
gui1117
added
A4-backport-stable2506
paritytech-release-backport-bot bot
pushed a commit
that referenced
this pull request
Sep 30, 2025
# Description During statement store benchmarking we experienced deadlock-like behavior which we found happened during statement propagation. Every second statements were propagating, locking the index which possibly caused the deadlock. After the fix, the observed behavior no longer occurs. Even though there is a possibility to unsync the DB and the index for read operations and release locks earlier, which should be harmless, it leads to regressions. I suspect because of concurrent access to many calls of db.get(). Checked with the benchmarks in #9884 ## Integration This PR should not affect downstream projects. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit ed4eebb)
|
Successfully created backport PR for |
paritytech-release-backport-bot bot
pushed a commit
that referenced
this pull request
Sep 30, 2025
# Description During statement store benchmarking we experienced deadlock-like behavior which we found happened during statement propagation. Every second statements were propagating, locking the index which possibly caused the deadlock. After the fix, the observed behavior no longer occurs. Even though there is a possibility to unsync the DB and the index for read operations and release locks earlier, which should be harmless, it leads to regressions. I suspect because of concurrent access to many calls of db.get(). Checked with the benchmarks in #9884 ## Integration This PR should not affect downstream projects. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit ed4eebb)
|
Successfully created backport PR for |
|
Successfully created backport PR for |
paritytech-release-backport-bot bot
pushed a commit
that referenced
this pull request
Sep 30, 2025
# Description During statement store benchmarking we experienced deadlock-like behavior which we found happened during statement propagation. Every second statements were propagating, locking the index which possibly caused the deadlock. After the fix, the observed behavior no longer occurs. Even though there is a possibility to unsync the DB and the index for read operations and release locks earlier, which should be harmless, it leads to regressions. I suspect because of concurrent access to many calls of db.get(). Checked with the benchmarks in #9884 ## Integration This PR should not affect downstream projects. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit ed4eebb)
EgorPopelyaev
pushed a commit
that referenced
this pull request
Oct 1, 2025
Backport #9868 into `stable2509` from AndreiEres. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: Andrei Eres <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
EgorPopelyaev
pushed a commit
that referenced
this pull request
Oct 3, 2025
Backport #9868 into `stable2506` from AndreiEres. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: Andrei Eres <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
bee344
pushed a commit
that referenced
this pull request
Oct 7, 2025
# Description During statement store benchmarking we experienced deadlock-like behavior which we found happened during statement propagation. Every second statements were propagating, locking the index which possibly caused the deadlock. After the fix, the observed behavior no longer occurs. Even though there is a possibility to unsync the DB and the index for read operations and release locks earlier, which should be harmless, it leads to regressions. I suspect because of concurrent access to many calls of db.get(). Checked with the benchmarks in #9884 ## Integration This PR should not affect downstream projects. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
alvicsam
pushed a commit
that referenced
this pull request
Oct 17, 2025
# Description During statement store benchmarking we experienced deadlock-like behavior which we found happened during statement propagation. Every second statements were propagating, locking the index which possibly caused the deadlock. After the fix, the observed behavior no longer occurs. Even though there is a possibility to unsync the DB and the index for read operations and release locks earlier, which should be harmless, it leads to regressions. I suspect because of concurrent access to many calls of db.get(). Checked with the benchmarks in #9884 ## Integration This PR should not affect downstream projects. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
During statement store benchmarking we experienced deadlock-like behavior which we found happened during statement propagation. Every second statements were propagating, locking the index which possibly caused the deadlock. After the fix, the observed behavior no longer occurs.
Even though there is a possibility to unsync the DB and the index for read operations and release locks earlier, which should be harmless, it leads to regressions. I suspect because of concurrent access to many calls of db.get(). Checked with the benchmarks in #9884
Integration
This PR should not affect downstream projects.