Multiparty Senders: NS1R by arminsabouri · Pull Request #434 · payjoin/rust-payjoin

arminsabouri · 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?

arminsabouri · 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 13636268315

Details

407 of 477 (85.32%) changed or added relevant lines in 9 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.4%) to 79.69%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/send/v2/mod.rs	37	38	97.37%
payjoin-test-utils/src/lib.rs	58	60	96.67%
payjoin/src/send/multiparty/mod.rs	163	167	97.6%
payjoin/src/receive/multiparty/mod.rs	130	140	92.86%
payjoin/src/receive/multiparty/error.rs	0	22	0.0%
payjoin/src/send/multiparty/error.rs	0	31	0.0%

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

Totals
Change from base Build 13524514317:	0.4%
Covered Lines:	4528
Relevant Lines:	5682

💛 - 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

payjoin/src/receive/multi_party/mod.rs

arminsabouri · 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

payjoin/src/receive/v2/mod.rs

payjoin/src/send/mod.rs

payjoin/src/send/multi_party/mod.rs

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

arminsabouri · 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

DanGould · 2025-02-22T21:11:23Z

A few comments from my last review were left undone

this was pretty tricky to review since the commit message doesn't explain what changed or why it changed as described in this article

Putting design decisions (what) and rationale (why) in the commit message is best practice since it allows a reviewer (present for merge or future for debug or understanding) to anticipate the scope of the changes and reason about their impact.

Leaving the remaining work in the commit seems like tangent to this main explanation of rationale for and mechanism design for the changes made which ought to be documented in the commit message body.

The other thing is the rename

Module / error names are still multi_party and MultiParty vs multiparty and Multiparty

edit:

Another thing I was really hoping for in my original request was

duplicate extract_req code for multi party sender. In a seperate commit abstract to common util method

I may not have been clear in this original request. It seems like all of these changes got crammed into one commit, which makes it difficult to see that the change is only for deduplication. In the future I'd prefer to see duplicate code introduced in a first commit, and a second commit de-duplicate explicitly.

DanGould · 2025-02-22T21:13:58Z

payjoin/src/send/multi_party/mod.rs

+        use crate::send::test::ORIGINAL_PSBT;
+        let v2_sender = v2::Sender {
+            v1: v1::Sender {
+                psbt: Psbt::from_str(ORIGINAL_PSBT).unwrap(),


We are trying to Remove .unwrap() even in tests but this can be a follow up

Oh yeah. Let me just make that change here

DanGould · 2025-02-22T21:25:06Z

payjoin/src/receive/v2/mod.rs

+            // Additionally V1 sessions never have an optimistic merge opportunity
+            #[cfg(feature = "multiparty")]
+            {
+                params.optimistic_merge = false;
+            }


This comment implies the general Params type has potentially invalid state, which is something we want to avoid in type design. Granted Paams is not part of the public API. Just a note.

arminsabouri · 2025-02-22T23:12:35Z

It seems like all of these changes got crammed into one commit, which makes it difficult to see that the change is only for deduplication. In the future I'd prefer to see duplicate code introduced in a first commit, and a second commit de-duplicate explicitly.

The diff is not easy to review, I agree. And some small cosmetic changes we're made that could have been left in the first commit. The majority of the changes are removing common code that was introduced in the first commit and removing unused error variants in the multiparty::error. But there is also code that is unique to each type of sender which is why this diff has become cumbersome.

DanGould

Ok the sender/receiver module separation is beautiful, the v1/v2 apis are unchanged (YTB), and the rationale in the commit messages is tasty.

The one thing that I don't love is that the tests ended up in the integration::v2 module, and they feature gate them to require _multiparty. I'll approve since this is technically sound, but I'd really like to see that one final change made before this goes in so that we can maintain a clear v2 history in the test module and separate the compilation requirements to be strict on the minimum feature set required to use specific behavior.

DanGould · 2025-03-01T00:36:00Z

payjoin/src/receive/multiparty/mod.rs

+
+pub(crate) mod error;
+
+const SUPPORTED_VERSIONS: &[usize] = &[2];


You wish a multiparty receiver to error on version 1 rather than fall back? That's what this means, and why it's different than what's found in V2 (&[1, 2]).

Just checking.

If you wish to change this I'd rather follow up than delay this further.

The intention was to disallow pjv1 proposals from being merged. Yes, ideally we just fallback and perform a v2 pj.

DanGould · 2025-03-01T00:38:55Z

payjoin/src/receive/multiparty/mod.rs

+/// A multiparty proposal that has been merged by the receiver
+pub struct UncheckedProposal {
+    v1: v1::UncheckedProposal,
+    sender_contexts: Vec<SessionContext>,


contexts would be a better mirror for the other modules. When trying to mirror something else I prefer getting as close as possible to the original name. This is an anal comment.

I prefer verbosity and overly descriptive names. Esp in a codebase where you have different parties where session has different meanings. I'll change to contexts

DanGould · 2025-03-01T00:40:42Z

payjoin/src/receive/optional_parameters.rs

            disable_output_substitution: false,
            additional_fee_contribution: None,
            min_fee_rate: FeeRate::BROADCAST_MIN,
+            optimistic_merge: false,


I think we want to leave this open to be addressed for posterity, so I un-resolved it.

payjoin/src/send/multiparty/error.rs

DanGould · 2025-03-01T00:48:56Z

payjoin/src/send/multiparty/mod.rs

+    use crate::HpkeKeyPair;
+
+    #[test]
+    fn req_ctx_ser_de_roundtrip() -> Result<(), BoxError> {


I don't think this tests new behavior, it looks like a duplicate of the test in v2. To be removed, or updated if the multiparty sender is to be persisted for async function.

DanGould · 2025-03-01T00:53:19Z

payjoin/tests/integration.rs

+    #[cfg(all(
+        feature = "io",
+        feature = "v2",
+        feature = "_danger-local-https",
+        feature = "_multiparty"
+    ))]


‼️ I don't love that this change requires _multiparty to test v2 functionality, since the tests should pass without that feature gate. A follow up commit that moves ns1r tests to their own module would be acceptable but since this pollutes the v2 mod imports and history I'd really rather the commit be amended with this change.

It could either be a separate multiparty module OR a submodule i.e. integration/v2/multiparty. I'd prefer the former for test isolation, though If it would duplicate a huge amount of code and helpers in a v2 module could be shared, I'd prefer the latter (v2/multiparty submodule).

Commit history has been changed to move the ns1r test to integration::integration

DanGould · 2025-03-01T00:59:18Z

payjoin/tests/integration.rs

+            res = test_ns1r(&services) => assert!(res.is_ok(), "NS1R failed: {:#?}", res)
+            );
+
+            async fn test_ns1r(services: &TestServices) -> Result<(), BoxError> {


nit do_ns1r would be more consistent with our other tests

DanGould · 2025-03-01T01:01:09Z

payjoin/tests/integration.rs

+                struct InnerSenderTestSession {
+                    receiver_session: Receiver,
+                    sender_get_ctx: MultiPartyGetContext,
+                    script_pubkey: ScriptBuf,
+                }


An integration::[v2?::]multiparty mod would give this a more beautiful home.

moved to integration::multiparty

DanGould · 2025-03-01T01:02:37Z

payjoin/tests/integration.rs

+            let psbt = Psbt::from_str(&psbt)?;
+            Ok(psbt)


unnecessary assignment

DanGould · 2025-03-01T01:05:11Z

payjoin/tests/integration.rs

            assert_eq!(100, receiver_utxos.len(), "receiver doesn't have enough UTXOs");
            assert_eq!(
-                Amount::from_btc(3700.0)?, // 48*50.0 + 52*25.0 (halving occurs every 150 blocks)
+                Amount::from_btc(3650.0)?, // 50 (starting reciever blance) + 46*50.0 + 52*25.0 (halving occurs every 150 blocks)


If you gotta document it I think it's better off written explicitly Amount::from_btc(50 + 46*50.0 + 52*25.0) and getting real fancy would be with constants REWARD_H1 = 50, REWARD_H2 = 25. Code semantics are harder to go stale than comments.

arminsabouri · 2025-03-01T16:50:41Z

Hmm @DanGould I'm getting all green locally running ./contrib/test_local.sh. Is the int. test being flaky? Can we re-run it?

payjoin/tests/integration.rs

**Overview** 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 `_multiparty` flag. **Additional Round of Communication** In the traditional BIP77 payjoin protocol, the transaction concludes successfully once the sender verifies that the recipient has added the necessary inputs and outputs. The NS1R extension, however, introduces an additional communication round. Here, senders must re-sign a new PSBT that incorporates inputs and outputs from other senders. This additional PSBT is refered to as the merged PSBT. Senders can opt into merging their transfers by appending the `optimisticmerge` query parameter to their original payjoin body. When the recipient is NS1R-aware, it merges the original PSBTs from each sender and posts a new payjoin proposal to the same subdirectory. Senders then sign this merged PSBT, and the recipient finally consolidates all the signed PSBTs to broadcast the complete payjoin transaction with multiple senders. **Receiver Type States** The NS1R receiver type state follows the encapsulation paradigm used in `receiver::v2`, but with a key difference: it encapsulates a v1 type state rather than a v2 type state. This design decision is driven by the multiparty requirement, where PSBTs are merged as the initial step, and a `Vec<SessionContext>` is maintained across individual states. Since v2 type states include a session context specific to a single sender, they are incompatible with the multiparty model. In addition, this commit introduces a new type state, `FinalizedProposal`, which is employed when the receiver collects finalized PSBTs from all signers. **Sender Type States** The NS1R sender deviates from the standard v2 sender paradigm because it introduces additional query parameters that v2/v1 senders do not recognize. Consequently, this commit duplicates the POST request creation code from v2—with plans to de-duplicate this common functionality in the future—and similarly duplicates the directory response handling code. Moreover, a new NS1R-specific type, `FinalizeContext`, has been introduced. This type is used to extract the final POST request and process the response returned from the directory service. **Test Utilities** To support scenarios with multiple senders, the test utility suite has been refactored to allow the creation of multiple wallets during setup. The new function `create_and_fund_wallets` funds a list of wallets using a dedicated funding wallet. This approach was chosen over using coinbase rewards to avoid complexities related to fund maturity timing. Additionally, a new function, `handle_multiparty_proposal`, has been added to advance the receiver type state. This function processes a `payjoin::receive::multiparty::UncheckedProposal`, which aggregates a list of `v1::receive::UncheckedProposal` instances.

Extracting the POST request to read from the pj directory is duplicated in both the v2 sender and multiparty (ns1r) sender. This work introduces a common utility method `create_request` used in both mods.

DanGould

IT WAS A JOURNEY BUT WE MADE IT. MULTIPARTY PAYJOIN POC IS GOING TO GET MERGED. YEEREEEEEEE HAWWW 🤠

We both learned a lot about how the other works and the review process in this repository. So much good from this PR. Thank you Armin.

arminsabouri · 2025-03-03T18:51:36Z

IT WAS A JOURNEY BUT WE MADE IT. MULTIPARTY PAYJOIN POC IS GOING TO GET MERGED. YEEREEEEEEE HAWWW 🤠

We both learned a lot about how the other works and the review process in this repository. So much good from this PR. Thank you Armin.

Letss gooo! Thanks for sticking with me through the review process.

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

arminsabouri mentioned this pull request Jan 5, 2025

sender: allow for multiple payouts #437

Open

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

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

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

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

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

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

arminsabouri 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

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

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

arminsabouri 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

arminsabouri mentioned this pull request Jan 31, 2025

Abstract away additionalfeecontribution to its own struct #521

Merged

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

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

arminsabouri force-pushed the arm/feature/allow-for-many-payouts branch 2 times, most recently from 692ef63 to f37dc64 Compare February 22, 2025 17:35

arminsabouri requested a review from DanGould February 22, 2025 17:35

DanGould reviewed Feb 22, 2025

View reviewed changes

arminsabouri force-pushed the arm/feature/allow-for-many-payouts branch 2 times, most recently from b8be3f7 to 7b18bb5 Compare February 22, 2025 22:23

arminsabouri requested a review from DanGould February 22, 2025 22:36

arminsabouri force-pushed the arm/feature/allow-for-many-payouts branch from 7b18bb5 to de285a2 Compare February 22, 2025 22:56

arminsabouri mentioned this pull request Feb 24, 2025

Try preserving privacy for transactions greater than 2 outputs #551

Open

arminsabouri force-pushed the arm/feature/allow-for-many-payouts branch 3 times, most recently from 1727639 to 2715774 Compare February 28, 2025 21:56

DanGould approved these changes Mar 1, 2025

View reviewed changes

arminsabouri force-pushed the arm/feature/allow-for-many-payouts branch 2 times, most recently from 529e8af to 3542cbe Compare March 1, 2025 16:43

arminsabouri requested a review from DanGould March 1, 2025 16:48

DanGould reviewed Mar 3, 2025

View reviewed changes

payjoin/tests/integration.rs Outdated Show resolved Hide resolved

arminsabouri force-pushed the arm/feature/allow-for-many-payouts branch from 3542cbe to 43335a7 Compare March 3, 2025 16:14

arminsabouri added 2 commits March 3, 2025 12:01

Abstract creating v2 POST requests to common pub(crate) fns

16aadbd

Extracting the POST request to read from the pj directory is duplicated in both the v2 sender and multiparty (ns1r) sender. This work introduces a common utility method `create_request` used in both mods.

arminsabouri force-pushed the arm/feature/allow-for-many-payouts branch from 43335a7 to 16aadbd Compare March 3, 2025 17:02

DanGould approved these changes Mar 3, 2025

View reviewed changes

DanGould merged commit 2761db1 into payjoin:master Mar 3, 2025
6 checks passed

vnprc mentioned this pull request Mar 3, 2025

Socratic Seminar 38 TriangleBitDevs/TriangleBitDevs.github.io#33

Closed

spacebear21 mentioned this pull request Apr 1, 2025

Release payjoin 0.23 #618

Closed

17 tasks

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


		pub(crate) mod error;

		const SUPPORTED_VERSIONS: &[usize] = &[2];

Conversation

arminsabouri commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanGould commented Jan 15, 2025

Uh oh!

arminsabouri commented Jan 18, 2025

Uh oh!

coveralls commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 13636268315

Details

💛 - Coveralls

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminsabouri commented Jan 25, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminsabouri commented Feb 1, 2025

Uh oh!

DanGould commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Feb 22, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Dec 9, 2024 •

edited

Loading

coveralls commented Jan 23, 2025 •

edited

Loading

DanGould commented Feb 22, 2025 •

edited

Loading

DanGould Feb 22, 2025 •

edited

Loading