fix: use correct hash of quorum as cycle hash for regtest #6673

knst · 2025-05-17T15:48:16Z

Issue being fixed or feature implemented

feature_llmq_singlenode.py works unstable, roughly 10% failure rate. It fails with this error:

2025-05-17T15:03:08.586000Z TestFramework (INFO): InstantSend lock on tx: 202b6698cf0da8adfeff8430daff3d1cfe767d1120751bb7765f6df15d09a3ee is expecting
2025-05-17T15:04:09.033000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
        def check_instantlock():
            self.bump_mocktime(1)
            try:
                return node.getrawtransaction(txid, True)["instantlock"]
            except:
                return False
'''
2025-05-17T15:04:09.033000Z TestFramework (ERROR): Assertion failed

Deep in logs the failure look like this:

 node0 2025-05-17T14:40:45.628148Z (mocktime: 2014-12-12T05:56:24Z) [     isman] [llmq/instantsend.cpp:985] [ProcessPendingInstantSendLocks] [instantsend] CInstantSendManager::ProcessPendingInstantSendLocks -- txid=202b6698cf0da8adfeff8430daff3d1cfe767d1120751bb7765f6df15d09a3ee, islock=89b6a8bb50c2a6b07ebd2bbde0727a72ed4fbe4f78247aab509189b50fc7293f: invalid sig in islock, peer=1

Further debug shown that somehow sign-hash sometimes is different; while msgHash is same; signature is valid and bytes-per-bytes matched; just validation code does not work as expected.

What was done?

For non-rotating quorums on RegTest should be used directly quorum hash instead cycleQuorum; no quorum choosing procedure is needed.

How Has This Been Tested?

Run multiple times functional tests.

Breaking Changes

N/A; affect only RegTests for special case of non-rotating quorum used for InstantSend.

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone

coderabbitai · 2025-05-17T15:50:25Z

Walkthrough

The change modifies the quorum selection logic in the InstantSend module's ProcessPendingInstantSendLocks method. It introduces a conditional check on whether quorum rotation is enabled. If rotation is enabled, the quorum is selected using the existing llmq::SelectQuorumForSigning method. If rotation is disabled, the quorum is retrieved directly by type and cycle hash from the instant send lock. This update accommodates non-rotating quorum scenarios without altering function signatures, public APIs, or other logic and control flow.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b66448 and ff61071.

📒 Files selected for processing (1)

src/llmq/instantsend.cpp (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/llmq/instantsend.cpp

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends

✨ Finishing Touches

📝 Generate Docstrings

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.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

PastaPastaPasta

utACK 4b66448

UdjinM6 · 2025-05-17T17:22:28Z

src/llmq/instantsend.cpp

+    islock.cycleHash = (llmq_params_opt->useRotation ? quorum->m_quorum_base_block_index->GetAncestor(cycle_height)
+                                                     : quorum->m_quorum_base_block_index)
+                           ->GetBlockHash();


This should make no difference because quorum->m_quorum_base_block_index->nHeight % llmq_params_opt->dkgInterval is 0 for non-rotating quorums so cycle_height == quorum->m_quorum_base_block_index->nHeight and m_quorum_base_block_index->GetAncestor(cycle_height) == m_quorum_base_block_index.

yes, you are right; I reverted this part

UdjinM6 · 2025-05-17T17:24:59Z

src/llmq/instantsend.cpp

            nSignHeight = blockIndex->nHeight + dkgInterval - 1;
        }
+        // For RegTest non-rotating quorum cycleHash has directly quorum hash
+        auto quorum = llmq_params.useRotation ? llmq::SelectQuorumForSigning(llmq_params, m_chainstate.m_chain, qman,


SelectQuorumForSigning should handle this internally already

it should handle this internally, but somehow SelectQuorumForSigning returns sometimes is not exactly same quorum -> validation fails (see logs):

node0 2025-06-19T14:49:28.463903Z (mocktime: 2014-12-12T05:56:41Z) [ isman] [llmq/instantsend.cpp:957] [ProcessPendingInstantSendLocks] [instantsend] CInstantSendManager::ProcessPendingInstantSendLocks -- verified locks. count=1, alreadyVerified=0, vt=4, nodes=1 node0 2025-06-19T14:49:28.463934Z (mocktime: 2014-12-12T05:56:41Z) [ isman] [llmq/instantsend.cpp:976] [ProcessPendingInstantSendLocks] [instantsend] CInstantSendManager::ProcessPendingInstantSendLocks -- txid=11fe72d9be3c2fe4a2545429ed88c20928e21ece77de896423dbbb55fe00679d, islock=527c905af5e2f8d7e7be84e57c19ae6fe22b37bcc96735d475c21f1f9ce57927: invalid sig in islock, peer=1 node0 2025-06-19T14:49:28.463951Z (mocktime: 2014-12-12T05:56:41Z) [ isman] [llmq/instantsend.cpp:876] [ProcessPendingInstantSendLocks] [instantsend] CInstantSendManager::ProcessPendingInstantSendLocks -- doing verification on old active set node0 2025-06-19T14:49:28.468400Z (mocktime: 2014-12-12T05:56:41Z) [ isman] [llmq/instantsend.cpp:957] [ProcessPendingInstantSendLocks] [instantsend] CInstantSendManager::ProcessPendingInstantSendLocks -- verified locks. count=1, alreadyVerified=0, vt=4, nodes=1 node0 2025-06-19T14:49:28.468436Z (mocktime: 2014-12-12T05:56:41Z) [ isman] [net_processing.cpp:1799] [Misbehaving] [net] Misbehaving: peer=1 (0 -> 20) node0 2025-06-19T14:49:28.468448Z (mocktime: 2014-12-12T05:56:41Z) [ isman] [llmq/instantsend.cpp:976] [ProcessPendingInstantSendLocks] [instantsend] CInstantSendManager::ProcessPendingInstantSendLocks -- txid=11fe72d9be3c2fe4a2545429ed88c20928e21ece77de896423dbbb55fe00679d, islock=527c905af5e2f8d7e7be84e57c19ae6fe22b37bcc96735d475c21f1f9ce57927: invalid sig in islock, peer=1

invalid sig in islock is caused by wrong value signHash (I added extra logs while investigation issue) and that happens due to wrong quorum->qc->quorumHash.

I can not figure what exactly causes indeterminism in SelectQuorumForSigning, but this fix is quite reliable on my localhost (5-10% failures vs ~0% failures with the exactly same log message)

It looks like nSignHeight adjustment above works with rotating quorums only. These calculations could be tweaked to work with non-rotating quorums too I guess but I like simply using GetQuorum instead.

UdjinM6 · 2025-06-17T19:00:46Z

I actually reviewed it a month ago initially, just didn't post it cause I couldn't understand why any of this makes any difference. I kept drafts of my review to re-review it again a bit later and... I might be missing smth but I still don't see any changes in behaviour here and why implementing this could affect tests in any way 🙈

…ional tests 0d9418e test: reduce delay in wait_until from 0.5s to 0.05s (Konstantin Akimov) 876d6c8 test: enforce 1 second delay for wait_for_sporks helper (Konstantin Akimov) ec6e7bf test: enforce 1s delay for feature_mnehf test (Konstantin Akimov) 6ab3f7c test: reduce spamming quorum list to logs while waiting (Konstantin Akimov) 9d9975f test: simplify wait_for_quorum_list (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Waiting for 0.5s in functional test for every action is a bit excessive, especially for p2p tests that sending messages by localnetwork and waiting at least 0.5 seconds before checking if message is received. ## What was done? Decreasing default delay from 0.5s to 0.05s. It affects mostly p2p tests, but many other tests become faster too. For quorum formation; for sporks and some other dash specific features bigger delays (0.5s, 1s) are used. Further improvements are blocked by #6673, #6672, #6671 and are out of scope this PR. ## How Has This Been Tested? Speed up on CI for 30% and more. [develop] linux64-test https://gitlab.com/dashpay/dash/-/jobs/10049432489 ALL | ✓ Passed | 7241 s (accumulated) Runtime: 1272 s [PR] linux64-test https://gitlab.com/dashpay/dash/-/jobs/10067158169 ALL | ✓ Passed | 5421 s (accumulated) Runtime: 938 s **-25%** [develop] linux64-nowallet https://gitlab.com/dashpay/dash/-/jobs/10049432511 ALL | ✓ Passed | 2739 s (accumulated) Runtime: 488 s [PR] linux64-nowallet https://gitlab.com/dashpay/dash/-/jobs/10067158174 ALL | ✓ Passed | 1232 s (accumulated) Runtime: 243 s **-49%** [develop] linux64-tsan https://gitlab.com/dashpay/dash/-/jobs/10049432499 ALL | ✓ Passed | 10399 s (accumulated) Runtime: 2023 s [PR] linux64-tsan https://gitlab.com/dashpay/dash/-/jobs/10072993489 ALL | ✓ Passed | 8710 s (accumulated) Runtime: 1543 s **-25%** [develop] Functional tests on localhost (-O3, debug, no sanitizers, -j20): ALL | ✓ Passed | 6680 s (accumulated) Runtime: 372 s [PR] Functional tests on localhost (-O3, debug, no sanitizers, -j20): ALL | ✓ Passed | 4609 s (accumulated) Runtime: 365 s **Benefits of running locally in 20 parallel jobs are very slight. Accumulated time is decreased for 32% as expected, but total time is improved less than 2%.** It is because the slowest tests requires many quorums to be formed and they are still slow. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 0d9418e; hopefully it doesn't make tests flakey UdjinM6: utACK 0d9418e Tree-SHA512: 32405bd1f229af5146c96aea6031cee3f084d3ebfb3ec515ad743e79c3bc29a5c891d4330688d07b63b0e06ef7cd50240ab8b6d1a3939a56fe3e64a55918edd1

…understandings

UdjinM6

light ACK ff61071

UdjinM6 · 2025-06-20T21:02:34Z

src/llmq/instantsend.cpp

            nSignHeight = blockIndex->nHeight + dkgInterval - 1;
        }
+        // For RegTest non-rotating quorum cycleHash has directly quorum hash
+        auto quorum = llmq_params.useRotation ? llmq::SelectQuorumForSigning(llmq_params, m_chainstate.m_chain, qman,


It looks like nSignHeight adjustment above works with rotating quorums only. These calculations could be tweaked to work with non-rotating quorums too I guess but I like simply using GetQuorum instead.

… regtest ff61071 fix: use correct hash of quorum as cycle hash to prevent possible misunderstandings (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented feature_llmq_singlenode.py works unstable, roughly 10% failure rate. It fails with this error: 2025-05-17T15:03:08.586000Z TestFramework (INFO): InstantSend lock on tx: 202b6698cf0da8adfeff8430daff3d1cfe767d1120751bb7765f6df15d09a3ee is expecting 2025-05-17T15:04:09.033000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: '''' def check_instantlock(): self.bump_mocktime(1) try: return node.getrawtransaction(txid, True)["instantlock"] except: return False ''' 2025-05-17T15:04:09.033000Z TestFramework (ERROR): Assertion failed Deep in logs the failure look like this: ``` node0 2025-05-17T14:40:45.628148Z (mocktime: 2014-12-12T05:56:24Z) [ isman] [llmq/instantsend.cpp:985] [ProcessPendingInstantSendLocks] [instantsend] CInstantSendManager::ProcessPendingInstantSendLocks -- txid=202b6698cf0da8adfeff8430daff3d1cfe767d1120751bb7765f6df15d09a3ee, islock=89b6a8bb50c2a6b07ebd2bbde0727a72ed4fbe4f78247aab509189b50fc7293f: invalid sig in islock, peer=1 ``` Further debug shown that somehow sign-hash sometimes is different; while msgHash is same; signature is valid and bytes-per-bytes matched; just validation code does not work as expected. ## What was done? For non-rotating quorums on RegTest should be used directly quorum hash instead `cycleQuorum`; no quorum choosing procedure is needed. ## How Has This Been Tested? Run multiple times functional tests. ## Breaking Changes N/A; affect only RegTests for special case of non-rotating quorum used for InstantSend. ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: light ACK ff61071 Tree-SHA512: 7be0eb246f13f851de2c6ce6543f0bac8faf64cbc2a8f5fd6763f0aaca602cfb99e6df882d9e9e8a6d65fbdcdd9188e925427578d86729696dc23652856f7e1c

knst added this to the 23 milestone May 17, 2025

knst requested review from PastaPastaPasta and UdjinM6 May 17, 2025 17:24

knst mentioned this pull request May 20, 2025

test: set default delay to 0.05s for wait_until in functional tests #6631

Merged

5 tasks

PastaPastaPasta previously approved these changes Jun 17, 2025

View reviewed changes

UdjinM6 reviewed Jun 17, 2025

View reviewed changes

fix: use correct hash of quorum as cycle hash to prevent possible mis…

ff61071

…understandings

knst dismissed PastaPastaPasta’s stale review via ff61071 June 19, 2025 15:05

knst force-pushed the fix-single-node-quorum branch from 4b66448 to ff61071 Compare June 19, 2025 15:05

knst requested a review from UdjinM6 June 19, 2025 15:14

UdjinM6 approved these changes Jun 20, 2025

View reviewed changes

knst requested a review from PastaPastaPasta June 21, 2025 14:11

PastaPastaPasta approved these changes Jun 24, 2025

View reviewed changes

PastaPastaPasta merged commit f453998 into dashpay:develop Jun 24, 2025
37 of 38 checks passed

knst deleted the fix-single-node-quorum branch June 24, 2025 14:22

knst mentioned this pull request Nov 11, 2025

refactor: separate network and consensus logic 1/N [instantsend] #6959

Open

5 tasks

fix: use correct hash of quorum as cycle hash for regtest #6673

fix: use correct hash of quorum as cycle hash for regtest #6673

Uh oh!

Conversation

knst commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

coderabbitai bot commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 May 17, 2025

Choose a reason for hiding this comment

Uh oh!

knst Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

UdjinM6 May 17, 2025

Choose a reason for hiding this comment

Uh oh!

knst Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

UdjinM6 commented Jun 17, 2025

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

knst commented May 17, 2025 •

edited

Loading

coderabbitai bot commented May 17, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)