Avoid overflow in addition when checking counterparty feerates

This is harmless outside of debug builds - the feerate will
overflow causing it to either spuriously fail the first check, or
correctly pass it and fail the second check. In debug builds,
however, it panics due to integer overflow.

Found by the `full_stack_target` fuzz test in the
Chaincode-provided continuous fuzzing. Thanks Chaincode!

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 4b1f4a7d1c46005b00df61a66eccdcf4975b4026..8daea4d7548fda4d1f72d50b4e3f46774d404f8a 100644 (file)
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -870,14 +870,6 @@ impl<Signer: Sign> Channel<Signer> {
        fn check_remote_fee<F: Deref>(fee_estimator: &F, feerate_per_kw: u32) -> Result<(), ChannelError>
                where F::Target: FeeEstimator
        {
-               let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-               // Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
-               // occasional issues with feerate disagreements between an initiator that wants a feerate
-               // of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
-               // sat/kw before the comparison here.
-               if feerate_per_kw + 250 < lower_limit {
-                       return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", feerate_per_kw, lower_limit)));
-               }
                // We only bound the fee updates on the upper side to prevent completely absurd feerates,
                // always accepting up to 25 sat/vByte or 10x our fee estimator's "High Priority" fee.
                // We generally don't care too much if they set the feerate to something very high, but it
@@ -887,6 +879,14 @@ impl<Signer: Sign> Channel<Signer> {
                if feerate_per_kw as u64 > upper_limit {
                        return Err(ChannelError::Close(format!("Peer's feerate much too high. Actual: {}. Our expected upper limit: {}", feerate_per_kw, upper_limit)));
                }
+               let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+               // Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
+               // occasional issues with feerate disagreements between an initiator that wants a feerate
+               // of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
+               // sat/kw before the comparison here.
+               if feerate_per_kw + 250 < lower_limit {
+                       return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {} (- 250)", feerate_per_kw, lower_limit)));
+               }
                Ok(())
        }
 
@@ -5859,6 +5859,13 @@ mod tests {
                        "MAX_FUNDING_SATOSHIS is greater than all satoshis in existence");
        }
 
+       #[test]
+       fn test_no_fee_check_overflow() {
+               // Previously, calling `check_remote_fee` with a fee of 0xffffffff would overflow in
+               // arithmetic, causing a panic with debug assertions enabled.
+               assert!(Channel::<InMemorySigner>::check_remote_fee(&&TestFeeEstimator { fee_est: 42 }, u32::max_value()).is_err());
+       }
+
        struct Keys {
                signer: InMemorySigner,
        }