diff --git a/tests/test_radiant_reserves_use_the_radiant_interval.py b/tests/test_radiant_reserves_use_the_radiant_interval.py index 82b74219..0ac39e1b 100644 --- a/tests/test_radiant_reserves_use_the_radiant_interval.py +++ b/tests/test_radiant_reserves_use_the_radiant_interval.py @@ -20,23 +20,34 @@ 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 @@ -44,57 +55,171 @@ _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"} -#: 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"(? 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 + + tagged = {} + for name, field in MarginPolicy.__dataclass_fields__.items(): + if field.type not in (Timelock, "Timelock"): + continue + for chain in _INTERVAL_FOR: + if name.startswith(f"{chain}_"): + tagged[name] = chain + if not tagged: + raise AssertionError("no chain-tagged reserve fields found — the derivation is broken") + return tagged + + +def _identifiers(node: ast.AST) -> 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 +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) + + +# --- 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. - assert field in MarginPolicy.__dataclass_fields__, f"{field} no longer exists; update this scanner" + +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." + ) 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