v9.26.4 review remediations: disclose the redeem collateral floor gate, unify the activation-floor helper#420
Closed
gto90 wants to merge 1 commit into
Closed
Conversation
…on-floor helper Triple-review remediations for the v9.26.4 pruning PR: - docs: the release notes, PR/pruning explainers, and pruning plan no longer claim "zero consensus changes"; the redeem collateral floor gate (bad-collateral-release-not-vault on input 0, pre-floor fee-input classification) is disclosed as one narrowly scoped consensus rule that applies to every node, with the mainnet scan result referenced. - consensus: add DigiDollar::EarliestActivationFloor() in consensus/digidollar as the single source of truth for the DigiDollar activation floor. Callers: validation's EarliestDigiDollarActivationHeight, the "digidollar" prune-lock registration in node/chainstate.cpp, the health-scan seed in digidollar/health.cpp, and startup oracle price reconstruction in oracle/bundle_manager.cpp — previously four hand-duplicated copies of the same expression. Behavioral note: the helper returns 0 for NEVER_ACTIVE deployments, which the three startup copies already did; validation's copy previously returned min(nDDActivationHeight, min_activation_height) there. Unreachable on any configured network (main/testnet/regtest all set real start/timeout values, and DigiDollar validation never runs when the deployment can never activate). - fuzz: dd_prune_activation_floor and dd_prune_coin_gating now exercise the real shared helper via a constructed Consensus::Params instead of hand-copied models of the deleted inline expressions. - health: ScanUTXOSet's mempool/chain/consensus parameters lose their nullptr defaults; the pre-floor skip and the fail-closed incomplete-data check are gated on a non-null chain, so every caller must now opt in explicitly instead of silently reverting to the old silent-undercount behavior. - plan doc: sections 3b/3c annotated as-shipped (the promised unit-test files were not created; coverage lives in the fuzz target and functional phases F6/F9/F10/F14, and the tip-below-floor no-op case has no direct unit test).
gto90
requested review from
DigiSwarm,
JaredTate,
SmartArray,
digicontributer and
ycagel
There was a problem hiding this comment.
Pull request overview
This PR refines the v9.26.4 DigiDollar-compatible pruning work by (1) explicitly documenting the previously undisclosed consensus rule impact, (2) consolidating the DigiDollar activation-floor (“dd_floor”) computation into a single shared helper to prevent drift across call sites, and (3) tightening ScanUTXOSet’s API contract to avoid accidental fail-open behavior.
Changes:
- Document the redeem-collateral activation-floor gate as a narrowly scoped consensus rule in release notes and pruning docs.
- Introduce
DigiDollar::EarliestActivationFloor(const Consensus::Params&)as the single source of truth and update production + fuzz code to use it. - Remove defaulted
ScanUTXOSetparameters so callers must explicitly opt into/null outmempool/chain/consensus.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| V9.26.4_PRUNING_PLAN.md | Updates plan text to disclose the consensus rule and corrects “promised but not shipped” test-file sections. |
| V9.26.4_PRUNING_EXPLAINER.md | Updates pruning explainer to disclose the consensus rule. |
| V9.26.4_PR_EXPLAINER.md | Updates PR explainer to disclose the consensus rule (but still contains a few now-inconsistent “no consensus change” statements). |
| doc/release-notes/release-notes-9.26.4.md | Release notes updated to disclose the activation-floor collateral gating as a consensus rule. |
| src/consensus/digidollar.h | Declares DigiDollar::EarliestActivationFloor and documents its role as shared floor source-of-truth. |
| src/consensus/digidollar.cpp | Implements DigiDollar::EarliestActivationFloor. |
| src/node/chainstate.cpp | Switches prune-lock floor computation to the shared helper. |
| src/digidollar/validation.cpp | Switches validation’s earliest-activation-floor computation to the shared helper. |
| src/digidollar/health.h | Removes default args from SystemHealthMonitor::ScanUTXOSet and documents the rationale. |
| src/digidollar/health.cpp | Uses the shared helper for the activation floor in ScanUTXOSet. |
| src/oracle/bundle_manager.cpp | Uses the shared helper for floor-gated fail-closed startup behavior. |
| src/test/fuzz/digidollar_prune_blockdb.cpp | Updates fuzz targets to exercise the shared helper instead of replicated models. |
| src/test/digidollar_redteam_tests.cpp | Updates a ScanUTXOSet call site for the new signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
22
to
+26
| **What changes if you don't set `prune=`:** nothing. Your node behaves exactly | ||
| like v9.26.3. | ||
|
|
||
| **What we changed in consensus:** nothing. Zero consensus changes. A pruned node | ||
| accepts and rejects exactly the same blocks as a full node. The Groestl algolock | ||
| and the DigiDollar activation schedule are untouched. | ||
| **What we changed in consensus:** one narrowly scoped rule. Redemption collateral | ||
| classification is now gated on the DigiDollar activation floor (a pre-floor coin can |
Comment on lines
+25
to
+29
| **What we changed in consensus:** one narrowly scoped rule. Redemption collateral | ||
| classification is now gated on the DigiDollar activation floor (a pre-floor coin can | ||
| never be vault collateral), which is what makes a pruned node accept and reject | ||
| exactly the same blocks as a full node. The pre-release mainnet scan found no | ||
| existing coins the rule affects. The Groestl algolock and the DigiDollar activation |
Comment on lines
+83
to
+85
| - **Consensus, except the redeem collateral floor gate.** Mint/transfer rules, | ||
| oracle bundle rules, lock tiers — all byte-identical. The one change is in | ||
| redemption collateral classification (the activation-floor gate above). |
Comment on lines
+16
to
+18
| **How v9.26.4 does it** (one narrowly scoped consensus rule — the redeem collateral | ||
| floor gate that keeps pruned and full nodes in agreement — everything else opt-in | ||
| behind `-prune`): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediations from a triple-source review of #418, split out into their own PR so they can be reviewed independently. Stacked on
release/v9.26.4; the commit previously pushed to #418 directly was reverted there (596bd1f / 73b5779) and is carried here as 117278c.What this changes
1. Docs: disclose the consensus rule change (major).
The release notes, both explainers, and the pruning plan claimed "zero consensus changes", but
ValidateCollateralReleaseAmountgains two new unconditional gates that run on every node: input 0 below the activation floor is rejected (bad-collateral-release-not-vault), and pre-floor DD-lookalike fee inputs skip the structural collateral check. A v9.26.3 and a v9.26.4 full node can reach different verdicts on a crafted pre-floor coin, andV9.26.4_MAINNET_VALIDATION.md/V9.26.4_TESTNET_PRUNE_SUMMARY.mdalready call it a consensus fix. The gate itself is correct (it is what keeps pruned and full nodes in agreement); the operator-facing claim was not. All four docs now disclose it as one narrowly scoped consensus rule, with the mainnet scan result referenced.2. One source of truth for the activation floor (major).
The dd_floor formula was hand-duplicated in four production translation units (validation, chainstate prune lock, health scan seed, oracle startup reconstruction) plus two more mirrored copies inside the new fuzz target, which itself warns the copies must be kept in sync by hand. A drifted copy would silently reintroduce the pruned-vs-full divergence this release exists to close. Added
DigiDollar::EarliestActivationFloor(const Consensus::Params&)insrc/consensus/digidollar.{h,cpp}and pointed all four sites at it; the fuzz targets now exercise the real helper via a constructedConsensus::Paramsinstead of models.Behavioral note: the helper returns 0 for NEVER_ACTIVE deployments (as the three startup copies already did); validation's copy previously returned
min(nDDActivationHeight, min_activation_height)there. Unreachable on any configured network — main/testnet/regtest all set real start/timeout values, and DigiDollar validation never runs when the deployment can never activate.3.
ScanUTXOSetAPI safety (minor).The fail-closed contract (return false on unreadable post-floor block data) is gated on a non-null
chain, butchain/consensusdefaulted to nullptr, so a future caller omitting them would silently revert to the old silent-undercount behavior. The defaults are removed; the one test caller that relied on them now passes explicit nullptrs.4. Plan-doc accuracy (minor).
V9.26.4_PRUNING_PLAN.mdsections 3b/3c promised two unit-test files that were never created. They now carry "As shipped" notes pointing at the actual coverage (the fuzz target and functional phases F6/F9/F10/F14) and stating that the tip-below-floor no-op case has no direct unit test.Review-bot comments on #418
All six (4 Copilot, 2 CodeQL/code-quality) were validated against the head and found false, stale, or counterproductive as suggested; each thread has a reply with the reasoning and is resolved. No code changes came from them.
Verification
Full build passes locally on macOS arm64 (
makeexit 0, includingtest_digibyteand the fuzz binary). Unit suites could not be executed in the review environment (endpoint security kills freshly built DigiByte binaries); CI on this PR is the test run. Suggested local spot-checks:digidollar_txindex_tests,digidollar_validation_tests,digidollar_health_tests,digidollar_redteam_tests.