fix: Radiant reserves were converted with the Bitcoin interval (#579, #581) - #590
Merged
Merged
Conversation
…581) rxd_claim_burial and rxd_claim_inclusion count RADIANT blocks. They were normalised with policy.block_interval_s — the BITCOIN interval — at seven sites, and policy.rxd_block_interval_s was used for them nowhere. INERT ON EVERY SHIPPED PATH, which is the whole reason it survived: each constructor tags those fields BLOCKS, the conversion is then the identity, and the interval argument is never read. A wrong argument that is never used looks exactly like a right one. Verified inert before changing anything — no Timelock in src/ or scripts/ is constructed with TimeUnit.SECONDS and there is no MarginPolicy deserializer — so this commit is a behavioural no-op today and removes a trap for tomorrow. Both directions were reachable the moment a SECONDS value existed. Measured at 600/300: an 1800 s burial is 6 Radiant blocks and was read as 3, half the intended depth (unsafe); a 900 s burial is 3 honest Radiant blocks and was REFUSED at construction as "1 blk < safety floor 2" (refuses valid work). ONE HELPER, NOT SEVEN FIXES. `_radiant_reserve_blocks(policy, reserve)` makes the choice once. `MarginPolicy.__post_init__` now applies each field's own chain interval — Bitcoin's for btc_claim_reorg_depth, Radiant's for the rest. AND THE TEST THAT SHOULD HAVE CAUGHT IT (#581). The parity sweep bills its reference as deriving the answer "from the rule rather than from the code", and it normalised the Radiant reserves with the Bitcoin interval exactly as production did — the two agreed by SHARING a defect. Every swept policy was also BLOCKS-tagged, so all conversion arithmetic was the identity and no unit error of any size could have failed it. The reference now derives interval AND rounding independently (a reserve rounds UP; it had used normalize_to, which floors), and the sweep includes a SECONDS-tagged policy. Two guards, each verified to catch what the other cannot, and the split is stated in the scanner's own docstring rather than implied: - the parity sweep catches a regression INSIDE the helper — confirmed by switching it back to the Bitcoin interval, which fails it; - tests/test_radiant_reserves_use_the_radiant_interval.py catches a NEW call site that bypasses the helper — confirmed by planting one, which fails it. The scanner's honest-path case matters more than usual here: `rxd_block_interval_s` contains `block_interval_s` as a substring, so a naive pattern would flag every CORRECT call site — a guard refusing valid work on the very line that fixes the bug. It also carries a stale-guard ratchet asserting the fields it names still exist, since a rename would silently end its coverage. CI-equivalent: 10,990 passed, 192 skipped, 1 xfailed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every broadcast test in this file ran at MAXIMUM slack, which the suite could not see from the inside. `_armed_executor` defaults confs=1, so now_rxd == funded_h and blocks_left is the full t_rxd — while the BTC claim is 10 confirmations deep. Radiant does not reach that state: 10 Bitcoin confirmations is roughly 100 minutes, which is 20-plus Radiant blocks at the measured 222 s median, not zero. The fixture describes a co-occurrence the chain cannot produce. MEASURED through the production entry point, sweeping confs 1..199: the verdict flips BROADCAST -> DECLINED between 137 and 138. The suite used confs 1 and 172 and nothing else, so no case sat within 136 blocks of the flip on one side or 34 on the other. An off-by-one in `now_rxd = funded_h + max(cov_confs, 1) - 1` was therefore invisible, and both directions are real defects: one block LATE is a spurious squeeze after p is public, which is a forfeiture path; one block EARLY certifies a claim that cannot bury in time. Three cases: the last confs that still broadcasts (137), the first that declines (138), and an honest path at a REACHABLE depth (confs=24 against btc_confs=10) rather than at maximum slack. Plant-verified: dropping the `- 1` fails the boundary case and nothing else in the file — the 49 pre-existing tests stay green, which is the point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes #579. Closes the parity-sweep half of #581.
rxd_claim_burialandrxd_claim_inclusioncount Radiant blocks. They were normalised withpolicy.block_interval_s— the Bitcoin interval — at seven sites, andpolicy.rxd_block_interval_swas used for them nowhere.Why it survived everything
It is inert on every shipped path. Each constructor tags those fields BLOCKS, so the conversion is the identity and the interval argument is never read. A wrong argument that is never used looks exactly like a right one — which is why it passed review, the type system (
security/units.pyhas oneBlockSpanfor all chains), and an "auditor-grade" parity sweep.Verified inert before changing anything: no
Timelockinsrc/orscripts/is constructed withTimeUnit.SECONDS, and there is noMarginPolicydeserializer. This PR is a behavioural no-op today and removes a trap for tomorrow.Both directions were reachable the moment a SECONDS value existed, measured at 600/300:
The test that should have caught it (#581)
The parity sweep bills its reference as deriving the answer "from the rule rather than from the code" — and it normalised the Radiant reserves with the Bitcoin interval exactly as production did. The two agreed by sharing a defect. Every swept policy was also BLOCKS-tagged, so all conversion arithmetic was the identity and no unit error of any size could have failed it.
The reference now derives interval and rounding independently (a reserve rounds UP; it had used
normalize_to, which floors), and the sweep includes a SECONDS-tagged policy.Two guards, each verified to catch what the other cannot
Neither covers both, and the scanner's docstring says so rather than implying completeness.
Its honest-path case matters more than usual:
rxd_block_interval_scontainsblock_interval_sas a substring, so a naive pattern would flag every correct call site — a guard refusing valid work on the very line that fixes the bug. It also carries a stale-guard ratchet asserting the fields it names still exist, since a rename would silently end its coverage.CI-equivalent locally: 10,990 passed, 192 skipped, 1 xfailed.
🤖 Generated with Claude Code