test: speed-up feature_llmq_simplepose.py for 33% #6645
Conversation
….py to test_framework/masternodes.py
…h test It used to be tested only in feature_llmq_simplepose.py The functional test feature_llmq_simplepose.py one of the slowest and mining 3 quorums less make it significantly faster
|
This pull request has conflicts, please rebase. |
WalkthroughThe updates introduce several coordinated changes across functional test scripts and supporting test framework modules. In the masternode test framework, two new utility functions, 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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:
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: 0
🧹 Nitpick comments (4)
test/functional/test_framework/masternodes.py (2)
7-11: Simplify the function using a direct boolean returnThe function can be simplified by returning the boolean expression directly.
def check_punished(node, mn): info = node.protx('info', mn.proTxHash) - if info['state']['PoSePenalty'] > 0: - return True - return False + return info['state']['PoSePenalty'] > 0🧰 Tools
🪛 Ruff (0.8.2)
9-11: Return the condition
info['state']['PoSePenalty'] > 0directlyReplace with
return info['state']['PoSePenalty'] > 0(SIM103)
13-17: Simplify the function using a direct boolean returnSimilarly, this function can also be simplified by returning the boolean expression directly.
def check_banned(node, mn): info = node.protx('info', mn.proTxHash) - if info['state']['PoSeBanHeight'] != -1: - return True - return False + return info['state']['PoSeBanHeight'] != -1🧰 Tools
🪛 Ruff (0.8.2)
15-17: Return the condition
info['state']['PoSeBanHeight'] != -1directlyReplace with
return info['state']['PoSeBanHeight'] != -1(SIM103)
test/functional/feature_llmq_simplepose.py (2)
21-21: Remove unnecessary f-string prefix.The f-string on line 21 doesn't contain any placeholders, so the
fprefix can be removed.- self.extra_args = [[ f'-testactivationheight=dip0024@9999' ]] * 6 + self.extra_args = [[ '-testactivationheight=dip0024@9999' ]] * 6🧰 Tools
🪛 Ruff (0.8.2)
21-21: f-string without any placeholders
Remove extraneous
fprefix(F541)
28-28: Fix help text for --disable-spork23 option.The help text incorrectly mentions spork21 instead of spork23.
- help="Test with spork21 enabled") + help="Test with spork23 disabled")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/functional/feature_llmq_dkgerrors.py(2 hunks)test/functional/feature_llmq_simplepose.py(7 hunks)test/functional/test_framework/masternodes.py(1 hunks)test/functional/test_framework/test_framework.py(2 hunks)test/functional/test_runner.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/functional/feature_llmq_dkgerrors.py (1)
test/functional/test_framework/test_framework.py (1)
mine_quorum(1827-1913)
test/functional/test_framework/test_framework.py (1)
test/functional/test_framework/masternodes.py (2)
check_banned(13-17)check_punished(7-11)
🪛 Ruff (0.8.2)
test/functional/feature_llmq_simplepose.py
21-21: f-string without any placeholders
Remove extraneous f prefix
(F541)
test/functional/test_framework/masternodes.py
9-11: Return the condition info['state']['PoSePenalty'] > 0 directly
Replace with return info['state']['PoSePenalty'] > 0
(SIM103)
15-17: Return the condition info['state']['PoSeBanHeight'] != -1 directly
Replace with return info['state']['PoSeBanHeight'] != -1
(SIM103)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (13)
test/functional/test_runner.py (1)
129-129: LGTM: Properly added spork23-disabled test variantThe new test variant with spork23 disabled is correctly added to the test suite. This supports the PR objective of splitting the test into two versions.
test/functional/test_framework/test_framework.py (2)
29-29: LGTM: Properly imported masternode utility functionsThe new masternode utility functions are correctly imported from the new module.
1909-1912: LGTM: Added validation for masternode states after quorum miningThis enhancement ensures that after mining a quorum, all masternodes that should be valid are not punished or banned. This improves test validation and aligns with the PR objective of enhancing the
mine_quorumhelper.test/functional/feature_llmq_dkgerrors.py (2)
27-28: LGTM: Defined valid masternodes for testingGood addition of the
mninfos_validvariable that excludes the first masternode (which will be simulating errors) from the validation checks.
31-31: LGTM: Consistently added valid masternode list to all quorum mining callsAll
mine_quorumcalls have been consistently updated to include themninfos_validparameter, ensuring that masternodes simulating errors aren't checked for punishment or banning inappropriately.Also applies to: 37-37, 42-42, 51-51, 58-58, 64-64, 70-70
test/functional/feature_llmq_simplepose.py (8)
15-15: Good use of shared helper functions.Importing external helper functions for checking masternode state improves code reuse and consistency across test files. This aligns well with the PR's goal of making the test more maintainable.
26-28: Well-structured CLI option for test splitting.Adding the
--disable-spork23option enables running the test in two distinct modes, which is the key to the 33% speedup mentioned in the PR objectives. This approach allows testing both with and without PoSe banning while reducing the number of quorums generated per run.
31-34: Clear spork configuration based on CLI option.The implementation properly sets SPORK_23_QUORUM_POSE to either disabled (far future timestamp) or enabled (0) based on the command-line option. This is a clean way to control the test behavior.
50-56: Effective conditional test execution.This conditional logic ensures appropriate test paths are executed based on the spork state, which helps achieve the performance improvements mentioned in the PR objectives. When the spork is disabled, the test verifies that no banning occurs for non-reachable nodes.
61-65: Logical test flow for outdated protocol nodes.Similar to the non-reachable node tests, this ensures appropriate testing paths for outdated protocol nodes based on the spork state. This conditional execution helps reduce unnecessary quorum generations when testing with spork disabled.
91-92: Good function signature enhancement.The updated signature for
test_no_banningmakes the function more flexible by accepting aninvalidate_procparameter, allowing it to be used with different node invalidation strategies. This reduces code duplication.
160-162: Good defensive programming.Adding assertions to verify that all nodes start in a non-punished, non-banned state ensures the test begins from a clean, known state. This helps prevent misleading test results and improves reliability.
182-182: Consistent use of helper functions.The code consistently uses the imported
check_bannedandcheck_punishedhelper functions throughout the test, improving maintainability and code reuse. This aligns with good software engineering practices.Also applies to: 195-195, 204-204, 224-224
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/functional/test_framework/masternodes.py (1)
11-14:⚠️ Potential issueFix syntax error in return statement.
There's a syntax error on line 13 - a colon (
:) after the return condition is invalid in Python syntax.- return info['state']['PoSeBanHeight'] != -1: + return info['state']['PoSeBanHeight'] != -1🧰 Tools
🪛 Ruff (0.8.2)
13-13: SyntaxError: Expected a statement
13-14: SyntaxError: Expected a statement
🧹 Nitpick comments (1)
test/functional/test_framework/masternodes.py (1)
1-14: Consider adding error handling for RPC calls.The functions assume that the
protx('info', proTxHash)call will always succeed and the expected fields will be present in the response. Consider adding error handling to make these functions more robust.def check_punished(node, mn): - info = node.protx('info', mn.proTxHash) - return info['state']['PoSePenalty'] > 0 + try: + info = node.protx('info', mn.proTxHash) + return info['state'].get('PoSePenalty', 0) > 0 + except Exception as e: + node.log.error(f"Error checking punishment status: {str(e)}") + return False def check_banned(node, mn): - info = node.protx('info', mn.proTxHash) - return info['state']['PoSeBanHeight'] != -1 + try: + info = node.protx('info', mn.proTxHash) + return info['state'].get('PoSeBanHeight', -1) != -1 + except Exception as e: + node.log.error(f"Error checking ban status: {str(e)}") + return False🧰 Tools
🪛 Ruff (0.8.2)
9-9: SyntaxError: Expected a statement
9-10: SyntaxError: Expected a statement
13-13: SyntaxError: Expected a statement
13-14: SyntaxError: Expected a statement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/functional/feature_llmq_simplepose.py(7 hunks)test/functional/test_framework/masternodes.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/feature_llmq_simplepose.py
🧰 Additional context used
🪛 Ruff (0.8.2)
test/functional/test_framework/masternodes.py
9-9: SyntaxError: Expected a statement
9-10: SyntaxError: Expected a statement
13-13: SyntaxError: Expected a statement
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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
🔇 Additional comments (1)
test/functional/test_framework/masternodes.py (1)
1-6: LGTM: Good documentation and license header.The file header with license information and the docstring clearly explain the purpose of these utility functions.
knst
force-pushed
the
test-feature-llmq-simplepose
branch
from
dd130a3 to
9e3a0d6
Compare
knst
force-pushed
the
test-feature-llmq-simplepose
branch
from
9e3a0d6 to
7057df4
Compare
3 participants
Issue being fixed or feature implemented
The functional test
feature_llmq_simplepose.pyis the slowest one and its run alone is taking longer time than all other functional tests all together (running in simultaneous jobs).What was done?
Done 2 major refactors that improved performance:
mine_quorumhelper instead dedicated quorum generations infeature_llmq_simplepose.pyIt should also increase reliability of
feature_llmq_simplepose.pybecause it generate less quorums in each run and in total.How Has This Been Tested?
Total time of test-suite is dropped from 294seconds to just 249seconds (while running locally in 30 parallel jobs
-j30built with-O3):Also there's no more outsiders amongs functional tests: the slowest functional tests are even slow:
Breaking Changes
N/A
Checklist: