Fix spurious panic on bogus funding txn that confirm and are spent #1585
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
TheBlueMatt
force-pushed
the
2022-06-copy_from_slice-sucks
branch
2 times, most recently
from
1f80fad to
dfc6a32
Compare
|
Oops, had some rebase errors in use statements, had to resolve that. |
In c02b6a3 we moved the `payment_preimage` copy from inside the macro which only runs if we are spending an output we know is an HTLC output to doing it for any script that matches our expected length. This can panic if an inbound channel is created with a bogus funding transaction that has a witness program of the HTLC-Success/-Offered length but which does not have a second-to-last witness element which is 32 bytes. Luckily this panic is relatively simple for downstream users to work around - if an invalid-length-copy panic occurs, simply remove the ChannelMonitor from the bogus channel on startup and run without it. Because the channel must be funded by a bogus script in order to reach this panic, the channel will already have closed by the time the funding transaction is spent, and there can be no local funds in such a channel, so removing the `ChannelMonitor` wholesale is completely safe. In order to test this we have to disable an in-line assertion that checks that our transactions match expected scripts which we do by checking for the specific bogus script that we now use in `test_invalid_funding_tx`. Thanks to Eugene Siegel for reporting this issue.
TheBlueMatt
force-pushed
the
2022-06-copy_from_slice-sucks
branch
from
dfc6a32 to
6c480ae
Compare
|
Oops, sorry, one more, looks like |
ariard
approved these changes
Jul 1, 2022
There was a problem hiding this comment.
One More Time Code Review ACK 6c480ae 🎵
Oops, sorry, one more, looks like Vec::from(array) needed const generics which we don't support.
Hmmm, the previous branch built well on my local host. Some issue with bindings or other platforms than x86_64 ?
It was an MSRV issue. |
dunxen
approved these changes
Jul 1, 2022
| // in-line tests later. | ||
|
|
||
| #[cfg(test)] | ||
| pub fn deliberately_bogus_accepted_htlc_witness_program() -> Vec<u8> { |
There was a problem hiding this comment.
Looks all bogus to me, but perhaps the beginning of a new age techno rap.
jkczyz
approved these changes
Jul 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In c02b6a3 we moved the
payment_preimagecopy from inside the macro which only runs if weare spending an output we know is an HTLC output to doing it for
any script that matches our expected length. This can panic if an
inbound channel is created with a bogus funding transaction that
has a witness program of the HTLC-Success/-Offered length but which
does not have a second-to-last witness element which is 32 bytes.
Luckily this panic is relatively simple for downstream users to
work around - if an invalid-length-copy panic occurs, simply remove
the ChannelMonitor from the bogus channel on startup and run
without it. Because the channel must be funded by a bogus script in
order to reach this panic, the channel will already have closed by
the time the funding transaction is spent, and there can be no
local funds in such a channel, so removing the
ChannelMonitorwholesale is completely safe.
In order to test this we have to disable an in-line assertion that
checks that our transactions match expected scripts which we do by
checking for the specific bogus script that we now use in
test_invalid_funding_tx.Thanks to Eugene Siegel for reporting this issue.