test: consolidate masternode info tracking (MasternodeInfo) in functional tests, implement helpers, add type annotations
#6718
Conversation
WalkthroughThe changes refactor the functional test suite for Dash by standardizing how masternode node objects are accessed and enhancing type safety. The 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (19)
🚧 Files skipped from review as they are similar to previous changes (12)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code Graph Analysis (3)test/functional/feature_governance.py (1)
test/functional/feature_llmq_singlenode.py (2)
test/functional/feature_mnehf.py (2)
🪛 Ruff (0.11.9)test/functional/feature_governance.py12-12: Remove unused import: (F401) test/functional/feature_llmq_connections.py17-17: Remove unused import: (F401) 59-59: Function definition does not bind loop variable (B023) 64-64: Function definition does not bind loop variable (B023) 69-69: Function definition does not bind loop variable (B023) 89-90: Use a single (SIM117) 119-119: Function definition does not bind loop variable (B023) test/functional/feature_llmq_simplepose.py230-230: Function definition does not bind loop variable (B023) test/functional/feature_llmq_singlenode.py19-19: Remove unused import: (F401) test/functional/feature_mnehf.py21-21: Remove unused import: (F401) test/functional/interface_zmq_dash.py17-17: Remove unused import: (F401) test/functional/test_framework/test_framework.py2102-2102: Do not Replace (B011) 🪛 Pylint (3.3.7)test/functional/interface_zmq_dash.py[error] 224-224: Instance of 'DashZMQTest' has no 'quorum_hash' member (E1101) [error] 225-225: Instance of 'DashZMQTest' has no 'quorum_type' member (E1101) test/functional/test_framework/test_framework.py[refactor] 1183-1186: Unnecessary "else" after "raise", remove the "else" and de-indent the code inside it (R1720) [refactor] 1192-1192: Too many arguments (6/5) (R0913) [refactor] 1192-1192: Too many positional arguments (6/5) (R0917) [refactor] 1379-1379: Too many local variables (16/15) (R0914) [refactor] 2117-2117: Too many arguments (6/5) (R0913) [refactor] 2117-2117: Too many positional arguments (6/5) (R0917) ⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (31)
✨ 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: 5
🧹 Nitpick comments (2)
test/functional/feature_llmq_connections.py (1)
89-90: Consider simplifying nested with statements.The nested
withstatements can be combined for better readability.- with mn.get_node(self).assert_debug_log(['removing masternodes quorum connections']): - with mn.get_node(self).assert_debug_log(['keeping mn quorum connections']): + with (mn.get_node(self).assert_debug_log(['removing masternodes quorum connections']), + mn.get_node(self).assert_debug_log(['keeping mn quorum connections'])):🧰 Tools
🪛 Ruff (0.11.9)
89-90: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
test/functional/feature_llmq_simplepose.py (1)
230-230: Address potential loop variable binding issue.The lambda function captures the loop variable
mn, which could potentially cause issues if the lambda were executed after the loop completes. However, sincewait_untilcalls the lambda immediately within the same iteration, this is likely not a practical problem in this context.For better clarity and to avoid static analysis warnings, consider capturing the variable explicitly:
- self.wait_until(lambda: mn.get_node(self).getconnectioncount() == 0) + self.wait_until(lambda mn=mn: mn.get_node(self).getconnectioncount() == 0)🧰 Tools
🪛 Ruff (0.11.9)
230-230: Function definition does not bind loop variable
mn(B023)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
test/functional/feature_asset_locks.py(2 hunks)test/functional/feature_dip3_deterministicmns.py(7 hunks)test/functional/feature_dip3_v19.py(4 hunks)test/functional/feature_dip4_coinbasemerkleroots.py(3 hunks)test/functional/feature_governance.py(2 hunks)test/functional/feature_llmq_connections.py(6 hunks)test/functional/feature_llmq_data_recovery.py(5 hunks)test/functional/feature_llmq_dkgerrors.py(1 hunks)test/functional/feature_llmq_evo.py(5 hunks)test/functional/feature_llmq_rotation.py(3 hunks)test/functional/feature_llmq_signing.py(6 hunks)test/functional/feature_llmq_simplepose.py(6 hunks)test/functional/feature_llmq_singlenode.py(4 hunks)test/functional/feature_mnehf.py(4 hunks)test/functional/interface_zmq_dash.py(4 hunks)test/functional/p2p_quorum_data.py(10 hunks)test/functional/rpc_mnauth.py(2 hunks)test/functional/rpc_quorum.py(2 hunks)test/functional/test_framework/test_framework.py(21 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
test/functional/feature_dip3_v19.py (1)
test/functional/test_framework/test_framework.py (4)
DashTestFramework(1214-2155)MasternodeInfo(1138-1212)dynamically_add_masternode(1339-1375)dynamically_evo_update_service(1410-1434)
test/functional/feature_llmq_connections.py (1)
test/functional/test_framework/test_framework.py (9)
DashTestFramework(1214-2155)MasternodeInfo(1138-1212)get_node(1209-1212)wait_for_mnauth(2147-2155)mine_quorum(1865-1951)get_quorum_masternodes(2102-2107)wait_until(900-901)bump_mocktime(873-890)mine_cycle_quorum(1953-2075)
test/functional/feature_llmq_rotation.py (1)
test/functional/test_framework/test_framework.py (1)
get_node(1209-1212)
test/functional/feature_governance.py (1)
test/functional/test_framework/test_framework.py (1)
MasternodeInfo(1138-1212)
test/functional/feature_dip4_coinbasemerkleroots.py (1)
test/functional/test_framework/test_framework.py (2)
DashTestFramework(1214-2155)MasternodeInfo(1138-1212)
test/functional/feature_asset_locks.py (2)
test/functional/test_framework/test_framework.py (1)
get_node(1209-1212)test/functional/test_framework/util.py (1)
assert_equal(64-69)
test/functional/feature_mnehf.py (2)
test/functional/test_framework/test_framework.py (2)
MasternodeInfo(1138-1212)get_node(1209-1212)test/functional/test_framework/util.py (1)
assert_equal(64-69)
test/functional/feature_llmq_evo.py (1)
test/functional/test_framework/test_framework.py (2)
MasternodeInfo(1138-1212)dynamically_add_masternode(1339-1375)
test/functional/feature_llmq_simplepose.py (3)
test/functional/test_framework/test_framework.py (8)
DashTestFramework(1214-2155)MasternodeInfo(1138-1212)get_node(1209-1212)wait_until(900-901)stop_node(673-675)start_masternode(1522-1527)connect_nodes(695-735)connect_nodes(1239-1246)test/functional/test_framework/util.py (2)
assert_equal(64-69)force_finish_mnsync(525-531)test/functional/test_framework/masternodes.py (2)
check_banned(11-13)check_punished(7-9)
test/functional/rpc_quorum.py (2)
test/functional/test_framework/test_framework.py (1)
MasternodeInfo(1138-1212)test/functional/test_framework/util.py (1)
assert_equal(64-69)
test/functional/feature_llmq_data_recovery.py (3)
test/functional/test_framework/test_framework.py (6)
MasternodeInfo(1138-1212)get_node(1209-1212)restart_node(687-690)wait_until(900-901)connect_nodes(695-735)connect_nodes(1239-1246)test/functional/p2p_quorum_data.py (1)
restart_mn(126-133)test/functional/test_framework/util.py (1)
force_finish_mnsync(525-531)
🪛 Ruff (0.11.9)
test/functional/feature_llmq_connections.py
17-17: test_framework.test_framework.MasternodeInfo imported but unused
Remove unused import: test_framework.test_framework.MasternodeInfo
(F401)
59-59: Function definition does not bind loop variable mn
(B023)
64-64: Function definition does not bind loop variable mn
(B023)
69-69: Function definition does not bind loop variable mn
(B023)
89-90: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
119-119: Function definition does not bind loop variable mn
(B023)
test/functional/feature_governance.py
12-12: test_framework.test_framework.MasternodeInfo imported but unused
Remove unused import: test_framework.test_framework.MasternodeInfo
(F401)
test/functional/feature_mnehf.py
21-21: test_framework.test_framework.MasternodeInfo imported but unused
Remove unused import: test_framework.test_framework.MasternodeInfo
(F401)
test/functional/interface_zmq_dash.py
17-17: test_framework.test_framework.MasternodeInfo imported but unused
Remove unused import: test_framework.test_framework.MasternodeInfo
(F401)
test/functional/feature_llmq_singlenode.py
19-19: test_framework.test_framework.MasternodeInfo imported but unused
Remove unused import: test_framework.test_framework.MasternodeInfo
(F401)
test/functional/feature_llmq_simplepose.py
230-230: Function definition does not bind loop variable mn
(B023)
test/functional/test_framework/test_framework.py
1184-1184: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
2100-2100: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🪛 Pylint (3.3.7)
test/functional/interface_zmq_dash.py
[error] 224-224: Instance of 'DashZMQTest' has no 'quorum_hash' member
(E1101)
[error] 225-225: Instance of 'DashZMQTest' has no 'quorum_type' member
(E1101)
test/functional/test_framework/test_framework.py
[refactor] 1192-1192: Too many arguments (6/5)
(R0913)
[refactor] 1192-1192: Too many positional arguments (6/5)
(R0917)
[refactor] 1377-1377: Too many local variables (16/15)
(R0914)
[refactor] 2115-2115: Too many arguments (6/5)
(R0913)
[refactor] 2115-2115: Too many positional arguments (6/5)
(R0917)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: linux64_tsan-test / Test source
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64_nowallet-test / Test source
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
🔇 Additional comments (54)
test/functional/feature_governance.py (2)
10-13: LGTM! Import consolidation for better type safety.Adding
MasternodeInfoto the import statement supports the type annotation added later in the file.🧰 Tools
🪛 Ruff (0.11.9)
12-12:
test_framework.test_framework.MasternodeInfoimported but unusedRemove unused import:
test_framework.test_framework.MasternodeInfo(F401)
193-193: Good addition of type annotation for clarity.The type annotation comment helps with code readability and IDE support. Note that the static analysis tool incorrectly flags
MasternodeInfoas unused - this is a false positive since type annotation comments are valid usage.test/functional/feature_llmq_dkgerrors.py (1)
30-30: Excellent refactoring to improve encapsulation.Replacing direct
.nodeattribute access with.get_node(self)method calls improves encapsulation and consistency across the codebase. The logic remains unchanged while providing better abstraction for node access.Also applies to: 35-36, 41-41, 49-50, 55-57, 62-63, 68-69
test/functional/feature_dip4_coinbasemerkleroots.py (2)
17-20: LGTM! Added MasternodeInfo import for type annotations.The explicit import supports the type annotations added in the method signatures below.
56-56: Good addition of type annotations for better code clarity.Adding explicit type annotations for
MasternodeInfovariables improves code readability and enables better static type checking.Also applies to: 86-86
test/functional/feature_llmq_singlenode.py (2)
17-20: LGTM! Import consolidation for type safety.Adding
MasternodeInfoto the import statement supports the type annotation used later in the file.🧰 Tools
🪛 Ruff (0.11.9)
19-19:
test_framework.test_framework.MasternodeInfoimported but unusedRemove unused import:
test_framework.test_framework.MasternodeInfo(F401)
61-67: Excellent encapsulation improvement with get_node() method.Replacing direct
.nodeattribute access with.get_node(self)method calls improves encapsulation and follows the established pattern across the codebase. The type annotation comment on line 61 also enhances code clarity. Note that the static analysis tool incorrectly flagsMasternodeInfoas unused - this is a false positive since it's used for the type annotation comment.Also applies to: 118-118, 121-121, 123-123, 128-128, 133-134, 143-143
test/functional/rpc_quorum.py (3)
5-9: LGTM: Import statement properly updated for type annotations.The explicit import of
MasternodeInfoenables proper type annotations and aligns with the refactoring effort.
28-28: Good addition of type annotation.Adding
MasternodeInfotype annotation improves code clarity and enables better static analysis support.
31-31: Excellent improvement: Direct attribute access over dynamic calculation.Replacing
p2p_port(mn.node.index)withmn.nodePorteliminates the need for dynamic port calculation and directly uses the encapsulated port information from theMasternodeInfoobject. This is cleaner and more efficient.test/functional/feature_llmq_rotation.py (3)
143-143: LGTM: Consistent node access pattern.The change from
mn.nodetomn.get_node(self)follows the standardized node access pattern established in this refactoring effort.
196-196: LGTM: Proper node access method usage.Consistent application of the
get_node(self)method for accessing masternode nodes.
401-401: LGTM: Standardized node retrieval.The node access pattern is correctly updated to use the encapsulated
get_node(self)method.test/functional/feature_asset_locks.py (2)
106-106: LGTM: Consistent node access refactoring.The change from
mninfo[0].node.quorum(...)tomninfo[0].get_node(self).quorum(...)properly follows the new standardized node access pattern while maintaining identical functionality.
371-371: LGTM: Proper node method access.Correctly updated to use the
get_node(self)method for accessing the quorum RPC, consistent with the refactoring approach.test/functional/rpc_mnauth.py (3)
8-11: LGTM: Proper import for type annotations.Explicitly importing
MasternodeInfoenables type annotations and maintains consistency with the refactoring effort.
26-27: Excellent approach: Extract node once and reuse.Adding the type annotation and extracting the node using
get_node(self)once is more efficient than calling it multiple times throughout the method. This is a clean implementation of the new access pattern.
28-55: LGTM: Consistent use of extracted node variable.All node method calls correctly use the extracted
nodevariable instead of accessing throughmasternode.node. This maintains the same functionality while following the standardized access pattern.test/functional/feature_dip3_v19.py (2)
17-20: LGTM! Import statement properly updated.The addition of
MasternodeInfoto the import statement is consistent with its usage in type annotations throughout the file.
84-94: LGTM! Type annotations improve code clarity.The explicit type annotations for
MasternodeInfovariables enhance code readability and provide better static analysis support. All annotations are consistent with the return types ofdynamically_add_masternode.Also applies to: 107-107, 135-135
test/functional/feature_llmq_signing.py (3)
15-18: LGTM! Import statement properly updated.The addition of
MasternodeInfoto the import statement supports the type annotations used throughout the file.
42-42: LGTM! Consistent refactoring to use get_node() method.The replacement of direct
.nodeattribute access with.get_node(self)method calls is consistent with theMasternodeInfoclass design and improves encapsulation. This change standardizes how test nodes are accessed from masternode info objects.Also applies to: 50-50, 52-52, 54-54, 68-68, 72-72, 75-76, 82-82, 84-85, 98-98, 103-103, 108-108, 113-113, 116-116, 149-149, 173-175, 184-198
49-49: LGTM! Type annotations enhance code clarity.The explicit type annotations for
MasternodeInfovariables improve code readability and static analysis support.Also applies to: 97-97, 101-101, 148-148, 183-183
test/functional/interface_zmq_dash.py (3)
15-18: LGTM! Import statement properly updated.The addition of
MasternodeInfoto the import statement supports the type annotations used throughout the file.🧰 Tools
🪛 Ruff (0.11.9)
17-17:
test_framework.test_framework.MasternodeInfoimported but unusedRemove unused import:
test_framework.test_framework.MasternodeInfo(F401)
172-172: LGTM! Consistent refactoring to use get_node() method.The replacement of direct
.nodeattribute access with.get_node(self)method calls follows the same pattern as other files in this PR and improves encapsulation.Also applies to: 195-195, 225-225
224-224: Fix type annotation consistency.The type annotation is correct for the loop variable.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 224-224: Instance of 'DashZMQTest' has no 'quorum_hash' member
(E1101)
test/functional/feature_llmq_connections.py (3)
15-18: LGTM! Import statement properly updated.The addition of
MasternodeInfoto the import statement supports the type annotations used throughout the file.🧰 Tools
🪛 Ruff (0.11.9)
17-17:
test_framework.test_framework.MasternodeInfoimported but unusedRemove unused import:
test_framework.test_framework.MasternodeInfo(F401)
38-38: LGTM! Consistent refactoring to use get_node() method.The replacement of direct
.nodeattribute access with.get_node(self)method calls is consistent with the broader refactoring and improves encapsulation.Also applies to: 52-52, 87-87, 89-92, 103-105, 117-117, 121-121, 127-127
86-86: LGTM! Type annotations enhance code clarity.The explicit type annotations for
MasternodeInfovariables improve code readability and static analysis support.Also applies to: 102-102, 116-118, 120-120
test/functional/feature_mnehf.py (4)
19-22: LGTM! Import statement properly updated for MasternodeInfo.The import has been correctly updated to include
MasternodeInfoalongsideDashTestFramework, aligning with the refactoring to use the enhancedMasternodeInfoclass.🧰 Tools
🪛 Ruff (0.11.9)
21-21:
test_framework.test_framework.MasternodeInfoimported but unusedRemove unused import:
test_framework.test_framework.MasternodeInfo(F401)
44-45: LGTM! Proper migration to new MasternodeInfo API.The code correctly:
- Adds type annotation for the loop variable
- Replaces
mn.node.indexwithmn.nodeIdxfor node indexingThis aligns with the enhanced
MasternodeInfoclass design.
57-57: LGTM! Node access properly updated.The direct node access
self.mninfo[0].node.quorum(...)has been correctly replaced withself.mninfo[0].get_node(self).quorum(...), following the new encapsulated approach.
143-143: LGTM! Consistent node access pattern.This line correctly follows the same pattern as line 57, ensuring consistent usage of the
get_node(self)method throughout the file.test/functional/feature_llmq_evo.py (5)
13-13: LGTM! Optional import added for type annotations.The
Optionalimport is properly added to support the return type annotation in theget_mn_payee_for_blockmethod.
18-21: LGTM! Import statement correctly updated.The import has been properly updated to include
MasternodeInfofor use in type annotations throughout the file.
79-79: LGTM! Excellent type annotation additions.The type annotations for
evo_info,current_evo, andpayeevariables significantly improve code clarity and enable better static analysis. All annotations correctly useMasternodeInfo.Also applies to: 118-118, 122-122
161-161: LGTM! Method return type properly annotated.The
get_mn_payee_for_blockmethod now correctly declares its return type asOptional[MasternodeInfo], which matches the implementation that can return either aMasternodeInfoinstance orNone.
166-166: LGTM! Loop variable type annotations improve clarity.The inline type comments for loop variables help clarify that the iterations are over
MasternodeInfoinstances, enhancing code readability and static analysis support.Also applies to: 177-177
test/functional/p2p_quorum_data.py (4)
13-16: LGTM! Import statement properly updated.The import has been correctly updated to include
MasternodeInfo, supporting the comprehensive refactoring throughout this file.
126-133: LGTM! Method signature and implementation properly updated.The
restart_mnmethod correctly:
- Adds type annotation for the
mnparameter asMasternodeInfo- Replaces
mn.node.indexwithmn.nodeIdxfor node indexing- Uses
mn.get_node(self)for node access- Maintains the same functionality with improved encapsulation
147-188: LGTM! Consistent node access pattern throughout test_basics.All direct node accesses have been properly replaced with the
get_node(self)method, maintaining consistency with the newMasternodeInfoAPI. The changes preserve the original test logic while improving encapsulation.
414-416: LGTM! Local variable type annotations improve clarity.The type annotations for
mn1,mn2, andmn3variables clearly indicate they areMasternodeInfoinstances, enhancing code readability and enabling better static analysis.test/functional/feature_llmq_simplepose.py (3)
16-20: LGTM! Import statement properly updated.The import has been correctly updated to include
MasternodeInfo, supporting the type annotations and method calls throughout the file.
71-74: LGTM! Method signatures and implementations properly updated.All three methods (
isolate_mn,close_mn_port,force_old_mn_proto) correctly:
- Add type annotations for the
mnparameter asMasternodeInfo- Replace direct node access with
mn.get_node(self)method calls- Use
mn.nodeIdxfor node indexing- Maintain the same functionality with improved encapsulation
Also applies to: 76-86, 88-93
211-211: LGTM! Direct port access improves consistency.Using
mn.nodePortdirectly instead of a utility function withmn.nodeIdxis more consistent with the encapsulatedMasternodeInfodesign and eliminates unnecessary indirection.test/functional/feature_llmq_data_recovery.py (3)
7-12: Good addition of type annotations and explicit imports.The explicit import of
MasternodeInfoand addition of type annotations improves code clarity and enables better IDE support.
35-50: Consistent refactoring to use MasternodeInfo methods.The changes properly utilize the new
MasternodeInfostructure by:
- Using
mn.nodeIdxinstead ofmn.node.index- Calling
mn.get_node(self)instead of directmn.nodeaccessThis improves encapsulation and maintains consistency with the framework refactoring.
80-94: Well-structured return type annotations.The addition of return type annotations (
Optional[MasternodeInfo],List[Optional[MasternodeInfo]],List[MasternodeInfo]) enhances code documentation and type safety.test/functional/feature_dip3_deterministicmns.py (3)
11-20: Good use of constants and explicit imports.Importing
MASTERNODE_COLLATERALeliminates magic numbers and improves maintainability. The explicit import ofMasternodeInfoclarifies dependencies.
224-229: Well-structured masternode preparation with MasternodeInfo.The
prepare_mnmethod now properly creates and initializes aMasternodeInfoinstance with:
- Correct evo/legacy flags based on softfork status
- Generated addresses
- Proper parameters including operator reward and node port
This encapsulation makes the code more maintainable and type-safe.
231-262: Excellent refactoring of masternode lifecycle methods.The refactoring of
create_mn_collateral,register_fund_mn,register_mn,start_mn, andspend_mn_collateralto useMasternodeInfomethods and attributes:
- Improves code organization
- Ensures consistent collateral value calculation
- Properly encapsulates masternode state
The assertion in
start_mn(line 252) is a good defensive programming practice.test/functional/test_framework/test_framework.py (3)
1138-1187: Excellent refactoring of MasternodeInfo class.The new
MasternodeInfoclass is well-structured with:
- Comprehensive type annotations for all attributes
- Clear separation between evo and regular masternodes
- Useful helper methods for address generation and collateral management
- Proper encapsulation of masternode state
This significantly improves code maintainability and type safety.
🧰 Tools
🪛 Ruff (0.11.9)
1184-1184: Do not
assert False(python -Oremoves these calls), raiseAssertionError()Replace
assert False(B011)
🪛 Pylint (3.3.7)
[refactor] 1138-1138: Too many instance attributes (16/7)
(R0902)
1209-1212: Well-designed node access method.The
get_nodemethod with proper assertions ensures:
nodeIdxis set before accessing the node- The node exists in the test framework
- Clear error messages for debugging
This defensive programming approach prevents common errors.
1239-1246: Good encapsulation in connect_nodes method.The refactoring to use
mn.get_node(self)instead of directmn.nodeaccess maintains consistency with the new MasternodeInfo encapsulation pattern throughout the codebase.
| for mn in self.get_quorum_masternodes(self.quorum_hash): # type: MasternodeInfo | ||
| mn.get_node(self).quorum("sign", self.quorum_type, sign_id, sign_msg_hash) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify missing attributes: quorum_hash and quorum_type.
The code references self.quorum_hash and self.quorum_type but these attributes don't appear to be defined in the DashZMQTest class.
🏁 Script executed:
#!/bin/bash
# Description: Search for quorum_hash and quorum_type attribute definitions in DashZMQTest class
# Expected: Find where these attributes should be defined or initialized
# Search for quorum_hash and quorum_type in the test file
rg -A 3 -B 3 "quorum_hash|quorum_type" test/functional/interface_zmq_dash.py
# Search for these attributes in other test files to understand the pattern
rg -A 2 -B 2 "self\.quorum_hash|self\.quorum_type" test/functional/
# Check the DashTestFramework base class for these attributes
rg -A 3 -B 3 "quorum_hash|quorum_type" test/functional/test_framework/test_framework.pyLength of output: 11281
🏁 Script executed:
#!/bin/bash
# Inspect the DashZMQTest class definition and its initial attributes
rg -n "^class " test/functional/interface_zmq_dash.py
sed -n '1,200p' test/functional/interface_zmq_dash.pyLength of output: 7384
🏁 Script executed:
#!/bin/bash
# Locate set_dash_llmq_test_params definition
rg -n "def set_dash_llmq_test_params" test/functional/test_framework/test_framework.py
# Locate mine_cycle_quorum definition
rg -n "def mine_cycle_quorum" -n test/functional/test_framework/test_framework.py
rg -A 20 -B 0 "def mine_cycle_quorum" test/functional/test_framework/test_framework.py
# Search for assignments to self.quorum_hash and self.quorum_type in the framework
rg -n "self\.quorum_hash" test/functional/test_framework/test_framework.py
rg -n "self\.quorum_type" test/functional/test_framework/test_framework.pyLength of output: 1597
Initialize missing quorum attributes before use
The test_recovered_signature_publishers method references self.quorum_hash and self.quorum_type (lines 224–225) but neither is ever initialized in DashZMQTest, leading to an AttributeError when this code runs. You should set these attributes before invoking get_quorum_masternodes and the quorum("sign", …) call. For example:
• After generating the quorum in run_test, capture its hash and type:
# in run_test, instead of:
self.mine_cycle_quorum()
# do:
self.quorum_hash, self.quorum_type = self.mine_cycle_quorum()• Or explicitly initialize them in set_test_params (or right before the loop):
self.quorum_type = 103 # llmq_test_dip0024
self.quorum_hash = self.nodes[0].quorum("list")[“llmq_test_dip0024”][-1]Then update the loop to use the initialized values:
- for mn in self.get_quorum_masternodes(self.quorum_hash):
+ for mn in self.get_quorum_masternodes(self.quorum_hash):
mn.get_node(self).quorum("sign", self.quorum_type, sign_id, sign_msg_hash)• File: test/functional/interface_zmq_dash.py
Lines: 224–225
🧰 Tools
🪛 Pylint (3.3.7)
[error] 224-224: Instance of 'DashZMQTest' has no 'quorum_hash' member
(E1101)
[error] 225-225: Instance of 'DashZMQTest' has no 'quorum_type' member
(E1101)
🤖 Prompt for AI Agents
In test/functional/interface_zmq_dash.py around lines 224 to 225, the attributes
self.quorum_hash and self.quorum_type are used but never initialized, causing an
AttributeError. To fix this, initialize these attributes before the loop in
test_recovered_signature_publishers or in run_test by assigning self.quorum_hash
and self.quorum_type from the result of self.mine_cycle_quorum() or by setting
them explicitly in set_test_params or just before the loop. This ensures the
quorum hash and type are defined before calling get_quorum_masternodes and
quorum("sign", ...).
| self.log.info("checking that all MNs got probed") | ||
| for mn in self.get_quorum_masternodes(q): | ||
| self.wait_until(lambda: self.get_mn_probe_count(mn.node, q, False) == 4) | ||
| self.wait_until(lambda: self.get_mn_probe_count(mn.get_node(self), q, False) == 4) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix lambda function variable capture issue.
The lambda functions are capturing the loop variable mn by reference, which could lead to incorrect behavior when the lambda is executed after the loop iteration completes.
Apply this fix to properly capture the loop variable:
- for mn in self.get_quorum_masternodes(q):
- self.wait_until(lambda: self.get_mn_probe_count(mn.get_node(self), q, False) == 4)
+ for mn in self.get_quorum_masternodes(q):
+ self.wait_until(lambda mn=mn: self.get_mn_probe_count(mn.get_node(self), q, False) == 4)Apply the same pattern to lines 64 and 69.
Also applies to: 64-64, 69-69
🧰 Tools
🪛 Ruff (0.11.9)
59-59: Function definition does not bind loop variable mn
(B023)
🤖 Prompt for AI Agents
In test/functional/feature_llmq_connections.py at lines 59, 64, and 69, the
lambda functions capture the loop variable mn by reference, causing potential
incorrect behavior after the loop ends. To fix this, modify each lambda to
capture the current value of mn as a default argument in the lambda definition,
ensuring the correct mn is used when the lambda executes.
| for mn in self.mninfo: # type: MasternodeInfo | ||
| mn.get_node(self).setnetworkactive(False) | ||
| for mn in self.mninfo: # type: MasternodeInfo | ||
| self.wait_until(lambda: len(mn.get_node(self).getpeerinfo()) == 0) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix lambda function variable capture issue.
Similar issue with lambda function capturing loop variable by reference.
Apply this fix:
- for mn in self.mninfo: # type: MasternodeInfo
- self.wait_until(lambda: len(mn.get_node(self).getpeerinfo()) == 0)
+ for mn in self.mninfo: # type: MasternodeInfo
+ self.wait_until(lambda mn=mn: len(mn.get_node(self).getpeerinfo()) == 0)📝 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.
| self.wait_until(lambda: len(mn.get_node(self).getpeerinfo()) == 0) | |
| for mn in self.mninfo: # type: MasternodeInfo | |
| self.wait_until(lambda mn=mn: len(mn.get_node(self).getpeerinfo()) == 0) |
🧰 Tools
🪛 Ruff (0.11.9)
119-119: Function definition does not bind loop variable mn
(B023)
🤖 Prompt for AI Agents
In test/functional/feature_llmq_connections.py at line 119, the lambda function
captures the loop variable by reference, causing incorrect behavior. Fix this by
binding the current value of the loop variable as a default argument in the
lambda function to ensure it captures the intended value at each iteration.
kwvg
changed the title
MasternodeInfo) in functional tests, add type annotations, implement helpersMasternodeInfo) in functional tests, implement helpers, add type annotations
MasternodeInfo is used a lot in multiple tests and without annotations, the linter is running blind. Let's try to do something about that. Note, PEP 526 prohibits annotating types in loops, we are using PEP 484 comments there instead.
Relevant when we migrate feature_dip3_deterministicmns away from their own bespoke Masternode class
This makes crafting invalid addresses as part of testing easier, a valid entry will always use the loopback address (127.0.0.1), it's the port that differentiates. So let's only store what we need. Relevant when we migrate feature_dip3_deterministicmns away from their own bespoke Masternode class
node is self.nodes[nodeIdx], so node.index will always be nodeIdx, just fetch nodeIdx directly instead.
We don't register nodeIdx and node at the same time. This creates a problem that we aren't entirely sure if the node is online when we access node since we don't really own the TestNode instance, it is always in the control of BitcoinTestFramework. So let's try to be more explicit about that and add some belt-and-suspenders assertions.
Harmonize with the rest of the codebase
In order to generate BLS keypairs properly, we need to also record if we are running a legacy node, so we store that now too. set_params() is meant to be as flexible as possible, to allow ensuring that details are registered across different steps or all at once.
| self.nodeIdx = nodeIdx | ||
|
|
||
| def get_node(self, test: BitcoinTestFramework) -> TestNode: | ||
| if self.nodeIdx is None: |
There was a problem hiding this comment.
why exactly it's impossible to set .node after adding node to the list of test.nodes? looks a bit complex
| def set_node(self, nodeIdx): | ||
| def set_node(self, nodeIdx, friendlyName=None): | ||
| self.nodeIdx = nodeIdx | ||
| self.friendlyName = friendlyName or f"mn-{'evo' if self.evo else 'reg'}-{self.nodeIdx}" |
There was a problem hiding this comment.
nit: why mn-reg and mn-evo instead mn and evo? they both are reg.
…ow working with `assert_raises_rpc_error`, add ability to withhold submission to all ProTx creation RPCs 28aaf1d test: allow `assert_raises_rpc_error` with helper functions (Kittywhiskers Van Gogh) 5aeec2a test: validate that the 'submit' argument works as intended (Kittywhiskers Van Gogh) 43318a4 rpc: extend the ability to withhold submission to all ProTxes (Kittywhiskers Van Gogh) 968698b test: add helper for `update_service{,_evo}` calls (Kittywhiskers Van Gogh) 1493d67 test: add helper for `update_registrar{,_legacy}` calls (Kittywhiskers Van Gogh) 6ba685f test: add helper for `revoke` calls (Kittywhiskers Van Gogh) aa96ace test: add helper for `register_fund{,_evo,_legacy}` calls (Kittywhiskers Van Gogh) dbce9c9 test: add helper for `register{,_evo,_legacy}` calls (Kittywhiskers Van Gogh) Pull request description: ## Motivation Continuing on the work of [dash#6718](#6718), this pull request aims to do two things: * Leverage the amount of information stored in `MasternodeInfo` to make ProTx creation simpler by only needing to specify the values that need to be _updated_ instead of needing to read most of the fields in `MasternodeInfo` to construct the RPC call * This also comes with the bonus of `MasternodeInfo` knowing _which_ RPC call to make depending on whether the node is legacy regular, basic regular or basic evonode. * As ordering is managed by `register{,_fund}`/`revoke`/`update_{registrar,service}()`, the arguments can be supplied in any desired order (or in RPC call order as the arguments have been arranged in that way except for `submit`, which is placed earlier and must be specified mandatorily). * Allow testing both failure and success cases by introducing both support for `assert_raises_rpc_error` in these functions _and_ implementing support for withholding submission of a ProTx for non-ProRegTxes. These changes benefit the functional tests needed for extended addresses, where we need to test for _changes_ in input validation logic based on deployment status, evaluating both success and failure cases. For instance, this pull request reduces the amount of logic in `Node` (in `rpc_netinfo.py`, introduced in [dash#6674](#6674)) by a fair amount ([diff](https://github.com/dashpay/dash/compare/c788531d35c48820af951799ff8b67ee2dadb8b3..232eb493484b5632b48320d6bbce6c459ee17602#diff-8bacb0d5b8909182c16ac6696c69f805c5cddc3deebc8eb4c0fac77b37aabd5d)) and could see further reduction in future iterations. ## Additional Information * Depends on #6718 * Dependency for #6674 * As `fundsAddr` is an optional field in some ProTx RPCs that trigger different behavior if not supplied ([source](https://github.com/dashpay/dash/blob/ee34525451fbc3efffba6afadf812d2b7f5b7782/src/rpc/evo.cpp#L1237-L1246)), special handling has been introduced to make sure that if this behavior is desired, the argument itself is omitted as replacing it with a blank string does not trigger the intended behavior. * This means that for `revoke()` and `update_registrar()`, transaction withholding (setting `submit` to `false`) is not allowed as this would require setting `fundsAddr` to an empty string, which does not trigger the expected fallback behavior. * This does not extend to `update_service()` where a blank string is submitted as `protx update_service` does not seem to behave adversely when supplied a blank string. * To test that the `submit` argument works as intended in newly updated and existing RPCs, if `submit` is `True` and no expected error code and message are supplied, at random, `submit` is set to `False` and the hex serialized signed transaction output is broadcast using `sendrawtransaction` (see `use_srd` variable). ## Breaking Changes * A new optional field `submit` has been introduced to the `protx revoke`, `protx update_registrar`, `protx update_service` RPCs. It behaves identically to `submit` in `protx register` or `protx register_fund`. ## 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 - [x] 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: PastaPastaPasta: utACK 28aaf1d UdjinM6: utACK 28aaf1d Tree-SHA512: 2346c3ebcd9b4384a4884f0ad2e1f675fc23fc15575973109233770f006e1af98d3c22c0bbb819589a6f97e277a580e4f90b39d940fbe99f8a86961783d72e8e
492d678 test: fix and re-enable (hash/raw)recoveredsig zmq test (UdjinM6) Pull request description: ## Issue being fixed or feature implemented The issue with zmq test was discovered by @coderabbitai while reviewing #6718, see #6718 (comment) ## What was done? Use correct quorum type and hash, re-enable `test_recovered_signature_publishers()` ## How Has This Been Tested? Run `interface_zmq_dash.py` ## Breaking Changes n/a ## 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 492d678 Tree-SHA512: 2ff45dadada5dcc3f7e4404f3223ee598a7676fb272d29072a1865b468ca71c13ab20b25da549f3b36e15fba22c31b908cf65451c5d925d884397fe7a2ff77fd
4 participants
Motivation
While working on functional tests for dash#6666 (which in turn, relies on the foundation built by dash#6674, see
rpc_netinfo.py, source), the existing test infrastructure, while well optimized for testing interactions between masternodes, didn't offer enough flexibility for testing creation of masternodes.The tests that need to be implemented for extended addresses mimic
feature_dip3_deterministicmns.py(source) in objective and while taking cues from it, it was found that instead of theMasternodeInfoused in most of the codebase (source),feature_dip3_deterministicmns.pyimplements its own object,Masternode(source).In a similar vein,
rpc_netinfo.py, as implemented in c788531, implemented its own variant,Node(source) to address the testing needs for extended addresses. It became clear that without additional intervention, the test suite would have three different ways of representing masternode information (MasternodeInfo,Masternode,Node) and that it would be more beneficial to consolidate all three approaches together.So, taking cue from
Node(fromrpc_netinfo.py, not included indevelopas of this writing, source), this pull request aims to clean up, consolidate and improve uponMasternodeInfoin an attempt to remove the need forNode.Additional Information
PEP 526 ("Syntax for Variable Annotations") prohibit annotation of variables defined by a
fororwithstatement (source), to get around that, the syntax for type comments (source) as defined in PEP 484 ("Type Hints") have been used instead.The decision to remove
nodefromMasternodeInfodespite its convenience was rooted in the observation that there are cases wherenodeIdxandnodeare assigned separately (source, source), which highlighted the fact thatMasternodeInfohas no control over the lifetime of theTestNodeinstance as it is ultimately controlled byBitcoinTestFramework.To avoid potential error and to make it syntactically clear that ownership is vests with
BitcoinTestFramework,nodehas been removed and replaced withget_node(), which alongside some basic sanity checks, is just an alias forself.nodes[mn.nodeIdx]but is still less tedious to type or read.The reason why functions like
generate_addresses()accept a separateTestNode(source) is due to the practice of using a single node to instantiate all masternodes instead of keeping them as self-contained instances that mine their own blocks and fund their own collaterals (for instance,DashTestFrameworkusesnodes[0]to setup all the masternodes even if the masternodes themselves had a different index).The decision to replace
addrwithnodePorthas been inspired by the need to register masternodes with non-loopback addresses (like infeature_dip3_deterministicmns.pyorrpc_netinfo.py). As a node that is intended to be used will always need the same standardized loopback address, we can safely resort to only storing the port number and save a fewp2p_port()calls along the way.Breaking Changes
None expected. Affects only functional tests.
Checklist