invert the HTLC timelock direction (#482) - #559
Open
Zyrtnin wants to merge 12 commits into
Open
Conversation
… tests to migrate
DO NOT MERGE. The source-side inversion is complete and verified; the test suite
encodes the old relation in 531 places and is red.
VERIFIED AGAINST THE PAPER. Herlihy arXiv:1801.09515 §1: Alice generates the
secret and locks at 6-delta; Bob 5-delta; Carol 4-delta; Alice CLAIMS Carol's
4-delta leg. The secret-holder's LOCKED leg is the LONGEST, the leg they CLAIM the
shortest. Lemma 4.13 gives the gap. Our maker generates p, LOCKS the Radiant
covenant and CLAIMS the counter leg, so t_rxd must be the longer one.
Behaviour now, checked directly:
t_btc=72 t_rxd=144 ACCEPTED (longer by 72 >= margin 36)
t_btc=144 t_rxd=72 refused (the old direction)
t_btc=72 t_rxd=100 refused (longer by 28 < margin)
Changed: `assert_timelock_margin` (relation + message), the sizer's budget (minus
-> plus, with the margin's components unchanged because every one was always time
the TAKER needs after the reveal), `NegotiatedTerms`' cheap construction guard, the
role-invariant constant, and the field comments.
THE ATTACK WAS DOCUMENTED IN-TREE. `taker_should_act_before_rxd_maturity`'s
docstring read: "because t_BTC > t_RXD, a malicious maker can withhold the BTC
claim until after t_RXD opens, then claim BTC (revealing p) AND CSV-refund the
asset, taking both." That is #482 finding 1 stated exactly — mitigated with a
timing predicate asking the taker to bail early, rather than by fixing the
relation. The predicate is kept (it still detects a stalled maker) with its
rationale corrected.
WHY THIS STOPS HERE. 531 failures across 14 files, led by
test_eth_swap_run_timelock_bounds (100), test_swap_coordinator (58),
test_watch_decide (51). Many are not value swaps: a test asserting the ordering
guard REFUSES a pair needs its EXPECTATION inverted, not its numbers. Flipping 531
fixtures at pace is the exact condition under which "make it green" substitutes
for "make it correct", on the one change where being wrong recreates the bug
mirror-imaged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wo findings
test_swap_coordinator: 58 failures -> 25. `_terms` and `_eth_terms` now DERIVE
t_btc from t_rxd rather than each call site hand-maintaining a relation that just
changed direction — that is how a relation drifts out of a suite one site at a
time. The direct margin tests are inverted deliberately (argument order kept,
expectations flipped), and the invariant-constant test now asserts the NEW text
and that the old text is absent.
Added tests/test_timelock_ordering_invariant.py: the rule stated ONCE, with
Herlihy's own 6-delta/5-delta/4-delta example parametrised across scales, the old
direction refused, and the theft window asserted empty by construction.
FINDING 1 — a small t_rxd is no longer expressible. The invariant needs
`t_rxd > t_btc + margin` with t_btc >= 1, so `t_rxd <= margin` cannot be
constructed at all. The burial-band tests used t_rxd as low as 6. This is a real
consequence of the inversion, not a fixture problem: the margin check now subsumes
part of what gate 3b caught, and 3b becomes unreachable for those values.
FINDING 2 — the SIZING ANCHOR must invert too, and the two runners disagree
about it today:
scripts/eth_swap_run.py:593 now + max_covenant_confirm_wait_s
scripts/eth_swap_two_host.py:437 now
Under the OLD relation (`rxd_refund_opens < eth_timeout - margin`) sizing from the
LATEST plausible confirm was the conservative end. Under the inverted relation
(`rxd_refund_opens > eth_timeout + margin`) an EARLY confirm is what shifts the
window left and breaks the invariant, so the conservative anchor becomes the
EARLIEST plausible confirm — `now`. eth_swap_run.py's anchor becomes UNSAFE;
eth_swap_two_host.py's is already right.
The punctuality gate follows from that: it inverts from "did the covenant confirm
LATER than assumed" to "did it confirm EARLIER", and once sized from `now` an
earlier confirm is only reachable through clock skew, so the gate largely
collapses into asserting the actual confirm is at or after the sizing anchor.
Neither finding is in the design doc's §5, which flagged the gate as the least
certain part and was right to.
STILL RED. Not for merge.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…te further SOURCE-SIDE WORK IS NOW COMPLETE. What remains is fixture migration. 1. THE UNSAFE ANCHOR — a real correctness fix, not a test change. scripts/eth_swap_run.py sized `expected_rxd_lock_time_unix_s` from `now + max_covenant_confirm_wait_s`, the LATEST plausible confirm. eth_swap_two_host.py used `now`. The two runners disagreed and nothing reconciled them. The refund opens at `actual_confirm + t_rxd`, and t_rxd is committed before broadcast. Confirming LATER than assumed only adds margin; confirming EARLIER breaks the invariant. So the conservative anchor is the EARLIEST plausible confirm — `now`. Reserving the wait was correct under the OLD relation and is exactly wrong under this one. Both runners now agree, and a test pins the agreement (verified by restoring the old anchor: 1 fails). 2. THE PUNCTUALITY GATE now asserts the INVARIANT rather than a proxy for it. It asked "does the covenant confirm by the time the sizing assumed" — the right question when a LATE confirm pushed the refund past the deadline. Inverted, a late confirm is safe and an early one eats the margin, so the check is now `earliest_confirm + t_rxd >= eth_timeout + margin` taken at `now` with no allowance added. `max_covenant_confirm_wait_s` stays validated and meaningful operationally, but no longer belongs in that arithmetic. 3. FIXTURES. `_eth_terms` derives t_rxd from the ETH deadline the way production sizes it — a hardcoded block count cannot be right for an arbitrary deadline, and every ETH test then fails on a margin it was not testing. My first attempt derived against 600 s when the ETH fixtures configure 300 s, which halved the window; the comment records that so the next reader does not repeat it. test_swap_coordinator: 58 -> 20 failures. 13 other files not yet migrated. STILL RED. Not for merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n exposed test_swap_coordinator is GREEN (58 -> 0). Two real defects came out of the migration; neither is a fixture problem. 1. THE INVERSION SILENTLY DROPPED THE EXPIRED-DEADLINE REFUSAL. The old gate demanded the RXD refund open BEFORE eth_timeout, so an expired or near-expiry deadline failed automatically. Inverting made a close deadline the EASY case, so both tests covering it went green-by-vacuity and the protection vanished with nothing to show for it. A requirement that holds only as a side effect of arithmetic is one refactor away from gone. `assert_eth_deadline_is_claimable` now asserts it directly: the maker cannot act until the counter leg is final, so the deadline must leave room for finality + stall + rounding. Pre-fund gate ONLY — after funding the deadline is legitimately closer every second, and applying it there would refuse honest swaps for being underway. 2. THE ORDERING GATE COMPARED AGAINST A WINDOW THE MAKER HAD ALREADY SPENT. `t_rxd` is a relative CSV from covenant MINING. Step 3 anchors at `now`, correct only if the covenant mines now — it does not, it was locked first and verified at step 5. Every confirmation it already has is a block of `t_rxd` gone, and the MAKER picks that number by locking early and presenting late. Step 3 sees a t_rxd that outlasts eth_timeout + margin; the chain opens the refund sooner by exactly the elapsed depth, which is when the maker refunds RXD while still holding p for the ETH leg. Under the OLD relation an overstated window was the CONSERVATIVE direction, which is why it survived every prior review. Inverting turned the same arithmetic into the dangerous one. This is the #531 conflation exactly — that fix corrected it for the burial floor and left the ordering gate on the negotiated value. Step 7 re-runs the ordering check against the REMAINING window, where cov_confs is known. Proved by planting: with the elapsed subtraction removed the attack test fails and the honest-path test still passes, so the assertion is load-bearing and is about DEPTH, not about the terms. Also: `_assert_eth_timelock_ordering`'s docstring asserted the OLD relation in full confidence one scroll above corrected code. Prose that states an invariant becomes evidence to the next reader; wrong, it is manufactured corroboration (the #505 shape). Corrected rather than left to be cited. FIXTURES. Scenarios were scaled UP, not loosened: under the inverted relation a t_rxd below the margin is not a swap that can exist, so testing the burial floor there would have been testing a fiction. The default ETH deadline was anchored to the frozen clock, which also defuses a hardcoded 1779710245 that has since passed in real time. Two tests had their EXPECTATION inverted, not their numbers: a stalled maker lock is now safe and must be ACCEPTED, and "deadline too close" is replaced by the direction that actually robs the taker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
THIS IS THE MOST SERIOUS THING #482 TURNED UP, and it is not in the source. `conformance/htlc-handshake-vectors.json` is a PUBLISHED artifact whose stated purpose is to let a second implementation check its bytes against pyrxd. It encoded the inverted timelock rule, in three places at once: notes.timelock_ordering: "t_btc MUST exceed t_rxd by at least the verifier's own margin. The taker locks first and holds the LONGER refund." margin-ok-gap-40-margin-36 t_btc 60 / t_rxd 20 -> ACCEPT margin-inverted-ordering t_btc 20 / t_rxd 60 -> REJECT The second of those is the CORRECT ordering, published under the name "inverted-ordering" and marked reject. So an implementer who got the direction right would fail conformance, and the suite would tell them — with the authority of a spec and a passing test run — to swap it to the layout where the party holding `p` can refund the leg it locked and still claim the other. A conformance suite that is merely incomplete misses defects. This one PROPAGATES a defect, and does it to exactly the audience least able to argue back: someone porting the protocol who has no independent view of which direction is right. Every vector is REGENERATED from the builders, not hand-edited — the covenant SPK commits to `refund_csv = t_rxd` and the BTC P2TR taptree commits to `t_btc`, so both sets of bytes change with the swap and hand-editing the numbers would have left the hashes describing the old layout. `margin-cross-unit-inversion` keeps its purpose: t_btc 43200s (72 blk) vs t_rxd 12 blk is now a genuine inversion the same-unit construction guard cannot see. The test file argued FOR the wrong rule too, calling it "counterintuitive and load-bearing" — the shape where prose asserting an invariant becomes evidence for the next reader (#505). Corrected, with Herlihy cited. Also migrated: test_radiant_leg derives t_btc from the covenant's own CSV (a fixed 72 is unconstructible once csv drops to its default of 6), and the canonical t_rxd=72/t_btc=144 pair is inverted across 14 files. Full suite 543 -> 194 failures. tests/test_swap_coordinator.py, test_radiant_leg, test_btc_htlc_leg, test_watch_quorum, test_watch_reconciler and 6 others green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A defect I introduced two commits ago and did not see: inverting `budget_s` in `eth_absolute_to_rxd_relative_blocks` changed the gate from an UPPER bound on t_rxd to a LOWER one, but the rounding and the search direction were left as they were. t_rxd = ceil(budget / interval) - 1 the LARGEST t with t*I < budget ...on refusal: t_rxd -= 1 step toward acceptance Against a lower bound both are backwards. The starting value is one block short of the smallest acceptable one, and every step moves further from acceptance, so the loop ran its full 3 steps and raised "the sizer and the gate disagree by more than a rounding step" — a message about float rounding for what was a reversed inequality. 50 tests across two files failed on it and I read the message as a fixture problem for longer than I should have. Now `ceil(budget / interval)` with `+= 1` on refusal. The post-loop re-check flipped with it: the step-down could cross the safety FLOOR, the step-up can cross the BIP68 CAP. Re-checking the floor after an upward search is a check that can no longer fail, and would have left the cap unguarded — the same pairing error one level up. test_t_rxd_sizer_and_gate_agree is green, including its three-way boundary property (gate accepts sized / refuses sized-1 / matches an independent exact rational model), which is what localised this. Two of its tests needed their MEANING inverted rather than their numbers: - the sizer is now the SMALLEST value the gate accepts, not the largest, so the give-away direction is upward: overshooting locks the maker's asset longer than the swap needs, where it used to shorten the taker's claim window. - the dangerous interval MISMATCH swapped ends. Sizing at the fast tail and checking at the nominal used to refuse; it now over-satisfies and passes. What refuses is the reverse pairing. Left alone that test goes green while the defect it exists for is still reachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d it Two more defects from the same inversion, both found by tests that had to be re-derived rather than re-numbered. 1. THE SIZER RETURNED A WINDOW ONE BLOCK LONGER THAN REQUIRED. Starting at `ceil(budget / interval)` and stepping UP can only ever move away from the minimum. `ceil` is the exact minimum in exact arithmetic, but at fractional intervals the float division rounds the quotient up across an integer boundary, and nothing could then find the shorter window — the search only goes one way. Now starts one below and steps up, bounded by the floor. This is the mirror of the pre-existing note about `ceil(x) - 1` legitimately being one block LOW, which is how the shape was recognised. Not a fund-safety bug: everything returned has been accepted by the gate. It locks the maker's asset longer than the swap needs. 2. THE PROPERTY TEST'S CONSERVATISM ASSERTION DROPPED THE GATE'S CEIL. The gate compares `ceil(t * interval)`. Under the old inequality the raw product was a conservative proxy — never larger — so omitting the ceil was safe, and the assertion had none. Reversed, the same omission understates the window by up to a second and fails on CORRECT output: at eth_timeout=44, interval=1.5 the sizer returns 29, whose 43.5s projects to ceil = 44 and exactly meets a 44s budget. That case was already pinned as an @example from an earlier hypothesis flake, so it failed immediately rather than waiting for a seed to find it again. test_eth_rxd_timelock: every direction-bearing test inverted, not renumbered. Stall tolerance now GROWS the maker's lock instead of shrinking the taker's window (the margin is added, not subtracted). The gate refuses an EARLY covenant confirmation, not a late one — and a new paired test requires a LATE one to be ACCEPTED, since that is now the safe direction and refusing it would be a guard refusing valid work on the swap's commonest honest deviation. `test_gate_failclosed_on_confirm_wait_squeeze` changed purpose and says so: the confirm-wait is inert in the arithmetic now, so what is worth pinning is that it STAYS inert and cannot be quietly reintroduced. Full suite 543 -> 233 failures. 15 files green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three patterns worth naming, because the mechanical swap was wrong in all three. DERIVE WHERE CALLERS VARY THE INPUT. test_watch_v2's `_terms` hardcoded t_rxd=144 while callers pass t_btc freely (144, 200, a SECONDS value) — so the fixed value collided with whichever caller happened to match it. t_rxd now derives as t_btc + 72 for BLOCKS and no caller can collide. SHRINK THE OTHER SIDE WHEN THE CONSTANTS HANG OFF ONE. test_watch_decide derives REFUND_OPENS = LOCK + t_rxd and dozens of `now=` literals from it. Inverting 72/144 the usual way moves it 172 -> 244 and invalidates every one. Keeping t_rxd=72 and dropping t_btc to 36 satisfies the ordering and leaves the heights — which are what those tests are actually about — untouched. RELATIVE CONSTANTS MOVE WITH THE PAIR. test_btc_locked_refunds_only_when_funding _matured checks confirmations against t_btc: 100 was immature at 144 and is well past maturity at 72, so the test would have asserted the opposite of its name. test_swap_state's ordering guard keeps both refusal cases and gains an honest-path test: (t_btc=50, t_rxd=72) was refused before and is now the CORRECT layout, so it is asserted as valid rather than deleted — a refusal test whose input became legal is precisely the kind that goes green by vacuity when it is merely renumbered. Full suite 543 -> 152 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dering Found by a test, not by review — and only after the fixture work had cleared enough noise for it to surface. dust_swap_run.py:132 t_btc = t_rxd + margin + 4 eth_swap_run.py:719 t_btc = t_rxd + margin + 4 eth_swap_two_host.py:249 t_btc = t_rxd + margin + 4 That is the layout where the party holding `p` can refund the leg it locked and still claim the other. All three now build `t_rxd - margin - 4`. They were found by grepping every `t_btc =` construction in scripts/ and src/ as a SET after the first one turned up, rather than fixing the one the failing test named — the same discipline the CREATE-address and token-vs-photon fixes each missed the first time. BOTH TWO-HOST SELF-CHECKS WERE PASSING FOR THE WRONG REASON. Each builds a "hostile too-tight envelope" and asserts the margin check rejects it. Written in the old direction those envelopes no longer CONSTRUCT — the ordering guard rejects them first — so `assert_timelock_margin` never ran and the check passed on an exception raised by something else entirely. Both are inverted so the envelope constructs and is refused by the check under test. This is the failure mode where a refusal is triggered by the wrong thing and looks identical to success. test_eth_timelock_mutant_killers: boundaries re-MEASURED against the real sizer, not re-derived by hand — the file's own comment records that deriving them by hand is what put an off-by-one in it the first time. A `_size_for_budget` helper now names the budget directly, since every boundary here is about the budget and the old `7068 + N` idiom silently means a budget 14136s off once the margin changed sign. The step-walk class inverts with the search: `emitted - analytic`, because left as `analytic - emitted` it returns a negative and blames the arithmetic for drifting when only the direction of the walk moved. Full suite 543 -> 105 failures, 92 of them in one file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e defect class The runner's feasibility system assumed bound B caps t_rxd from above. Under the corrected relation it is a FLOOR, and that changes the shape of the set rather than a comparison in it. BEFORE: floors below, the deadline capping above, and the feasible set an interval squeezed between them. Bound B's cap and bound C's floor were algebraically the same integer and rounded ONE APART at a fractional fast tail — a measured p10, i.e. the normal case — so the set went empty while a closed-form sum said the deadline was roomy. The operator was refused --t-rxd-blocks 2324 and advised, in the same message, to omit the flag and let it derive 2324. Two funded covenants were burned on that. AFTER: every bound is a floor and the only ceiling is the BIP68 field width, so the set is [max(floors), 65535] and cannot be emptied by two bounds disagreeing about a rounding. There is nothing left to disagree. The class is gone by construction, not closed by a better guard — and the test class that recorded it now asserts exactly that, on the ORIGINAL brute-forced rows, so a ceiling sneaking back below the cap turns them red first. THE 3-HOUR DEADLINE FROM THE INCIDENT IS NOW SIMPLY FINE: (497, 65535). What is infeasible instead is a deadline too FAR — past ~23 days no 16-bit relative CSV reaches it. Measured: 2_000_000s yields (55752, 65535), 2_400_000s yields None. SO THE REMEDY INVERTED, and this is the part that would have hurt an operator. `_smallest_workable_eth_timeout_s` searched UPWARD for more time. Upward now walks away from feasibility on every step, and had it found anything it would have advised lengthening a deadline that was already too long — a guard that is confidently wrong, names the right flag, and leads to the exact layout the swap must avoid. It is `_largest_workable_eth_timeout_s`, searching down. It also cannot walk down one second at a time from a value 400,000s past the boundary, so it starts from the analytic BIP68 limit (BIP68_MAX * fast - margin) and steps down to VERIFY. The upward search could begin at the requested value because feasibility started just above it; the infeasible region is unbounded above now, so the search has to begin at the boundary rather than walk to it. Two tests briefly papered over that with pytest.skip and the repo's skip audit refused them — the mechanism worked. `_assert_t_rxd_opens_before_the_eth_deadline` -> `_assert_t_rxd_outlasts_the_eth _deadline`, refusing "too SHORT" where it refused "too LONG". Its message says so explicitly: the old text told operators to shorten a window that needed lengthening. test_eth_swap_run_timelock_bounds: 92 -> 15 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…anished test_eth_swap_run_timelock_bounds is green (92 -> 0). TWO CLASSES OF DEFECT ARE NOW UNREPRESENTABLE, and both are asserted on their ORIGINAL brute-forced rows rather than being deleted — the rows are the evidence that these specific inputs are fixed, which a fresh property test would not give. - L-15, the empty feasible set. Bound B's cap and bound C's floor were the same integer and rounded one apart at a fractional tail. With every bound a floor and only the BIP68 field width above, there is nothing left to disagree. Each row now yields a healthy range, and the test additionally pins that `hi` IS the field width — a ceiling coming back below it turns these red first. - L-17, the derivation landing outside the set. The sizer aimed at the largest accepted value and the runner capped a block lower. Both aim at the same floor now, so `derived == window[0]` EXACTLY — a sharper property than the old "lands inside", and the equality is what makes the gap unrepresentable. THREE TESTS WOULD HAVE ASSERTED A BUG had they been renumbered rather than re-derived: - the resume check. It was a cap that FELL as the deadline approached, so a t_rxd valid at the start could drift above it. The floor falls too — so a window long enough at the start is always still long enough, and demanding a refusal would strand the swap the check exists to save. Paired with a real resume refusal (too SHORT for what remains) so the honest path cannot be met by a bound that accepts everything. - the vulnerable-window floor, probed at one below the DERIVED value. The deadline floor dominates now and the derived value sits far above this bound, so that probe still satisfies it: the assertion was vacuous while looking identical to a real one. It locates this predicate's own floor by scanning it alone. - the feasible-range brute force, which probed past the top of the range. That top is the BIP68 width, so it walked into values the predicates accept and the range excludes — a disagreement about field width, not about bounds. Two tests were REMOVED with the reason recorded in place, not silently: both parsed a "minimum:" out of a refusal on rows that no longer refuse, and both are covered by the two tests added for those same rows. Full suite 543 -> 14 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FULL SUITE GREEN. 543 -> 0. Two more production sites, found the same way as the last three — by a test: watchtower_dust_run.py --t-btc must be > --t-rxd (CLI guard AND defaults) btc_swap_two_host.py self-check terms t_rxd=20 / t_btc=60 The watchtower's guard is the one to note: it enforced the exploitable ordering at the command line, with "(BTC is the longer leg)" in the message, so an operator following it built the layout where the party holding p takes both legs. Its --t-btc/--t-rxd defaults said the same thing. THE TEST-SIDE CHANGES ARE ALL RESCALINGS OF DERIVED CONSTANTS, and each would have been a silent false pass if the numbers had been left alone: - test_btc_htlc_leg passed 143 confirmations against what is now a 72-block CSV — long mature — while asserting a "not yet mature" refusal that cannot happen. - test_watch_claim_executor squeezed at height 199 against a refund that now opens at 244; the window was wide open and no squeeze occurred. Same for the corroborated-depth test, whose whole point is a SAFE->SQUEEZED flip. - test_swap_gate_binding squeezed at 1_070 against a 144-block t_rxd, leaving 74 blocks — nowhere near the 6-block burial. - test_swap_invariants I5 is a property test over a generated domain; written the old way it asserted the exploitable ordering across all of it. TWO FIXTURE DEFECTS THE MIGRATION EXPOSED: - `_eth_terms` sized t_rxd against ITS OWN module clock while other files import it and freeze time elsewhere. test_swap_gate_binding's _NOW is 59M seconds later, so a deadline written as 40_000s out asked for a t_rxd past the BIP68 cap. It takes the caller's clock now. - the derived t_rxd carried no headroom for covenant depth already elapsed. Step 7 checks the REMAINING window, and a MEASURED policy requires the covenant be burial-deep BEFORE funding — so a t_rxd sized to exactly meet the deadline is always short by that depth. The fixture now carries the allowance production must carry. The gate was right; the fixture was modelling a swap that cannot happen. Also: the sizer's "smallest the GATE accepts" property only holds where the gate is the binding constraint. Where the safety FLOOR stops the search the gate legitimately accepts less, and asserting otherwise demands a value below the floor. Found by hypothesis at floor_blocks=2. VERIFIED as CI runs it, not as the local default does (`-o "addopts="`, which is what stops 187 integration tests being silently deselected): pytest tests/security/ --cov=pyrxd.security --cov-fail-under=100 -> 866 passed, 100% ruff check + ruff format --check -> clean, 556 files mypy (the three CI paths) -> no issues reachability + timelock-ordering invariant -> 18 passed Full coverage run reported separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Zyrtnin
marked this pull request as ready for review
September 1, 2026 06:01
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
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 #482.
t_rxdmust exceedt_btc, not the reverse. The maker holds the preimagep, locks the Radiant leg and claims the counter leg, so the leg it locks carries the longer timeout (Herlihy, arXiv:1801.09515 §1). Built the old way, the maker can let its own leg mature, refund it, and still claim the counter leg withp— taking both.Full CI gate green: 10,505 passed, 90.42% coverage, security coverage 100%, mypy clean.
The published conformance vectors taught the exploitable direction
The most serious finding, and it is not in the source.
conformance/htlc-handshake-vectors.jsonexists so a second implementation can check its bytes against pyrxd. It encoded the inverted rule in three places:The second is the correct ordering, published under the name "inverted-ordering" and marked reject. An implementer who got the direction right would fail conformance and be told — with the authority of a spec and a passing run — to swap it to the vulnerable layout.
Every vector is regenerated from the builders, not hand-edited: the covenant SPK commits to
refund_csv = t_rxdand the BTC taptree commits tot_btc, so both sets of bytes move with the swap.Five production sites built the exploitable ordering
scripts/dust_swap_run.py:132t_btc = t_rxd + margin + 4scripts/eth_swap_run.py:719t_btc = t_rxd + margin + 4scripts/eth_swap_two_host.py:249t_btc = t_rxd + margin + 4scripts/watchtower_dust_run.py:237--t-btc > --t-rxdscripts/btc_swap_two_host.py:855t_rxd=20 / t_btc=60The watchtower one enforced it at the command line, with "(BTC is the longer leg)" in the refusal — an operator following the message built the vulnerable layout.
Found by grepping every
t_btc =construction as a set after the first turned up, not by fixing the one the failing test named.Defects the inversion itself introduced or exposed
Each was caught by a test, and each is the kind that survives review because the old direction made it the safe one.
The expired-deadline refusal silently vanished. It used to fall out of the old arithmetic; inverted, a close deadline is the easy case, so both covering tests went green-by-vacuity. Now asserted directly by
assert_eth_deadline_is_claimable. A requirement that holds only as a side effect is one refactor away from gone.The ordering gate compared against a window the maker had already spent.
t_rxdis a relative CSV from covenant mining; the gate anchored atnowand the covenant was locked earlier. The maker picks that gap by locking early and presenting late. Overstating the window was conservative under the old relation and is the theft direction under this one. New step 7 re-checks against the remaining window wherecov_confsis known — the [MEDIUM] pre_btc_lock_check 3b uses the negotiated t_rxd, not the remaining window — it admits swaps that are SQUEEZED at the earliest possible claim #531 conflation, one gate over. Proved by planting: the attack test fails without the subtraction, the honest-path test still passes.The sizer searched away from the gate. I inverted
budget_sand leftceil(x) - 1with a step-down loop against what is now a lower bound. Every step moved further from acceptance; 50 tests failed with "the sizer and the gate disagree", a message about float rounding for a reversed inequality.The sizer then overshot by a block, because
ceilis the exact minimum only in exact arithmetic and an upward-only search cannot recover. Starts one below now.The runner's feasible set was squeezed between a floor and a cap. Bound B is a floor now, every bound is a floor, and the only ceiling is the BIP68 field width — so the L-15 empty-set class (two bounds algebraically equal rounding one apart) and the L-17 derivation-gap class are unrepresentable, not fixed. The 3-hour deadline from the incident that burned two funded covenants is simply fine:
(497, 65535).The infeasibility remedy reversed.
_smallest_workable_eth_timeout_ssearched upward for more time. Upward now walks away from feasibility, and had it found anything it would have advised lengthening a deadline already too long.Tests: re-derived, not renumbered
543 failures. The mechanical swap was wrong in most of them, and several would have asserted a bug had the numbers just been updated:
test_swap_invariantsI5 is a property test over a generated domain; the old form asserted the exploitable ordering across all of it.Where a hardcoded value could collide with a caller's, fixtures now derive (
t_rxdfromt_btc,t_btcfrom the covenant's own CSV). Where constants hang off one side, the other side moved instead. Historical defect rows are kept and inverted rather than deleted — they are the evidence those specific inputs are fixed.Not done
_assert_eth_lock_timing_still_safe) no longer discriminates much: a late maker lock is now safe, and it does not read the covenant's actual depth. Step 7 covers the pre-fund case; the recheck deserves its own follow-up.