Feat/v2 receiver event split#1685
Open
DanGould wants to merge 3 commits into
Open
Conversation
Collaborator
Coverage Report for CI Build 28364097711Coverage increased (+0.2%) to 85.737%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
The v2 receiver session is rebuilt by replaying an append-only event log. The CommittedOutputs and CommittedInputs events persisted only a summary, the output set and the contributed inputs, so a session resumed at WantsInputs or WantsFeeRange did not match the live state: the post-substitution change_vout, the randomly inserted receiver inputs, and the change increment were all lost. Resumed at WantsInputs after an output substitution, this can silently route the receiver's own change to a sender output, which no check guards, since the sender accepts any increase to one of its own outputs. Split the shared receiver typestates into two parts. OriginalContext holds the session invariant, the original PSBT and params, fixed once the receiver outputs are identified. WorkingProposal holds the RNG-mutated remainder, the working PSBT, the change vout, and the contributed inputs, none of which can be reconstructed from a summary. WantsOutputs, WantsInputs, and WantsFeeRange now hold the context by value alongside their mutable part. The commit events carry only the WorkingProposal; replay copies it verbatim and re-threads the invariant from the predecessor state, so live, persisted, and replayed states are identical by construction, without re-storing the original PSBT and params (about 16 KB) in every commit event. Route the additional_fee_contribution sanitization through a single OriginalContext::new, used by both the live identify_receiver_outputs path and the replay reconstruction. Replay previously rebuilt WantsOutputs from raw params, so a session resumed at WantsOutputs whose sender pointed the fee output at a receiver-owned vout threaded an un-nulled parameter forward and could subtract the sender's contribution from the receiver's own output. The shared coin selection and fee code is reachable from BIP78 v1 receivers, so accessors keep v1 and the v2 fallback-tx impls reading the original PSBT without exposing the split fields. Add replay-vs-live equality tests at WantsInputs and WantsFeeRange that drive a real substitution and input contribution, an owned-vout provenance test resuming at WantsOutputs, and an exact sender-subsidy fee test that catches a context and remainder PSBT swap.
The split commit events serialize only the WorkingProposal, a strict subset of the fields a fuller serialization would carry. The event enum is externally tagged and does not deny unknown fields, so a payload that also carries the invariant original PSBT and params deserializes cleanly: the extra fields are ignored on read and the invariant is re-threaded from the predecessor state. Add a regression test that drives a real session, an output substitution plus an input contribution, reconstructs such a richer payload by splicing the invariant back into the two commit events, then deserializes and replays it. It reaches the identical state, so a log written with the extra fields needs no migration to read under the split events. This pins the subset relationship. A later change that adds deny_unknown_fields or otherwise narrows the read path would fail here.
The replay-vs-live WantsInputs assertion only catches a lossy reconstruction, one that pins change_vout to owned_vouts[0], when the output substitution shuffles the drain output off owned_vouts[0]. Under the unseeded thread_rng the drain stays put on a sizable fraction of runs, so a buggy replay matches the live state by coincidence and the test passes anyway, giving roughly 60 to 75 percent detection power per run. Thread a seeded rng through replace_receiver_outputs so the asserted post-substitution state is fixed and the drain always moves off owned_vouts[0]. Extract the shuffle body into replace_receiver_outputs_with_rng, leaving the public method a wrapper that still supplies thread_rng, so production behavior is unchanged. Guard the seed with an assert_ne so a future rand draw that stops moving the drain fails loudly instead of silently weakening the test.
DanGould
force-pushed
the
feat/v2-receiver-event-split
branch
3 times, most recently
from
46b837c to
92f1a19
Compare
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
2 participants
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.
1) What
Fix a silent fund-safety bug in the BIP 77 v2 receiver state machine. In the process this restructures the receiver's persisted event format so fewer bytes get persisted.
A v2 receiver session resumed (replayed) from its event log could diverge from the live state it was built from. In the worst case resuming at
WantsInputsafter an output substitution could silently misdirected the receiver's own funds to the sender, with no error anywhere.AFAIU nobody is using output substitution, where this manifests, in production where this bug. And it's not something a malicious sender can trigger, just a random event.
I have NOT gone through the code and the tests yet, which is why this is draft, but I think the commit log at least carries rationale for the change.
Co-authored by Claude
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.