Detect underflows in build_closing_transaction
#3452
Conversation
In `build_closing_transaction`, we check that `value_to_holder` and `value_to_counterparty`, which are signed, are not lower than the dust limit. However, in doing this check, we convert them to signed integers, which could result in an underflow and a failed detection. This scenario should not be reachable, but here we add debug_asserts to positive ensure that scenario isn't hit.
| @@ -4368,10 +4368,12 @@ impl<SP: Deref> Channel<SP> where | |||
| total_fee_satoshis += (-value_to_counterparty) as u64; | |||
| } | |||
|
|
|||
| debug_assert!(value_to_counterparty >= 0); | |||
There was a problem hiding this comment.
I mean panicing in debug is great, but we think its unreachable, we should have some code to handle it (FC the channel) in release builds as well.
arik-so
force-pushed
the
stop-disbursement-underflow
branch
from
f8cf343 to
35f96e4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3452 +/- ##
==========================================
- Coverage 89.70% 89.68% -0.02%
==========================================
Files 130 130
Lines 107422 107430 +8
Branches 107422 107430 +8
==========================================
- Hits 96362 96351 -11
- Misses 8669 8680 +11
- Partials 2391 2399 +8 ☔ View full report in Codecov by Sentry. |
|
Hm, I ran rustfmt and it appears a bunch of whitespace changes crept in here. I'm happy to undo them all. |
arik-so
force-pushed
the
stop-disbursement-underflow
branch
from
42becfe to
28fff28
Compare
Following up on the previous commit, where we added debug_asserts within `build_closing_transaction` to ensure neither `value_to_holder` nor `value_to_counterparty` underflow, we now also force-close the channel in the (presumably impossible) event that it did happen.
arik-so
force-pushed
the
stop-disbursement-underflow
branch
from
28fff28 to
a652980
Compare
| (closing_signed, signed_tx, shutdown_result) | ||
| } | ||
| Err(err) => { | ||
| let shutdown = self.context.force_shutdown(true, ClosureReason::ProcessingError {err: err.to_string()}); |
There was a problem hiding this comment.
locked_close_channel mentions 2 things that need to happen in addition to calling force_shutdown, and they are being handled in convert_chan_phase_err.
Do we need to do something similar here?
There was a problem hiding this comment.
It gets handled by the call site:
rust-lightning/lightning/src/ln/channelmanager.rs
Line 9598 in e0d81ca
|
So sorry, but what does "lo" mean? |
|
"lol" with the last l missing :) |
In
build_closing_transaction, we check thatvalue_to_holderandvalue_to_counterparty, which are signed, are not lower than the dust limit. However, in doing this check, we convert them to signed integers, which could result in an underflow and a failed detection.This scenario should not be reachable, but here we add debug_asserts to positive ensure that scenario isn't hit.
Addresses #3410.