Skip to content

fix: adjust quorum rotation data results in some edge cases, add tests #6625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 7, 2025
Merged

fix: adjust quorum rotation data results in some edge cases, add tests #6625

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
422a695
fix: default to genesis block as the last base block
UdjinM6 Apr 3, 2025
9c98f99
fix: off-by-one: do not ignore the base block which matches the reque…
UdjinM6 Apr 3, 2025
275ed8e
test: a few trivial checks for `quorum rotationinfo` RPC
UdjinM6 Apr 3, 2025
4f57afa
chore: fix clang format
UdjinM6 Apr 4, 2025
8b9a728
chore: drop redundant debug logging in tests
UdjinM6 Apr 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/llmq/snapshot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,13 +392,14 @@ bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotMan
uint256 GetLastBaseBlockHash(Span<const CBlockIndex*> baseBlockIndexes, const CBlockIndex* blockIndex,
bool use_legacy_construction)
{
uint256 hash;
if (!use_legacy_construction) {
std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(),
[](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; });
}
// default to genesis block
uint256 hash{Params().GenesisBlock().GetHash()};
Comment on lines +399 to +400
Copy link

@coderabbitai coderabbitai bot Apr 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Copy link
Collaborator

@knst knst Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to set genesis block here? Is it still possible situation when hash can be default?

Copy link

@coderabbitai coderabbitai bot Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

  1. When baseBlockIndexes is empty
  2. 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.

for (const auto baseBlock : baseBlockIndexes) {
if (baseBlock->nHeight >= blockIndex->nHeight)
if (baseBlock->nHeight > blockIndex->nHeight)
break;
hash = baseBlock->GetBlockHash();
}
Expand Down
17 changes: 17 additions & 0 deletions test/functional/feature_llmq_rotation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,23 @@ def run_test(self):
self.wait_until(lambda: self.nodes[0].getbestblockhash() == new_quorum_blockhash)
assert_equal(self.nodes[0].quorum("list", llmq_type), new_quorum_list)

self.log.info("Test 'quorum rotationinfo' RPC")
genesis_blockhash = self.nodes[0].getblockhash(0)
block_count = self.nodes[0].getblockcount()
hmc_base_blockhash = self.nodes[0].getblockhash(block_count - (block_count % 24) - 24 - 8)
best_block_hash = self.nodes[0].getbestblockhash()
rpc_qr_info = self.nodes[0].quorum("rotationinfo", best_block_hash, False, hmc_base_blockhash)
self.log.info(rpc_qr_info)
assert_equal(rpc_qr_info["mnListDiffTip"]["blockHash"], best_block_hash)
assert_equal(rpc_qr_info["mnListDiffTip"]["baseBlockHash"], rpc_qr_info["mnListDiffH"]["blockHash"])
assert_equal(rpc_qr_info["mnListDiffH"]["baseBlockHash"], rpc_qr_info["mnListDiffAtHMinusC"]["blockHash"])
assert_equal(rpc_qr_info["mnListDiffAtHMinusC"]["blockHash"], hmc_base_blockhash)
assert_equal(rpc_qr_info["mnListDiffAtHMinusC"]["baseBlockHash"], hmc_base_blockhash)
assert_equal(rpc_qr_info["mnListDiffAtHMinusC"]["newQuorums"], [])
assert_equal(rpc_qr_info["mnListDiffAtHMinusC"]["deletedQuorums"], [])
assert_equal(rpc_qr_info["mnListDiffAtHMinus2C"]["baseBlockHash"], rpc_qr_info["mnListDiffAtHMinus3C"]["blockHash"])
assert_equal(rpc_qr_info["mnListDiffAtHMinus3C"]["baseBlockHash"], genesis_blockhash)

def test_getmnlistdiff_quorums(self, baseBlockHash, blockHash, baseQuorumList, expectedDeleted, expectedNew, testQuorumsCLSigs = True):
d = self.test_getmnlistdiff_base(baseBlockHash, blockHash, testQuorumsCLSigs)

Expand Down
Loading