More fee rate guard protection

[benalleng]

This is a followup to #1845 with some different fee checks as well as some further enhancements on existing changes from that PR.

Coded with gGLM-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]

Coverage Report for CI Build 35689112778

Coverage increased (+0.06%) to 86.761%

Details

• Coverage increased (+0.06%) from the base build.
• Patch coverage: 4 uncovered changes across 2 files (109 of 113 lines covered, 96.46%).
• 2 coverage regressions across 1 file.

Uncovered Changes

File	Changed	Covered	%
payjoin/src/core/receive/error.rs	2	0	0.0%
payjoin/src/core/send/error.rs	2	0	0.0%
Total (5 files)	113	109	96.46%

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
payjoin/src/core/send/error.rs	2	41.98%

---

Coverage Stats

Relevant Lines:	16814
Covered Lines:	14588
Line Coverage:	86.76%
Coverage Strength:	336.7 hits per line

---

💛 - Coveralls

[xstoicunicornx]

utACK 2aec221

Straight forward guards added to protect against an sender output with insufficient value getting selected for fee contributions and overflowing multiplication in fee rate calculations.

One small nit below. Another item that can be follow up is maybe we want to add tests to exercise these error paths? (disclaimer - these code snippets are untested)

// payjoin/src/core/receive/mod.rs
    #[test]
    fn psbt_fee_rate_rejects_overflowing_fee() {
        let mut original = original_from_test_vector();
        // A sender is free to claim any prevout value it likes. Anything above
        // u64::MAX / 1000 sats makes the sat/kwu conversion overflow, which used
        // to wrap silently in release builds and hand back a bogus fee rate.
        original
            .psbt
            .inputs
            .first_mut()
            .expect("test vector has an input")
            .witness_utxo
            .as_mut()
            .expect("test vector input has a witness utxo")
            .value = Amount::MAX;

        let err = original
            .clone()
            .psbt_fee_rate()
            .expect_err("Fee rate should not be computable from an overflowing fee");
        assert!(matches!(err, InternalPayloadError::FeeCalculationOverflow));

        // The same PSBT must be rejected at the first receiver check rather than
        // panicking or passing the min-fee-rate comparison with a wrapped value.
        let err = original
            .check_broadcast_suitability(Some(FeeRate::from_sat_per_vb_u32(1)), |_| Ok(true))
            .expect_err("Broadcast suitability should fail on an overflowing fee");
        assert!(matches!(
            err,
            Error::Protocol(ProtocolError::OriginalPayload(PayloadError(
                InternalPayloadError::FeeCalculationOverflow
            )))
        ));
    }

// payjoin/src/core/send/mod.rs
        #[test]
        fn test_fee_calculation_overflow() -> Result<(), BoxError> {
            let mut ctx = create_psbt_context()?;
            // The guard only runs when a minimum fee rate was requested.
            ctx.min_fee_rate = FeeRate::from_sat_per_vb_u32(1);

            let mut proposal = ctx.original_psbt.clone();
            // A malicious receiver claims an absurd value for an input it
            // contributed. Anything above u64::MAX / 1000 sats makes the
            // sat/kwu conversion overflow, which used to wrap silently in
            // release builds and could pass the min-fee-rate check.
            proposal.inputs[0]
                .witness_utxo
                .as_mut()
                .expect("test vector input has a witness utxo")
                .value = Amount::MAX;

            let err = ctx
                .check_fees(&proposal, Amount::ZERO)
                .expect_err("Fee rate should not be computable from an overflowing fee");
            assert!(matches!(err, InternalProposalError::FeeCalculationOverflow));

            Ok(())
        }

[DanGould]

ACK 67913e3

I see the tests @xstoicunicornx suggested were included and .position(|txo| txo == sender_fee_output) was kept without the additional comment since the original comment describes this behavior.

Regression test to prevent .position(|txo| txo.script_pubkey == sender_fee_output.script_pubkey) from being re-introduced was not added, however. Difficult to place it?

Edit: since there was no rationale against it, I just added the test and rebased into the commit that introduced it so it's harder to regress.

[benalleng]

That's a doozy of a test but it does look like it does what its supposed to 😅

[xstoicunicornx]

utACK 0110cc3

Fixes look good and have complete test coverage.

[xstoicunicornx]

nit

Suggested change

	/// Fee calculation overflowed in arithmetic and was incomputeable
	/// Fee calculation overflowed in arithmetic and was incomputable

[DanGould]