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 33894569727

Coverage increased (+0.1%) to 86.761%

Details

Coverage increased (+0.1%) from the base build.
Patch coverage: 2 uncovered changes across 2 files (171 of 173 lines covered, 98.84%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
payjoin/src/core/receive/common/mod.rs	110	109	99.09%
payjoin/src/core/send/error.rs	1	0	0.0%
Total (5 files)	173	171	98.84%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	16625
Covered Lines:	14424
Line Coverage:	86.76%
Coverage Strength:	340.39 hits per line

💛 - Coveralls

spacebear21 · 2026-08-27T16:16:50Z

+                // Determine the additional amount that the sender will pay in fees,
+                // clamped to the fee output's value so a malicious contribution
+                // cannot underflow the sender output


This seems like something that should have been validated earlier. We probably shouldn't even proceed with the payjoin if a sender is intentionally specifying a max fee contribution that they can't match

I guess my only question is how do we do this check earlier? Don't we need all the receiver contributed inputs to be able to calculate this?

i think the receiver should ignore any maxadditionalfeecontribution outside of the valid range [0;value of change output (+ dust value????)], and the sender should have an additional check when making the original PSBT that ensures their change output can fully cover maxadditionalfeecontribution

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-02T03:55:27Z

+            let change_output = &mut payjoin_psbt.unsigned_tx.output[self.proposal.change_vout];
+            change_output.value =
+                change_output.value.checked_sub(receiver_additional_fee).ok_or_else(|| {
+                    InternalPayloadError::FeeTooHigh(


Maybe we should have a different error for this, since the issue isn't the fee being too high but the output being too low value? Don't feel strongly either way.

Hmm, I tend to want to keep it simple here but I can see where you are coming from.

What do you think the error should be and what should it display?

OutputValueTooSmall but still compare feeRates?

Yeah basically. And then maybe include the highest feasible fee for the output in the error enum? Since thats the only useful thing you can provide to the user to recover from this error at this state in the state machine.

But again, don't feel strongly on this one.

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.

xstoicunicornx · 2026-09-04T19:23:24Z

+            let change_output = &mut payjoin_psbt.unsigned_tx.output[self.proposal.change_vout];
+            change_output.value =
+                change_output.value.checked_sub(receiver_additional_fee).ok_or_else(|| {
+                    InternalPayloadError::FeeTooHigh(


Yeah basically. And then maybe include the highest feasible fee for the output in the error enum? Since thats the only useful thing you can provide to the user to recover from this error at this state in the state machine.

But again, don't feel strongly on this one.

xstoicunicornx · 2026-09-04T20:23:44Z

+            // sender minfeerate makes the fee exceed the change output's value.
+            let change_output = &mut payjoin_psbt.unsigned_tx.output[self.proposal.change_vout];
+            change_output.value =
+                change_output.value.checked_sub(receiver_additional_fee).ok_or_else(|| {


Suggested change

change_output.value.checked_sub(receiver_additional_fee).ok_or_else(|| {

change_output.value.checked_sub(receiver_additional_fee + change_output.script_pubkey.minimal_non_dust()).ok_or_else(|| {

Should we account for dust here?

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

Ensure NAN and negative feerates handled

d3ff1af

benalleng force-pushed the fee-rate-overflow branch from 5cda85f to 35f6215 Compare September 4, 2026 15:43

benalleng added 4 commits September 4, 2026 12:20

Clamp fee contribution to sender output

91b8ae3

Reject fee exceeding change output

9e4da55

Ensure smaller output sizes do not underflow fee calculation

d7ec54d

Reject minfeerate above sanity ceiling

221b9df

benalleng force-pushed the fee-rate-overflow branch 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

	change_output.value.checked_sub(receiver_additional_fee).ok_or_else(\|\| {
	change_output.value.checked_sub(receiver_additional_fee + change_output.script_pubkey.minimal_non_dust()).ok_or_else(\|\| {

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 33894569727

Coverage increased (+0.1%) to 86.761%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 Sep 1, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

xstoicunicornx 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benalleng commented Aug 26, 2026 •

edited

Loading

coveralls commented Aug 26, 2026 •

edited

Loading

spacebear21 Sep 1, 2026 •

edited

Loading