-
Notifications
You must be signed in to change notification settings - Fork 50
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
Multiparty Senders: NS1R #434
Multiparty Senders: NS1R #434
Conversation
d78288f
to
5ea6853
Compare
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? |
a4407be
to
5b61773
Compare
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. |
7411bd9
to
ac1447d
Compare
Pull Request Test Coverage Report for Build 13636268315Details
💛 - Coveralls |
d645e2b
to
f0c9b6b
Compare
payjoin/src/receive/mod.rs
Outdated
#[cfg(all(feature = "v2", feature = "multi_party"))] | ||
pub mod multi_party; | ||
|
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.
Move this below optional_parameters with the version stuff
f0c9b6b
to
204d456
Compare
204d456
to
329e0e1
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.
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) fn
s. 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.
329e0e1
to
b1ee525
Compare
A struct with named fields replaces the tuple abstraction over sender Params `maxadditionalfeecontribution` and `additionalfeeoutputindex`. Cherry-pick'd off #434
b1ee525
to
9945d04
Compare
@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 |
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.
Made it a long way. It seems like most of the TODO items have been addressed
Remaining TODO's discussed in person:
- Multiparty sender type state structs should just wrap v2 structs [dan's edit: they probably don't need to carry
ohttp_relay
fields]
Done
- Finalize multiparty sender type state should be its own struct which we can transition to from
GetContext.process_response
Not done. This didn't get its own struct, it combined two with process_response_and_finalize
which produces 2 categories of errors. "and" in a function name is usually a smell.
- Missing process response for finalize type state
Done
- duplicate extract_req code for multi party sender. In a seperate commit abstract to common util method
This wasn't done in a separate commit
- Use the closure pattern for proposal finalization as in other
finalize_proposal
methods
Kind of done but also merged with other functionality
That said 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.
692ef63
to
f37dc64
Compare
A few comments from my last review were left undone
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
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. |
payjoin/src/send/multi_party/mod.rs
Outdated
use crate::send::test::ORIGINAL_PSBT; | ||
let v2_sender = v2::Sender { | ||
v1: v1::Sender { | ||
psbt: Psbt::from_str(ORIGINAL_PSBT).unwrap(), |
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.
We are trying to Remove .unwrap()
even in tests but this can be a follow up
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.
Oh yeah. Let me just make that change here
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.
Done
payjoin/src/receive/v2/mod.rs
Outdated
// Additionally V1 sessions never have an optimistic merge opportunity | ||
#[cfg(feature = "multiparty")] | ||
{ | ||
params.optimistic_merge = false; | ||
} |
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.
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.
b8be3f7
to
7b18bb5
Compare
7b18bb5
to
de285a2
Compare
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 |
1727639
to
2715774
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.
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.
|
||
pub(crate) mod error; | ||
|
||
const SUPPORTED_VERSIONS: &[usize] = &[2]; |
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.
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.
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.
The intention was to disallow pjv1 proposals from being merged. Yes, ideally we just fallback and perform a v2 pj.
/// A multiparty proposal that has been merged by the receiver | ||
pub struct UncheckedProposal { | ||
v1: v1::UncheckedProposal, | ||
sender_contexts: Vec<SessionContext>, |
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.
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.
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.
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
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.
Done
@@ -23,6 +25,7 @@ impl Default for Params { | |||
disable_output_substitution: false, | |||
additional_fee_contribution: None, | |||
min_fee_rate: FeeRate::BROADCAST_MIN, | |||
optimistic_merge: false, |
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.
I think we want to leave this open to be addressed for posterity, so I un-resolved it.
payjoin/src/send/multiparty/mod.rs
Outdated
use crate::HpkeKeyPair; | ||
|
||
#[test] | ||
fn req_ctx_ser_de_roundtrip() -> Result<(), BoxError> { |
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.
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.
payjoin/tests/integration.rs
Outdated
#[cfg(all( | ||
feature = "io", | ||
feature = "v2", | ||
feature = "_danger-local-https", | ||
feature = "_multiparty" | ||
))] |
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.
_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).
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.
Commit history has been changed to move the ns1r test to integration::integration
payjoin/tests/integration.rs
Outdated
res = test_ns1r(&services) => assert!(res.is_ok(), "NS1R failed: {:#?}", res) | ||
); | ||
|
||
async fn test_ns1r(services: &TestServices) -> Result<(), BoxError> { |
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.
nit do_ns1r
would be more consistent with our other tests
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.
Done
payjoin/tests/integration.rs
Outdated
struct InnerSenderTestSession { | ||
receiver_session: Receiver, | ||
sender_get_ctx: MultiPartyGetContext, | ||
script_pubkey: ScriptBuf, | ||
} |
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.
An integration::[v2?::]multiparty
mod would give this a more beautiful home.
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.
moved to integration::multiparty
payjoin/tests/integration.rs
Outdated
let psbt = Psbt::from_str(&psbt)?; | ||
Ok(psbt) |
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.
unnecessary assignment
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.
Done
@@ -679,7 +941,7 @@ mod integration { | |||
let receiver_utxos = receiver.list_unspent(None, None, None, None, None)?; | |||
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) |
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.
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.
529e8af
to
3542cbe
Compare
Hmm @DanGould I'm getting all green locally running |
3542cbe
to
43335a7
Compare
**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.
43335a7
to
16aadbd
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.
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. |
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 thev2 reciever is assembling a payjoin it calls in the v1
try_preserving_privacy
method which invalidates for txs with > 2outputs. We would either need to relax this constraint or write a
bespoke version of
try_preserving_privacy
that optimizes for themultiparty 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