Take the full funding transaction from the user on generation #856
Conversation
|
Concept ACK, will be needed for dual-funding/splicing support anyway. |
Codecov Report
@@ Coverage Diff @@
## main #856 +/- ##
==========================================
+ Coverage 90.60% 92.48% +1.87%
==========================================
Files 51 51
Lines 27109 33438 +6329
==========================================
+ Hits 24563 30925 +6362
+ Misses 2546 2513 -33
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2021-03-check-tx
branch
from
61216fa to
3e4fd4f
Compare
TheBlueMatt
force-pushed
the
2021-03-check-tx
branch
from
1358066 to
edc5a1d
Compare
| for inp in funding_transaction.input.iter() { | ||
| if inp.witness.is_empty() { | ||
| return Err(APIError::APIMisuseError { | ||
| err: "Funding transaction must be fully signed and spend Segwit outputs".to_owned() |
There was a problem hiding this comment.
By transaction must be fully signed you meant use only SIGHASH_ALL otherwise you might have in-flight malleability breaking the signatures for the first commitment transaction ?
Ideally we have the signatures available in the witnesses so in the future we might parse them to verify sighash types flag to protect users against footgunish behaviors.
There was a problem hiding this comment.
I don't think so - the witness can change but that's fine, it'd only be if the transaction is, eg, SIGHASH_SINGLE and someone on the network can arbitrarily change where the transaction funds are going. I dont think we need to document "don't construct the transaction such that someone can arbitrarily modify it".
There was a problem hiding this comment.
If the witness change in the wrong way with a SIGHASH_SINGLE as you point it that's not fine. Should we document "don't construct the transaction such that someone can arbitrarily modify it " ? I would say we suffer from cognitive dissonance there. It's quite obvious to us but in practice users might authorize malleation or even RBF for fee management (see https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-March/002981.html).
There was a problem hiding this comment.
Right, I guess we could re-enforce the last line - if you don't broadcast the transaction yourself, you can't RBF either :).
| err: "Multiple outputs matched the expected script and value".to_owned() | ||
| }); | ||
| } | ||
| if idx > u16::max_value() as usize { |
There was a problem hiding this comment.
If you assume minimal-sized output (8 bytes value + 2 bytes scriptPubkey), given that MAX_STANDARD_TX_WEIGHT is about 100k bytes, I think you can just reject a transaction with an idx above 10_000 ?
There was a problem hiding this comment.
Sure, but its nice to get an error instead of a panic, even if you did something kinda stupid.
|
Code Review b42f594 If you wanna document more malleation risks, I'm happy. |
|
Added a note |
|
Code Review ACK 26a2f25 Thanks for the comment :) |
Instead of relying on the user to ensure the funding transaction is correct (and panicing when it is confirmed), we should check it is correct when it is generated. By taking the full funding transaciton from the user on generation, we can also handle broadcasting for them instead of doing so via an event.
TheBlueMatt
force-pushed
the
2021-03-check-tx
branch
from
26a2f25 to
3f2efcd
Compare
|
Squashed with no changes, will merge after CI. |
Instead of relying on the user to ensure the funding transaction is
correct (and panicing when it is confirmed), we should check it is
correct when it is generated. By taking the full funding transaciton
from the user on generation, we can also handle broadcasting for
them instead of doing so via an event.