use FeeRate for max_fee_rate arg, and default it to BROADCAST_MIN #491

nothingmuch · 2025-01-16T21:05:21Z

Tested manually by modifying the sender to zero out the additional fee contribution, sending a payment with a feerate of 1 sat/vb, with the caveat that if unconfirmed txs were in the mempool the receiver still accepted (both with and without the changes of this PR)

Closes #452
See also #489

coveralls · 2025-01-16T21:08:30Z

Pull Request Test Coverage Report for Build 12957169876

Details

44 of 46 (95.65%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 78.642%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/receive/error.rs	0	2	0.0%

Totals
Change from base Build 12954415194:	0.1%
Covered Lines:	3638
Relevant Lines:	4626

💛 - Coveralls

DanGould · 2025-01-18T13:31:26Z

I believe a smoke test is preventing this from requesting review

DanGould · 2025-01-19T00:47:13Z

I don't believe we actually have the tooling to smoke test this, since the receiver max_fee_rate only comes into effect if the sender were unwilling to pay for an additional receiver input (our receiver adds one input paid for by the sender by custom), OR the receiver is batching, and payjoin CLI has no way to do either of those configurations of of the box at the moment.

For this reason I believe I'd be OK merging this as-is and following up with the default fix for #489 in the payjoin crate, since this would depend on building tooling and that was NOT part of the plan for opening this issue, and I think merging is preferable to closing if those are the two options.

DanGould

I think the latter part of this PR addressing #489 to default to BROADCAST_MIN really ought to apply to payjoin dev kit itself in ProvisionalProposal::apply_fee's max_feerate passed from finalize_proposal's max_feerate_sat_per_vb, the name being a separate problem.

nothingmuch · 2025-01-19T12:55:06Z

I don't believe we actually have the tooling to smoke test this, since the receiver max_fee_rate only comes into effect if the sender were unwilling to pay for an additional receiver input

I planned to just modify the sender to modify the sender to remove that contribution in order to manually test this, just haven't gotten to it yet (partly because I ignored your advice to test against the public directory, partly because i got sidetracked with other things yesterday), so please don't merge yet

I think the latter part of this PR addressing #489 to default to BROADCAST_MIN really ought to apply to payjoin dev kit itself in ProvisionalProposal::apply_fee's max_feerate passed from finalize_proposal's max_feerate_sat_per_vb

So FeerRate -> Option<FeeRate>? Not sure how to handle this otherwise, it did occur to me to do it this way but I reasoned doing it in payjoin-cli was enough for didactic purposes...

the name being a separate problem.

max_effective_fee_rate? that it's typed eliminates the need to specify the unit in the var name

nothingmuch · 2025-01-19T16:21:58Z

I've manually tested it and appears to work, but there's one thing I'm confused about which is that with the max effective feerate set back to 0, if i chain two transactions without mining a block to confirm, the 2nd one does not get rejected.

I have to go again, so I will update the PR with the bikeshedding changes later, and I will try to figure out what's going on, i suspect the effective feerate is potentially calculated incorrectly if the sender is doing CPFP

DanGould · 2025-01-20T02:42:09Z

So FeerRate -> Option<FeeRate>? Not sure how to handle this otherwise, it did occur to me to do it this
way but I reasoned doing it in payjoin-cli was enough for didactic purposes...

Ah, I forgot it was mandatory, but it would make more sense to provide a sane default of BROADCAST_MIN for max_feerate if you agree it is indeed sane.

CPFP is almost definitely miscalculated. not sure how we'd even deal with that or other ancestor payments without bringing in all the mempool policy stuff from core. Must that be the responsibility of the payjoin crate? Should that be the responsibility of the payjoin crate? Or can it be dealt with via a dependency somehow?

nothingmuch · 2025-01-20T14:02:30Z

Ah, I forgot it was mandatory, but it would make more sense to provide a sane default of BROADCAST_MIN for max_feerate if you agree it is indeed sane.

👍, I will move BROADCAST_MIN defaulting to the payjoin crate and modify cli crate to pass None

Must that be the responsibility of the payjoin crate? Should that be the responsibility of the payjoin crate?

IMO definitely not, I was just surprised that it seemed to produce the opposite of what I would have expected, namely that the receiver accepted the payjoin, so just something to investigate

nothingmuch · 2025-01-23T23:05:24Z

rebased, implemented Option, and pushed as a separate commit for easier review, but IMO the last 3 commits should be squashed to reduce churn before merging

DanGould

Some questions, but otherwise lgtm. I don't see a real need to squash, imo the bigger "churn" to control for is the number of times we need to go back and forth to get something merged.

DanGould · 2025-01-24T17:12:19Z

payjoin/src/receive/v1/mod.rs

@@ -829,7 +831,7 @@ pub(crate) mod test {
            .commit_outputs()
            .commit_inputs();

-        let payjoin = payjoin.apply_fee(None, FeeRate::ZERO);
+        let payjoin = payjoin.apply_fee(None, Some(FeeRate::ZERO));


Should this not now take advantage of the default by passing None?

I feel like our reference should take advantage of our sane defaults if we believe them to be sane.

Hmm, I suppose but I also didn't want to change this test since it explicitly covers an edge case.

I guess the sensible thing is to check that both None and Some(ZERO) work

That's reasonable. In haste I thought this was in the main payjoin-cli implementation for some reason, which is why I mentioned "reference."

i believe the reference is actually using finalize_psbt with our configured value, which is appropriate, so this comment may be disregarded.

Above and beyond would be like you said to check None in the unit test as well.

DanGould · 2025-01-24T17:13:34Z

payjoin/src/receive/v1/mod.rs

-        min_feerate_sat_per_vb: Option<FeeRate>,
-        max_feerate_sat_per_vb: FeeRate,
+        min_feerate: Option<FeeRate>,
+        max_effective_feerate: FeeRate,


Should the apply_fee argument also be renamed to max_effective_feerate?

There are other places we're using fee_rate as an argument name instead of feerate and those should be made consistent.

nothingmuch · 2025-01-24T17:36:38Z

Some questions, but otherwise lgtm. I don't see a real need to squash, imo the bigger "churn" to control for is the number of times we need to go back and forth to get something merged.

Fair, but since you posted some feedback if you don't mind i'm going to squash and apply that feedback

DanGould · 2025-01-24T18:03:52Z

agreed squash makes sense in this conversation

nothingmuch · 2025-01-24T19:32:49Z

after wasting some time doing the opposite, going with fee_rate for var names, because that's more consistent with rust-bitcoin ecosystem's naming conventions, and doesn't break payjoin-cli's CLI args

nothingmuch · 2025-01-24T19:53:16Z

... with the exception of optional_parameters::Params.min_feerate since that's pub and part of v1

FeeRate's serde support is used for serialization, whereas on the command line it's parsed as a number

Also renames to max_effective_fee_rate

DanGould · 2025-01-24T20:10:08Z

breaking pub v1 stuff is completely fine

The actual params value is minfeerate= in the serialized form

DanGould

ACK 95aaabd please go ahead and merge

DanGould reviewed Jan 19, 2025

View reviewed changes

nothingmuch marked this pull request as ready for review January 19, 2025 19:32

nothingmuch force-pushed the typed-cli-args branch from 13bf515 to 9a55e8e Compare January 23, 2025 23:04

DanGould approved these changes Jan 24, 2025

View reviewed changes

nothingmuch added 2 commits January 24, 2025 21:03

Use FeeRate for receiver's max_fee_rate

198971f

FeeRate's serde support is used for serialization, whereas on the command line it's parsed as a number

Option wrap max fee rate, default to BROADCAST_MIN

b14176f

Also renames to max_effective_fee_rate

nothingmuch force-pushed the typed-cli-args branch from 9a55e8e to 5d410bf Compare January 24, 2025 20:09

Rename: s/feerate/fee_rate/ for consistency

95aaabd

nothingmuch force-pushed the typed-cli-args branch from 5d410bf to 95aaabd Compare January 24, 2025 20:33

DanGould approved these changes Jan 24, 2025

View reviewed changes

nothingmuch merged commit 877333a into payjoin:master Jan 24, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use FeeRate for max_fee_rate arg, and default it to BROADCAST_MIN #491

use FeeRate for max_fee_rate arg, and default it to BROADCAST_MIN #491

nothingmuch commented Jan 16, 2025 •

edited

Loading

coveralls commented Jan 16, 2025 •

edited

Loading

DanGould commented Jan 18, 2025

DanGould commented Jan 19, 2025

DanGould left a comment •

edited

Loading

nothingmuch commented Jan 19, 2025

nothingmuch commented Jan 19, 2025

DanGould commented Jan 20, 2025

nothingmuch commented Jan 20, 2025

nothingmuch commented Jan 23, 2025

DanGould left a comment

DanGould Jan 24, 2025

nothingmuch Jan 24, 2025

DanGould Jan 24, 2025 •

edited

Loading

DanGould Jan 24, 2025

nothingmuch commented Jan 24, 2025

DanGould commented Jan 24, 2025

nothingmuch commented Jan 24, 2025

nothingmuch commented Jan 24, 2025

DanGould commented Jan 24, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

use FeeRate for max_fee_rate arg, and default it to BROADCAST_MIN #491

use FeeRate for max_fee_rate arg, and default it to BROADCAST_MIN #491

Conversation

nothingmuch commented Jan 16, 2025 • edited Loading

coveralls commented Jan 16, 2025 • edited Loading

Pull Request Test Coverage Report for Build 12957169876

Details

💛 - Coveralls

DanGould commented Jan 18, 2025

DanGould commented Jan 19, 2025

DanGould left a comment • edited Loading

Choose a reason for hiding this comment

nothingmuch commented Jan 19, 2025

nothingmuch commented Jan 19, 2025

DanGould commented Jan 20, 2025

nothingmuch commented Jan 20, 2025

nothingmuch commented Jan 23, 2025

DanGould left a comment

Choose a reason for hiding this comment

DanGould Jan 24, 2025

Choose a reason for hiding this comment

nothingmuch Jan 24, 2025

Choose a reason for hiding this comment

DanGould Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

DanGould Jan 24, 2025

Choose a reason for hiding this comment

nothingmuch commented Jan 24, 2025

DanGould commented Jan 24, 2025

nothingmuch commented Jan 24, 2025

nothingmuch commented Jan 24, 2025

DanGould commented Jan 24, 2025 • edited Loading

DanGould left a comment • edited Loading

Choose a reason for hiding this comment

nothingmuch commented Jan 16, 2025 •

edited

Loading

coveralls commented Jan 16, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

DanGould Jan 24, 2025 •

edited

Loading

DanGould commented Jan 24, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading