fix: restore BLS scheme after failed block validation#7345
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an RAII guard (ScopedBLSLegacyScheme) and SetBLSLegacyScheme helper, updates ConnectBlock to optionally return the "effective" bls::bls_legacy_scheme observed during validation, instantiates the guard in ConnectBlock/TestBlockValidity/VerifyDB to ensure restoration on scope exit, and updates callers (ConnectTip, VerifyDB reconnect) to re-apply the effective scheme after ConnectBlock. Adds a regression test exercising a v19-boundary validation failure to assert the scheme is preserved. Sequence Diagram(s)sequenceDiagram
participant ConnectTip
participant ConnectBlock
participant ScopedGuard as ScopedBLSLegacyScheme
participant BLS as bls::bls_legacy_scheme
ConnectTip->>ConnectBlock: call(&effective_bls_legacy_scheme)
ConnectBlock->>ScopedGuard: instantiate (snapshot)
ScopedGuard->>BLS: snapshot value
ConnectBlock->>BLS: may modify during validation
ConnectBlock->>ConnectBlock: write current value to *effective_bls_legacy_scheme
ConnectBlock->>ScopedGuard: exit destroys guard
ScopedGuard->>BLS: restore original value
ConnectTip->>BLS: SetBLSLegacyScheme(effective_value)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/test/evo_deterministicmns_tests.cppsrc/test/evo_deterministicmns_tests.cpp:5:10: fatal error: 'test/util/setup_common.h' file not found ... [truncated 2200 characters] ... rom ClangFrontend__CFrontend_decl.CFrontend_decl_funct.function_decl in file "src/clang/cFrontend_decl.ml", line 90, characters 12-151 src/test/validation_block_tests.cppsrc/test/validation_block_tests.cpp:5:10: fatal error: 'boost/test/unit_test.hpp' file not found ... [truncated 1130 characters] ... lib/clang/18/include" src/validation.cppsrc/validation.cpp:7:10: error: 'validation.h' file not found with include; use "quotes" instead ... [truncated 2200 characters] ... ans.CTrans_funct.exec_with_node_creation in file "src/clang/cTrans.ml" (inlined), line 104, characters 20-38 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/validation.cpp`:
- Line 2599: The call to bls_scheme_guard.Dismiss() inside ConnectBlock causes
the switched BLS scheme to persist across non-tip replays (e.g.
CVerifyDB::VerifyDB calling ConnectBlock on a temporary cache), so move the
persistence decision out of ConnectBlock and into the real chain advance site
(CChainState::ConnectTip) or gate it behind an explicit opt-in flag;
specifically, remove or stop calling ScopedBLSLegacyScheme::Dismiss() from
within ConnectBlock and instead create and dismiss ScopedBLSLegacyScheme in
ConnectTip (around the actual Flush/Commit, m_chain.SetTip and UpdateTip
sequence) so that bls::bls_legacy_scheme only flips when the tip truly advances,
or add an explicit parameter on ConnectBlock (e.g., persist_scheme=false) and
have ConnectTip call ConnectBlock with persist_scheme=true and perform the
Dismiss() there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 044045a9-71a1-40ba-978c-ff4e44e465b0
📒 Files selected for processing (2)
src/test/evo_deterministicmns_tests.cppsrc/validation.cpp
thepastaclaw
force-pushed
the
fix/bls-scheme-restore
branch
2 times, most recently
from
aacbd36 to
67427a9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/validation_block_tests.cpp`:
- Around line 384-410: The test mutates the global bls::bls_legacy_scheme flag
without restoring it; capture the current value of bls::bls_legacy_scheme before
calling bls::bls_legacy_scheme.store(true) and restore it at the end of the test
(or use an RAII/scope-guard) so the global state is reset regardless of test
assertions or failures; reference the mutation point
(bls::bls_legacy_scheme.store(true)) and ensure restoration runs after the
TestBlockValidity/ProcessNewBlock checks (or in a teardown/scope guard) to avoid
cross-test leakage.
In `@src/validation.cpp`:
- Around line 3088-3090: The tip is published with the old BLS scheme because
SetBLSLegacyScheme(effective_bls_legacy_scheme, __func__) is called after
UpdateTip; move the SetBLSLegacyScheme call to immediately after
m_chain.SetTip(*pindexNew) and before UpdateTip(pindexNew) so the process-wide
BLS scheme is updated prior to notifying waiters (locate the sequence
m_chain.SetTip(*pindexNew); UpdateTip(pindexNew); SetBLSLegacyScheme(...) and
reorder to m_chain.SetTip(*pindexNew); SetBLSLegacyScheme(...);
UpdateTip(pindexNew)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 82e1b1aa-3913-48be-b707-4994ca6f17aa
📒 Files selected for processing (3)
src/test/validation_block_tests.cppsrc/validation.cppsrc/validation.h
|
Addressed latest CodeRabbit findings in a34f16a: restored the test-local BLS scheme after the regression check and moved SetBLSLegacyScheme before UpdateTip so tip notifications observe the effective scheme. Validation: |
thepastaclaw
force-pushed
the
fix/bls-scheme-restore
branch
from
67427a9 to
a34f16a
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
fix/bls-scheme-restore
branch
from
a34f16a to
82e5634
Compare
|
Follow-up pushed in 82e5634: moved the BLS regression check into the existing Evo activation suite so CI no longer tries to run a disabled filtered test in nowallet/tsan builds. Validation: @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/evo_deterministicmns_tests.cpp (1)
1001-1027: ⚡ Quick winUse scoped restoration for
bls::bls_legacy_schemeto avoid cross-test state leaks.The end-of-test restore is skipped if a fatal abort/exception happens after Line 1008, which can leave the global scheme mutated for subsequent tests.
Suggested hardening
- const bool previous_bls_scheme{bls::bls_legacy_scheme.load()}; + const bool previous_bls_scheme{bls::bls_legacy_scheme.load()}; + struct ScopedRestoreBlsScheme { + explicit ScopedRestoreBlsScheme(bool saved) : saved_scheme(saved) {} + ~ScopedRestoreBlsScheme() { bls::bls_legacy_scheme.store(saved_scheme); } + bool saved_scheme; + } bls_scheme_restore_guard{previous_bls_scheme}; @@ - bls::bls_legacy_scheme.store(previous_bls_scheme);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/evo_deterministicmns_tests.cpp` around lines 1001 - 1027, Replace the manual save/restore of bls::bls_legacy_scheme with a scoped RAII guard so the original value is always restored even if the test aborts or throws; specifically, instead of storing previous_bls_scheme and calling bls::bls_legacy_scheme.store(previous_bls_scheme) at the end, create a small scoped restorer object (or use an existing SCOPE_EXIT/ScopedRestore helper) that captures bls::bls_legacy_scheme and its current value on construction and calls store(original) in its destructor, and then call bls::bls_legacy_scheme.store(true) to set the test value — this ensures bls::bls_legacy_scheme is restored no matter how the test exits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 1001-1027: Replace the manual save/restore of
bls::bls_legacy_scheme with a scoped RAII guard so the original value is always
restored even if the test aborts or throws; specifically, instead of storing
previous_bls_scheme and calling
bls::bls_legacy_scheme.store(previous_bls_scheme) at the end, create a small
scoped restorer object (or use an existing SCOPE_EXIT/ScopedRestore helper) that
captures bls::bls_legacy_scheme and its current value on construction and calls
store(original) in its destructor, and then call
bls::bls_legacy_scheme.store(true) to set the test value — this ensures
bls::bls_legacy_scheme is restored no matter how the test exits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2605dc77-85b5-4bbd-87f1-352ee65d2ff2
📒 Files selected for processing (4)
src/test/evo_deterministicmns_tests.cppsrc/test/validation_block_tests.cppsrc/validation.cppsrc/validation.h
✅ Files skipped from review due to trivial changes (1)
- src/test/validation_block_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/validation.h
thepastaclaw
force-pushed
the
fix/bls-scheme-restore
branch
from
82e5634 to
17e316a
Compare
|
Addressed the latest CodeRabbit nitpick in 17e316a: the BLS regression now uses a scoped restorer so Validation: @coderabbitai review |
|
✅ Action performedReview finished.
|
| ✅ Review complete (commit 9ffe3c1bff)\n\nAutomated code review verdict: ship. Reviewed the BLS scheme restoration path for failed block validation and found no significant issues; success-path scheme behavior remains preserved. |
|
CI note: the only red item is |
1 participant
Restore BLS scheme after failed block validation
Issue being fixed or feature implemented
V19-boundary block validation can temporarily switch the process-wide BLS
scheme from legacy to basic while validating a candidate block. If validation
fails after that switch, early returns from
ConnectBlockorTestBlockValiditycould leavebls::bls_legacy_schemein the wrong stateeven though the active chain tip did not advance.
What was done?
bls::bls_legacy_schemesnapshots in validation code.ConnectBlockfailure paths.only after a full successful real block connection.
TestBlockValidityso dry-run andproposal validation restores on both success and failure.
candidate block which flips the scheme and then fails later with
bad-txns-inputs-missingorspent.How Has This Been Tested?
Tested on macOS 15.6 arm64 in a local Dash Core worktree.
git diff --checkmake -C src -j8 test/test_dashevo_dip3_activation_tests/v19_boundary_validation_failure_restores_bls_schemeevo_dip3_activation_testsshipBreaking Changes
None.
Checklist
code-owners and collaborators only)