Bound incoming HTLC witnessScript to min/max limits #447
Conversation
lightning/src/ln/channelmonitor.rs
Outdated
| @@ -397,6 +396,20 @@ enum InputMaterial { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| pub const ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 138; //Here we have a diff due to HTLC CLTV expiry being < 2^15 in test | |||
There was a problem hiding this comment.
Hmmm, can we just drop these constants from channelmonitor and replace them with an fn weight_to_type(weight: usize) -> type enum.
There was a problem hiding this comment.
You mean type_to_weight ? As these constants are used as type assertions based on a known weight
|
Well they’re at least used for converting a known weight to the input type. If we want to assert an input has a given weight we can move these constants to functional_test_utils.
… On Jan 14, 2020, at 16:55, Antoine Riard ***@***.***> wrote:
@ariard commented on this pull request.
In lightning/src/ln/channelmonitor.rs:
> @@ -397,6 +396,20 @@ enum InputMaterial {
}
}
+#[cfg(test)]
+pub const ACCEPTED_HTLC_SCRIPT_WEIGHT: usize = 138; //Here we have a diff due to HTLC CLTV expiry being < 2^15 in test
You mean type_to_weight ? As these constants are used as type assertions based on a known weight
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
ariard
force-pushed
the
2020-01-fix-weight-computation
branch
from
0ae2166 to
0eb0621
Compare
|
Updated at 0eb0621, introducing HTLCType::weight_to_htlctype |
lightning/src/ln/chan_utils.rs
Outdated
| } | ||
|
|
||
| impl HTLCType { | ||
| pub(super) fn weight_to_htlctype(witness_script_len: usize) -> HTLCType { |
There was a problem hiding this comment.
This should return an Option instead of having a NoneHTLC enum
There was a problem hiding this comment.
I disagree a bit here, I don't find it makes callsites clearer, like "else if (if let Some(htlc_type) = weight_to_htlctype(script.len()) { htlc_type == HTLCType::Offered } else { false })"
There was a problem hiding this comment.
You should be able to make the callsets else if weight_to_htlctype(script.len()) == Some(HTLCType::Offered), which reads fine to me?
There was a problem hiding this comment.
My periodic reminder of Rust greatness.
lightning/src/ln/chan_utils.rs
Outdated
| } | ||
|
|
||
| impl HTLCType { | ||
| pub(super) fn weight_to_htlctype(witness_script_len: usize) -> HTLCType { |
There was a problem hiding this comment.
Could use some at least basic docs, like "checks if a given tx weight is potentially is pre-signed HTLC transaction"
ariard
force-pushed
the
2020-01-fix-weight-computation
branch
from
0eb0621 to
8c17afd
Compare
lightning/src/util/macro_logger.rs
Outdated
| else if HTLCType::weight_to_htlctype(inp.witness.last().unwrap().len()) == Some(HTLCType::AcceptedHTLC) { write!(f, "timeout-")?; break } | ||
| } | ||
| } | ||
| write!(f, " tx")?; |
There was a problem hiding this comment.
nit: awkward space here.
There was a problem hiding this comment.
Hmm it was intentional at first, but I'm fine too with "preimage-timeout-tx". Corrected
The logger which decides what to refer to an on-chain claim tx was assuming that all inputs would have a witness. While this was fine for the one-input case, it broke the fuzzer which was connecting a consensus-invalid transaction. Further, in the case we have multiple inputs, some may not have a witness, which we shouldn't crash on. This fixes 9df0250.
ariard
force-pushed
the
2020-01-fix-weight-computation
branch
from
8c17afd to
c96653b
Compare
lightning/src/ln/chan_utils.rs
Outdated
|
|
||
| impl HTLCType { | ||
| /// Check if a given tx witnessScript weight matchs one of a pre-signed HTLC | ||
| pub(crate) fn weight_to_htlctype(witness_script_len: usize) -> Option<HTLCType> { |
There was a problem hiding this comment.
Wait, why is this called "weight", its redeemScript length.
There was a problem hiding this comment.
The spec refers to weight (BOLT3), I always use it at so but yes you're right it's false. Keep witnessScript tho
lightning/src/ln/chan_utils.rs
Outdated
| } | ||
|
|
||
| impl HTLCType { | ||
| /// Check if a given tx witnessScript weight matchs one of a pre-signed HTLC |
Fix a crash where previously we weren't able to detect any accepted HTLC if its witness-encoded cltv expiry was different from expected ACCEPTED_HTLC_SCRIPT_WEIGHT. This should work for any cltv expiry included between 0 and 16777216 on mainnet, testnet and regtest.
ariard
force-pushed
the
2020-01-fix-weight-computation
branch
from
c96653b to
fbc7885
Compare
Fix a crash where previously we weren't able to detect any accepted
HTLC if its witness-encoded cltv expiry was different from expected
ACCEPTED_HTLC_SCRIPT_WEIGHT. This should work for any cltv expiry
included between 0 and 16777216 on mainnet, testnet and regtest.