-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tx cut-through #332
Tx cut-through #332
Conversation
2a58510
to
65720a7
Compare
99e11c8
to
ef24150
Compare
b0e4c85
to
d0c3b92
Compare
Can we get the common integration boilerplate in before this PR? |
d0c3b92
to
bf661ed
Compare
At the end of each integration test, check the resulting transaction inputs/outputs, and the resulting balances for the sender and receiver. Note that the sender sweep test case requires the original PSBT fee rate to be high enough to cover the receiver input, because neither the sender nor the receiver can contribute additional fees in the payjoin PSBT due to current API limitations. Originally added these changes to the tx-cut-through branch but pulled them out into a separate PR for easier review. @Spacebear: Regression testing was the original reason for adding these checks, but I also like that it helps self-document what exactly is going on in each scenario. These changes were originally part of #332 but I pulled them out into a separate PR for independent review and to make #332 lighter. I've found them very helpful in testing cut-through scenarios, e.g. receiver forwards payment. The fee checks are especially useful when the sender and the receiver are both on the hook for fees. I ran these tests on a loop for ~an hour (187 iterations) and didn't get any errors, so I'm reasonably confident that they're not flaky.
bf661ed
to
7419d81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a relief to see this working I went commit by commit to review and most everything seemed well reasoned to me.
My only real concern is the random order output list construction. Does the solution I proposed in an earlier review have an issue, or did we decide to follow up with it? I can't remember. I would like to see a proper random output list if I haven't already said otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only real concern is the random order output list construction. Does the solution I proposed in an earlier review have an issue, or did we decide to follow up with it? I can't remember. I would like to see a proper random output list if I haven't already said otherwise.
The issue with that proposed solution is it doesn't preserve the relative ordering of receiver/sender outputs, which would break the BIP78 spec. I replied there pretty late, it may have gone unnoticed.
I agree that this is an important (solvable) problem that should be addressed.
7419d81
to
7221857
Compare
Applied all the minor suggestions/fixes from your review and rebased. Also left a few open questions in replies where I'm sure if changes are needed. The main remaining to-do is implementing truly random output ordering. |
7221857
to
00c5935
Compare
The last commit adds random output ordering for additional outputs. It occurred to me while doing this that input contribution as implemented in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shuffling looks right to me. I'll want to review it again before merge but It appears correct and I appreciate unit test very much.
00c5935
to
5a28670
Compare
This effectively splits the `ProvisionalProposal` state into three states that must be completed in order: - `WantsOutputs` allows the receiver to substitute or add output(s) and produces a WantsInputs - `WantsInputs` allows the receiver to contribute input(s) as needed to fund their outputs and produces a ProvisionalProposal - `ProvisionalProposal` is only responsible for finalizing the payjoin proposal and producing a PSBT that will be acceptable to the sender
`contribute_witness_input` is now `contribute_witness_inputs`. It takes an arbitrary number of inputs as a HashMap and returns a `ProvisionalProposal`.
When disable_output_substitition is true, the receiver may not change the script pubkey or decrease the value of their original output, but is allowed to increase the amount or add new outputs.
- Output substitution functions return a modified WantsOutputs instead of a WantsInputs. `freeze_outputs` is used to proceed to the WantsInputs step. - Similarly, contribute_witness_inputs returns a modified WantsInputs and `freeze_inputs` returns the ProvisionalProposal - One change output (aka drain script) must be identified if outputs are replaced. The change output receives all excess input amount from the receiver, and will be responsible for paying receiver fees if applicable.
Modify `apply_fee` to enable fee contributions from the receiver for fees not covered by the sender. This includes - any fees that pay for additional inputs, in excess of the sender's max_additional_fee_contribution. - all fees that pay for additional *outputs*. - in the case where the sender doesn't have a fee output (e.g. a sweep transaction), all tx fees are deducted from the receiver output.
Adds two integration tests for transaction cut-through: - Receiver UTXO consolidation - Receiver forwarding payment to a third-party Also fixes some issues in outputs and inputs contribution logic that became apparent while testing.
Partially addresses payjoin#312
Additional outputs should be inserted at random indices for privacy, while preserving the relative ordering of receiver/sender outputs from the original PSBT. This commit also ensures that `disable_output_substitution` checks are made for every receiver output, in the unlikely case that there are multiple receiver outputs in the original PSBT.
5a28670
to
c692ad8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience on my review of the last commit c692ad8.
My main question is whether or not we can simplify by making max_feerate a requirement rather than handling an option.
change requests:
- apply cli max feerate argument to both v1 and v2
- make cli
fee_rate
/*feerate
argument names consistent with one another\ - make
receive::error::InternalRequestError::FeeTooHigh
mirrorPsbtBelowFeeRate
and use the rate rather than absolute fee to match the parameter mismatch even though the check is absolute
I also ask a question about the excessive_feerate test hack
This ensures that the receiver does not pay more in fees than they would by building a separate transaction at max_feerate instead. That prevents a scenario where the sender specifies a `minfeerate` param that would require the receiver to pay more than they are willing to spend in fees. Privacy-conscious receivers should err on the side of indulgence with their preferred max feerate in order to satisfy their preference for privacy. This commit also adds a `max-feerate` receive option to payjoin-cli.
c692ad8
to
59c3d27
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell all of my concerns with the typestate machine for Tx cut-through have been resolved. I imagine as we test it and integrate with our next targets (Liana, bria?), and incorporate #355 things might change slightly but I think the big problems are all solved. How far we've come since that whiteboard session in Nashville. Kudos Spacebear
I think the max_fee_rate config and cli argument parsing is still departs from the way --fee_rate
handles config/cliargument passing in the sender. The sender has no config.rs
fee_rate option to set but the receiver --max_fee_rate
does. We can follow up with that after, as well as #358.
This is one of the biggest typestate PRs ever done in this library. So stoked to have it in!
self.config.max_fee_rate.map_or(Ok(FeeRate::ZERO), |fee_rate| { | ||
FeeRate::from_sat_per_vb(fee_rate).ok_or(Error::Server("Invalid fee rate".into())) | ||
})?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raather than fail here with a FeeRate::ZERO
max_fee_rate, I think the config could make max_fee_rate compulsory so that this validation issue would crop up right away and not only after receiving a proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only fails if from_sat_per_vb
fails (from an integer overflow). If unspecified it defaults to FeeRate::ZERO which is accepted by apply_fee and would only fail if the receiver needs to contribute an additional fee. In most cases that's not an issue because the sender's additional_fee_contribution the receiver input fee for regular payjoins.
fwiw i added a docstring to that effect in apply_fee
: "A max_feerate of zero indicates that the receiver is not willing to pay any additional fees."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I misunderstood (apply_fee is sooo uncomplicated /s) But I guess that was default behavior from before so it makes sense as a default
Yeah I tried implement it for the receiver in the same way as the sender, but the sender can immediately create the pj_request in Also, the way the sender applies |
This PR changes the receiver API interface to add support for transaction cut-through. Enables #313.
Summary of changes:
disableoutputsubstitution
checks), while designating a change output.max_feerate
, which ensures they wouldn't pay more in fees than their input/output contributions are worth at that max feerate.