From 6eac466fde6cfb9398ac763ad17b0a1960f5ea72 Mon Sep 17 00:00:00 2001 From: Mudwood Labs Date: Thu, 3 Sep 2026 01:16:56 -0700 Subject: [PATCH 1/2] pin the #579 fix that nothing was pinning, in both directions A security panel reported that one of the seven #579 sites had no test that fails when reverted. Verified, and it was worse than reported: switching claim_executor._check_value_cap back to the Bitcoin interval passed the ENTIRE suite - 10,993 tests, its own module, and the scanner written for exactly this defect class. TWO INDEPENDENT REASONS IT WAS INVISIBLE. The scanner was line-based. At that site the field and the interval sit on two different lines of one call, so no single line contained both. It is now an AST scan per CALL, which is line-agnostic - and which also removes the reason the old pattern had to exclude the correct spelling by hand, since `rxd_block_interval_s` only CONTAINS `block_interval_s` as text, never as an identifier. And no test could express the defect. #579 is inert while every fixture tags the burial in BLOCKS: normalize_to is then the identity and the interval argument is never read, so the wrong argument and the right one produce identical output. No test in that module supplied a SECONDS-tagged burial. A test whose fixture cannot represent the bug passes for a reason unrelated to the code being correct, which is why the new one makes the two intervals DIFFER (600 vs 300) and picks an amount BETWEEN the two ceilings - 2000 photons, legitimate at the real ceiling of 3000, refused at the wrong one of 1500. Equal values hide conflations; this is the smallest fixture in which the two readings disagree. A companion test asserts the fixture itself still straddles, because if those ceilings ever coincide the main test proves nothing while still passing. THE SCAN IS NOW SYMMETRIC. A Bitcoin reserve converted with the RADIANT interval is the same defect and was covered by nothing. I found that by accident: a restore during this verification rewrote btc_claim_reorg_depth to use rxd_block_interval_s, and the whole suite passed. Both directions are now scanned. The field-to-chain map is read off MarginPolicy by prefix instead of a hand-typed tuple of two names. A hand-kept list of what a guard covers is the same artifact that produced the bug being guarded - #511 added rxd_claim_inclusion and it was left off a validation list, fail-open. Deriving it means a new rxd_/btc_ reserve is covered the day it is added. Verified by planting both directions, each caught (3 failures and 1). The restores were confirmed with git diff rather than by eye: the earlier bad restore above was a string replace that silently matched an EARLIER, correct call site, leaving the planted defect live while reporting success. Co-Authored-By: Claude Opus 5 (1M context) --- ...diant_reserves_use_the_radiant_interval.py | 175 +++++++++++++----- tests/test_watch_claim_executor.py | 77 +++++++- 2 files changed, 207 insertions(+), 45 deletions(-) diff --git a/tests/test_radiant_reserves_use_the_radiant_interval.py b/tests/test_radiant_reserves_use_the_radiant_interval.py index 82b74219..009b5fbe 100644 --- a/tests/test_radiant_reserves_use_the_radiant_interval.py +++ b/tests/test_radiant_reserves_use_the_radiant_interval.py @@ -20,81 +20,168 @@ 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. +WHAT THIS COVERS, AND WHAT IT DOES NOT. The scan is per CALL, over the AST. It was +per LINE, over a regex, and that missed a site that had shipped: in +`claim_executor._check_value_cap` the field and the interval sit on two different +lines of one call, so no line contained both. Measured — reverting that site passed +the entire suite (10,993 tests), its own module, AND this file. The behavioural +guard for it now lives in `TestTheValueCapReadsTheRadiantInterval`. + +IT IS ALSO SYMMETRIC NOW. A Bitcoin reserve converted with the RADIANT interval is +the same defect and was covered by nothing. Found by accident: a bad restore +rewrote `btc_claim_reorg_depth` to use `rxd_block_interval_s` and the whole suite +passed. The field-to-chain map is read off `MarginPolicy` by prefix rather than +hand-typed, so a new `rxd_`/`btc_` reserve is covered the day it is added — the +previous hand-kept tuple is the artifact that let #511's `rxd_claim_inclusion` be +left off a list in the first place. + +Still not covered: a regression INSIDE `_radiant_reserve_blocks`, whose parameter is +a generic `reserve` with no field name anywhere in the call. That half is covered by +`test_assess_claim_finality_parity_sweep_byte_equivalent`, whose reference derives +the interval from the rule independently and 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 +import ast 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") +#: Which interval belongs to which chain. The ONLY two spellings that exist. +_INTERVAL_FOR = {"rxd": "rxd_block_interval_s", "btc": "block_interval_s"} + + +def _chain_tagged_reserves() -> dict[str, str]: + """Every ``Timelock`` field on :class:`MarginPolicy` that names a chain, DERIVED. + + This was a hand-typed tuple of two field names. A hand-kept list of the things a + guard covers is the same artifact that produced the bug it guards: ``#511`` added + ``rxd_claim_inclusion`` and it was left off the validation list, fail-open. Reading + the prefix off the dataclass means a new ``rxd_``/``btc_`` reserve is covered the + day it is added, by nobody remembering anything. + """ + from pyrxd.btc_wallet.taproot import Timelock + from pyrxd.gravity.swap_coordinator import MarginPolicy -#: 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"(? set[str]: + """Every attribute and bare name mentioned anywhere inside *node*. + + Exact identifiers, which is the point of using the AST rather than the text: + ``rxd_block_interval_s`` CONTAINS ``block_interval_s`` as a substring, so the + line-based pattern this replaces had to exclude the correct spelling by hand. + """ + out: set[str] = set() + for child in ast.walk(node): + if isinstance(child, ast.Attribute): + out.add(child.attr) + elif isinstance(child, ast.Name): + out.add(child.id) + return out def _offenders() -> list[str]: + """Call sites converting a chain's reserve with the OTHER chain's interval. + + Per CALL, not per LINE. The line-based version could not see the shape that + shipped in `claim_executor._check_value_cap`, where the field and the interval sit + on two different lines of one call — measured: reverting that site passed the + entire suite, and this file, and was caught by nothing. + """ + tagged = _chain_tagged_reserves() 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: + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + for node in ast.walk(tree): + if not isinstance(node, ast.Call): continue - if _BAD.search(line): - hits.append(f"{path.relative_to(_ROOT)}:{lineno}: {stripped[:100]}") + names = _identifiers(node) + for field, chain in tagged.items(): + if field not in names: + continue + wrong = _INTERVAL_FOR[next(c for c in _INTERVAL_FOR if c != chain)] + if wrong in names and _INTERVAL_FOR[chain] not in names: + hits.append(f"{path.relative_to(_ROOT)}:{node.lineno}: {field} ({chain}) converted with {wrong}") 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" + tree = ast.parse("blocks = _reserve_to_blocks(policy.rxd_claim_burial, policy.block_interval_s)") + call = next(n for n in ast.walk(tree) if isinstance(n, ast.Call)) + names = _identifiers(call) + assert "rxd_claim_burial" in names and "block_interval_s" in names + assert "rxd_block_interval_s" not in names + + +def test_the_scanner_sees_a_call_SPLIT_OVER_LINES() -> None: + """The shape that defeated the line-based version, and the reason for the rewrite. + + In `claim_executor._check_value_cap` the field and the interval are on separate + lines of one call. A per-line regex matches neither line.""" + src = ( + "burial = self._policy.rxd_claim_burial.normalize_to(\n" + " TimeUnit.BLOCKS, block_interval_s=self._policy.block_interval_s\n" + ").value\n" + ) + call = next( + n for n in ast.walk(ast.parse(src)) if isinstance(n, ast.Call) and "rxd_claim_burial" in _identifiers(n) + ) + names = _identifiers(call) + assert "block_interval_s" in names and "rxd_block_interval_s" not in names 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) + so a naive pattern flags every correct call site — a guard refusing valid work, on + the very line that fixes the bug.""" + tree = ast.parse("b = _reserve_to_blocks(policy.rxd_claim_burial, policy.rxd_block_interval_s)") + names = _identifiers(next(n for n in ast.walk(tree) if isinstance(n, ast.Call))) + assert "block_interval_s" not in names, "the AST must not see a substring" + + +def test_the_scan_is_SYMMETRIC() -> None: + """A Bitcoin reserve converted with the RADIANT interval is the same defect. + + Not hypothetical: while verifying this file I accidentally rewrote + `btc_claim_reorg_depth` to use `rxd_block_interval_s`, and the whole suite — + 10,993 tests — passed. The old scanner only looked one way.""" + assert _chain_tagged_reserves()["btc_claim_reorg_depth"] == "btc" + tree = ast.parse( + "d = policy.btc_claim_reorg_depth.normalize_to(BLOCKS, block_interval_s=policy.rxd_block_interval_s)" + ) + names = _identifiers(next(n for n in ast.walk(tree) if isinstance(n, ast.Call))) + assert "btc_claim_reorg_depth" in names and "rxd_block_interval_s" in names -def test_no_radiant_reserve_is_converted_with_the_bitcoin_interval() -> None: +def test_no_reserve_is_converted_with_the_other_chains_interval() -> None: offenders = _offenders() assert not offenders, ( - "a RADIANT-chain reserve is being converted with the BITCOIN interval:\n " + "a chain's reserve is being converted with the OTHER chain's 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" +def test_the_derivation_still_finds_the_fields_this_guards() -> None: + """Stale-guard ratchet. If every field were renamed out from under the prefix + rule, the scan would pass forever by covering nothing.""" + tagged = _chain_tagged_reserves() + assert {"rxd_claim_burial", "rxd_claim_inclusion", "btc_claim_reorg_depth"} <= set(tagged) diff --git a/tests/test_watch_claim_executor.py b/tests/test_watch_claim_executor.py index 600308d2..71129a18 100644 --- a/tests/test_watch_claim_executor.py +++ b/tests/test_watch_claim_executor.py @@ -8,6 +8,7 @@ from __future__ import annotations +import dataclasses import hashlib import json import os @@ -19,7 +20,7 @@ from pyrxd.btc_wallet.htlc_leg import BitcoinTaprootLeg from pyrxd.btc_wallet.keys import generate_keypair from pyrxd.btc_wallet.payment import BtcUtxo -from pyrxd.btc_wallet.taproot import btc_txid_from_raw +from pyrxd.btc_wallet.taproot import Timelock, TimeUnit, btc_txid_from_raw from pyrxd.gravity.swap_coordinator import MarginPolicy from pyrxd.gravity.swap_state import NegotiatedTerms, SwapRecord, SwapState from pyrxd.gravity.watch import ( @@ -911,3 +912,77 @@ async def test_a_REACHABLE_state_still_broadcasts(self) -> None: """ ex, _leg, rec, _ = await _armed_executor(confs=24, btc_confs=10) assert await ex.execute("s1", rec, _claim_decision()) is ExecOutcome.BROADCAST + + +class TestTheValueCapReadsTheRadiantInterval: + """`_check_value_cap` converts a RADIANT burial, so it must use RADIANT's interval. + + #579 fixed seven sites; this one had NO test that failed when reverted. Measured + rather than argued: switching it back to `policy.block_interval_s` passed all + 10,993 tests, `test_watch_claim_executor.py`, and the scanner written for exactly + this defect class — which is line-based and cannot see a call split over two + lines, the shape here. + + It survives because it is INERT while every fixture tags the burial in BLOCKS: + `normalize_to` is then the identity and the interval argument is never read. No + test in this file supplied a SECONDS-tagged burial, so the wrong argument and the + right one produced identical output. **A test whose fixture cannot express the + defect passes for a reason unrelated to the code being correct.** + + So the fixture below deliberately makes the two intervals DIFFER (600 vs 300) and + picks an amount BETWEEN the two ceilings. Equal values hide conflations; this is + the smallest fixture in which the two readings disagree. + """ + + #: 1800 s is 6 Radiant blocks (ceiling 3000) or 3 Bitcoin blocks (ceiling 1500). + BURIAL_S, RXD_INTERVAL, BTC_INTERVAL = 1800, 300.0, 600.0 + COST, FACTOR = 1000, 2.0 + CEILING_RADIANT, CEILING_BITCOIN = 3_000, 1_500 + #: Between the two. Legitimate at the real ceiling, refused at the wrong one. + AMOUNT = 2_000 + + def _policy(self) -> MarginPolicy: + return dataclasses.replace( + MarginPolicy.estimated(), + block_interval_s=self.BTC_INTERVAL, + rxd_block_interval_s=self.RXD_INTERVAL, + rxd_claim_burial=Timelock(self.BURIAL_S, TimeUnit.SECONDS), + ) + + async def _gate(self, amount: int): + terms, _p, raw, txid, locator, _b = await _build_real_claim(radiant_amount=amount) + ex = ClaimExecutor( + resolve_leg=_resolver(_FakeRadiantLeg(_FakeChainIO())), + claim_status_source=_FakeStatusSource(claim_txid=txid), + claim_bytes_source=_FakeBytesSource({txid: raw}), + policy=self._policy(), + network="mainnet", + reorg_cost_per_block=self.COST, + reorg_safety_factor=self.FACTOR, + claim_dust_ceiling=10_000, + enable_autonomous_mainnet_custody=True, + ) + rec = SwapRecord(state=SwapState.SECRET_REVEALED, terms=terms, counterchain_locator=locator) + return ex._check_value_cap(rec) + + def test_the_two_intervals_really_do_disagree_here(self) -> None: + """Guards the fixture itself. If these ever coincide the test below proves + nothing while still passing — the failure mode it exists to catch.""" + policy = self._policy() + radiant = policy.rxd_claim_burial.normalize_to(TimeUnit.BLOCKS, block_interval_s=policy.rxd_block_interval_s) + bitcoin = policy.rxd_claim_burial.normalize_to(TimeUnit.BLOCKS, block_interval_s=policy.block_interval_s) + assert radiant.value == 6 and bitcoin.value == 3 + assert self.CEILING_BITCOIN < self.AMOUNT < self.CEILING_RADIANT + + async def test_an_honest_claim_inside_the_RADIANT_ceiling_is_allowed(self) -> None: + """The assertion that fails when #579 is reverted here. With Bitcoin's + interval the burial reads 3 blocks, the ceiling halves to 1500, and this + legitimate 2000-photon claim is refused — a guard refusing valid work, and + for an autonomous claim a refusal can mean the swap is not claimed at all.""" + assert await self._gate(self.AMOUNT) is None + + async def test_a_claim_above_the_RADIANT_ceiling_is_still_refused(self) -> None: + """The other half. A gate that allows everything would also pass the test + above, so pin that it still says no — and that it names the real ceiling.""" + reason = await self._gate(self.CEILING_RADIANT + 1) + assert reason is not None and str(self.CEILING_RADIANT) in reason From 839a4dbfd598843b488a68d40dd0052b192e6bfb Mon Sep 17 00:00:00 2001 From: Mudwood Labs Date: Thu, 3 Sep 2026 08:47:21 -0700 Subject: [PATCH 2/2] make the reserve scan prove it actually scanned `_offenders()` returning [] is the PASS condition, and it also returns [] when the scan walked nothing at all - a repointed `_SRC`, a changed glob, or the source moving under it all read as "clean". A guard whose success and whose vacuity produce the same output is not a guard. That is the same defect this file's own subject had, and the same one that let the browser render-drift suite pass over fields it never rendered: structural about the CHECK, silent about whether the check had any subject. Two assertions, both derived rather than remembered: * the scan must reach a plausible amount of source (>50 files, currently 209 and 15,688 Call nodes) * every field `_chain_tagged_reserves()` derives must appear in at least one CALL in that tree. A field nobody converts anywhere is a field this scanner cannot be protecting, however many files it walks, and it would pass forever. Measured: rxd_claim_burial 7 calls, btc_claim_reorg_depth 6, rxd_claim_inclusion 3, and a synthetic never-converted field 0 - so the assertion has a real gap to detect and is not merely true by construction. Verified by repointing `_SRC` at an empty directory: 4 failures where the scan alone reported clean. Co-Authored-By: Claude Opus 5 (1M context) --- ...diant_reserves_use_the_radiant_interval.py | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/test_radiant_reserves_use_the_radiant_interval.py b/tests/test_radiant_reserves_use_the_radiant_interval.py index 009b5fbe..0ac39e1b 100644 --- a/tests/test_radiant_reserves_use_the_radiant_interval.py +++ b/tests/test_radiant_reserves_use_the_radiant_interval.py @@ -50,6 +50,8 @@ import ast from pathlib import Path +import pytest + _ROOT = Path(__file__).resolve().parent.parent _SRC = _ROOT / "src" @@ -185,3 +187,39 @@ def test_the_derivation_still_finds_the_fields_this_guards() -> None: rule, the scan would pass forever by covering nothing.""" tagged = _chain_tagged_reserves() assert {"rxd_claim_burial", "rxd_claim_inclusion", "btc_claim_reorg_depth"} <= set(tagged) + + +# --- the scan must prove it actually scanned ------------------------------- +# +# `_offenders()` returning [] is the PASS condition, and it also returns [] when it +# walked nothing at all: a repointed `_SRC`, a changed glob, or the source moving +# under it all read as "clean". A guard whose success and whose vacuity produce the +# same output is not a guard — the same defect this file's own subject had, and the +# same one that let the browser render-drift suite pass over unrendered fields. + + +def _calls_mentioning(field: str) -> int: + """How many Call nodes in the scanned tree mention *field* at all.""" + total = 0 + for path in sorted(_SRC.rglob("*.py")): + for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"), filename=str(path))): + if isinstance(node, ast.Call) and field in _identifiers(node): + total += 1 + return total + + +def test_the_scan_reaches_a_plausible_amount_of_source() -> None: + files = list(_SRC.rglob("*.py")) + assert len(files) > 50, f"only {len(files)} source files found — is _SRC still the package root?" + + +@pytest.mark.parametrize("field", sorted(_chain_tagged_reserves())) +def test_each_guarded_field_is_actually_REACHED_by_the_scan(field: str) -> None: + """The sharper check. A field nobody converts anywhere is a field this scanner + cannot be protecting, however many files it walks — and it would pass forever.""" + count = _calls_mentioning(field) + assert count > 0, ( + f"{field!r} is derived as a chain-tagged reserve but appears in NO call in the " + f"scanned source, so the scan covers it vacuously. Either the field is dead " + f"(remove it) or the scan is no longer reading the right tree." + )