chore: avoid stored renotify permits in verify queue#5249
Open
chenyukang wants to merge 1 commit into
Open
Conversation
353a9b3 to
fa4bcce
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a tx-pool worker wakeup issue by changing VerifyQueue::re_notify() to avoid accumulating tokio::sync::Notify permits that can cause self-wakeup loops, and adds a regression test for that behavior. It also includes an unrelated light-client genesis-proof guard and CI workflow “chunk result” validation.
Changes:
- Switch
VerifyQueue::re_notify()fromnotify_one()tonotify_waiters()to wake only currently-waiting workers. - Add an async test to ensure
re_notify()does not store a permit when no tasks are waiting. - Update light-client proof generation to handle
last_block.is_genesis()and update CI integration-test workflows to fail if chunked tests don’t succeed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| util/light-client-protocol-server/src/lib.rs | Avoids underflow / invalid parent MMR access when replying proofs for genesis blocks. |
| tx-pool/src/component/verify_queue.rs | Changes re_notify() to use notify_waiters() to avoid stored permits/self-wakeup. |
| tx-pool/src/component/tests/chunk.rs | Adds a regression test covering the “no stored permit” behavior for re_notify(). |
| .github/workflows/ci_integration_tests_windows.yaml | Ensures the summary job fails when the chunked test job doesn’t succeed. |
| .github/workflows/ci_integration_tests_ubuntu.yaml | Ensures the summary job fails when the chunked test job doesn’t succeed. |
| .github/workflows/ci_integration_tests_macos.yaml | Ensures the summary job fails when the chunked test job doesn’t succeed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+188
to
+193
| queue.re_notify(); | ||
| assert!( | ||
| tokio::time::timeout(std::time::Duration::from_millis(10), queue_rx.notified()) | ||
| .await | ||
| .is_err() | ||
| ); |
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
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.
Problem Summary:
Fix
VerifyQueue::re_notify()to wake only currently waiting workers without storing a notification permit.Previously,
re_notify()usednotify_one(). When theOnlySmallCycleTxworker found that the queue still contained only large-cycle transactions,notify_one()could store a permit if no other worker was waiting. The same worker could then consume that permit and repeatedly wake itself.This PR switches
re_notify()tonotify_waiters(), which preserves the intended handoff behavior while avoiding stored self-wakeup permits.Related changes
owner/repo:Check List
Tests
Side effects