Fix (and test) panic when our counterparty uses a bogus funding tx #890
Conversation
Codecov Report
@@ Coverage Diff @@
## main #890 +/- ##
==========================================
+ Coverage 90.23% 90.98% +0.75%
==========================================
Files 57 57
Lines 29207 33281 +4074
==========================================
+ Hits 26355 30282 +3927
- Misses 2852 2999 +147
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
So I had a look on this and the crash you're describing was there before #838 ?
IIUC, before this PR, in case of bogus transaction we set up ShutdownComplete in Channel::block_connected (now Channel::transactions_confirmed) failure path. Once the function return to ChannelManager::block_connected (now ChannelManager::do_chain_event), the error is processed and Channel::force_shutdown would have been called triggering the crash.
| // | ||
| // Previously, all other major lightning implementations had failed to properly sanitize | ||
| // funding transactions from their counterparties, leading to a multi-implementation critical | ||
| // security vulnerability (though we always sanitized properly, we've previously had |
Ouch, huh. That's entirely possible, I just assumed. Updated the commit message. |
TheBlueMatt
force-pushed
the
2021-04-fix-chan-shutdown-crash
branch
from
1fe1559 to
bf5a08e
Compare
I don't think this is true. Looking at 0.0.13, if we get an Err back from chan.block_connected we push an error message and then we return false. We don't ever call force_shutdown, so there is no assertion. I believe this code hunk caused it - 60b962a#diff-645d12c7269d09caab57d2e0c1fd34f672ee259994c86ad4bad7f4da9183671cR3364. |
During the block API refactor, we started calling Channel::force_shutdown when a channel is closed due to a bogus funding tx. However, we still set the channel's state to Shutdown prior to doing so, leading to an assertion in force_shutdown (that the channel is not already closed). This removes the state-set call and adds a (long-overdue) test for this case. Fixes: 60b962a
TheBlueMatt
force-pushed
the
2021-04-fix-chan-shutdown-crash
branch
from
bf5a08e to
eb42caf
Compare
|
Squashed without changes and reverted to the previous commit message, with a Fixes tag pointing to the specific commit. |
There was a problem hiding this comment.
Code Review ACK eb42caf
I don't think this is true. Looking at 0.0.13, if we get an Err back from chan.block_connected we push an error message and then we return false. We don't ever call force_shutdown, so there is no assertion. I believe this code hunk caused it - 60b962a#diff-645d12c7269d09caab57d2e0c1fd34f672ee259994c86ad4bad7f4da9183671cR3364.
Yep after verification I was wrong, we did move the force_shutdown call in the Err branch in #838. Bug wasn't there before.
During the block API refactor, we started calling
Channel::force_shutdown when a channel is closed due to a bogus
funding tx. However, we still set the channel's state to Shutdown
prior to doing so, leading to an assertion in force_shutdown (that
the channel is not already closed).
This removes the state-set call and adds a (long-overdue) test for
this case.