Fee rate check hardening by benalleng · Pull Request #1845 · payjoin/rust-payjoin

benalleng · 2026-08-26T17:26:15Z

This hardens the math on several areas of fee calculation. Namely this does checks that may occur in the bitcoin network but ensures that we do the checks early in the state machine ensuring that we can error out and either try again or close the session rather than accept a tx that will not succeed broadcasting.

Coded up with GLM-5.3

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2026-08-26T17:30:36Z

Coverage Report for CI Build 34396838644

Coverage increased (+0.2%) to 86.812%

Details

Coverage increased (+0.2%) from the base build.
Patch coverage: 3 uncovered changes across 2 files (244 of 247 lines covered, 98.79%).
1 coverage regression across 1 file.

Uncovered Changes

File	Changed	Covered	%
payjoin/src/core/receive/common/mod.rs	150	148	98.67%
payjoin/src/core/send/error.rs	1	0	0.0%
Total (5 files)	247	244	98.79%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
payjoin/src/core/send/mod.rs	1	98.65%

Coverage Stats


Relevant Lines:	16765
Covered Lines:	14554
Line Coverage:	86.81%
Coverage Strength:	337.69 hits per line

💛 - Coveralls

spacebear21 · 2026-08-27T16:23:38Z

+        let tx = self.psbt.clone().extract_tx_fee_rate_limit().map_err(|e| match e {
+            bitcoin::psbt::ExtractTxError::AbsurdFeeRate { fee_rate, .. } =>
+                InternalPayloadError::OriginalPsbtAbsurdFee(
+                    fee_rate,
+                    bitcoin::Psbt::DEFAULT_MAX_FEE_RATE,
+                ),
+            _ => unreachable!("Input UTXOs validated and fee computed successfully"),
+        })?;
+        Ok(original_psbt_fee / tx.weight())


What's the benefit of sanity checking the sender's fee rate here?

I guess that is a pretty simple and cheap check and sort of addresses an adjecent version of your statement above We probably shouldn't even proceed with the payjoin if a sender is intentionally specifying a ~~max~~ fee contribution that they can't match keeping this at the OriginalPayload stage. In this case its not the max fee though but actually a selected fee by the sender

I am also not sure if we need to do this sanity check here, as an absurdly high fee shouldn't affect anyone but the sender? In general I think a check like this should fall to the wallets to catch. Also I think this check is redundant because extract_tx_fee_rate_limit() seems to return ExtractTxError::AbsurdFeeRate by calling psbt.fee() under the hood and mapping the original Error::FeeOverflow error, but we would have already encountered said error because we already call psbt.fee() above.

If we do want to do this sanity check though I think it would be more fitting to put it into check_broadcast_suitability instead (since we are already checking for min fee rate there and intuitively seems like more appropriate place to check this). Or I guess we could also just map Error::FeeOverflow error to the OriginalPsbtAbsurdFee error when we call self.psbt.fee() above.

xstoicunicornx · 2026-09-02T04:24:37Z

+        let tx = self.psbt.clone().extract_tx_fee_rate_limit().map_err(|e| match e {
+            bitcoin::psbt::ExtractTxError::AbsurdFeeRate { fee_rate, .. } =>
+                InternalPayloadError::OriginalPsbtAbsurdFee(
+                    fee_rate,
+                    bitcoin::Psbt::DEFAULT_MAX_FEE_RATE,
+                ),
+            _ => unreachable!("Input UTXOs validated and fee computed successfully"),
+        })?;
+        Ok(original_psbt_fee / tx.weight())


I am also not sure if we need to do this sanity check here, as an absurdly high fee shouldn't affect anyone but the sender? In general I think a check like this should fall to the wallets to catch. Also I think this check is redundant because extract_tx_fee_rate_limit() seems to return ExtractTxError::AbsurdFeeRate by calling psbt.fee() under the hood and mapping the original Error::FeeOverflow error, but we would have already encountered said error because we already call psbt.fee() above.

If we do want to do this sanity check though I think it would be more fitting to put it into check_broadcast_suitability instead (since we are already checking for min fee rate there and intuitively seems like more appropriate place to check this). Or I guess we could also just map Error::FeeOverflow error to the OriginalPsbtAbsurdFee error when we call self.psbt.fee() above.

xstoicunicornx

Just one small new comment about accounting for dust when determining whether receiver output is of sufficient value.

DanGould

Some questions regarding rationale of the changes as-is

DanGould · 2026-09-07T08:46:49Z

+                Some(fee_output) if !owned_vouts.contains(&additional_fee_output_index) => {
+                    let max_contribution = fee_output
+                        .value
+                        .checked_sub(fee_output.script_pubkey.minimal_non_dust())


introducing the dust check introduces a lot of complexity beyond just preventing an underflow. It also This function uses the default value of 0.00003 BTC/kB (3 sat/vByte) also.

On the sender side, there is a defined min_fee_rate that could be provided to minimal_non_dust_custom. On the receiver side, a minimum feerate is passed in two places, once at check_broadcast_suitability and more consequentially at apply_fee_range. It seems that either of these values may be passed to a minimal_non_dust_custom function that more accurately calculates dust, if necessary.

Or wait, is it just the max_contribution effective fee rate supplied to custom, because the max fee subtracts the most sats, I think it's actually THAT supplied to _custom.

But still, taking on this responsibility of preventing dust calculations at least deserves a rationale. Is it essential?

Eager to get this one through. Pretty sure maxadditionalfeecontribution is the one that has potential to make a dust and therefore should be provided to minimal_non_dust_custom

Since it is looking for a feerate not an amount what exact benefit are you looking for creating some precise calculation between minimal_non_dust() and minimal_non_dust_custom(1 sat/vbyte) by using maxadditionalfeecontribution?

Should we even be going outside of the realm of what bitcoin will generally relay through the network or should we be using something like max(min_fee_rate, 3 sat/vbyte) so the dust will always be safely relay-able and satisfy the user's expected dust min at the time of the pj.

I went with minimal_non_dust_custom(max(min_fee_rate, FeeRate::Dust)) where FeeRate::Dust is just the dust relay min of 3 sat / vbyte so the user's preference is accounted for before relay minimum.

On the sender side, there is a defined min_fee_rate that could be provided to minimal_non_dust_custom. On the receiver side, a minimum feerate is passed in two places, once at check_broadcast_suitability and more consequentially at apply_fee_range. It seems that either of these values may be passed to a minimal_non_dust_custom function that more accurately calculates dust, if necessary.

I don't think I'm understanding why the min fee rate should be considered when ensuring that an output is over relay-able dust limit? Wouldn't that strictly be determined by node implementation? For example, with the updated implementation here if the min fee rate is over 3 sat/vByte then we are requiring the output value to be unnecessarily high.

But still, taking on this responsibility of preventing dust calculations at least deserves a rationale. Is it essential?

Thinking on this more I tend to think this is an important point to consider. Do we need to add complexity to account for dust limits right now? I think harding the fee rate check is the more urgent item and accounting for dust limits is a completely separate potential optimization that we would want to think through carefully. There do seem to already be tools for implementors to check the minimal output value required on their own and using the 3 sat/vByte limit also makes an assumption about node dust limit configuration. So maybe for now we should just focus on the fee rate check hardening?

Ok, will do I will create a follow-up issue for us to address this fully.

This ensures that the sender output is capable of paying for its fee contribution otherwise it is ignored.

By calculating the sender maxadditionalfeecontribution is within the range that the sender inputs can afford to pay this should avoid most cases where a receiver ignores this contribution from the sender in the happy path.

benalleng force-pushed the fee-rate-overflow branch from f01f0e4 to 2c25a0e Compare August 26, 2026 17:59

benalleng marked this pull request as ready for review August 26, 2026 18:15

benalleng requested review from DanGould and spacebear21 as code owners August 26, 2026 18:15

benalleng changed the title ~~Fee rate hardening~~ Fee rate check hardening Aug 26, 2026

spacebear21 requested changes Aug 27, 2026

View reviewed changes

benalleng force-pushed the fee-rate-overflow branch from 2c25a0e to 5cda85f Compare August 27, 2026 18:19

xstoicunicornx reviewed Sep 2, 2026

View reviewed changes

benalleng force-pushed the fee-rate-overflow branch 2 times, most recently from 35f6215 to 221b9df Compare September 4, 2026 16:20

benalleng requested review from spacebear21 and xstoicunicornx September 4, 2026 16:20

xstoicunicornx reviewed Sep 4, 2026

View reviewed changes

Comment thread payjoin/src/core/receive/common/mod.rs

Comment thread payjoin/src/core/receive/common/mod.rs Outdated

DanGould reviewed Sep 7, 2026

View reviewed changes

Ensure NAN and negative feerates handled

a258f85

benalleng force-pushed the fee-rate-overflow branch 3 times, most recently from 997f620 to 0c38ce1 Compare September 9, 2026 19:38

benalleng added 5 commits September 9, 2026 15:43

Clamp fee contribution to sender output

be853e1

This ensures that the sender output is capable of paying for its fee contribution otherwise it is ignored.

Reject fee exceeding change output

8bd2a11

Ensure smaller output sizes do not underflow fee calculation

e9306d6

Reject minfeerate above sanity ceiling

d641de2

benalleng force-pushed the fee-rate-overflow branch from 0c38ce1 to d641de2 Compare September 9, 2026 19:44

benalleng mentioned this pull request Sep 9, 2026

Should OutputTooSmall and FeeTooHigh be treated as separate errors? #1871

Open

Uh oh!

Conversation

benalleng commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 34396838644

Coverage increased (+0.2%) to 86.812%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

benalleng Sep 9, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

benalleng Sep 9, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

benalleng commented Aug 26, 2026 •

edited

Loading

coveralls commented Aug 26, 2026 •

edited

Loading

benalleng Sep 9, 2026 •

edited

Loading

benalleng Sep 9, 2026 •

edited

Loading