test: fix intermittent error in wait_for_quorum_commitment #6785
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughA logging statement was added to the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/llmq/dkgsessionmgr.cpp (1)
334-336: Missing error check for secret-key contribution fetch
db->Read(...)returnsfalsewhen the secret-key contribution is absent, but the code ignores the return value and silently inserts a default-constructedCBLSSecretKeyinto the cache. Down-stream consumers will treat this as a valid (yet zero) key and may fail later with obscure errors.CBLSSecretKey skContribution; - db->Read(std::make_tuple(DB_SKCONTRIB, llmqType, pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), skContribution); + if (!db->Read(std::make_tuple(DB_SKCONTRIB, llmqType, + pQuorumBaseBlockIndex->GetBlockHash(), proTxHash), + skContribution)) { + LogPrint(BCLog::LLMQ, + "%s -- missing sk contribution for llmq=%d block=%s protx=%s\n", + __func__, ToUnderlying(llmqType), + pQuorumBaseBlockIndex->GetBlockHash().ToString(), + proTxHash.ToString()); + return false; + }
🧹 Nitpick comments (1)
src/llmq/dkgsessionmgr.cpp (1)
319-324: Prefer category-scoped logging to reduce noise
LogPrintfwrites unconditionally to the main log. In a busy network, every missing contribution would now hit the log, potentially spamming nodes during catch-up and replay. Use the existingLLMQdebug category instead so operators can opt-in, and keep the log level consistent with nearby messages.- LogPrintf("%s -- this node does not have verified contribution for llmq=%d block=%s protx=%s\n", - __func__, ToUnderlying(llmqType), pQuorumBaseBlockIndex->GetBlockHash().ToString(), - proTxHash.ToString()); + LogPrint(BCLog::LLMQ, + "%s -- this node does not have verified contribution for llmq=%d block=%s protx=%s\n", + __func__, ToUnderlying(llmqType), + pQuorumBaseBlockIndex->GetBlockHash().ToString(), + proTxHash.ToString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/llmq/dkgsessionmgr.cpp(1 hunks)test/functional/test_framework/test_framework.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/llmq/dkgsessionmgr.cpp
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_framework/test_framework.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
src/llmq/dkgsessionmgr.cpp (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
test/functional/test_framework/test_framework.py (1)
Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (2)
test/functional/test_framework/test_framework.py (2)
2075-2076: LGTM! Correctly filters out invalid quorum commitments.This change appropriately addresses the intermittent error described in issue #6748. By skipping quorum commitments with a zero public key (96 characters of zeros), the function now properly excludes invalid/empty commitments that were previously causing false positives when the node had no actual quorum information.
2083-2083: LGTM! Improved responsiveness by using default sleep interval.Removing the explicit
sleep=1parameter allows the function to use the default 0.05-second sleep interval, which should make quorum commitment detection more responsive and help reduce the likelihood of timeout errors.
…m commitment It fix intermittent error in feature_asset_locks.py and other functional tests that uses quorums
knst
force-pushed
the
tests-fix-waiting-qc
branch
from
5a768a5 to
ab9fc70
Compare
kwvg
changed the title
Co-authored-by: UdjinM6 <[email protected]>
4 participants
Issue being fixed or feature implemented
Fixes #6748
RPC
dkgstatusreturn mineable commitment even if node does not have any information about quorum commitment.See
CQuorumBlockProcessor::GetMineableCommitmentsfor details of implementation.It happens if
wait_for_quorum_commitmentis called before node received QC by p2p.This failure can happen not only for feature_asset_locks.py but for each functional tests that uses quorums, but it happened most often for feature_asset_locks.py because there is only 2 nodes instead 3 and it happens more often.
Issue indeed has been observed by me in other functional tests, but that had happens rarely compare to feature_asset_locks.py
What was done?
Helper
wait_for_quorum_commitmentvalidates that not just commitment is provided (which can be mock), but a public key is not 00...00.How Has This Been Tested?
Run multiple feature_asset_locks.py and wait for a failure like that to happen, which happens no more with this PR:
Breaking Changes
N/A
Checklist: