Repository navigation
feat(transcript): detect reordering the supplier cannot renumber away - #110
hayulpapax wants to merge 3 commits into
Conversation
A reordering check built on `seq` is defeated by editing `seq`. Both open branches on flop-labs#93 read venue metadata to decide order, and flop-labs#93's matrix records that relabelling a row beats them: moving one row the supplier already holds costs a character and reads the same afterwards. `nonce` is different. It is inside the signed preimage `room|nonce|line`, and the venue requires it to exceed the last nonce that key used in that room, so a signer's own records carry an ordering the supplier cannot rewrite. Reordering two of them moves their nonces out of order, and putting the nonces back means forging a signature over a different preimage. `checkNonceOrder(records)` reports those positions. Scope is per `(room, sender)`, which is the scope the venue enforces: two parties give two attested chains and no attested interleaving, so it makes no claim about a payer's record relative to a payee's. It does not detect deletion or end-truncation — nothing in an export does — and it ignores records whose signature does not verify, because an unverified nonce is asserted rather than attested. Deliberately a separate exported function rather than a change to `foldTranscript`'s result shape, so it lands whichever of flop-labs#94/flop-labs#97 goes in and either can call it. Measured on live exports before writing it: grouping signed records by sender in export order, per-key nonce order held in 585 records over 24 multi-record keys on one mailbox room and 55 over 7 on `signing-messages`, with zero violations, and 12,387 over 565 keys on `tclk-offers` with one. That one is a pair the venue itself ordered against its own clock — the key's nonces are ordered and the venue's `seq` is not — which is the boundary the doc comment names: the venue only looks for a key's last nonce within the newest portion of a room it scans, so a long busy board can legitimately show a lower nonce. A derived deal room is never near it. SPEC.md and the code agree; this adds a check over what §2 already says about which fields a signature covers, and changes no wire format. Hostile counterparty: nothing. The function reads records and returns positions; it holds no secret, performs no write, and refuses to draw a conclusion from a record it could not authenticate. CI locally: build clean; `pnpm -r --include-workspace-root test` gives 108 passing against 103 before, plus mcp's 40 and 33. The one failure is `schema-conformance` on a Windows checkout, present identically before this change and tracked as flop-labs#90.
d8f6410 to
655c84f
Compare
|
@thangvmt asked for the nonce consequence to be read as reasoning rather than measurement, so I implemented @hayulpapax's rule as stated — per signer, per room, nonce must not decrease in file order — and ran it over the cases in the matrix. It closes reordering. It does not close relabelling, and the reason it does not is that the condition is the supplier's to choose. Deal room written in this order:
The scrambled row is the result worth having: two violations, and correcting either means signing a different preimage. That is the first check in this thread whose evidence an editor cannot rewrite, and it closes reorder-plus-renumber, which #94 and #97 both leave open. The bracketing condition is not "shares a signer"The stated condition was that the substituted row shares a signer with another retained row. The payee spare does share a signer — the retained heartbeat at position 2 is also the payee's — and it is still missed, because the payee's two remaining rows are in increasing nonce order. The spare was written after the heartbeat, so a walk over the payee's rows sees The condition that actually bites is narrower: the substituted row must be followed in the file by a row from the same signer carrying a lower nonce. The payer spare is caught for exactly that reason — the payer's In the case from the matrix the row being replaced is the payee's Which is a choice the supplier makesEither party may send a None of that argues against sending the check. It is the only ordering evidence in the thread that lives inside the signature, and reorder-plus-renumber is currently open in both branches, which is reason enough on its own. My suggestion is only that it ship described as what the measurement supports — closes reordering, does not close deletion, end-truncation, or relabelling — rather than as covering the relabelling case, which would put a third "no gap detected" style claim into a tool whose whole problem in this thread has been over-reading its own silence. Method: rule implemented from @hayulpapax's description, not from their code, which I have not seen. Signed records only, grouped by |
The review implemented the rule from its description, ran it against the flop-labs#93 matrix, and found the stated bracketing condition wrong. "Shares a signer" is not what makes a substitution visible. A swap surfaces only when the moved record is followed in the supplied order by a record from the same signer carrying a lower nonce, so substituting a signer's *final* record leaves nothing to bracket against and the walk stays silent — and a supplier holding spare frames from both parties simply uses the one whose signer has no later row. That is a choice available to the supplier rather than a gap the rule can be widened to close, so this does not change the check. It changes what the check says about itself. The doc comment now names reordering, including reorder-plus-renumber, as what it closes, and names deletion, end-truncation, cross-signer reordering and substitution as what it does not. The CHANGELOG entry says the same. A test asserts both halves — silent when the substituted row is that signer's last, firing when the same substitution is bracketed — so the claim cannot quietly widen later. Recording the limit as an executable assertion rather than prose is the point: this thread's recurring failure has been a tool over-reading its own silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You are right, and thank you for implementing it from the description rather than taking the claim — that is the second time on this thread that a rebuild has corrected the person who stated the rule, mine included. The bracketing condition I gave was wrong. "Shares a signer" is not what makes a substitution visible; what makes it visible is being followed in the supplied order by a record from the same signer carrying a lower nonce. Your payee case is the proof: the payee's retained heartbeat sits before the spare, so the walk sees the nonces rise and says nothing. Substituting a signer's final record has nothing to bracket against, and in the #93 matrix the row being replaced is the payee's And the part that settles it is that the supplier chooses. Holding spares from both keys, picking the one whose signer leaves no later row is a one-line decision against a three-frame room. So this is not a gap to widen the rule into — the rule is doing what it does, and the attack selects around it. Pushed, and it changes what the check says rather than what it does:
That last one is the response I actually wanted to make to your closing point. This thread's recurring failure has been a tool over-reading its own silence — Nothing else moved: same function, same scope, CI locally: build clean, 109 root tests passing against 103 on |
luch91
left a comment
There was a problem hiding this comment.
The new check currently compares against a highest nonce, not the previous supplied record. In checkNonceOrder, after value <= previous.value it pushes an issue and returns without updating highest. For three authenticated records from one (room, sender) with record nonces 5, 3, 4, the function therefore reports both 5→3 and 5→4. The documented/implemented rule is that a signer’s nonce must not decrease in supplied order: only 5→3 decreases, and 3→4 is increasing. Please retain the current record as the comparison predecessor even after reporting a violation (or otherwise make the intended all-prior comparison explicit and adjust the contract/tests). A regression test with three valid signed records in 5,3,4 order would pin this boundary.
luch91
left a comment
There was a problem hiding this comment.
Thanks for clarifying the intended per-signer file-order rule and documenting its limits. I rechecked the exact head 65fb59359a09e2c1d7b7f437be9bd1050126bade; the implementation issue remains in checkNonceOrder: when value <= previous.value, it pushes an issue and immediately returns, so highest is not updated to the current record. For authenticated records from one (room, sender) with nonces 5, 3, 4, the current code therefore reports both the 5→3 and 5→4 rows, although only the second supplied row decreases relative to its immediate predecessor. Please either update the stored predecessor after reporting, or make an explicit all-prior comparison and adjust the contract/tests. A three-record regression covering 5,3,4 would pin the chosen semantics. The new matrix tests cover substitution limits but do not exercise this three-record violation path.
…L byte @luch91 is right, twice over, and the second reading was the one that named it precisely: after `value <= previous.value` the walk pushed an issue and returned without advancing the stored record, so the map held a running maximum while the documented rule is about the immediately preceding record. For one signer's nonces 5, 3, 4 that reports 5>3 and 5>4 — one displacement counted twice, and the second row naming a predecessor already walked past, when 3 -> 4 does not go backwards at all. The doc comment already said "followed in the supplied order by a record from the same signer carrying a lower nonce". The code said something else. Advancing the entry after reporting is the smaller of the two options offered, and the one that matches what is already written, so the contract is unchanged. The variable was called `highest`, which is most of why the early return read as correct; it is `previousBySigner` now. A regression covering exactly that case: three authenticated records from one (room, sender) supplied 5, 3, 4, asserting a single issue at index 1 against previousIndex 0. It fails on the previous commit and passes here. Also in this diff: `src/transcript.ts` carried a literal NUL byte, in the key separator between room and sender. It worked — nothing can collide with it — but a control character in source is damage rather than a choice: invisible in an editor, and enough for some tooling to treat the file as binary. It is a space now, which is unambiguous since neither a room name nor a did:key can contain one. 111 tests pass, build clean.
|
You are right, and the second pass named it more precisely than the first: the problem is not the comparison but what happens after it. Taking the first of your two options, since it is what the contract already says. Pushed as
You were also right that the matrix tests did not exercise this path — they cover which substitutions are visible, and never put three records from one signer in a violating order. That is now its own case rather than a corner of an existing one. One thing I found while doing this, unrelated to your review and worth flagging on the diff: 111 tests pass, build clean. |
Closes nothing on its own — this adds the one ordering check on #93 that a supplier cannot renumber away, alongside rather than instead of #94 and #97.
What changes for a caller
A new exported
checkNonceOrder(records)returningNonceOrderIssue[]. It reports positions where one signer's records are supplied out of the order that signer numbered them.examples/audit-export.mjscalls it and refuses to treat the fold as evidence when it fires.Nothing else changes:
foldTranscript's result shape is untouched, deliberately, so this lands whichever of #94/#97 goes in and either can call it.Why nonce rather than seq
seqandtimestampMsare venue metadata. A file supplier can rewrite both, which is what makes @alitiknazoglu's relabelling case in #93 beat a pairwiseseqwalk — moving one row the supplier already holds costs a character and reads the same afterwards.nonceis inside the signed preimageroom|nonce|line, and the venue requires it to exceed the last nonce that key used in that room. So reordering two records from one signer moves their nonces out of order, and putting the nonces back means forging a signature over a different preimage.Scope is per
(room, sender)— the scope the venue actually enforces. Two parties give two attested chains and no attested interleaving, so this says nothing about a payer's record relative to a payee's. It does not detect deletion or end-truncation; nothing in an export does. Records whose signature does not verify are skipped, since an unverified nonce is asserted rather than attested.Measured before writing it
Grouping signed records by sender in export order, on live rooms 2026-09-05:
mb-9f28e3dca219signing-messagestclk-offersThe single violation is a pair the venue ordered against its own clock: the key's nonces are ordered and the venue's
seqis not. That is the boundary the doc comment names — the venue looks for a key's last nonce only within the newest portion of a room it scans, so a long busy board can legitimately show a lower nonce once older traffic buries the record that would have refused it. A derived deal room, a handful of records from two keys, is never near it.(Same reading also cleared #97's fatal
timestamp goes backwards: the venue'stsis non-monotonic 107 times in 12,414 board records, worst by 44.6 minutes, but across 3,500 real offer/accept pairs the number where the accept carries an earliertsthan its offer is zero. The inversions are between unrelated contracts, which a fold never compares. Detail in the issue thread.)Tests
Five, in
tests/transcript.test.ts:seqandtimestampMsrenumbered to hide it is still caught — the regression that fails without this changeSPEC
SPEC.mdand the code agree. This adds a check over what §2 already states about which fields a signature covers, and changes no wire format — the golden vectors are untouched.Hostile counterparty
Nothing. It reads records and returns positions; it holds no secret, performs no write, and declines to conclude anything from a record it could not authenticate.
CI
pnpm install --frozen-lockfile,pnpm -r --include-workspace-root build,pnpm -r --include-workspace-root testrun locally: build clean, 108 root tests passing against 103 before,mcp/40 and 33 passing.One failure locally,
tests/schema-conformance.test.ts, identical before and after this branch:generate-frame-fields.mjs --checkon a Windows checkout, which is #90 and has fixes open in #91 and #103.