Correct the on-chain script checked in gossip verification #1666
Conversation
There was a problem hiding this comment.
Generally looks good mod the failing tests, i.e., the get_channel_script function (https://github.com/TheBlueMatt/rust-lightning/blob/2022-08-fix-script-check/lightning/src/routing/gossip.rs#L1945) also needs updating.
Also, could you maybe add a check to some test case to ensure we catch this (or similar) issue not only down the line in bindings generation, if there ever were to be a regression?
|
Pushed a fix, as for a check for this, I'm not entirely sure how to write one that is actually useful - sure I could write a test that checks the specific scripts which failed here, but that's not all that interesting, sadly we had several checks which ultimately should have caught this, but of course we replicated the same bug into the tests, which made them not all that helpful. |
Alright, then hopefully tests will just catch if there ever would be a regression. Seems the fix didn't do the trick entirely though. |
|
Seems we're still getting a test panic. With the amount of test panics that arose, I'm happy with the coverage 😆 |
The `bitcoin_key_1` and `bitcoin_key_2` fields in `channel_announcement` messages are sorted according to node_ids rather than the keys themselves, however the on-chain funding script is sorted according to the bitcoin keys themselves. Thus, with some probability, we end up checking that the on-chain script matches the wrong script and rejecting the channel announcement. The correct solution is to use our existing channel funding script generation function which ensure we always match what we generate. This was found in testing the Java bindings, where a test checks that retunring the generated funding script in `chain::Access` results in the constructed channel ending up in our network graph.
TheBlueMatt
force-pushed
the
2022-08-fix-script-check
branch
from
43814f3 to
6265da0
Compare
|
Squashed without further changes. |
4 participants
The
bitcoin_key_1andbitcoin_key_2fields inchannel_announcementmessages are sorted according to node_idsrather than the keys themselves, however the on-chain funding
script is sorted according to the bitcoin keys themselves. Thus,
with some probability, we end up checking that the on-chain script
matches the wrong script and rejecting the channel announcement.
The correct solution is to use our existing channel funding script
generation function which ensure we always match what we generate.
This was found in testing the Java bindings, where a test checks
that retunring the generated funding script in
chain::Accessresults in the constructed channel ending up in our network graph.