fix: intermittent error in feature_dip3_v19.py #6703
Conversation
WalkthroughThe changes introduce clarifying comments in the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ 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: 0
🧹 Nitpick comments (1)
test/functional/feature_dip3_v19.py (1)
108-112: Fix logging format and line length issues.The chainlock synchronization logic is correct and implements the necessary sequencing to prevent "bad-cbtx-invalid-clsig" errors. However, there are style issues to address:
Apply this diff to fix the pylint warnings:
- self.log.info("Chainlocks in CbTx can be only in Basic scheme. Wait one...") + self.log.info("Chainlocks in CbTx can be only in Basic scheme. Wait one...") self.wait_until(lambda: self.nodes[0].getbestchainlock()['height'] > 200) - self.log.info(f"block {self.nodes[0].getbestchainlock()['height']} is chainlocked after v19 activation") + height = self.nodes[0].getbestchainlock()['height'] + self.log.info("block %d is chainlocked after v19 activation", height)This addresses:
- Line length violation by breaking the f-string formatting
- Lazy logging format by using % formatting instead of f-strings
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 110-110: Line too long (112/100)
(C0301)
[warning] 110-110: Use lazy % formatting in logging functions
(W1203)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/chainparams.cpp(1 hunks)src/evo/cbtx.h(1 hunks)test/functional/feature_dip3_v19.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/functional/feature_dip3_v19.py (1)
test/functional/test_framework/test_framework.py (3)
delay_v20_and_mn_rr(1219-1221)wait_until(904-905)activate_by_name(1223-1262)
🪛 Pylint (3.3.7)
test/functional/feature_dip3_v19.py
[convention] 110-110: Line too long (112/100)
(C0301)
[warning] 110-110: Use lazy % formatting in logging functions
(W1203)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-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: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (3)
src/evo/cbtx.h (1)
54-56: LGTM! Excellent documentation of a critical issue.This comment effectively documents the BLS signature scheme transition issue that can cause "Invalid Block errors" on devnet/regtest networks. It provides valuable context for understanding why the broader changes in this PR are necessary to prevent chainlock signature scheme conflicts.
src/chainparams.cpp (1)
928-933: Excellent addition of critical safety assertions.These assertions enforce the proper ordering of consensus activation heights and will prevent the configuration errors that lead to "bad-cbtx-invalid-clsig" failures. The explanatory comments clearly document the technical dependencies:
- V20's chainlock signatures in coinbase transactions require V19's BLS scheme changes to be active first
- V20's coinbase transaction features require masternodes (DIP0003) to be operational
- MN_RR's credit pool reallocation depends on V20's credit pool infrastructure
This proactive validation will catch misconfigurations during initialization rather than allowing intermittent runtime failures.
test/functional/feature_dip3_v19.py (1)
50-50: Good implementation of activation delay.The call to
delay_v20_and_mn_rr(height=260)properly implements the fix by ensuring V20 and MN_RR activation is delayed until block 260. This respects the new ordering constraints enforced insrc/chainparams.cppand prevents the timing issues that cause test failures.
|
@knst CI fails |
It can happen on Regtest or Devnet if v20 is just activated but the best
chainlock is still in legacy (pre-v19) scheme.
It causes errors on CI similar to this one:
2025-05-31T10:28:49.821000Z TestFramework (INFO): Checking that protxs with duplicate EvoNodes fields are rejected
2025-05-31T10:29:50.653000Z TestFramework (INFO): dynamically_prepare_masternode failed
2025-05-31T10:30:50.784000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "DASH/test/functional/test_framework/test_framework.py", line 162, in main
self.run_test()
File "DASH/test/functional/feature_dip3_v19.py", line 87, in run_test
self.dynamically_evo_update_service(evo_info_0, 8)
File "DASH/test/functional/test_framework/test_framework.py", line 1384, in dynamically_evo_update_service
tip = self.generate(self.nodes[0], 1)[0]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "DASH/test/functional/test_framework/test_framework.py", line 805, in generate
sync_fun() if sync_fun else self.sync_all()
^^^^^^^^^^^^^^^
File "DASH/test/functional/test_framework/test_framework.py", line 874, in sync_all
self.sync_blocks(nodes)
File "DASH/test/functional/test_framework/test_framework.py", line 840, in sync_blocks
raise AssertionError("Block sync timed out after {}s:{}".format(
AssertionError: Block sync timed out after 60s:
'4227eea732e686fcf608bcbc5525a02336bb69c2eed70e8a59d6439b2122190f'
'4227eea732e686fcf608bcbc5525a02336bb69c2eed70e8a59d6439b2122190f'
'4227eea732e686fcf608bcbc5525a02336bb69c2eed70e8a59d6439b2122190f'
'4227eea732e686fcf608bcbc5525a02336bb69c2eed70e8a59d6439b2122190f'
'4227eea732e686fcf608bcbc5525a02336bb69c2eed70e8a59d6439b2122190f'
'4227eea732e686fcf608bcbc5525a02336bb69c2eed70e8a59d6439b2122190f'
'0ade04ee0ae89d1168792a65c28956557c9f2991d141180b9faf7a9ae986cf24'
2025-05-31T10:30:50.837000Z TestFramework (INFO): Stopping nodes
While underlying error is `bad-cbtx-invalid-clsig`:
2025-05-31T10:28:49.932591Z [util/system.h:57] [error] ERROR: ConnectBlock(DASH): ProcessSpecialTxsInBlock for block 4f8767d0ad2e7c8882ba4b2819a1d8eb29fbc608ff8201ed1c857d6d3ec51cc9 failed with bad-cbtx-invalid-clsig
2025-05-31T10:28:49.932632Z [validationinterface.cpp:254] [BlockChecked] [validation] BlockChecked: block hash=4f8767d0ad2e7c8882ba4b2819a1d8eb29fbc608ff8201ed1c857d6d3ec51cc9 state=bad-cbtx-invalid-clsig
2025-05-31T10:28:49.932660Z [validation.cpp:1634] [InvalidChainFound] InvalidChainFound: invalid block=4f8767d0ad2e7c8882ba4b2819a1d8eb29fbc608ff8201ed1c857d6d3ec51cc9 height=202 log2_work=8.665336 date=2014-12-04T17:46:38Z
2025-05-31T10:28:49.932667Z [validation.cpp:1639] [InvalidChainFound] InvalidChainFound: current best=0ade04ee0ae89d1168792a65c28956557c9f2991d141180b9faf7a9ae986cf24 height=201 log2_work=8.658211 date=2014-12-04T17:36:30Z
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/feature_dip3_v19.py (1)
108-111: Address code style issues in chainlock waiting logic.The chainlock waiting logic is correct and necessary for the fix, but there are style issues to address:
- Line 110 exceeds the 100-character limit (112 characters)
- Use lazy % formatting in logging functions
Apply this diff to fix the style issues:
- self.log.info("Chainlocks in CbTx can be only in Basic scheme. Wait one...") - self.wait_until(lambda: self.nodes[0].getbestchainlock()['height'] > 200) - self.log.info(f"block {self.nodes[0].getbestchainlock()['height']} is chainlocked after v19 activation") + self.log.info("Chainlocks in CbTx can be only in Basic scheme. Wait one...") + self.wait_until(lambda: self.nodes[0].getbestchainlock()['height'] > 200) + chainlock_height = self.nodes[0].getbestchainlock()['height'] + self.log.info("block %d is chainlocked after v19 activation", chainlock_height)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 110-110: Line too long (112/100)
(C0301)
[warning] 110-110: Use lazy % formatting in logging functions
(W1203)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/chainparams.cpp(1 hunks)src/evo/cbtx.h(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/util/setup_common.cpp(1 hunks)test/functional/feature_dip3_v19.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/util/setup_common.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/evo/cbtx.h
- src/chainparams.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/functional/feature_dip3_v19.py (1)
test/functional/test_framework/test_framework.py (3)
delay_v20_and_mn_rr(1219-1221)wait_until(904-905)activate_by_name(1223-1262)
🪛 GitHub Actions: Clang Diff Format Check
src/test/evo_deterministicmns_tests.cpp
[error] 866-867: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix formatting issues.
🪛 Pylint (3.3.7)
test/functional/feature_dip3_v19.py
[convention] 110-110: Line too long (112/100)
(C0301)
[warning] 110-110: Use lazy % formatting in logging functions
(W1203)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_sqlite-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_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (4)
src/test/evo_deterministicmns_tests.cpp (2)
869-869: LGTM! Test setup extension aligns with PR objectives.The addition of
-testactivationheight=v20@500and-testactivationheight=mn_rr@500parameters creates a test scenario where v19, v20, and mn_rr activate simultaneously at block 500. This complements the functional test approach and helps validate the activation order constraints added in the chainparams.
866-867:❓ Verification inconclusive
Fix code formatting issue.
The pipeline failure indicates clang-format differences on these lines. Please run clang-format to fix the formatting issues.
🏁 Script executed:
#!/bin/bash # Check the specific formatting issue around line 869 echo "Current formatting around the constructor:" sed -n '866,870p' src/test/evo_deterministicmns_tests.cpp echo -e "\nRunning clang-format to show expected formatting:" clang-format --lines=866:870 src/test/evo_deterministicmns_tests.cppLength of output: 664
Run clang-format locally to fix the formatting mismatch
The CI failure shows a clang-format diff around the constructor initializer list in src/test/evo_deterministicmns_tests.cpp (approx. lines 866–870):
TestChainV19BeforeActivationSetup::TestChainV19BeforeActivationSetup() : TestChainSetup(494, {"-testactivationheight=v19@500", "-testactivationheight=v20@500", "-testactivationheight=mn_rr@500"}) {Please install or invoke clang-format using the project’s style configuration on this file, review the resulting diff in your local environment, and commit the formatted code.
Once you’ve applied and verified the formatting locally, update the PR.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 866-867: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix formatting issues.
test/functional/feature_dip3_v19.py (2)
50-50: LGTM! Proper delay of v20 and MN_RR activation.The
delay_v20_and_mn_rr(height=260)call effectively separates v20 and MN_RR activation from v19 activation (at height 200), creating a 60-block gap. This aligns perfectly with the PR objective to fix intermittent errors by ensuring chainlocks are established using the BASIC scheme before v20 activation proceeds.
112-112: LGTM! Explicit v20 activation completes the fix.The explicit activation of v20 at height 260 completes the fix by ensuring that:
- v19 has been active for 60 blocks (200-260)
- Chainlocks using the BASIC scheme have been established
- v20 activation won't encounter the "bad-cbtx-invalid-clsig" error
This should effectively resolve the intermittent CI failures described in the PR objectives.
There was a problem hiding this comment.
p2p_node_network_limited.py --v2transport is flakey it appears too, based on the CI here.
ACK 63f509d, tests ran 20 times locally w/o error
yeah, it's know issue. I have particular solution, but it's still flakey. I will create PR soon |
Issue being fixed or feature implemented
#6702
It can happen on Regtest or Devnet if v20 is just activated but the best chainlock is still in legacy (pre-v19) scheme.
It causes errors on CI similar to this one:
While underlying error is
bad-cbtx-invalid-clsig:What was done?
Chainlocks in CbTx can be only in Basic scheme.
This PR splits activation of v19 and v20 to different blocks and wait until the best chainlocks will be formed in BASIC scheme.
How Has This Been Tested?
Run functional test
feature_dip3_v19.pymultiple times.Breaking Changes
On RegTest
v20is enforced to be activated after v19. In practice, any chainlocks in CbTx formed without v19 activated will stop the chain. Added assert forregtestto be sure that combination of various-testactivationheightwon't stop chainChecklist: