fix: deduplicate QRINFO base blocks by thepastaclaw · Pull Request #7349 · dashpay/dash

thepastaclaw · 2026-06-09T00:49:16Z

Issue being fixed or feature implemented

QRINFO request handling accepts caller-provided base block hashes and carries the
validated block indexes through quorum rotation response construction. Repeated
base hashes do not add useful information, so this change canonicalizes those
base indexes before the builder phases use them.

Provenance: thepastaclaw/tracker#1450

What was done?

Sort validated QRINFO base block indexes by height for both construction
paths.
Remove duplicate base block index pointers before response construction.
Add focused unit coverage for repeated base block selection behavior.
Add RPC functional coverage showing repeated base hashes produce the same
result as a single base hash.

How Has This Been Tested?

Environment: macOS arm64, local Dash Core worktree based on upstream/develop.

Commands run:

git diff --check upstream/develop..HEAD — passed
make -C src -j$(sysctl -n hw.ncpu) test/test_dash — passed earlier in this
worktree before the final Python-only test addition
src/test/test_dash --run_test=llmq_snapshot_tests/get_last_base_block_hash_repeated_base_blocks_test
— passed
test/functional/feature_llmq_rotation.py — skipped locally because this
worktree was configured with --disable-wallet
code-review dashpay/dash upstream/develop fix/qrinfo-dedupe-base-hashes ...
— passed with recommendation: ship

Also checked:

gh api repos/dashpay/dash --jq '.default_branch' — develop
Open PR search for duplicate QRINFO/baseBlockHashes dedupe work — no duplicate
open PR found

Breaking Changes

None expected.

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 (for repository code-owners and collaborators only)

thepastaclaw · 2026-06-09T00:49:27Z

@coderabbitai review

coderabbitai · 2026-06-09T00:49:33Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

github-actions · 2026-06-09T00:54:21Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2026-06-09T00:54:58Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0a803374-c80a-4a78-bd8e-ff30e6ff4da7

📥 Commits

Reviewing files that changed from the base of the PR and between e34105f and 470ec0e.

📒 Files selected for processing (2)

src/llmq/snapshot.cpp
src/test/llmq_snapshot_tests.cpp

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

src/llmq/snapshot.cpp
src/test/llmq_snapshot_tests.cpp

Walkthrough

This PR modifies the quorum rotation snapshot construction to unconditionally sort base block indexes by ascending block height. Additionally, deduplication is applied only on the non-legacy construction path; the legacy path remains height-sorted but preserves duplicate entries to maintain legacy peer wire response behavior. The implementation adds the <algorithm> header and modifies BuildQuorumRotationInfo to call std::sort unconditionally and std::unique/erase conditionally. A unit test verifies GetLastBaseBlockHash behavior with repeated block entries for both sorted and unsorted scenarios, and a functional test confirms the RPC returns consistent results when the same base block hash is provided multiple times.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the main change: deduplicating QRINFO base blocks by adding sorting and deduplication logic to the builder. It is specific, clear, and directly reflects the primary objective of the changeset.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, explaining both the issue being fixed and the implementation approach, including testing performed and backward compatibility considerations.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with 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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

thepastaclaw · 2026-06-09T03:39:04Z

✅ Review complete (commit 470ec0e)

thepastaclaw

Code Review

Both agents converged on no findings for this small, well-scoped fix that sorts and deduplicates QRINFO baseBlockIndexes. CodeRabbit also reported no inline findings. The change is covered by both unit and functional tests.

PastaPastaPasta · 2026-06-15T12:21:51Z

Might this be a breaking change? Especially how we have the "legacy construction" flag? seems for some reason at some point the exact structure was deemed important?

Address review feedback: the prior commit unconditionally sorted and deduplicated baseBlockIndexes, which touched the legacy construction path used to serve peers < EFFICIENT_QRINFO_VERSION. Restrict deduplication to the non-legacy path so the wire response to older peers stays bit-for-bit identical to the pre-fix behavior. The legacy path keeps the sort it always relied on for baseBlockIndexes.back() and GetLastBaseBlockHash(). Strengthen the unit test to explicitly prove the legacy no-op invariant: GetLastBaseBlockHash returns the same hash whether the sorted input contains duplicates or not, so legacy multiplicity is harmless. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

thepastaclaw · 2026-06-15T13:19:14Z

Good catch. I narrowed the change in 9f3086755d so the legacy-construction path keeps the old normalization behavior: sort only, no dedupe.

The repeated-base-hash fix now applies to the non-legacy path used by the RPC. I also added a direct unit assertion documenting that duplicates are a no-op for legacy GetLastBaseBlockHash, so we preserve compatibility while still fixing the repeated-base RPC behavior.

Validation:

./src/test/test_dash --run_test=llmq_snapshot_tests

thepastaclaw

Code Review

Carried-forward prior findings: none (prior review at e34105f had no issues). New delta at 9f30867 correctly narrows the dedupe added in the first commit to the non-legacy QRINFO construction path, preserving bit-for-bit wire compatibility for peers below EFFICIENT_QRINFO_VERSION while keeping the sort the legacy path already relied on. The unit test addition explicitly asserts the legacy no-op invariant (duplicate vs. unique input yields identical GetLastBaseBlockHash output). Both Claude and Codex agents reported zero findings; verification against src/llmq/snapshot.cpp and src/test/llmq_snapshot_tests.cpp confirms the change is correct, well-scoped, and adequately tested.

UdjinM6 · 2026-06-16T21:18:45Z

        if (use_legacy_construction) {
+            // Legacy construction (served to peers < EFFICIENT_QRINFO_VERSION) only needs the
+            // input sorted; do not deduplicate so the wire response stays bit-for-bit identical
+            // to the pre-fix behavior for older peers.
+            std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),
+                      [](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });
+        } else {
            std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),
                      [](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });


Suggested change

if (use_legacy_construction) {

// Legacy construction (served to peers < EFFICIENT_QRINFO_VERSION) only needs the

// input sorted; do not deduplicate so the wire response stays bit-for-bit identical

// to the pre-fix behavior for older peers.

std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),

[](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });

} else {

std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),

[](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });

// Sort in all cases: the legacy path (served to peers < EFFICIENT_QRINFO_VERSION)

// relies on the order for baseBlockIndexes.back() and GetLastBaseBlockHash().

std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),

[](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });

if (!use_legacy_construction) {

// Only deduplicate on the non-legacy path; leave the legacy path untouched so the

// wire response to older peers stays bit-for-bit identical to the pre-fix behavior.

Applied in 470ec0e, thanks!

UdjinM6

utACK 470ec0e

thepastaclaw

Code Review

Small, well-scoped change hoisting the sort of baseBlockIndexes out of the legacy-only branch and adding dedup on the non-legacy QRINFO construction path only. Behavior is correctness-preserving (GetLastBaseBlockHash is robust to duplicates) and the asymmetric dedup is intentionally conservative for wire compatibility per recent commit history. Unit and functional coverage are appropriate.

PastaPastaPasta

utACK 470ec0e

fix: deduplicate QRINFO base blocks

e34105f

thepastaclaw marked this pull request as ready for review June 9, 2026 03:38

thepastaclaw commented Jun 9, 2026

View reviewed changes

thepastaclaw commented Jun 15, 2026

View reviewed changes

UdjinM6 reviewed Jun 16, 2026

View reviewed changes

refactor: clarify QRINFO base-block sorting

470ec0e

UdjinM6 approved these changes Jun 16, 2026

View reviewed changes

thepastaclaw commented Jun 16, 2026

View reviewed changes

PastaPastaPasta approved these changes Jun 17, 2026

View reviewed changes

PastaPastaPasta merged commit c5758ca into dashpay:develop Jun 17, 2026
50 checks passed

Conversation

thepastaclaw commented Jun 9, 2026

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

thepastaclaw commented Jun 9, 2026

Uh oh!

coderabbitai Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

PastaPastaPasta commented Jun 15, 2026

Uh oh!

thepastaclaw commented Jun 15, 2026

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

UdjinM6 Jun 16, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading

github-actions Bot commented Jun 9, 2026 •

edited

Loading

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading

thepastaclaw commented Jun 9, 2026 •

edited

Loading