Added test for testing sciptpubkey of closing message per bolt 2 spec #193
Conversation
src/ln/channel.rs
Outdated
| @@ -2098,7 +2102,7 @@ impl Channel { | |||
| if !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() { | |||
| return Err(HandleError{err: "Remote end sent us a closing_signed while there were still pending HTLCs", action: None}); | |||
| } | |||
| if msg.fee_satoshis > 21000000 * 10000000 { | |||
| if msg.fee_satoshis > 21000000 * 10000000 { //is this test required? We check for a much lower amount below, thus if that one will fail, this one will always fail? | |||
There was a problem hiding this comment.
You can almost certainly cause an overflow in build_closing_transaction if we dont (which results in a panic in rust in debug mode).
There was a problem hiding this comment.
ok cwl, wasnt exactly sure, I will leave a comment about this.
|
I'd actually kinda like to figure out why the BOLT suggests that we check it against an acceptable type. Like, checking the size is definitely useful for fee reasons, but if its within the right size, why should we care what script they want to use? |
|
This is the reason they give in the official bolt specs: |
SWvheerden
force-pushed
the
Bolt2-specs-errors-of-closing-messages-
branch
from
25a6350 to
9f6ce0a
Compare
src/ln/channel.rs
Outdated
| //TODO: Check shutdown_scriptpubkey form as BOLT says we must? WHYYY | ||
|
|
||
| //Check shutdown_scriptpubkey form as BOLT says we must | ||
| if !(msg.scriptpubkey.is_p2pkh())&&!(msg.scriptpubkey.is_p2sh())&&!(msg.scriptpubkey.is_v0_p2wpkh())&&!(msg.scriptpubkey.is_v0_p2wsh()){ |
There was a problem hiding this comment.
nit: adding a space before/after AND operator isn't nicer for code readability ?
SWvheerden
force-pushed
the
Bolt2-specs-errors-of-closing-messages-
branch
from
f7fff08 to
b58bab0
Compare
|
|
||
| //Check shutdown_scriptpubkey form as BOLT says we must | ||
| if !(msg.scriptpubkey.is_p2pkh()) && !(msg.scriptpubkey.is_p2sh()) | ||
| && !(msg.scriptpubkey.is_v0_p2wpkh()) && !(msg.scriptpubkey.is_v0_p2wsh()){ |
There was a problem hiding this comment.
nit: having the same indentation on the second line of an if as the body looks confusing to me. I usually double-indent this kind of thing, but I've also seen people prefer just an extra linebreak after the {
There was a problem hiding this comment.
nit: redundant parentheses
https://doc.rust-lang.org/reference/expressions.html#expression-precedence
I added a test to check if the sciptpubkey is either one of the 4 required scriptbupkey's.
I flagged another test, I am not sure if that is not required. I will update the code pending the review.