fix: refuse a zero relative lock however it is spelled - #612
Merged
Conversation
Assertive text is a claim, and no test evaluates claims — tests pin that a
sentence is emitted, never that it is correct. Five docstrings/comments in the
swap wallet layer had drifted from the code beside them. Each correction says
what is true and what the text used to say, so anyone who relied on the old
wording knows to re-check. NO BEHAVIOUR CHANGES.
taproot.refund_leaf_script — "Any caller that builds a leaf ... gets the floor by
construction" was false: the refusal is scoped to TimeUnit.BLOCKS. BIP68 quantises
time locks to 512 s, so Timelock(0..511, SECONDS) all encode nSequence 0x00400000
(zero time units) — the same no-op relative lock a 0-block CSV is — and are emitted
without complaint. swap_state.NegotiatedTerms carried the identical over-broad claim
("unrepresentable for every caller") over an identically BLOCKS-scoped condition;
corrected too, since leaving one would have contradicted the other.
btc_wallet.htlc_leg — "txid_of resolves ... VIA THE NODE — never a local segwit
parse" had the mechanism exactly backwards, in the BtcFundingReader Protocol, in
its txid_of stub, and in confirmations_of_claim's own docstring, which contradicted
the comment three lines below it. All three shipped readers (mempool.space,
Bitcoin Core, MultiSource) call taproot.btc_txid_from_raw. The safety property is
unchanged and is better served by the local derivation: the gated txid must be that
of the exact bytes p was scraped from, and serialising them asks nobody.
eth_wallet.erc20_leg.claim — "checked alongside both parties" describes round 4's
address list; round 5 removed the refundee on purpose (a claim never touches it, so
refusing there refuses valid work and hands the counterparty a free unilateral veto)
and the call site passes claimant only.
eth_wallet.htlc_leg module docstring — "funded balance == negotiated amount" where
the check is `bal < expected_amount_wei`, a deliberate LOWER bound (an == check is
griefable by a 1-wei force-send). verify_funded documents it correctly at the check;
EthHtlcLocator.amount_wei repeated the same == claim and is corrected with it.
eth_wallet.locator.EthHtlcLocator.chain_id — "the leg refuses a chain_id mismatch
up front" is false; nothing in src/ or scripts/ compares locator.chain_id to
anything. EthRpc.assert_chain compares the NODE to the LEG's expected_chain_id, a
different check. The field is now documented as a record, not a gate.
Two gaps are REPORTED rather than fixed, because both are behaviour changes on
fund-moving code and need their own review: the BLOCKS-only scoping of the zero-
timelock floors, and the absent locator/leg chain-id comparison.
Verified: ruff check + ruff format --check clean; full suite 10985 passed,
197 skipped, 4 xfailed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`refund_leaf_script`'s floor was `unit is BLOCKS and value < 1`, under a comment
saying "every caller gets the floor by construction". That was false for a
SECONDS-tagged Timelock. BIP68 quantises time locks to 512 s, so
Timelock(0..511, SECONDS) all encode to nSequence 0x00400000 - ZERO time units,
the same no-op relative lock, emitted without complaint:
Timelock( 0, seconds) -> leaf 03000040b275 accepted
Timelock(511, seconds) -> leaf 03000040b275 accepted (identical bytes)
Timelock( 0, blocks ) -> refused
REACHABLE END TO END. Nothing in `pyrxd` or `scripts/` constructs a SECONDS
`t_btc`, which is why this was first reported as latent - but the unit tag comes
off the WIRE. `NegotiatedTerms.from_dict` -> `_timelock_from_dict` accepts
`{"value": 0, "unit": "seconds"}`; `swap_state`'s own `t_btc` floor is scoped to
BLOCKS in exactly the same way so it is skipped; its ordering guard only fires
when both units match, so that is skipped too; and `assert_timelock_margin`
normalises to 0 blocks and passes. A counterparty-authored envelope therefore
produced a refund leaf spendable in its own funding block - the #482-shaped free
option, one unit tag away from the floor meant to stop it.
FIXED BY DERIVING THE CHECK FROM THE ENCODING rather than the tag. Whatever the
unit, whatever units are added later, a relative lock that encodes to zero is
refused. That is the difference between a floor and a floor for the one spelling
someone thought of, which is the defect class this repo keeps meeting.
IT REFUSES NOTHING HONEST. The only newly-rejected inputs are SECONDS 0..511,
every one of which is ALREADY a zero lock - a caller asking for 511 seconds is
silently getting none today. Both directions are planted: reverting to the
BLOCKS-only floor fails 6, and over-tightening to also refuse an honest 512 s
fails 3.
The accepted leaves are byte-identical, pinned by a test, because this leaf is
committed to the taptree: changing it would move the HTLC address and strand
every existing swap.
Found as a secondary assertion in a lead that called this latent. It is not
latent, and "no builder constructs one" is not the same claim as "no path
produces one" when a deserialiser reads the tag from an untrusted envelope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Zyrtnin
added a commit
that referenced
this pull request
Sep 4, 2026
…ed (#613) Five prose claims in fund-moving wallet code, flagged by a reviewer that **explicitly marked them unverified**. All five held on checking — but one's *secondary* assertion was wrong, in the direction of understating the problem, and that turned into #612. | lead | verdict | |---|---| | `taproot.py` "any caller gets the floor" | held — and the "latent" half is **wrong**, see #612 | | `htlc_leg.py` "`txid_of` resolves via the node" | held; **mechanism backwards, property fine** — all three shipped readers derive locally, which serves the property *better* | | `erc20_leg.py` "checked alongside both parties" | held — the call passes claimant only, deliberately, per the reasoning directly above it | | `htlc_leg.py` "funded balance == negotiated amount" | held — the check is `<`, a deliberate lower bound, correctly documented elsewhere | | `locator.py` "pins which network" | held, **and there is a real gap** | ## Reported, not fixed **Nothing compares `locator.chain_id` to anything.** `EthRpc.assert_chain` compares the *node* to the *leg's* expected chain id, so a leg pointed at the wrong network is caught — but a locator *from* a different network driven by a correctly-configured leg is not, and `claim()` against an address with no code on the target chain does not revert. Adding that check is a behaviour change on fund-moving code and wants its own review, so it is reported rather than slipped into a prose pass. ## Bonus, unprompted `EthHtlcLocator.amount_wei` carried the identical false `==` claim, and `swap_state.py` carried the twin over-broad "unrepresentable for every caller" — which would have contradicted the taproot correction sitting one call away. Comments and docstrings only; no behaviour changed. Verified independently: full suite green at baseline skip/xfail counts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Mudwood Labs <opensource@mudwoodlabs.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#613 documented the BLOCKS-only floor and said 'left as-is deliberately - widening the refusal is a behaviour change that needs its own review'. This branch IS that review, and the fix: the refusal now keys on the encoded magnitude, so it is unit-agnostic by construction. Kept the fix.
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
1 participant
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.
refund_leaf_script's floor wasunit is BLOCKS and value < 1, under a comment saying "every caller gets the floor by construction". That was false for a SECONDS-tagged Timelock. BIP68 quantises time locks to 512 s, soTimelock(0..511, SECONDS)all encode tonSequence = 0x00400000— zero time units, the same no-op lock, emitted without complaint:Reachable end to end, not latent
This was first reported as latent because nothing in
pyrxdorscripts/constructs a SECONDSt_btc. But the unit tag comes off the wire:NegotiatedTerms.from_dict→_timelock_from_dictaccepts{"value": 0, "unit": "seconds"}swap_state's ownt_btcfloor is scoped to BLOCKS in exactly the same way → skippedassert_timelock_marginnormalises to 0 blocks and passesSo a counterparty-authored envelope produced a refund leaf spendable in its own funding block — the #482-shaped free option, one unit tag away from the floor meant to stop it.
"No builder constructs one" is not the same claim as "no path produces one", when a deserialiser reads the tag from an untrusted envelope.
Fixed by deriving the check from the encoding
csv_script_operand() & SEQUENCE_LOCKTIME_MASK == 0— whatever the unit, whatever units are added later, a relative lock that encodes to zero is refused. That is the difference between a floor and a floor for the one spelling someone thought of, which is the defect class this repo keeps meeting.It refuses nothing honest
The only newly-rejected inputs are SECONDS 0..511, every one of which is already a zero lock — a caller asking for 511 seconds is silently getting none today. Both directions planted: reverting to the BLOCKS-only floor fails 6; over-tightening to also refuse an honest 512 s fails 3.
Accepted leaves are byte-identical, pinned by a test — this leaf is committed to the taptree, so changing it would move the HTLC address and strand every existing swap.
Full suite: 11,013 passed, 192 skipped, 1 xfailed.
🤖 Generated with Claude Code