fix: adjust quorum rotation data results in some edge cases, add tests #6625
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
WalkthroughThe changes update the logic for obtaining the last valid base block hash and expand the testing of quorum rotation. In the block snapshot module, the In parallel, the test framework for quorum rotation was enhanced by adding a new scenario that verifies the output of the "quorum rotationinfo" RPC call. The test retrieves multiple block hashes and validates the structure and consistency of the returned RPC data, confirming correct block hash associations and quorum list content. No changes were made to public or exported entity declarations. 📜 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 (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/llmq/snapshot.cpp(1 hunks)test/functional/feature_llmq_rotation.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/snapshot.cpp
[error] 399-399: Clang format differences found. Please run 'clang-format' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (1)
test/functional/feature_llmq_rotation.py (1)
234-249: Good test coverage for quorum rotationinfo RPC.This test ensures that the changes to the
GetLastBaseBlockHashfunction are properly validated. The assertions verify that:
- Block hashes are correctly passed through the rotation info response
- The base block hash hierarchy is maintained (tip → H → HMinusC)
- The HMinusC base block hash matches the provided hmc_base_blockhash
- Empty quorum lists are correctly returned for HMinusC
- The genesis block hash is correctly used as the fallback for the oldest blocks
These tests directly address the issues fixed in the
GetLastBaseBlockHashfunction, ensuring the rotation info results are meaningful and compact.
Comment on lines
+399
to
+400
| // default to genesis block | ||
| uint256 hash{Params().GenesisBlock().GetHash()}; |
There was a problem hiding this comment.
Good addition of a sensible default value.
Initializing the hash variable to the genesis block hash ensures the function always returns a valid hash, even if no valid base blocks are found in the loop. This addresses an issue where baseBlockHash could incorrectly return all zeros in some edge cases.
However, there is a Clang format issue reported in the pipeline:
[error] 399-399: Clang format differences found. Please run 'clang-format' to fix formatting issues.
Please run clang-format to fix this formatting issue.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 399-399: Clang format differences found. Please run 'clang-format' to fix formatting issues.
Collaborator
There was a problem hiding this comment.
why do we need to set genesis block here? Is it still possible situation when hash can be default?
There was a problem hiding this comment.
@knst The genesis block initialization ensures the function always returns a valid hash rather than an uninitialized value.
This change prevents undefined behavior that could occur in two scenarios:
- When
baseBlockIndexesis empty - When all base blocks have heights greater than the target block's height
In either case, the loop would finish without assigning anything to hash, potentially returning all zeros or garbage data. By defaulting to the genesis block hash, we ensure a meaningful fallback value is always returned.
After this change, there's no possibility of the hash remaining in an uninitialized state.
🧬 Code Definitions Used
### Relevant Code Snippets for `src/llmq/snapshot.cpp`
1. **Function: `GetLastBaseBlockHash`**
- **Filename:** `src/llmq/snapshot.cpp`
- **Line Range:** 215-216
- **Description:**
```cpp
uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex,
bool use_legacy_construction)
```
- **Parameters:**
- `baseBlockIndexes`: A span of pointers to `CBlockIndex` representing base block indexes.
- `blockIndex`: A pointer to the `CBlockIndex` for which the last base block hash is to be determined.
- `use_legacy_construction`: A boolean indicating whether to use legacy construction logic.
- **Returns:** The hash of the last base block as a `uint256`.
- **Exceptions/Error Handling:** None specified; assumes valid inputs.
2. **Function: `BuildQuorumRotationInfo`**
- **Filename:** `src/llmq/snapshot.cpp`
- **Line Range:** 48-48
- **Description:**
```cpp
bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
const ChainstateManager& chainman, const CQuorumManager& qman,
const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request,
bool use_legacy_construction, CQuorumRotationInfo& response, std::string& errorRet)
```
- **Parameters:**
- `dmnman`: A reference to a `CDeterministicMNManager` instance.
- `qsnapman`: A reference to a `CQuorumSnapshotManager` instance.
- `chainman`: A reference to a `ChainstateManager` instance.
- `qman`: A reference to a `CQuorumManager` instance.
- `qblockman`: A reference to a `CQuorumBlockProcessor` instance.
- `request`: A `CGetQuorumRotationInfo` request object.
- `use_legacy_construction`: A boolean indicating whether to use legacy construction logic.
- `response`: A reference to a `CQuorumRotationInfo` object to populate with the response.
- `errorRet`: A string to capture any error messages.
- **Returns:** `true` if successful, `false` otherwise.
- **Exceptions/Error Handling:** Populates `errorRet` with error messages for various failure conditions.
3. **Function: `GetSnapshotForBlock`**
- **Filename:** `src/llmq/snapshot.h`
- **Line Range:** 231-231
- **Description:**
```cpp
std::optional<CQuorumSnapshot> GetSnapshotForBlock(const Consensus::LLMQType llmqType, const CBlockIndex* pindex)
```
- **Parameters:**
- `llmqType`: The type of the LLMQ (Long-Living Masternode Quorum).
- `pindex`: A pointer to the `CBlockIndex` for which the snapshot is requested.
- **Returns:** An optional `CQuorumSnapshot` if found, otherwise `std::nullopt`.
- **Exceptions/Error Handling:** None specified; assumes valid inputs.
4. **Function: `StoreSnapshotForBlock`**
- **Filename:** `src/llmq/snapshot.h`
- **Line Range:** 232-232
- **Description:**
```cpp
void StoreSnapshotForBlock(const Consensus::LLMQType llmqType, const CBlockIndex* pindex, const CQuorumSnapshot& snapshot)
```
- **Parameters:**
- `llmqType`: The type of the LLMQ.
- `pindex`: A pointer to the `CBlockIndex` for which the snapshot is being stored.
- `snapshot`: The `CQuorumSnapshot` to store.
- **Returns:** None.
- **Exceptions/Error Handling:** None specified; assumes valid inputs.
knst
reviewed
Apr 4, 2025
Comment on lines
+399
to
+400
| // default to genesis block | ||
| uint256 hash{Params().GenesisBlock().GetHash()}; |
Collaborator
There was a problem hiding this comment.
why do we need to set genesis block here? Is it still possible situation when hash can be default?
knst
approved these changes
Apr 4, 2025
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Apr 7, 2025
…edge cases, add tests 8b9a728 chore: drop redundant debug logging in tests (UdjinM6) 4f57afa chore: fix clang format (UdjinM6) 275ed8e test: a few trivial checks for `quorum rotationinfo` RPC (UdjinM6) 9c98f99 fix: off-by-one: do not ignore the base block which matches the requested block (UdjinM6) 422a695 fix: default to genesis block as the last base block (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Before this PR you could get `baseBlockHash` as all 0s for some cases. Also, a known `baseBlockHash` that matched the `cycleHash` was ignored which means that results included quorum changes from an older known `baseBlockHash` till the `cycleHash`(which is the same as the `baseBlockHash` in question) i.e. these changes were already known to the requester. ## What was done? Pls see individual commits ## How Has This Been Tested? Run tests ## Breaking Changes n/a, results should simply be a bit more meaningful and compact ## 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)_ ACKs for top commit: PastaPastaPasta: utACK 8b9a728 Tree-SHA512: 88e5463ca0a40e3decb5cfcab948d7be2febc3583f7a24d455a5a93eb730e34c25246a1384fcfeea887a91348651e6250e226afa02b73f4403d8d10624a84d55
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Apr 14, 2025
…edge cases, add tests 8b9a728 chore: drop redundant debug logging in tests (UdjinM6) 4f57afa chore: fix clang format (UdjinM6) 275ed8e test: a few trivial checks for `quorum rotationinfo` RPC (UdjinM6) 9c98f99 fix: off-by-one: do not ignore the base block which matches the requested block (UdjinM6) 422a695 fix: default to genesis block as the last base block (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Before this PR you could get `baseBlockHash` as all 0s for some cases. Also, a known `baseBlockHash` that matched the `cycleHash` was ignored which means that results included quorum changes from an older known `baseBlockHash` till the `cycleHash`(which is the same as the `baseBlockHash` in question) i.e. these changes were already known to the requester. ## What was done? Pls see individual commits ## How Has This Been Tested? Run tests ## Breaking Changes n/a, results should simply be a bit more meaningful and compact ## 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)_ ACKs for top commit: PastaPastaPasta: utACK 8b9a728 Tree-SHA512: 88e5463ca0a40e3decb5cfcab948d7be2febc3583f7a24d455a5a93eb730e34c25246a1384fcfeea887a91348651e6250e226afa02b73f4403d8d10624a84d55
PastaPastaPasta
added a commit
that referenced
this pull request
Apr 15, 2025
8b4ab03 fix: suppress MIN_MASTERNODE_PROTO_VERSION bump in 6608 (pasta) aca04d1 chore: bump build to 22.1.2 (pasta) d9d8c24 docs: add release notes for 22.1.2 (pasta) fb45240 Merge #6608: fix: `cycleHash` should represent a cycle starting block of the signing quorum (pasta) 9d1498c Merge #6625: fix: adjust quorum rotation data results in some edge cases, add tests (pasta) dfc1119 Merge #6622: fix: efficient build mnlistdiffs in rotation info (pasta) 6fd626b Merge #6586: fix: revert deployment images back to Ubuntu 22.04 LTS (`jammy`), pin QEMU to avoid segfault (pasta) affa9d1 Merge #6599: fix: follow-up #6546 to bump copyright year in COPYING and debian's package (pasta) f6163a2 Merge #6593: fix: resolve potential deadlock in coinjoin_tests (pasta) 243e0ab Merge #6585: fix: Do not assert special tx type for cbtx in simplified mn list diff output (pasta) 497f95c Merge #6581: perf: speedup of CBLSLazyPublicKey::operator== when comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient (pasta) Pull request description: ## Issue being fixed or feature implemented Backports for a new version, v22.1.2 ## What was done? See release notes ## How Has This Been Tested? ## Breaking Changes None ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] 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 _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 8b4ab03 knst: utACK 8b4ab03 Tree-SHA512: 9f2f65e315940197cc2b75a6b0a858d624256cbe668272ff6dfa216eceda1ba9338484d47afc569202b3b5cc75b4dcb825209efe3a3d3ccec57c741f75c40577
PastaPastaPasta
added a commit
that referenced
this pull request
Apr 23, 2025
8b4ab03 fix: suppress MIN_MASTERNODE_PROTO_VERSION bump in 6608 (pasta) aca04d1 chore: bump build to 22.1.2 (pasta) d9d8c24 docs: add release notes for 22.1.2 (pasta) fb45240 Merge #6608: fix: `cycleHash` should represent a cycle starting block of the signing quorum (pasta) 9d1498c Merge #6625: fix: adjust quorum rotation data results in some edge cases, add tests (pasta) dfc1119 Merge #6622: fix: efficient build mnlistdiffs in rotation info (pasta) 6fd626b Merge #6586: fix: revert deployment images back to Ubuntu 22.04 LTS (`jammy`), pin QEMU to avoid segfault (pasta) affa9d1 Merge #6599: fix: follow-up #6546 to bump copyright year in COPYING and debian's package (pasta) f6163a2 Merge #6593: fix: resolve potential deadlock in coinjoin_tests (pasta) 243e0ab Merge #6585: fix: Do not assert special tx type for cbtx in simplified mn list diff output (pasta) 497f95c Merge #6581: perf: speedup of CBLSLazyPublicKey::operator== when comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient (pasta) 4298d73 chore: bump to 22.1.1 (pasta) fc65a16 chore: release notes for 22.1.1 (pasta) 38762f7 Merge #6574: fix: ReconnectionInfo should also store Dash-specific flags (pasta) 580b74c Merge #6566: fix(qt): avoid leaking balance and CJ info in GUI when in discreet mode (pasta) Pull request description: ## Issue being fixed or feature implemented ## What was done? ff-ed `master` 21.1.0 -> 21.1.2, merging it back into `develop` now ## How Has This Been Tested? ## Breaking Changes ## 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)_ ACKs for top commit: PastaPastaPasta: utACK 73fa780; gonna merge as this changes docs only Tree-SHA512: f04fe461edc05c38771d684bd5d60327076251b8004723027276b104989ea6d84f7f77cce31f310f058e81f2c25411ab5a15b563cd3db66091ccaf33da459e3c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Issue being fixed or feature implemented
Before this PR you could get
baseBlockHashas all 0s for some cases. Also, a knownbaseBlockHashthat matched thecycleHashwas ignored which means that results included quorum changes from an older knownbaseBlockHashtill thecycleHash(which is the same as thebaseBlockHashin question) i.e. these changes were already known to the requester.What was done?
Pls see individual commits
How Has This Been Tested?
Run tests
Breaking Changes
n/a, results should simply be a bit more meaningful and compact
Checklist: