Multiparty Senders: NS1R #434

0xBEEFCAF3 · 2024-12-09T14:40:24Z

The following is an expansion of the BIP77 protocol that allows for
multiple senders to opt in to a optimisitic merge with other potential
senders at a cost of an additional round of communication. The protocol
does not introduce new message types instead it re-uses the existing v2
structs. Everything touching NS1R is feature gated behind the
multi_party flag.

The remaining work is:
Multi party version of try_preserving_privacy. Currently when the
v2 reciever is assembling a payjoin it calls in the v1
try_preserving_privacy method which invalidates for txs with > 2
outputs. We would either need to relax this constraint or write a
bespoke version of try_preserving_privacy that optimizes for the
multiparty case. The latter is more favorable in my opnion.

For more context please take a look at: https://github.com/orgs/payjoin/discussions/411

Outside of this PR we should consider expanding BIP77 to describe the protocol upgrades made in this PR

DanGould · 2025-01-15T18:35:25Z

Can any of this be broken up and merged before all of the TODOs are done? It'd be cool to include this to maintain as an experimental feature as soon as possible and would help inform the shape of a stable API? What does the minimum merge look like?

0xBEEFCAF3 · 2025-01-18T00:01:35Z

Can any of this be broken up and merged before all of the TODOs are done? It'd be cool to include this to maintain as an experimental feature as soon as possible and would help inform the shape of a stable API? What does the minimum merge look like?

Absolutely! Everything multi party related should be featured gated so the risk is minimal. The most minimal change that I would like to make before considering merging are reverting the hacks I had to make to v2 or v1 receiver and sender. This is not a lot of work. After that I will open up for reviews.

After that the bulk of remaining work is really just validation on both the sender and receiver sides.

coveralls · 2025-01-23T21:04:47Z

Pull Request Test Coverage Report for Build 13229947560

Details

339 of 383 (88.51%) changed or added relevant lines in 10 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.6%) to 79.617%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/send/mod.rs	56	57	98.25%
payjoin-test-utils/src/lib.rs	58	60	96.67%
payjoin/src/send/multi_party/mod.rs	58	67	86.57%
payjoin/src/receive/multi_party/mod.rs	133	143	93.01%
payjoin/src/receive/multi_party/error.rs	0	22	0.0%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/receive/optional_parameters.rs	1	72.86%

Totals
Change from base Build 13203244614:	0.6%
Covered Lines:	4328
Relevant Lines:	5436

💛 - Coveralls

payjoin/src/send/mod.rs

DanGould · 2025-01-24T21:36:44Z

payjoin/src/receive/mod.rs

+#[cfg(all(feature = "v2", feature = "multi_party"))]
+pub mod multi_party;
+


Move this below optional_parameters with the version stuff

DanGould · 2025-01-24T21:38:17Z

payjoin/src/receive/multi_party/mod.rs

+    pub fn try_preserving_privacy(
+        &self,
+        candidate_inputs: impl IntoIterator<Item = InputPair>,
+    ) -> Result<InputPair, SelectionError> {
+        // TODO(armins) classic v1 privacy preserving selection doenst allow for many outputs
+        // lets just pick the first one
+        // self.v1.try_preserving_privacy(candidate_inputs)
+        Ok(candidate_inputs.into_iter().next().unwrap())
+    }


Get rid of this fn as public if it's not actually avoiding UIH / other privacy pitfalls

DanGould · 2025-01-24T21:39:27Z

payjoin/src/receive/multi_party/mod.rs

+    sender_contexts: Vec<SessionContext>,
+}
+
+impl MultiPartyProvisionalProposal {


Just name these structs as e.g. ProvisionalProposal so that they're fully qualified as ..::multi_party::ProvisionalProposal and not ..::multi_party::MultiPartyProvisionalProposal

DanGould · 2025-01-24T21:41:30Z

payjoin/src/receive/multi_party/mod.rs

+        let mut agg_psbt = self.v2_proposals.first().expect("checked above").v1().psbt.clone();
+        for proposal in self.v2_proposals.iter().skip(1) {
+            let proposal = proposal.v1().psbt.clone();
+            agg_psbt.combine(proposal).unwrap();


handle, don't unwrap

0xBEEFCAF3 · 2025-01-25T21:50:49Z

@DanGould moved the psbt merge operation into a submodule of a newly created psbt mod in 0ff1e4f

The next item is break out multiparty related things in the sender into a multi-party sender submod

DanGould

There's a ton here. It's already come a long way.

In order of importance I left comments regarding changes to existing public API, changes to existing modules, the multi-party implementation, my hygiene preferences for innline comments vs docstrings and commit separation.

I think the first commit can be pulled out of this PR and merged. It's unrelated and just nice-to-have. The PSBT merging seems mostly there, and we can merge it early because it should only be enabled in the experimental multi-party feature (which we may want to hide during the experimental phase as _multi-party).

I do wonder how much it makes sense for the MergePsbtExt to be a Trait vs. a couple of feature-gated pub(crate) fns. I don't really think it's its own trait, since the trait isn't relied on in a function signature, but rather an extension of functionality, so I lean toward the latter. I think even feature gating the new methods in PsbtExt rather than defining a completely new trait would be appropriate since they're internal, but pure functions are simpler.

The multi-party state machine is shaping up. Awesome to see tests work with minimal disruption to the existing abstractions. I'd like to have my comments addressing public API mutations addressed before merge, since afaict they only serve to enable multi-party and make less sense in the context of exclusive v1, v2 use. Same with the changes to existing modules where the abstraction layers get muddy. Let's get those clean before merge.

I'm OK merging the multi-party state machine with some rough edges in the name of progress. We're not going to get it perfect the first time, and it's not going to make complete sense until we have integration into payjoin-cli.

I would like to see the inline TODOs that don't define a TODO addressed, straggling lines removed, intermediate commit lint errors addressed, and inline comments pushed into docstrings or abstracted into new functions with their own docstrings. I'd also like to see the optional_parameters docstring update as its own commit.

This is a lot of comment, but hey this is a lot of PR. We're getting there. Let's pull the independent elements of this out of this to be merged this week and I think the more critical API problems are addressable in short order as well.

I didn't review the integration test implementation for the sake of time and because this review has already been so long. I believe the principles conveyed in the review apply there too, especially the one about duplication in payjoin-test-utils and tech debt. I'll revisit on the next request for review.

payjoin/src/send/mod.rs

payjoin/src/psbt/merge/mod.rs

DanGould · 2025-01-28T17:30:14Z

payjoin/src/psbt/merge/mod.rs

A nit would be to just name this merge.rs instead of merge/mod.rs since there's no other file in the folder.

payjoin/src/psbt/merge/mod.rs

DanGould · 2025-01-28T17:31:26Z

payjoin/src/psbt/merge/mod.rs

+    }
+}
+
+// Tests


this comment can be removed

DanGould · 2025-01-28T18:14:03Z

payjoin/src/receive/v2/mod.rs

@@ -27,7 +27,7 @@ const SUPPORTED_VERSIONS: &[usize] = &[1, 2];
 static TWENTY_FOUR_HOURS_DEFAULT_EXPIRY: Duration = Duration::from_secs(60 * 60 * 24);

 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
-struct SessionContext {
+pub struct SessionContext {


This is internal so pub(crate) instead of pub.

DanGould · 2025-01-28T18:15:03Z

payjoin/src/receive/v2/mod.rs

+    // TODO hack to get multi party working. A better solution would be to allow extract_v2_req to be seperate from the rest of the v2 context
+    pub fn new(v1: v1::PayjoinProposal, context: SessionContext) -> Self { Self { v1, context } }


I don't want to affect the v2 API with a hack. This could be merged as pub(crate) if possible or otherwise integrated into the state machine as your TODO suggests.

DanGould · 2025-01-28T18:16:06Z

payjoin/src/send/mod.rs

+        // TODO cannot check fee for optimistic v2 merges as they include utxo with missing info
+        if !self.allow_optimistic_merge {
+            self.check_fees(&proposal, contributed_fee)?;
+        }


What is the TODO here? This is a general comment. Record what is TO be DOne.

DanGould · 2025-01-28T18:18:53Z

payjoin/src/send/mod.rs

@@ -408,6 +416,7 @@ fn serialize_url(
    fee_contribution: Option<AdditionalFeeContribution>,
    min_fee_rate: FeeRate,
    version: &str,
+    opt_in_to_optimistic_merge: bool,


Would prefer to have a send::multi_party::serialize_url that encapsulates send::serialize_url to abstract these sepecifics away from the general case.

DanGould · 2025-01-28T18:19:10Z

payjoin/src/send/multi_party/mod.rs

+
+impl<'a> SenderBuilder<'a> {
+    /// Build a sender with optimistic merge enabled
+    /// TODO(armins) this is pretty much skipping all the validating and checks


tell me what you want TODO

A struct with named fields replaces the tuple abstraction over sender Params `maxadditionalfeecontribution` and `additionalfeeoutputindex`. Cherry-pick'd off #434

0xBEEFCAF3 · 2025-02-01T22:32:15Z

@DanGould I'm going to break out the psbt merge commits in its own PR. That work can be independently reviewed while I work on the remainding mp sender

cherry-picked off of #434

The following is an expansion of the BIP77 protocol that allows for multiple senders to opt in to a optimisitic merge with other potential senders at a cost of an additional round of communication. The protocol does not introduce new message types instead it re-uses the existing v2 structs. Everything touching NS1R is feature gated behind the `multi_party` flag. The remaining work is: Multi party version of `try_preserving_privacy`. Currently when the v2 reciever is assembling a payjoin it calls in the v1 `try_preserving_privacy` method which invalidates for txs with > 2 outputs. We would either need to relax this constraint or write a bespoke version of `try_preserving_privacy` that optimizes for the multiparty case. The latter is more favorable in my opnion.

0xBEEFCAF3 marked this pull request as draft December 9, 2024 14:40

0xBEEFCAF3 mentioned this pull request Jan 5, 2025

sender: allow for multiple payouts #437

Open

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch 2 times, most recently from d78288f to 5ea6853 Compare January 10, 2025 22:04

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch 3 times, most recently from a4407be to 5b61773 Compare January 17, 2025 02:52

0xBEEFCAF3 changed the title ~~WIP - 2S1R example~~ WIP - NS1R example Jan 20, 2025

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch 4 times, most recently from 7411bd9 to ac1447d Compare January 23, 2025 21:01

0xBEEFCAF3 changed the title ~~WIP - NS1R example~~ Multiparty Senders: NS1R Jan 23, 2025

0xBEEFCAF3 marked this pull request as ready for review January 23, 2025 21:04

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch 2 times, most recently from d645e2b to f0c9b6b Compare January 25, 2025 03:48

DanGould reviewed Jan 25, 2025

View reviewed changes

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch from f0c9b6b to 204d456 Compare January 25, 2025 21:49

0xBEEFCAF3 requested a review from DanGould January 25, 2025 21:51

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch from 204d456 to 329e0e1 Compare January 26, 2025 22:51

DanGould requested changes Jan 28, 2025

View reviewed changes

0xBEEFCAF3 mentioned this pull request Jan 31, 2025

Abstract away additionalfeecontribution to its own struct #521

Merged

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch from 329e0e1 to b1ee525 Compare January 31, 2025 03:25

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch from b1ee525 to 9945d04 Compare February 1, 2025 22:30

0xBEEFCAF3 mentioned this pull request Feb 2, 2025

Add support for merging unique psbts #523

Merged

DanGould added a commit that referenced this pull request Feb 5, 2025

Add support for merging unique psbts (#523)

b795944

cherry-picked off of #434

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch 3 times, most recently from ccc4ba9 to 00e024e Compare February 8, 2025 04:05

0xBEEFCAF3 force-pushed the arm/feature/allow-for-many-payouts branch from 00e024e to 9884aee Compare February 9, 2025 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiparty Senders: NS1R #434

Multiparty Senders: NS1R #434

0xBEEFCAF3 commented Dec 9, 2024 •

edited

Loading

DanGould commented Jan 15, 2025

0xBEEFCAF3 commented Jan 18, 2025

coveralls commented Jan 23, 2025 •

edited

Loading

DanGould Jan 24, 2025

DanGould Jan 24, 2025

0xBEEFCAF3 Jan 25, 2025

DanGould Jan 24, 2025

0xBEEFCAF3 Jan 25, 2025

DanGould Jan 24, 2025

0xBEEFCAF3 Jan 25, 2025

0xBEEFCAF3 commented Jan 25, 2025

DanGould left a comment

DanGould Jan 28, 2025

DanGould Jan 28, 2025

DanGould Jan 28, 2025

DanGould Jan 28, 2025

DanGould Jan 28, 2025

DanGould Jan 28, 2025

DanGould Jan 28, 2025

0xBEEFCAF3 commented Feb 1, 2025

		#[cfg(all(feature = "v2", feature = "multi_party"))]
		pub mod multi_party;

		// TODO hack to get multi party working. A better solution would be to allow extract_v2_req to be seperate from the rest of the v2 context
		pub fn new(v1: v1::PayjoinProposal, context: SessionContext) -> Self { Self { v1, context } }

Multiparty Senders: NS1R #434

Are you sure you want to change the base?

Multiparty Senders: NS1R #434

Conversation

0xBEEFCAF3 commented Dec 9, 2024 • edited Loading

DanGould commented Jan 15, 2025

0xBEEFCAF3 commented Jan 18, 2025

coveralls commented Jan 23, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13229947560

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xBEEFCAF3 commented Jan 25, 2025

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xBEEFCAF3 commented Feb 1, 2025

0xBEEFCAF3 commented Dec 9, 2024 •

edited

Loading

coveralls commented Jan 23, 2025 •

edited

Loading