test: functional tests for invalid QuorumCommitment #6724
Conversation
WalkthroughThe changes extend the LLMQ DKG errors functional test by adding a new method ✨ Finishing Touches
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: 4
🧹 Nitpick comments (2)
test/functional/feature_llmq_dkgerrors.py (1)
91-98: Replace unnecessarygetattrcalls
getattr(qc, 'membersSig')/'quorumSig'are constant attribute accesses,
offering no safety benefit over direct access and triggering Ruff B009.- self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'quorumSig', getattr(qc, 'membersSig')), qc)[1]) - self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'membersSig', getattr(qc, 'quorumSig')), qc)[1]) + self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'quorumSig', qc.membersSig), qc)[1]) + self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'membersSig', qc.quorumSig), qc)[1])test/functional/test_framework/test_framework.py (1)
1859-1859:mine_quorumkeeps growing – consider decomposing & avoid magic number
mine_quorum()already breaches several complexity metrics (R0913/R0915).
Introducingskip_maturitystretches the signature to 11 parameters and re-introduces the magic number8, which duplicatesSIGN_HEIGHT_OFFSET.Recommendations
- Extract the maturity-block mining into a small private helper.
- Replace
8with a named constant (either re-use the C++ constant if exposed or defineSIGN_HEIGHT_OFFSET = 8near the top of the file).- if not skip_maturity: - # Mine 8 (SIGN_HEIGHT_OFFSET) more blocks to make sure that the new quorum gets eligible for signing sessions - self.generate(self.nodes[0], 8, sync_fun=lambda: self.sync_blocks(nodes)) + if not skip_maturity: + self._mine_quorum_maturity(nodes) + + def _mine_quorum_maturity(self, nodes): + """Advance the chain so the freshly-mined quorum becomes signing-eligible.""" + SIGN_HEIGHT_OFFSET = 8 # keep in one place + self.generate(self.nodes[0], SIGN_HEIGHT_OFFSET, sync_fun=lambda: self.sync_blocks(nodes))This keeps the public API stable and trims
mine_quorum().
Refactoring now will pay off as more flags and thresholds are inevitably added.Also applies to: 1936-1939
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/functional/feature_llmq_dkgerrors.py(3 hunks)test/functional/test_framework/messages.py(1 hunks)test/functional/test_framework/test_framework.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/functional/test_framework/test_framework.py (1)
test/functional/test_framework/test_node.py (1)
generate(343-345)
🪛 Ruff (0.11.9)
test/functional/feature_llmq_dkgerrors.py
95-95: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
96-96: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🪛 Pylint (3.3.7)
test/functional/test_framework/test_framework.py
[refactor] 1848-1848: Too many arguments (6/5)
(R0913)
[refactor] 1848-1848: Too many positional arguments (6/5)
(R0917)
[refactor] 1859-1859: Too many arguments (12/5)
(R0913)
[refactor] 1859-1859: Too many positional arguments (12/5)
(R0917)
[refactor] 1859-1859: Too many local variables (20/15)
(R0914)
[refactor] 1859-1859: Too many statements (59/50)
(R0915)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
| def __repr__(self): | ||
| return f"CFinalCommitmentPayload(nVersion={self.nVersion} nHeight={self.nHeight} commitment={self.commitment}" | ||
|
|
There was a problem hiding this comment.
Incomplete __repr__ output
The formatted string lacks a closing parenthesis, producing a malformed representation and making debugging harder.
- return f"CFinalCommitmentPayload(nVersion={self.nVersion} nHeight={self.nHeight} commitment={self.commitment}"
+ return (
+ f"CFinalCommitmentPayload(nVersion={self.nVersion}, "
+ f"nHeight={self.nHeight}, commitment={self.commitment})"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __repr__(self): | |
| return f"CFinalCommitmentPayload(nVersion={self.nVersion} nHeight={self.nHeight} commitment={self.commitment}" | |
| def __repr__(self): | |
| return ( | |
| f"CFinalCommitmentPayload(nVersion={self.nVersion}, " | |
| f"nHeight={self.nHeight}, commitment={self.commitment})" | |
| ) |
🤖 Prompt for AI Agents
In test/functional/test_framework/messages.py around lines 1435 to 1437, the
__repr__ method's formatted string is missing a closing parenthesis, resulting
in an incomplete and malformed output. Fix this by adding the missing closing
parenthesis at the end of the returned string to properly close the
representation.
| def wait_for_quorum_list(self, quorum_hash, nodes, timeout=15, llmq_type_name="llmq_test"): | ||
| def wait_func(): | ||
| self.log.info("quorums: " + str(self.nodes[0].quorum("list"))) | ||
| if quorum_hash in self.nodes[0].quorum("list")[llmq_type_name]: | ||
| return True | ||
| self.bump_mocktime(sleep, nodes=nodes) | ||
| self.generate(self.nodes[0], 1, sync_fun=lambda: self.sync_blocks(nodes)) | ||
| return False | ||
| self.wait_until(wait_func, timeout=timeout, sleep=sleep) | ||
| return quorum_hash in self.nodes[0].quorum('list')[llmq_type_name] | ||
| self.log.info(f"quorums: {self.nodes[0].quorum('list')}") | ||
| self.wait_until(wait_func, timeout=timeout, sleep=0.05) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the nodes argument instead of only querying self.nodes[0]
wait_for_quorum_list() receives nodes but ignores it and only polls node 0.
If node 0 lags behind (IBD, networking hiccup, etc.) the helper wrongly blocks, while if another node lags behind it might return too early. Leveraging the supplied list makes the helper both precise and future-proof.
-def wait_for_quorum_list(self, quorum_hash, nodes, timeout=15, llmq_type_name="llmq_test"):
- def wait_func():
- return quorum_hash in self.nodes[0].quorum('list')[llmq_type_name]
- self.log.info(f"quorums: {self.nodes[0].quorum('list')}")
- self.wait_until(wait_func, timeout=timeout, sleep=0.05)
+def wait_for_quorum_list(self, quorum_hash, nodes, timeout=15, llmq_type_name="llmq_test"):
+ def all_nodes_have_quorum():
+ return all(
+ quorum_hash in n.quorum("list")[llmq_type_name]
+ for n in nodes
+ )
+ self.log.info(
+ "quorums(list) h(%d): %s",
+ self.nodes[0].getblockcount(),
+ {i: n.quorum('list') for i, n in enumerate(nodes)},
+ )
+ self.wait_until(all_nodes_have_quorum, timeout=timeout, sleep=0.05)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def wait_for_quorum_list(self, quorum_hash, nodes, timeout=15, llmq_type_name="llmq_test"): | |
| def wait_func(): | |
| self.log.info("quorums: " + str(self.nodes[0].quorum("list"))) | |
| if quorum_hash in self.nodes[0].quorum("list")[llmq_type_name]: | |
| return True | |
| self.bump_mocktime(sleep, nodes=nodes) | |
| self.generate(self.nodes[0], 1, sync_fun=lambda: self.sync_blocks(nodes)) | |
| return False | |
| self.wait_until(wait_func, timeout=timeout, sleep=sleep) | |
| return quorum_hash in self.nodes[0].quorum('list')[llmq_type_name] | |
| self.log.info(f"quorums: {self.nodes[0].quorum('list')}") | |
| self.wait_until(wait_func, timeout=timeout, sleep=0.05) | |
| def wait_for_quorum_list(self, quorum_hash, nodes, timeout=15, llmq_type_name="llmq_test"): | |
| def all_nodes_have_quorum(): | |
| return all( | |
| quorum_hash in n.quorum("list")[llmq_type_name] | |
| for n in nodes | |
| ) | |
| self.log.info( | |
| "quorums(list) h(%d): %s", | |
| self.nodes[0].getblockcount(), | |
| {i: n.quorum('list') for i, n in enumerate(nodes)}, | |
| ) | |
| self.wait_until(all_nodes_have_quorum, timeout=timeout, sleep=0.05) |
🤖 Prompt for AI Agents
In test/functional/test_framework/test_framework.py around lines 1842 to 1846,
the method wait_for_quorum_list uses only self.nodes[0] to check the quorum
list, ignoring the nodes argument passed to it. To fix this, update the
wait_func to check the quorum_hash presence across all nodes in the provided
nodes list instead of just the first node. This ensures the function accurately
reflects the quorum state across the intended nodes and avoids blocking or
premature returns due to relying on a single node.
| def wait_for_quorums_list(self, quorum_hash_0, quorum_hash_1, nodes, llmq_type_name="llmq_test", timeout=15): | ||
| def wait_func(): | ||
| self.log.info("h("+str(self.nodes[0].getblockcount())+") quorums: " + str(self.nodes[0].quorum("list"))) | ||
| if quorum_hash_0 in self.nodes[0].quorum("list")[llmq_type_name]: | ||
| if quorum_hash_1 in self.nodes[0].quorum("list")[llmq_type_name]: | ||
| return True | ||
| self.bump_mocktime(sleep, nodes=nodes) | ||
| self.generate(self.nodes[0], 1, sync_fun=lambda: self.sync_blocks(nodes)) | ||
| return False | ||
| self.wait_until(wait_func, timeout=timeout, sleep=sleep) | ||
| quorums = self.nodes[0].quorum("list")[llmq_type_name] | ||
| return quorum_hash_0 in quorums and quorum_hash_1 in quorums | ||
| self.log.info(f"h({self.nodes[0].getblockcount()}) quorums: {self.nodes[0].quorum('list')}") | ||
| self.wait_until(wait_func, timeout=timeout, sleep=0.05) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same single-node limitation in wait_for_quorums_list()
Identical to the previous comment: the helper accepts nodes but never uses it. Checking all nodes prevents false positives/negatives.
-def wait_for_quorums_list(self, quorum_hash_0, quorum_hash_1, nodes, llmq_type_name="llmq_test", timeout=15):
- def wait_func():
- quorums = self.nodes[0].quorum("list")[llmq_type_name]
- return quorum_hash_0 in quorums and quorum_hash_1 in quorums
- self.log.info(f"h({self.nodes[0].getblockcount()}) quorums: {self.nodes[0].quorum('list')}")
- self.wait_until(wait_func, timeout=timeout, sleep=0.05)
+def wait_for_quorums_list(self, quorum_hash_0, quorum_hash_1, nodes, llmq_type_name="llmq_test", timeout=15):
+ def all_nodes_have_both():
+ return all(
+ quorum_hash_0 in n.quorum("list")[llmq_type_name] and
+ quorum_hash_1 in n.quorum("list")[llmq_type_name]
+ for n in nodes
+ )
+ self.log.info(
+ "quorums(list) h(%d): %s",
+ self.nodes[0].getblockcount(),
+ {i: n.quorum('list') for i, n in enumerate(nodes)},
+ )
+ self.wait_until(all_nodes_have_both, timeout=timeout, sleep=0.05)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def wait_for_quorums_list(self, quorum_hash_0, quorum_hash_1, nodes, llmq_type_name="llmq_test", timeout=15): | |
| def wait_func(): | |
| self.log.info("h("+str(self.nodes[0].getblockcount())+") quorums: " + str(self.nodes[0].quorum("list"))) | |
| if quorum_hash_0 in self.nodes[0].quorum("list")[llmq_type_name]: | |
| if quorum_hash_1 in self.nodes[0].quorum("list")[llmq_type_name]: | |
| return True | |
| self.bump_mocktime(sleep, nodes=nodes) | |
| self.generate(self.nodes[0], 1, sync_fun=lambda: self.sync_blocks(nodes)) | |
| return False | |
| self.wait_until(wait_func, timeout=timeout, sleep=sleep) | |
| quorums = self.nodes[0].quorum("list")[llmq_type_name] | |
| return quorum_hash_0 in quorums and quorum_hash_1 in quorums | |
| self.log.info(f"h({self.nodes[0].getblockcount()}) quorums: {self.nodes[0].quorum('list')}") | |
| self.wait_until(wait_func, timeout=timeout, sleep=0.05) | |
| def wait_for_quorums_list(self, quorum_hash_0, quorum_hash_1, nodes, llmq_type_name="llmq_test", timeout=15): | |
| def all_nodes_have_both(): | |
| return all( | |
| quorum_hash_0 in n.quorum("list")[llmq_type_name] and | |
| quorum_hash_1 in n.quorum("list")[llmq_type_name] | |
| for n in nodes | |
| ) | |
| self.log.info( | |
| "quorums(list) h(%d): %s", | |
| self.nodes[0].getblockcount(), | |
| {i: n.quorum('list') for i, n in enumerate(nodes)}, | |
| ) | |
| self.wait_until(all_nodes_have_both, timeout=timeout, sleep=0.05) |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 1848-1848: Too many arguments (6/5)
(R0913)
[refactor] 1848-1848: Too many positional arguments (6/5)
(R0917)
🤖 Prompt for AI Agents
In test/functional/test_framework/test_framework.py around lines 1848 to 1853,
the method wait_for_quorums_list accepts a nodes parameter but only checks the
first node's quorum list, which can cause inaccurate results. Modify the
wait_func to iterate over all nodes in the nodes list and verify that both
quorum_hash_0 and quorum_hash_1 are present in each node's quorum list before
returning True. This ensures the check is performed across all nodes to avoid
false positives or negatives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/feature_llmq_dkgerrors.py (1)
95-96: Replacegetattrwith direct attribute access for better readability.Using
getattrwith constant attribute names is unnecessary and can be simplified.- self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'quorumSig', getattr(qc, 'membersSig')), qc)[1]) - self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'membersSig', getattr(qc, 'quorumSig')), qc)[1]) + self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'quorumSig', qc.membersSig), qc)[1]) + self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'membersSig', qc.quorumSig), qc)[1])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/functional/feature_llmq_dkgerrors.py(3 hunks)test/functional/test_framework/messages.py(1 hunks)test/functional/test_framework/test_framework.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/test_framework/messages.py
🧰 Additional context used
🪛 Ruff (0.11.9)
test/functional/feature_llmq_dkgerrors.py
95-95: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
96-96: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🪛 Pylint (3.3.7)
test/functional/test_framework/test_framework.py
[refactor] 2111-2111: Too many arguments (12/5)
(R0913)
[refactor] 2111-2111: Too many positional arguments (12/5)
(R0917)
[refactor] 2111-2111: Too many local variables (20/15)
(R0914)
[refactor] 2111-2111: Too many statements (59/50)
(R0915)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (5)
test/functional/test_framework/test_framework.py (2)
2111-2111: LGTM! New parameter supports enhanced quorum testing.The
skip_maturityparameter is well-implemented with a sensible default that preserves existing behavior. This enables precise control over quorum readiness for testing invalid commitment scenarios.Note: The method complexity (12 parameters) flagged by static analysis reflects the comprehensive nature of this test framework method rather than an issue with this specific change.
2188-2190: LGTM! Conditional logic correctly implements the skip_maturity feature.The implementation properly controls whether to mine the 8 maturity blocks based on the parameter. The logic preserves backward compatibility (mines blocks by default) while enabling tests to control quorum eligibility timing for enhanced validation scenarios.
test/functional/feature_llmq_dkgerrors.py (3)
6-15: LGTM! Imports are correctly added for the new test functionality.The new imports are appropriate for the block manipulation and quorum commitment testing being added.
82-107: LGTM! Well-structured test method for quorum commitment validation.The method correctly:
- Mines a quorum with controlled maturity
- Sets up test environment by temporarily invalidating the block
- Tests multiple invalid quorum commitment scenarios
- Properly restores the block state and continues execution
This addresses the PR objective of detecting BlsChecker component errors through functional testing.
108-133: LGTM! The helper method correctly implements invalid block testing.The method properly:
- Deep copies blocks to avoid side effects
- Targets quorum commitment transactions (type 6)
- Applies transformations to create invalid conditions
- Rehashes transactions after payload modification (addressing previous review feedback)
- Validates expected error responses
This provides a solid foundation for testing various invalid quorum commitment scenarios.
kwvg
left a comment
kwvg
left a comment
There was a problem hiding this comment.
Mostly LGTM, minor suggestions
| self.nodes[0].invalidateblock(best) | ||
|
|
||
| self.test_invalid(block, 'bad-qc-commitment-type', lambda qc : (setattr(qc, 'llmqType', 77), qc)[1]) | ||
| self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'llmqType', 106), qc)[1]) |
There was a problem hiding this comment.
Can we have enum class LLMQType constants mirrored in Python and have this expressed as subtractions of known-good values? Not a dealbreaker but worth considering since it would convey what is a valid value and what we did to make it invalid.
There was a problem hiding this comment.
we don't have it currently.
Either constant is defined when it's used multiple times over test:
test/functional/feature_asset_locks.py:llmq_type_test = 106 # LLMQType::LLMQ_TEST_PLATFORM
Or just directly specified llmq_type:
test/functional/feature_llmq_evo.py: self.test_quorum_members_are_evo_nodes(quorum_i_hash, llmq_type=106)
There was a problem hiding this comment.
If there's additional iteration on this PR or work on resolving the TODOs in a future PR, do consider defining them and adjusting tests to use them
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/feature_llmq_dkgerrors.py (1)
92-98: Fix style issues and improve code claritySeveral style and clarity improvements can be made:
- Remove unnecessary whitespace before colons in lambda expressions
- Replace
getattrwith direct property access for better readability- Consider using descriptive constants instead of magic numbers
- self.test_invalid(block, 'bad-qc-commitment-type', lambda qc : (setattr(qc, 'llmqType', 77), qc)[1]) - self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'llmqType', 106), qc)[1]) + self.test_invalid(block, 'bad-qc-commitment-type', lambda qc: (setattr(qc, 'llmqType', 77), qc)[1]) + self.test_invalid(block, 'bad-qc-invalid', lambda qc: (setattr(qc, 'llmqType', 106), qc)[1]) # TODO: test quorumIndex for rotation quorums # self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'quorumIndex', 2), qc)[1]) - self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'quorumSig', getattr(qc, 'membersSig')), qc)[1]) - self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'membersSig', getattr(qc, 'quorumSig')), qc)[1]) - self.test_invalid(block, 'bad-qc-invalid', lambda qc : (setattr(qc, 'quorumPublicKey', b'\x00' * 48), qc)[1]) + self.test_invalid(block, 'bad-qc-invalid', lambda qc: (setattr(qc, 'quorumSig', qc.membersSig), qc)[1]) + self.test_invalid(block, 'bad-qc-invalid', lambda qc: (setattr(qc, 'membersSig', qc.quorumSig), qc)[1]) + self.test_invalid(block, 'bad-qc-invalid', lambda qc: (setattr(qc, 'quorumPublicKey', b'\x00' * 48), qc)[1])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/functional/feature_llmq_dkgerrors.py(3 hunks)test/functional/test_framework/test_framework.py(2 hunks)
🧰 Additional context used
🧠 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.
test/functional/feature_llmq_dkgerrors.py (2)
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#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
test/functional/test_framework/test_framework.py (1)
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.
🧬 Code Graph Analysis (1)
test/functional/test_framework/test_framework.py (1)
test/functional/test_framework/test_node.py (1)
generate(343-345)
🪛 Ruff (0.11.9)
test/functional/feature_llmq_dkgerrors.py
96-96: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
97-97: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
🪛 Flake8 (7.2.0)
test/functional/feature_llmq_dkgerrors.py
[error] 92-92: whitespace before ':'
(E203)
[error] 93-93: whitespace before ':'
(E203)
[error] 96-96: whitespace before ':'
(E203)
[error] 97-97: whitespace before ':'
(E203)
[error] 98-98: whitespace before ':'
(E203)
🪛 Pylint (3.3.7)
test/functional/test_framework/test_framework.py
[refactor] 2111-2111: Too many arguments (12/5)
(R0913)
[refactor] 2111-2111: Too many positional arguments (12/5)
(R0917)
[refactor] 2111-2111: Too many local variables (20/15)
(R0914)
[refactor] 2111-2111: Too many statements (59/50)
(R0915)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / 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
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (5)
test/functional/test_framework/test_framework.py (1)
2111-2111: LGTM! Clean implementation of skip_maturity functionality.The addition of the
skip_maturityparameter is well-implemented with a sensible default that maintains backward compatibility. The conditional logic correctly skips the maturity blocks when needed, and the updated log message appropriately reflects the quorum's maturity status.This change effectively supports the PR's objective of testing quorum commitment validation by allowing the creation of quorums that haven't yet reached signing eligibility.
Also applies to: 2188-2193
test/functional/feature_llmq_dkgerrors.py (4)
6-16: LGTM: Well-organized importsAll new imports are necessary for the block manipulation and payload testing functionality being added.
32-35: LGTM: Good integration with existing test flowTesting quorum commitment validation early in the test sequence before other DKG errors is a logical approach.
83-107: LGTM: Well-structured quorum commitment validation testsThe method provides comprehensive testing of invalid quorum commitment scenarios. The approach of invalidating/reconsidering blocks to test different scenarios is sound.
109-134: LGTM: Well-implemented block manipulation helperThe helper method correctly handles all aspects of block manipulation:
- Deep copying to preserve the original
- Finding and modifying quorum commitment transactions
- Proper rehashing after payload changes (addressing the past review concern)
- Comprehensive error validation
The implementation properly addresses the transaction hash staleness issue that was flagged in previous reviews.
c9ef70a tests: add is_mature for quorum generation logs (Konstantin Akimov) 59060b5 fmt: order imports and fix gap in feature_llmq_dkgerrors.py (Konstantin Akimov) 58377f8 test: added functional tests for invalid CQuorumCommitment (Konstantin Akimov) bb0b8b0 test: add serialization/deserialization of CFinalCommitmentPayload (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented As I noticed implementing dashpay#6692 if BlsChecker works incorrectly it won't be caught by unit or functional tests. See also dashpay#6692 (comment) how 6692 has been tested without this PR. ## What was done? This PR introduces new functional tests to validated that `llmqType`, `membersSig`, `quorumSig` and `quorumPublicKey` are indeed validated by Dash Core as part of consensus. ## How Has This Been Tested? See changes in `feature_llmq_dkgerrors.py` ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] 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: kwvg: utACK c9ef70a UdjinM6: utACK c9ef70a Tree-SHA512: ad61f8c845f6681765105224b2a84e0b206791e2c9a786433b9aa91018ab44c1fa764528196fd079f42f08a55794756ba8c9249c6eb10af6fe97c33fa4757f44
c9ef70a tests: add is_mature for quorum generation logs (Konstantin Akimov) 59060b5 fmt: order imports and fix gap in feature_llmq_dkgerrors.py (Konstantin Akimov) 58377f8 test: added functional tests for invalid CQuorumCommitment (Konstantin Akimov) bb0b8b0 test: add serialization/deserialization of CFinalCommitmentPayload (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented As I noticed implementing dashpay#6692 if BlsChecker works incorrectly it won't be caught by unit or functional tests. See also dashpay#6692 (comment) how 6692 has been tested without this PR. ## What was done? This PR introduces new functional tests to validated that `llmqType`, `membersSig`, `quorumSig` and `quorumPublicKey` are indeed validated by Dash Core as part of consensus. ## How Has This Been Tested? See changes in `feature_llmq_dkgerrors.py` ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] 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: kwvg: utACK c9ef70a UdjinM6: utACK c9ef70a Tree-SHA512: ad61f8c845f6681765105224b2a84e0b206791e2c9a786433b9aa91018ab44c1fa764528196fd079f42f08a55794756ba8c9249c6eb10af6fe97c33fa4757f44
4 participants
Issue being fixed or feature implemented
As I noticed implementing #6692 if BlsChecker works incorrectly it won't be caught by unit or functional tests. See also #6692 (comment) how 6692 has been tested without this PR.
What was done?
This PR introduces new functional tests to validated that
llmqType,membersSig,quorumSigandquorumPublicKeyare indeed validated by Dash Core as part of consensus.How Has This Been Tested?
See changes in
feature_llmq_dkgerrors.pyBreaking Changes
N/A
Checklist: