use FeeRate for max_fee_rate arg, and default it to BROADCAST_MIN

[nothingmuch]

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]

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]

I believe a smoke test is preventing this from requesting review

[DanGould]

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]

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]

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]

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]

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]

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]

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.

[nothingmuch]

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

[DanGould]

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]

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]

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]

agreed squash makes sense in this conversation

[nothingmuch]

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]

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

[DanGould]

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