Tx cut-through #332

spacebear21 · 2024-07-30T04:16:48Z

This PR changes the receiver API interface to add support for transaction cut-through. Enables #313.

Summary of changes:

The receiver can replace all of their original outputs with a new set of outputs (if allowed by disableoutputsubstitution checks), while designating a change output.
The receiver can contribute a custom set of inputs. Excess input value is added to their change output.
The receiver may have to pay for additional fees, in which case the additional fee value is deducted from their change output.
The receiver may specify a max_feerate, which ensures they wouldn't pay more in fees than their input/output contributions are worth at that max feerate.

payjoin/src/receive/mod.rs

DanGould · 2024-08-16T15:27:04Z

Can we get the common integration boilerplate in before this PR?

@Spacebear

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.

DanGould

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.

payjoin-cli/src/app/mod.rs

payjoin-cli/src/app/v2.rs

payjoin/src/receive/mod.rs

payjoin-cli/src/app/v1.rs

payjoin-cli/src/app/v2.rs

spacebear21

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.

payjoin/src/receive/mod.rs

payjoin-cli/src/app/v2.rs

payjoin-cli/src/app/v1.rs

payjoin-cli/src/app/mod.rs

spacebear21 · 2024-08-26T19:52:57Z

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.

payjoin/src/receive/mod.rs

spacebear21 · 2024-08-28T22:54:37Z

The last commit adds random output ordering for additional outputs.

It occurred to me while doing this that input contribution as implemented in contribute_witness_inputs is not truly random and might benefit from using interleave_shuffle as well.

DanGould

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.

payjoin/src/receive/mod.rs

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.

DanGould

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 mirror PsbtBelowFeeRate 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

payjoin-cli/src/app/config.rs

payjoin-cli/src/main.rs

payjoin/src/receive/error.rs

payjoin/src/receive/mod.rs

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.

DanGould

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!

DanGould · 2024-09-12T15:33:53Z

payjoin-cli/src/app/v2.rs

+            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()))
+            })?,


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.

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."

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

spacebear21 · 2024-09-12T18:02:33Z

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.

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 send_payjoin and doesn't need it later, whereas the receiver just starts an http server in receive_payjoin and needs to access the value later when processing the request. I struggled with it a little bit and figured config was the easiest place to keep that state. Actually now I wonder if the resume operation would also need to to be updated to apply max-fee-rate in async payjoin.

Also, the way the sender applies fee-rate seems overly complicated. It does a bunch of fee/weight calculations manually that rust-bitcoin could do for us, and is parsed as a f32 which may be overkill. FeeRate::from_sat_per_vb takes a u64 so I'm not convinced that fractional amounts are really that useful.

spacebear21 force-pushed the tx-cut-through branch 2 times, most recently from 2a58510 to 65720a7 Compare July 30, 2024 19:54

spacebear21 added this to the payjoin-0.20.0 milestone Jul 30, 2024

spacebear21 force-pushed the tx-cut-through branch 2 times, most recently from 99e11c8 to ef24150 Compare August 5, 2024 16:29

DanGould reviewed Aug 9, 2024

View reviewed changes

payjoin/src/receive/mod.rs Outdated Show resolved Hide resolved

spacebear21 force-pushed the tx-cut-through branch 5 times, most recently from b0e4c85 to d0c3b92 Compare August 15, 2024 22:36

spacebear21 commented Aug 15, 2024

View reviewed changes

spacebear21 force-pushed the tx-cut-through branch from d0c3b92 to bf661ed Compare August 22, 2024 17:57

DanGould mentioned this pull request Aug 22, 2024

Add amount checks to integration tests #351

Merged

spacebear21 force-pushed the tx-cut-through branch from bf661ed to 7419d81 Compare August 22, 2024 22:46

spacebear21 marked this pull request as ready for review August 23, 2024 00:03

spacebear21 requested a review from DanGould August 23, 2024 00:04

DanGould reviewed Aug 25, 2024

View reviewed changes

spacebear21 commented Aug 26, 2024

View reviewed changes

spacebear21 force-pushed the tx-cut-through branch from 7419d81 to 7221857 Compare August 26, 2024 19:47

DanGould reviewed Aug 28, 2024

View reviewed changes

payjoin/src/receive/mod.rs Outdated Show resolved Hide resolved

spacebear21 force-pushed the tx-cut-through branch from 7221857 to 00c5935 Compare August 28, 2024 22:48

DanGould reviewed Aug 29, 2024

View reviewed changes

payjoin/src/receive/mod.rs Show resolved Hide resolved

payjoin/src/receive/mod.rs Show resolved Hide resolved

spacebear21 force-pushed the tx-cut-through branch from 00c5935 to 5a28670 Compare August 30, 2024 16:45

spacebear21 added 2 commits September 5, 2024 21:37

Allow contributing multiple inputs

4325e93

`contribute_witness_input` is now `contribute_witness_inputs`. It takes an arbitrary number of inputs as a HashMap and returns a `ProvisionalProposal`.

spacebear21 added 6 commits September 5, 2024 21:37

Implement disable_output_substitution check correctly

4062b56

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.

Add batching tests

b737500

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.

Add custom error types for output and input contributions

f4f2f37

Partially addresses payjoin#312

spacebear21 force-pushed the tx-cut-through branch from 5a28670 to c692ad8 Compare September 6, 2024 01:37

DanGould requested changes Sep 9, 2024

View reviewed changes

spacebear21 force-pushed the tx-cut-through branch from c692ad8 to 59c3d27 Compare September 11, 2024 21:36

spacebear21 requested a review from DanGould September 11, 2024 22:05

DanGould mentioned this pull request Sep 12, 2024

Rethink fee estimation to ensure accuracy and prevent overspending #358

Closed

DanGould approved these changes Sep 12, 2024

View reviewed changes

spacebear21 merged commit 6153fdb into payjoin:master Sep 12, 2024
4 checks passed

spacebear21 deleted the tx-cut-through branch September 12, 2024 18:23

DanGould mentioned this pull request Sep 16, 2024

Enable transaction cut-through #313

Closed

spacebear21 mentioned this pull request Sep 18, 2024

Allow recipeint to contribute fee if receiver sets their own minfeerate #41

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tx cut-through #332

Tx cut-through #332

spacebear21 commented Jul 30, 2024 •

edited

Loading

DanGould commented Aug 16, 2024

DanGould left a comment

spacebear21 left a comment

spacebear21 commented Aug 26, 2024

spacebear21 commented Aug 28, 2024 •

edited

Loading

DanGould left a comment

DanGould left a comment

DanGould left a comment

DanGould Sep 12, 2024

spacebear21 Sep 12, 2024

DanGould Sep 12, 2024

spacebear21 commented Sep 12, 2024

Tx cut-through #332

Tx cut-through #332

Conversation

spacebear21 commented Jul 30, 2024 • edited Loading

DanGould commented Aug 16, 2024

DanGould left a comment

Choose a reason for hiding this comment

spacebear21 left a comment

Choose a reason for hiding this comment

spacebear21 commented Aug 26, 2024

spacebear21 commented Aug 28, 2024 • edited Loading

DanGould left a comment

Choose a reason for hiding this comment

DanGould left a comment

Choose a reason for hiding this comment

DanGould left a comment

Choose a reason for hiding this comment

DanGould Sep 12, 2024

Choose a reason for hiding this comment

spacebear21 Sep 12, 2024

Choose a reason for hiding this comment

DanGould Sep 12, 2024

Choose a reason for hiding this comment

spacebear21 commented Sep 12, 2024

spacebear21 commented Jul 30, 2024 •

edited

Loading

spacebear21 commented Aug 28, 2024 •

edited

Loading