fix: restore BLS scheme after failed block validation#7345
fix: restore BLS scheme after failed block validation#7345thepastaclaw wants to merge 4 commits into
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
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 (1)
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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 43094d6) |
|
CI note: the only red item is |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR cleanly fixes the BLS scheme restoration on failed block validation via RAII guards in TestBlockValidity, ConnectBlock, and VerifyDB, while preserving the post-activation state in ConnectTip and VerifyDB's level-4 reconnect via a captured effective scheme. Verification confirms the design is sound; only two minor observations remain: a defensible but subtle change in when listeners observe the new scheme, and an unrelated whitespace insertion in a test file. No blockers; comment-only review.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/validation.cpp`:
- [SUGGESTION] src/validation.cpp:3059-3089: BlockChecked / flush window now runs with pre-switch BLS scheme on V19-boundary connects
On a successful V19-boundary ConnectBlock, the inner `ScopedBLSLegacyScheme` guard unconditionally restores the entry scheme before ConnectBlock returns, and the new scheme is re-applied only at line 3089 (after `m_chain.SetTip`). As a result, `GetMainSignals().BlockChecked(...)` (line 3060), `dbTx->Commit()`, `FlushStateToDisk`, `m_mempool->removeForBlock` / `removeExpiredAssetUnlock`, and `m_chain.SetTip` all execute with the OLD scheme on the boundary block — a transient inversion compared to the prior behavior. A scan of current BlockChecked listeners (net_processing, rpc/mining, test/util/mining, bitcoin-chainstate) and the intervening flush/mempool work shows none read `bls::bls_legacy_scheme` in that window, so this appears benign today. The final state set at line 3089 is correct. Worth calling out because future code reading `bls::bls_legacy_scheme` inside a BlockChecked listener or the flush window would observe a value inconsistent with the new tip; an alternative would be to apply the effective scheme before `BlockChecked` rather than after `SetTip`.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
fix/bls-scheme-restore
branch
from
17e316a to
e590e34
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
| FuncV19Activation(setup); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(v19_boundary_validation_failure_restores_bls_scheme) |
There was a problem hiding this comment.
that's improper location for bls tests
Use src/test/bls_tests.cpp
There was a problem hiding this comment.
Fixed in 35436f0529b425cdcfc3832f5d547f3d3d7dc93f — moved v19_boundary_validation_failure_restores_bls_scheme from src/test/evo_deterministicmns_tests.cpp into src/test/bls_tests.cpp, alongside the other BLS coverage.
Validation from the worktree:
git diff --check HEAD^ HEADbls_testsfocused run: 17 cases passedevo_dip3_activation_testsfocused run: 13 cases passed
The v19_boundary_validation_failure_restores_bls_scheme regression test exercises BLS-scheme bookkeeping after a failed block validation at the V19 activation boundary, so it belongs alongside the other BLS tests in src/test/bls_tests.cpp rather than the deterministicmns suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
NACK, adding one more un-typed bool to ConnectBlock is not error-prune, I will give other patch later today.
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
Ack — agreed on avoiding another untyped bool in |
|
@thepastaclaw please review this commit b0a03c6 and find down-sides compare to PR if any. Pull freely these changes to PR and adjust / improve if there's any inaccuraties or mistakes. |
Adopts knst's review feedback: drop the second untyped bool parameter on ConnectBlock and move the commit semantics inline. ScopedBLSLegacyScheme is constructed at the top of ConnectBlock and Commit()ed right after WriteBestBlock, so fJustCheck dry runs (which return earlier) restore the scheme on scope exit and real connects keep it. TestBlockValidity no longer needs its own manual save/restore, and ConnectTip drops the outer guard now that ConnectBlock encapsulates the decision. VerifyDB keeps an outer guard since the chain tip isn't advanced there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@knst thanks — I reviewed What changed:
Downsides / adjustments I found vs directly taking the commit:
Validation run locally:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/validation.cpp (1)
3055-3073:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the post-
ConnectBlock()failure window inConnectTip().
ConnectBlock()now commits the switched BLS scheme before returning, butConnectTip()can still return at Line 3073 beforem_chain.SetTip()runs. IfFlushStateToDisk()fails there, the active tip did not advance whilebls::bls_legacy_schemeremains switched. Add an outer guard inConnectTip()and commit it only after the tip is advanced.Proposed fix
{ + ScopedBLSLegacyScheme bls_scheme_guard; auto dbTx = m_evoDb.BeginTransaction(); CCoinsViewCache view(&CoinsTip()); bool rv = ConnectBlock(blockConnecting, state, pindexNew, view); GetMainSignals().BlockChecked(blockConnecting, state); @@ } @@ // Update m_chain & related variables. m_chain.SetTip(*pindexNew); + bls_scheme_guard.Commit(); UpdateTip(pindexNew);If the guard must span the whole function, declare it before the inner transaction block instead of inside that block.
🤖 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/validation.cpp` around lines 3055 - 3073, The issue is that ConnectBlock() commits a BLS scheme switch before ConnectTip() advances the chain tip via m_chain.SetTip(), creating a window where FlushStateToDisk() can fail and leave the chain tip unchanged while the BLS scheme remains switched. To fix this, add an outer guard (transaction/RAII object) around the ConnectBlock() call and subsequent operations in ConnectTip(), declaring it before the inner transaction block rather than inside it, and ensure it is only committed after m_chain.SetTip() has successfully advanced the tip so that the BLS scheme switch and tip advancement remain atomic.
🤖 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.
Outside diff comments:
In `@src/validation.cpp`:
- Around line 3055-3073: The issue is that ConnectBlock() commits a BLS scheme
switch before ConnectTip() advances the chain tip via m_chain.SetTip(), creating
a window where FlushStateToDisk() can fail and leave the chain tip unchanged
while the BLS scheme remains switched. To fix this, add an outer guard
(transaction/RAII object) around the ConnectBlock() call and subsequent
operations in ConnectTip(), declaring it before the inner transaction block
rather than inside it, and ensure it is only committed after m_chain.SetTip()
has successfully advanced the tip so that the BLS scheme switch and tip
advancement remain atomic.
|
@coderabbitai review |
✅ Action performedReview finished.
|
ConnectBlock commits the BLS-scheme switch when it succeeds, but ConnectTip can still fail at FlushStateToDisk() before m_chain.SetTip() runs. In that window the global bls::bls_legacy_scheme would stay switched while the active tip stayed unchanged, leaving the node in an inconsistent state. Add an outer ScopedBLSLegacyScheme guard at the top of ConnectTip that spans the inner ConnectBlock and the SetTip call, and Commit() it only after m_chain.SetTip() returns. Any early return before that point now restores the original scheme on scope exit. ConnectBlock keeps its own self-contained guard so direct callers (VerifyDB, TestBlockValidity) continue to work without a bool parameter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed in
Validation run locally:
@coderabbitai review |
|
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review of the BLS scheme RAII guard PR. Prior review at 894ec38 had no findings, so nothing carries forward. The new delta (43094d6) adds the ScopedBLSLegacyScheme guard inside ConnectTip and commits it after m_chain.SetTip(), closing the window between ConnectBlock success and tip advancement. Guard placement, fJustCheck semantics, commit ordering in both ConnectBlock and ConnectTip, and the V19-boundary regression test all check out. No in-scope defects found.
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
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)