diff --git a/CHANGELOG.md b/CHANGELOG.md index ab42317b..b350a295 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,36 @@ follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixed + +- **Radiant-chain reserves were converted with the BITCOIN interval at seven sites (#579).** + `rxd_claim_burial` and `rxd_claim_inclusion` count RADIANT blocks and were normalised + with `policy.block_interval_s`; `policy.rxd_block_interval_s` was used for them nowhere. + + **Inert on every shipped path**, because each constructor tags those fields BLOCKS and + the conversion is then the identity — the interval argument is never read. That is why + it survived review, the type system (`security/units.py` has one `BlockSpan` for all + chains) and an "auditor-grade" parity sweep: a wrong argument that is never used looks + exactly like a right one. + + 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; a + 900 s burial is 3 honest Radiant blocks and was **refused at construction** as + "1 blk < safety floor 2". Unsafe one way, refusing valid work the other. + + One `_radiant_reserve_blocks` helper now makes the choice once, and + `MarginPolicy.__post_init__` applies each field's own chain interval — Bitcoin's for + `btc_claim_reorg_depth`, Radiant's for the rest. + +- **The parity sweep reproduced the same conflation and could not have caught it (#581).** + `test_assess_claim_finality_parity_sweep_byte_equivalent` 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 — so the two agreed by + sharing a defect. Every swept policy was also BLOCKS-tagged, making all conversion + arithmetic the identity, so no unit error of any size could have failed it. The reference + now derives the interval and the rounding independently, and the sweep includes a + SECONDS-tagged policy. + ### Added - **`pyrxd glyph inspect --verify-wave` names the signer.** For a VERIFIED v2 diff --git a/src/pyrxd/gravity/swap_coordinator.py b/src/pyrxd/gravity/swap_coordinator.py index 0b104827..8be2c469 100644 --- a/src/pyrxd/gravity/swap_coordinator.py +++ b/src/pyrxd/gravity/swap_coordinator.py @@ -396,7 +396,10 @@ def __post_init__(self) -> None: value = getattr(self, label) if not isinstance(value, Timelock): raise ValidationError(f"MarginPolicy.{label} must be a Timelock") - blocks = value.normalize_to(TimeUnit.BLOCKS, block_interval_s=self.block_interval_s).value + # RADIANT fields convert with RADIANT's interval (#579); btc_claim_reorg_depth + # is a Bitcoin quantity and keeps the Bitcoin one. + interval = self.block_interval_s if label == "btc_claim_reorg_depth" else self.rxd_block_interval_s + blocks = value.normalize_to(TimeUnit.BLOCKS, block_interval_s=interval).value if blocks < floor: raise ValidationError( f"MarginPolicy.{label} = {blocks} blk < safety floor {floor}. " @@ -1048,6 +1051,27 @@ def _reserve_to_blocks(reserve: Timelock, block_interval_s: float) -> int: return math.ceil(reserve.value / block_interval_s) +def _radiant_reserve_blocks(policy: MarginPolicy, reserve: Timelock) -> int: + """Convert a RADIANT-chain reserve to blocks, using RADIANT's interval. + + `rxd_claim_burial` and `rxd_claim_inclusion` count RADIANT blocks. They were + converted with `policy.block_interval_s` — the BITCOIN interval — at seven + sites, and `policy.rxd_block_interval_s` was used for them nowhere (#579). + + Inert while every constructor tags them BLOCKS, because the conversion is then + the identity and the interval is never read. That is exactly why it survived: + a wrong argument that is never used looks like a working one. A SECONDS-tagged + value makes it live in BOTH directions — measured at 600/300, an 1800 s burial + (6 Radiant blocks) was used as 3, half the intended depth; and a 900 s burial + (3 honest Radiant blocks) was REFUSED at construction as "1 blk < floor 2". + + Exists so the choice is made once rather than at each call site. Enforced by + `tests/test_radiant_reserves_use_the_radiant_interval.py`, which fails if any + site converts one of these fields with the Bitcoin interval again. + """ + return _reserve_to_blocks(reserve, policy.rxd_block_interval_s) + + def _claim_floor_blocks(policy: MarginPolicy, *, burial: int, counter_reserve: int) -> int: """Blocks that must remain before the maker's refund opens for a claim started NOW to be buried in time. THE SINGLE DEFINITION — both the fund-time gate and the claim-time assessor call it. @@ -1063,7 +1087,7 @@ def _claim_floor_blocks(policy: MarginPolicy, *, burial: int, counter_reserve: i Two gates computing the same quantity from separate expressions is the shape that produced #531 and the runner's empty-feasible-set class. Derived once here instead. """ - return burial + counter_reserve + _reserve_to_blocks(policy.rxd_claim_inclusion, policy.block_interval_s) + return burial + counter_reserve + _radiant_reserve_blocks(policy, policy.rxd_claim_inclusion) def assess_claim_finality( @@ -1129,7 +1153,7 @@ def assess_claim_finality( rxd_blocks = t_rxd.normalize_to(TimeUnit.BLOCKS, block_interval_s=policy.block_interval_s).value # Reserves round UP when seconds-tagged (flooring under-counts a reserve — unsafe); # t_rxd above floors, which is safe for a deadline (only shrinks the window). - flat_burial = _reserve_to_blocks(policy.rxd_claim_burial, policy.block_interval_s) + flat_burial = _radiant_reserve_blocks(policy, policy.rxd_claim_burial) # VALUE-SCALED burial (red-team HIGH): the taker's claim must bury deep enough that # reorging it costs at least the value at stake; the flat burial is only a FLOOR. # Effective value: the explicit per-assessment value (watchtower per-record) overrides @@ -1528,7 +1552,7 @@ def __init__( # _reserve_to_blocks(policy.rxd_claim_burial, ...)) — NOT the hardcoded estimate (red-team # LOW): an operator who measures a burial != 6 would otherwise get a floor that blesses an # N the gate's actual (larger) squeeze reserve makes insufficient — false assurance. - burial_blocks = _reserve_to_blocks(mp.rxd_claim_burial, mp.block_interval_s) + burial_blocks = _radiant_reserve_blocks(mp, mp.rxd_claim_burial) min_n = fin_reserve_blocks + burial_blocks - 1 if config.maker_stall_safety_window_blocks < min_n: raise ValidationError( @@ -1777,7 +1801,7 @@ def _asset_funding_depth(self) -> int | None: policy = self.config.margin_policy if not policy.is_measured: return None - return _reserve_to_blocks(policy.rxd_claim_burial, policy.block_interval_s) + return _radiant_reserve_blocks(policy, policy.rxd_claim_burial) def _assert_t_rxd_can_reach_a_safe_claim(self, terms: NegotiatedTerms, *, cov_confs: int) -> PreBtcLockGate | None: """None when the swap can still reach a SAFE claim; a refusing gate otherwise. @@ -1794,7 +1818,7 @@ def _assert_t_rxd_can_reach_a_safe_claim(self, terms: NegotiatedTerms, *, cov_co """ mp = self.config.margin_policy try: - flat_burial = _reserve_to_blocks(mp.rxd_claim_burial, mp.block_interval_s) + flat_burial = _radiant_reserve_blocks(mp, mp.rxd_claim_burial) burial = max(flat_burial, _value_scaled_burial_blocks(mp, mp.value_at_risk_photons)) # max(flat, value-scaled) — the SAME term the claim-time gate uses. Checking only the # value-scaled component let the FLAT burial dominate unnoticed, and made this inert @@ -1814,7 +1838,7 @@ def _assert_t_rxd_can_reach_a_safe_claim(self, terms: NegotiatedTerms, *, cov_co f"t_rxd is {int(terms.t_rxd.value)} blocks and the maker's covenant is already " f"{elapsed} deep, leaving {remaining} — but a safe claim needs {required} " f"(burial {burial} + counter-leg reserve {counter_reserve} + " - f"{_reserve_to_blocks(mp.rxd_claim_inclusion, mp.block_interval_s)} to be mined). " + f"{_radiant_reserve_blocks(mp, mp.rxd_claim_inclusion)} to be mined). " "This swap can NEVER reach a safe claim: the taker would reveal, find every " "claim SQUEEZED, and be left choosing between a reorg-reversible claim and " "walking away from a funded counter leg. Negotiate a longer t_rxd, fund " diff --git a/src/pyrxd/gravity/watch/claim_executor.py b/src/pyrxd/gravity/watch/claim_executor.py index 7e67153f..bc005b8a 100644 --- a/src/pyrxd/gravity/watch/claim_executor.py +++ b/src/pyrxd/gravity/watch/claim_executor.py @@ -639,8 +639,12 @@ def _check_value_cap(self, record: SwapRecord) -> str | None: return f"value-bearing {variant} autonomous claim has no in-record value bound; refusing (set accept_unbounded_reorg_risk for dust)" if self._reorg_cost_per_block is None: return "no reorg_cost_per_block configured; cannot bound the value-vs-reorg risk (refusing)" + # RADIANT's interval for a RADIANT reserve (#579). This read + # `policy.block_interval_s` — Bitcoin's — and fed the result to + # `max_protected_value`, so a seconds-tagged burial would have halved the + # value ceiling this gate exists to enforce. burial_blocks = self._policy.rxd_claim_burial.normalize_to( - TimeUnit.BLOCKS, block_interval_s=self._policy.block_interval_s + TimeUnit.BLOCKS, block_interval_s=self._policy.rxd_block_interval_s ).value ceiling = max_protected_value( rxd_claim_burial_blocks=burial_blocks, diff --git a/tests/test_margin_policy_validates_every_timelock_field.py b/tests/test_margin_policy_validates_every_timelock_field.py index e25991e8..6eb3a514 100644 --- a/tests/test_margin_policy_validates_every_timelock_field.py +++ b/tests/test_margin_policy_validates_every_timelock_field.py @@ -90,7 +90,22 @@ def test_a_bare_int_FAILS_CLOSED_rather_than_later(self, label: str) -> None: def test_a_SECONDS_tagged_value_is_normalised_before_the_floor_is_applied(self) -> None: """The floor is in BLOCKS, so a seconds-tagged reserve must be converted first — otherwise - `Timelock(1, SECONDS)` would pass a 2-block floor on its raw number.""" + `Timelock(1, SECONDS)` would pass a 2-block floor on its raw number. + + The VALUES changed with #579, the property did not. `rxd_claim_burial` counts RADIANT + blocks, so it converts at the Radiant interval (300 s default), not the Bitcoin one + (600 s). Under the old arithmetic 600 s read as 1 block; it is 2 Radiant blocks, which is + what a Radiant burial of 600 s actually is. Deliberately using the two intervals' + DIFFERENT values here — equal intervals would hide exactly the conflation this pins. + """ with pytest.raises(ValidationError, match="rxd_claim_burial"): - MarginPolicy(**_base(rxd_claim_burial=t.Timelock(600, t.TimeUnit.SECONDS))) # 1 block - MarginPolicy(**_base(rxd_claim_burial=t.Timelock(1200, t.TimeUnit.SECONDS))) # 2 blocks + MarginPolicy(**_base(rxd_claim_burial=t.Timelock(300, t.TimeUnit.SECONDS))) # 1 RXD block + MarginPolicy(**_base(rxd_claim_burial=t.Timelock(600, t.TimeUnit.SECONDS))) # 2 RXD blocks + + def test_a_BITCOIN_reserve_still_converts_at_the_BITCOIN_interval(self) -> None: + """The other half of #579: only the RADIANT fields moved. `btc_claim_reorg_depth` counts + Bitcoin blocks and must keep the Bitcoin interval, or the fix would have swapped one + conflation for its mirror image.""" + with pytest.raises(ValidationError, match="btc_claim_reorg_depth"): + MarginPolicy(**_base(btc_claim_reorg_depth=t.Timelock(600, t.TimeUnit.SECONDS))) # 1 BTC block + MarginPolicy(**_base(btc_claim_reorg_depth=t.Timelock(1200, t.TimeUnit.SECONDS))) # 2 BTC blocks diff --git a/tests/test_radiant_reserves_use_the_radiant_interval.py b/tests/test_radiant_reserves_use_the_radiant_interval.py new file mode 100644 index 00000000..82b74219 --- /dev/null +++ b/tests/test_radiant_reserves_use_the_radiant_interval.py @@ -0,0 +1,100 @@ +"""A RADIANT-chain reserve must never be converted with the BITCOIN interval. + +`rxd_claim_burial` and `rxd_claim_inclusion` count RADIANT blocks. They were +converted with `policy.block_interval_s` — Bitcoin's — at seven sites, and +`policy.rxd_block_interval_s` was used for them nowhere (#579). + +It survived because it is INERT while every constructor tags those fields BLOCKS: +`normalize_to`/`_reserve_to_blocks` are then 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 neither review nor the type system nor the parity +sweep caught it — `security/units.py` has one `BlockSpan` for all chains, so a +Radiant block and a Bitcoin block are the same type to it. + +Both failure directions are real once a SECONDS value exists, measured at 600/300: + +* an 1800 s burial is 6 Radiant blocks and was read as **3** — half the intended + depth, the unsafe direction; +* a 900 s burial is 3 honest Radiant blocks and was **REFUSED at construction** + as "1 blk < safety floor 2" — a guard refusing valid work. + +Seven sites is why this is a scanner and not seven fixes. + +WHAT THIS COVERS, AND WHAT IT DOES NOT. The scanner is line-based: it catches a +CALL SITE that names a Radiant field and the Bitcoin interval together — the shape +that actually shipped. It cannot see a regression INSIDE +`_radiant_reserve_blocks`, whose parameter is a generic `reserve` with no field +name on the line. + +That half is covered by `test_assess_claim_finality_parity_sweep_byte_equivalent`, +whose reference derives the interval from the rule independently and now sweeps a +SECONDS-tagged policy. Verified rather than assumed: switching the helper back to +the Bitcoin interval fails the parity sweep, and adding a fresh call site that +bypasses the helper fails this file. Neither guard covers both, and saying so is +the point — a scanner presented as complete is worse than one with a stated edge. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parent.parent +_SRC = _ROOT / "src" + +#: Fields that count RADIANT blocks. +_RADIANT_FIELDS = ("rxd_claim_burial", "rxd_claim_inclusion") + +#: A conversion naming a Radiant field and the BITCOIN interval in one expression. +#: `rxd_block_interval_s` contains `block_interval_s`, so the pattern must exclude +#: the correct spelling explicitly rather than by substring. +_BAD = re.compile( + r"(? list[str]: + hits = [] + for path in sorted(_SRC.rglob("*.py")): + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + stripped = line.strip() + if stripped.startswith("#") or "test" in path.name: + continue + if _BAD.search(line): + hits.append(f"{path.relative_to(_ROOT)}:{lineno}: {stripped[:100]}") + return hits + + +def test_the_scanner_catches_the_shipped_defect() -> None: + """Non-vacuity: a scanner matching nothing is indistinguishable from a clean tree.""" + shipped = " burial_blocks = _reserve_to_blocks(policy.rxd_claim_burial, policy.block_interval_s)" + assert _BAD.search(shipped), "the pattern must fire on the line that actually shipped" + + +def test_the_scanner_accepts_the_CORRECT_spelling() -> None: + """Honest path. `rxd_block_interval_s` contains `block_interval_s` as a substring, + so a naive pattern would flag every correct call site — a guard that refuses + valid work, on the very line that fixes the bug.""" + fixed = " burial_blocks = _reserve_to_blocks(policy.rxd_claim_burial, policy.rxd_block_interval_s)" + assert not _BAD.search(fixed) + + +def test_no_radiant_reserve_is_converted_with_the_bitcoin_interval() -> None: + offenders = _offenders() + assert not offenders, ( + "a RADIANT-chain reserve is being converted with the BITCOIN interval:\n " + + "\n ".join(offenders) + + "\n\nUse `_radiant_reserve_blocks(policy, reserve)`, which picks the interval once." + ) + + +@pytest.mark.parametrize("field", _RADIANT_FIELDS) +def test_the_fields_this_guards_still_exist(field: str) -> None: + """Stale-guard ratchet: if a field is renamed, this scanner silently stops + covering it and would keep passing forever.""" + from pyrxd.gravity.swap_coordinator import MarginPolicy + + assert field in MarginPolicy.__dataclass_fields__, f"{field} no longer exists; update this scanner" diff --git a/tests/test_swap_coordinator.py b/tests/test_swap_coordinator.py index 13f95316..1493aa2d 100644 --- a/tests/test_swap_coordinator.py +++ b/tests/test_swap_coordinator.py @@ -1511,13 +1511,22 @@ def test_assess_claim_finality_parity_sweep_byte_equivalent(): def _old(confs, now, locked, t_rxd, policy): bi, rbi = policy.block_interval_s, policy.rxd_block_interval_s - rxd_blocks = t_rxd.normalize_to(t.TimeUnit.BLOCKS, block_interval_s=bi).value - rxd_burial = policy.rxd_claim_burial.normalize_to(t.TimeUnit.BLOCKS, block_interval_s=bi).value - depth = policy.btc_claim_reorg_depth.normalize_to(t.TimeUnit.BLOCKS, block_interval_s=bi).value + + def _reserve(tl, interval): + """A RESERVE rounds UP — flooring under-counts it, the unsafe direction. + Restated here from the rule rather than copied from `_reserve_to_blocks`.""" + return tl.value if tl.unit is t.TimeUnit.BLOCKS else math.ceil(tl.value / interval) + + rxd_blocks = t_rxd.normalize_to(t.TimeUnit.BLOCKS, block_interval_s=rbi).value + # RADIANT quantities convert at RADIANT's interval (#579). This reference used + # `bi` for both, which is the production conflation restated — so the two agreed + # by sharing a defect, and no swept policy was seconds-tagged to expose it. + rxd_burial = _reserve(policy.rxd_claim_burial, rbi) + depth = _reserve(policy.btc_claim_reorg_depth, bi) # a BITCOIN quantity # #511: a claim decided on at height `now` cannot be mined at `now`, so its burial starts # at `now + 1` at the earliest and by `now + inclusion` with the reserve. Derived here from # that sentence, independently of how the production floor is expressed. - inclusion = policy.rxd_claim_inclusion.normalize_to(t.TimeUnit.BLOCKS, block_interval_s=bi).value + inclusion = _reserve(policy.rxd_claim_inclusion, rbi) blocks_left = (locked + rxd_blocks) - now if confs >= depth: return ClaimFinality.SAFE if blocks_left >= rxd_burial + inclusion else ClaimFinality.SQUEEZED @@ -1537,6 +1546,20 @@ def _old(confs, now, locked, t_rxd, policy): btc_claim_reorg_depth=t.Timelock(6, t.TimeUnit.BLOCKS), rxd_claim_burial=t.Timelock(6, t.TimeUnit.BLOCKS), ), + # A SECONDS-TAGGED policy. Every policy above is BLOCKS-tagged, which makes + # every conversion the identity — so the sweep exercised no unit arithmetic + # at all, and could not have failed on #579 however wrong the intervals were. + # 1800 s of Radiant burial is 6 Radiant blocks; read at the Bitcoin interval + # it is 3, half the intended depth. This row is the difference between a + # sweep that looks exhaustive and one that is. + MarginPolicy( + margin=t.Timelock(36, t.TimeUnit.BLOCKS), + block_interval_s=600.0, + is_measured=False, + rxd_block_interval_s=300.0, + btc_claim_reorg_depth=t.Timelock(3600, t.TimeUnit.SECONDS), # 6 BTC blocks + rxd_claim_burial=t.Timelock(1800, t.TimeUnit.SECONDS), # 6 RXD blocks + ), ] locked = 1000 for policy in policies: diff --git a/tests/test_watch_claim_executor.py b/tests/test_watch_claim_executor.py index 398375a9..600308d2 100644 --- a/tests/test_watch_claim_executor.py +++ b/tests/test_watch_claim_executor.py @@ -869,3 +869,45 @@ async def test_the_executor_PINS_the_recorded_covenant_outpoint(): f"the executor passed pin_outpoint={leg.chain_io.pin_seen!r} — it is re-discovering by scan, " "so a second payment to the covenant address still bricks the autonomous claim" ) + + +# --------------------------------------------------------- tests: the decision BOUNDARY + + +class TestTheBroadcastBoundaryIsPinned: + """Every other broadcast test in this file runs at MAXIMUM slack, and that is a + problem the suite cannot 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. + + Measured through the production entry point, sweeping `confs` 1..199: the verdict + flips BROADCAST -> DECLINED between **137 and 138**. The suite's fixtures use 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 the executor's depth-to-height + derivation (`now_rxd = funded_h + max(cov_confs, 1) - 1`) was therefore invisible. + + Both directions of that off-by-one 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. + """ + + async def test_the_last_confs_that_still_broadcasts(self) -> None: + ex, _leg, rec, _ = await _armed_executor(confs=137) + assert await ex.execute("s1", rec, _claim_decision()) is ExecOutcome.BROADCAST + + async def test_one_block_further_declines(self) -> None: + ex, _leg, rec, _ = await _armed_executor(confs=138) + assert await ex.execute("s1", rec, _claim_decision()) is ExecOutcome.DECLINED + + async def test_a_REACHABLE_state_still_broadcasts(self) -> None: + """The honest path at a co-occurring depth rather than at maximum slack. + + With the BTC claim 10 confirmations deep, roughly 20-plus Radiant blocks have + also passed. Pinning that here means the happy path is exercised at a state + the chain can actually be in — the default `confs=1` never is. + """ + ex, _leg, rec, _ = await _armed_executor(confs=24, btc_confs=10) + assert await ex.execute("s1", rec, _claim_decision()) is ExecOutcome.BROADCAST