Make ChannelSigner solely responsible for validating commitment sigs
#3878
Conversation
As part of the custom transactions project, we want to make channel generic over any funding, htlc, and revokeable scripts used on-chain. Hence, this commit removes the validation of holder commitment signatures from channel, as it requires building the funding, htlc, and revokeable scripts to create the expected sighash. This signature validation is now the sole responsibility of `ChannelSigner::validate_holder_commitment`. This commit updates `InMemorySigner` to honor this new API contract.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
| fn validate_holder_commitment( | ||
| &self, holder_tx: &HolderCommitmentTransaction, | ||
| outbound_htlc_preimages: Vec<PaymentPreimage>, | ||
| &self, channel_parameters: &ChannelTransactionParameters, | ||
| holder_tx: &HolderCommitmentTransaction, outbound_htlc_preimages: Vec<PaymentPreimage>, | ||
| secp_ctx: &Secp256k1<secp256k1::All>, | ||
| ) -> Result<(), ()>; |
There was a problem hiding this comment.
Do we add a logger here ? This would add a generic on the ChannelSigner trait as far as I see.
There was a problem hiding this comment.
Logging is great, but we shouldn't add it to the trait, rather store a logger in the InMemorySigner directly.
| let htlc_tx = chan_utils::build_htlc_transaction( | ||
| &bitcoin_tx.txid, | ||
| holder_tx.feerate_per_kw(), | ||
| channel_parameters.as_holder_broadcastable().contest_delay(), | ||
| &htlc, | ||
| &channel_parameters.channel_type_features, | ||
| &holder_keys.broadcaster_delayed_payment_key, | ||
| &holder_keys.revocation_key, | ||
| ); | ||
| let htlc_redeemscript = chan_utils::get_htlc_redeemscript( | ||
| &htlc, | ||
| &channel_parameters.channel_type_features, | ||
| &holder_keys, | ||
| ); | ||
| let htlc_sighashtype = | ||
| if channel_parameters.channel_type_features.supports_anchors_zero_fee_htlc_tx() { | ||
| EcdsaSighashType::SinglePlusAnyoneCanPay | ||
| } else { | ||
| EcdsaSighashType::All | ||
| }; | ||
| let htlc_sighash = hash_to_message!( | ||
| &sighash::SighashCache::new(&htlc_tx) | ||
| .p2wsh_signature_hash( | ||
| 0, | ||
| &htlc_redeemscript, | ||
| htlc.to_bitcoin_amount(), | ||
| htlc_sighashtype | ||
| ) |
There was a problem hiding this comment.
For code moves, please move the code as-is first (with only indentation changes) and then run rustfmt in a separate commit. That way git show --color-moved --color-moved-ws=ignore-space-change highlights it as a clean move.
| @@ -8892,7 +8892,7 @@ pub fn test_duplicate_funding_err_in_funding() { | |||
| funding_created_msg.funding_output_index += 10; | |||
| nodes[1].node.handle_funding_created(node_c_id, &funding_created_msg); | |||
| get_err_msg(&nodes[1], &node_c_id); | |||
| let err = "Invalid funding_created signature from peer".to_owned(); | |||
| let err = "Failed to validate our commitment".to_owned(); | |||
There was a problem hiding this comment.
Ha, the old error was more descriptive :)
| fn validate_holder_commitment( | ||
| &self, holder_tx: &HolderCommitmentTransaction, | ||
| outbound_htlc_preimages: Vec<PaymentPreimage>, | ||
| &self, channel_parameters: &ChannelTransactionParameters, | ||
| holder_tx: &HolderCommitmentTransaction, outbound_htlc_preimages: Vec<PaymentPreimage>, | ||
| secp_ctx: &Secp256k1<secp256k1::All>, | ||
| ) -> Result<(), ()>; |
There was a problem hiding this comment.
Logging is great, but we shouldn't add it to the trait, rather store a logger in the InMemorySigner directly.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
As part of the custom transactions project, we want to make channel generic over any funding, htlc, and revokeable scripts used on-chain.
Hence, this commit removes the validation of holder commitment signatures from channel, as it requires building the funding, htlc, and revokeable scripts to create the expected sighash.
This signature validation is now the sole responsibility of
ChannelSigner::validate_holder_commitment.This commit updates
InMemorySignerto honor this new API contract.