Parse sender params #30

DanGould · 2022-07-31T04:45:28Z

This PR clarifies the sender::Params struct to more closely resemble spec (additional fee contribution != fee contribution) and parses it from query: &str. hyper and reqwest can pass query.unwrap_or("") which can be shown in integration.

The changed Params struct varied from spec in that it allowed independent maxadditionalfeerate and additionalfeeoutput index. Spec says those come together or not at all. Else we log:warn! and ignore.

Kixunil · 2022-07-31T09:14:45Z

Did you find the names confusing? I intentionally named them shorter for non-public code. Perhaps a comment would be better? I'm not a hater of long names it's just that those seemed good enough for non-public code.

Edit: oh, sorry, there are also public items not named after spec. Theses really should be renamed.

Kixunil · 2022-07-31T09:24:39Z

payjoin-client/src/main.rs

@@ -57,7 +57,7 @@ fn main() {
        .psbt;
    let psbt = load_psbt_from_base64(psbt.as_bytes()).unwrap();
    println!("Original psbt: {:#?}", psbt);
-    let pj_params = bip78::sender::Params::with_fee_contribution(bip78::bitcoin::Amount::from_sat(10000), None);
+    let pj_params = bip78::sender::Params::non_incentivizing();


There's nothing against spec here, this is client-side code where client wants the library to detect the change coming from bitcoind. It's useful in simple scenarios like this one.

From doc:

change_index specifies which output can be used to pay fee. I None is provided, then
the output is auto-detected unless the supplied transaction has more than two outputs

ok, Let's keep it. That's useful.

Kixunil · 2022-07-31T09:37:09Z

bip78/src/sender/mod.rs

@@ -36,7 +36,7 @@ type InternalResult<T> = Result<T, InternalValidationError>;
 /// Builder for sender-side payjoin parameters
 ///
 /// These parameters define how client wants to handle PayJoin.
-pub(super) struct Params {
+pub struct Params {


This destroys forward-compatibility and is definitely not acceptable. #[non_exhaustive] would work but builders are still more convenient for user code.

I'm confused about this comment. That change is only on an intermediate commit. The original is pub struct Params { as exists on master right now

The fields are public and adding a public field to a public struct is breaking change unless it's non_exhaustive.

Kixunil · 2022-07-31T09:40:13Z

Also could you please split sender and receiver contributions into two PRs? It's much easier to handle them that way.

Kixunil · 2022-07-31T09:46:17Z

I now realized what you meant by this PR, in case you read this before #27 see this comment: #27 (comment)

DanGould · 2022-08-01T07:05:10Z

A note to clarify what may be confusion, there are receiver params aka uri::PjParams { pj: Url, pjos: bool } and sender::Params { v, disableoutputsubstitution, maxadditionalfeecontribution, additionalfeeoutputindex, minfeerate}. Only the latter are the subject of this PR

DanGould · 2022-08-01T07:13:48Z

I'm can try to separate the name issues from the architectural ones. I think names like non_incentivising are confusing because the receiver can already get a fee without a change output. additional fee is on top of what's already sent.

I think the naming & interface clean up is better off in another pr. There's more work than what's needed to get a basic receiver to parse the sender params, which is the goal of this change.

DanGould · 2022-08-01T08:47:11Z

This is now the minimum change to parse. I split it according to ##27 (comment). Other things I attempted before are noted in #31

bip78/src/params.rs

bip78/src/receiver/mod.rs

DanGould · 2022-08-16T05:07:38Z

rebased on #27, I believed I addressed at the comments

bip78/Cargo.toml

bip78/src/params.rs

Kixunil · 2022-08-23T11:13:22Z

bip78/src/receiver/mod.rs

+    ///
+    /// Note: mixed spends do not necessarily indicate distinct wallet fingerprints.
+    /// This check is intended to prevent some types of wallet fingerprinting.
+    pub fn assume_no_mixed_input_scripts(self) -> MaybeInputsSeen {


We can actually do this check internally, no need to burden the consumer with it. The consumer only needs to get one address type which is the same for all inputs and check that one.

Is this true? Can we stay up to date on all the possible bitcoin scripts?

It's a good point that allowing people to manually check would allow them to work around the library not being updated but if we fail to maintain it they probably should fork it rather than using an obsolete library for potentially security-critical software.

it'd be better if the alpha version of this forgoes mixed input script check since we want to use it for loptos. Is it blocking this PR or may we leave it for the next iteration like some of the warns? I'd love for us to get loptos out ASAP

Even if we skip the check AFAIK there are no wallets currently that support mixed inputs, so that's still useless. If you know of any wallet that does please let me know.

DanGould · 2022-08-28T13:32:34Z

Requested changes applied ✅

Kixunil

I'd like at least macros not be public, the rest are just suggestions.

bip78/src/lib.rs

bip78/src/params.rs

bip78/src/receiver/mock.rs

bip78/src/receiver/mod.rs

Kixunil · 2022-08-31T12:01:11Z

bip78/src/receiver/mod.rs

        }
    }
 }

+impl MaybeInputsOwned {
+    /// The receiver should not be able to sign for any of these Original PSBT inputs.
+    /// Check that none of them are owned by the receiver downstream before proceeding.


Separate into paragraphs.

bip78/src/receiver/mod.rs

Kixunil · 2022-08-31T12:04:06Z

bip78/src/sender/mod.rs

@@ -36,14 +36,14 @@ type InternalResult<T> = Result<T, InternalValidationError>;
 /// Builder for sender-side payjoin parameters
 ///
 /// These parameters define how client wants to handle PayJoin.
-pub struct Params {
+pub struct Configuration {
    disable_output_substitution: bool,
    fee_contribution: Option<(bitcoin::Amount, Option<usize>)>,
    clamp_fee_contribution: bool,
    min_fee_rate: FeeRate,


Params could be inside but not sure if it helps anything.

bip78/tests/integration.rs

DanGould mentioned this pull request Jul 31, 2022

Check Received Proposal with Typestate #27

Closed

Kixunil reviewed Jul 31, 2022

View reviewed changes

DanGould force-pushed the parse-sender-params branch 2 times, most recently from 9c16190 to a259d1e Compare August 1, 2022 08:45

DanGould force-pushed the parse-sender-params branch from a259d1e to 43f5e38 Compare August 1, 2022 08:50

Kixunil requested changes Aug 8, 2022

View reviewed changes

DanGould force-pushed the parse-sender-params branch from 43f5e38 to 39bd78c Compare August 16, 2022 04:50

DanGould requested a review from Kixunil August 16, 2022 05:11

Kixunil requested changes Aug 23, 2022

View reviewed changes

DanGould force-pushed the parse-sender-params branch 3 times, most recently from c8f37b1 to 7f715a1 Compare August 26, 2022 09:57

Kixunil requested changes Aug 31, 2022

View reviewed changes

DanGould mentioned this pull request Oct 16, 2022

Nolooking Roadmap Lightning PayJoin Beta Tracking payjoin/nolooking#6

Open

36 tasks

DanGould added 3 commits October 27, 2022 18:18

Check proposal with type state machine

629b924

Test receiver checks

541501d

Move sender::Params to Configuration

a5f5b26

DanGould force-pushed the parse-sender-params branch from 7f715a1 to daabe0f Compare October 27, 2022 22:22

Parse optional parameters Params from query

98fb17f

DanGould force-pushed the parse-sender-params branch 2 times, most recently from d8294b9 to b0fcb51 Compare October 27, 2022 23:42

Log warnings in release

850f0d9

DanGould force-pushed the parse-sender-params branch from b0fcb51 to 850f0d9 Compare October 27, 2022 23:53

DanGould mentioned this pull request Jan 12, 2023

Complete receive payjoin feature groundwork payjoin/rust-payjoin#18

Merged

3 tasks

DanGould closed this Feb 2, 2024

DanGould deleted the parse-sender-params branch February 2, 2024 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse sender params #30

Parse sender params #30

DanGould commented Jul 31, 2022 •

edited

Loading

Kixunil commented Jul 31, 2022 •

edited

Loading

Kixunil Jul 31, 2022 •

edited

Loading

DanGould Aug 1, 2022

Kixunil Jul 31, 2022

DanGould Aug 1, 2022

Kixunil Aug 1, 2022

Kixunil commented Jul 31, 2022

Kixunil commented Jul 31, 2022

DanGould commented Aug 1, 2022

DanGould commented Aug 1, 2022 •

edited

Loading

DanGould commented Aug 1, 2022 •

edited

Loading

DanGould commented Aug 16, 2022

Kixunil Aug 23, 2022

DanGould Aug 24, 2022

Kixunil Aug 24, 2022

DanGould Aug 28, 2022

Kixunil Aug 31, 2022

DanGould commented Aug 28, 2022

Kixunil left a comment

Kixunil Aug 31, 2022

Kixunil Aug 31, 2022

Parse sender params #30

Parse sender params #30

Conversation

DanGould commented Jul 31, 2022 • edited Loading

Kixunil commented Jul 31, 2022 • edited Loading

Kixunil Jul 31, 2022 • edited Loading

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

Kixunil commented Jul 31, 2022

Kixunil commented Jul 31, 2022

DanGould commented Aug 1, 2022

DanGould commented Aug 1, 2022 • edited Loading

DanGould commented Aug 1, 2022 • edited Loading

DanGould commented Aug 16, 2022

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

DanGould commented Aug 28, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould commented Jul 31, 2022 •

edited

Loading

Kixunil commented Jul 31, 2022 •

edited

Loading

Kixunil Jul 31, 2022 •

edited

Loading

DanGould commented Aug 1, 2022 •

edited

Loading

DanGould commented Aug 1, 2022 •

edited

Loading