Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle fallible commitment secret #3152

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Handle fallible commitment secret #3152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b3d6ea
Handle fallible `release_commitment_secret`
alecchendev Jul 2, 2024
3413032
Test fallible commitment secret
alecchendev Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 52 additions & 31 deletions lightning/src/ln/async_signer_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,21 @@
//! Tests for asynchronous signing. These tests verify that the channel state machine behaves
//! properly with a signer implementation that asynchronously derives signatures.

use std::collections::HashSet;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: in most files we have a use crate::prelude::*;, which I believe would avoid this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, think that means this file will fail to build no-std, though we can also fix that when we drop the cfg flag.


use bitcoin::{Transaction, TxOut, TxIn, Amount};
use bitcoin::blockdata::locktime::absolute::LockTime;
use bitcoin::transaction::Version;

use crate::chain::channelmonitor::LATENCY_GRACE_PERIOD_BLOCKS;
use crate::chain::ChannelMonitorUpdateStatus;
use crate::events::bump_transaction::WalletSource;
use crate::events::{ClosureReason, Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose};
use crate::events::{ClosureReason, Event, MessageSendEvent, MessageSendEventsProvider};
use crate::ln::{functional_test_utils::*, msgs};
use crate::ln::msgs::ChannelMessageHandler;
use crate::ln::channelmanager::{PaymentId, RAACommitmentOrder, RecipientOnionFields};
use crate::util::test_channel_signer::SignerOp;
use crate::util::logger::Logger;

#[test]
fn test_async_commitment_signature_for_funding_created() {
Expand Down Expand Up @@ -127,11 +130,17 @@ fn test_async_commitment_signature_for_funding_signed() {

#[test]
fn test_async_commitment_signature_for_commitment_signed() {
do_test_async_commitment_signature_for_commitment_signed_revoke_and_ack(true);
do_test_async_commitment_signature_for_commitment_signed_revoke_and_ack(false);
for i in 0..=8 {
let enable_signer_op_order = vec![
SignerOp::GetPerCommitmentPoint,
SignerOp::ReleaseCommitmentSecret,
SignerOp::SignCounterpartyCommitment,
].into_iter().filter(|&op| i & (1 << op as u8) != 0).collect();
do_test_async_commitment_signature_for_commitment_signed_revoke_and_ack(enable_signer_op_order);
}
}

fn do_test_async_commitment_signature_for_commitment_signed_revoke_and_ack(enable_sign_counterparty_commit_first: bool) {
fn do_test_async_commitment_signature_for_commitment_signed_revoke_and_ack(enable_signer_op_order: Vec<SignerOp>) {
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
Expand Down Expand Up @@ -160,31 +169,33 @@ fn do_test_async_commitment_signature_for_commitment_signed_revoke_and_ack(enabl
// Mark dst's signer as unavailable and handle src's commitment_signed: while dst won't yet have a
// `commitment_signed` of its own to offer, it should publish a `revoke_and_ack`.
dst.disable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::GetPerCommitmentPoint);
dst.disable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::ReleaseCommitmentSecret);
dst.disable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::SignCounterpartyCommitment);
dst.node.handle_commitment_signed(&src.node.get_our_node_id(), &payment_event.commitment_msg);
check_added_monitors(dst, 1);

if enable_sign_counterparty_commit_first {
// Unblock CS -> no messages should be sent, since we must send RAA first.
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::SignCounterpartyCommitment);
let mut enabled_signer_ops = HashSet::new();
log_trace!(dst.logger, "enable_signer_op_order={:?}", enable_signer_op_order);
for op in enable_signer_op_order {
enabled_signer_ops.insert(op);
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, op);
dst.node.signer_unblocked(Some((src.node.get_our_node_id(), chan_id)));
let events = dst.node.get_and_clear_pending_msg_events();
assert!(events.is_empty(), "expected no message, got {}", events.len());

// Unblock revoke_and_ack -> we should send both RAA + CS.
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::GetPerCommitmentPoint);
dst.node.signer_unblocked(Some((src.node.get_our_node_id(), chan_id)));
get_revoke_commit_msgs(&dst, &src.node.get_our_node_id());
} else {
// Unblock revoke_and_ack -> we should send just RAA.
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::GetPerCommitmentPoint);
dst.node.signer_unblocked(Some((src.node.get_our_node_id(), chan_id)));
get_event_msg!(dst, MessageSendEvent::SendRevokeAndACK, src.node.get_our_node_id());

// Unblock commitment signed -> we should send CS.
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::SignCounterpartyCommitment);
dst.node.signer_unblocked(Some((src.node.get_our_node_id(), chan_id)));
get_htlc_update_msgs(dst, &src.node.get_our_node_id());
if enabled_signer_ops.contains(&SignerOp::GetPerCommitmentPoint) && enabled_signer_ops.contains(&SignerOp::ReleaseCommitmentSecret) {
// We are just able to send revoke_and_ack
if op == SignerOp::GetPerCommitmentPoint || op == SignerOp::ReleaseCommitmentSecret {
get_event_msg!(dst, MessageSendEvent::SendRevokeAndACK, src.node.get_our_node_id());
}
// We either just sent or previously sent revoke_and_ack
// and now we are able to send commitment_signed
if op == SignerOp::SignCounterpartyCommitment {
get_htlc_update_msgs(dst, &src.node.get_our_node_id());
}
} else {
// We can't send either message until RAA is unblocked
let events = dst.node.get_and_clear_pending_msg_events();
assert!(events.is_empty(), "expected no message, got {}", events.len());
}
}
}

Expand Down Expand Up @@ -296,12 +307,22 @@ enum UnblockSignerAcrossDisconnectCase {

#[test]
fn test_async_raa_peer_disconnect() {
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::AtEnd);
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::BeforeMonitorRestored);
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::BeforeReestablish);
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::AtEnd, true);
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::AtEnd, false);
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::BeforeMonitorRestored, true);
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::BeforeMonitorRestored, false);
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::BeforeReestablish, true);
do_test_async_raa_peer_disconnect(UnblockSignerAcrossDisconnectCase::BeforeReestablish, false);
}

fn do_test_async_raa_peer_disconnect(test_case: UnblockSignerAcrossDisconnectCase) {
fn do_test_async_raa_peer_disconnect(test_case: UnblockSignerAcrossDisconnectCase, raa_blocked_by_commit_point: bool) {
// `raa_blocked_by_commit_point` determines whether we block the RAA by blocking the
// signer on `GetPerCommitmentPoint` or `ReleaseCommitmentSecret`.
let block_raa_signer_op = if raa_blocked_by_commit_point {
SignerOp::GetPerCommitmentPoint
} else {
SignerOp::ReleaseCommitmentSecret
};
let chanmon_cfgs = create_chanmon_cfgs(2);
let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
Expand Down Expand Up @@ -334,7 +355,7 @@ fn do_test_async_raa_peer_disconnect(test_case: UnblockSignerAcrossDisconnectCas

// Mark dst's signer as unavailable and handle src's commitment_signed: while dst won't yet have a
// `commitment_signed` of its own to offer, it should publish a `revoke_and_ack`.
dst.disable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::GetPerCommitmentPoint);
dst.disable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, block_raa_signer_op);
dst.node.handle_commitment_signed(&src.node.get_our_node_id(), &payment_event.commitment_msg);
check_added_monitors(dst, 1);

Expand All @@ -359,13 +380,13 @@ fn do_test_async_raa_peer_disconnect(test_case: UnblockSignerAcrossDisconnectCas

if test_case == UnblockSignerAcrossDisconnectCase::BeforeReestablish {
// Reenable the signer before the reestablish.
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::GetPerCommitmentPoint);
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, block_raa_signer_op);
}

dst.node.handle_channel_reestablish(&src.node.get_our_node_id(), &reestablish_1[0]);

if test_case == UnblockSignerAcrossDisconnectCase::BeforeMonitorRestored {
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::GetPerCommitmentPoint);
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, block_raa_signer_op);
chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::Completed);
let (outpoint, latest_update, _) = dst.chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id).unwrap().clone();
dst.chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, latest_update);
Expand All @@ -384,7 +405,7 @@ fn do_test_async_raa_peer_disconnect(test_case: UnblockSignerAcrossDisconnectCas
}

// Mark dst's signer as available and retry: we now expect to see dst's RAA + CS.
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, SignerOp::GetPerCommitmentPoint);
dst.enable_channel_signer_op(&src.node.get_our_node_id(), &chan_id, block_raa_signer_op);
dst.node.signer_unblocked(Some((src.node.get_our_node_id(), chan_id)));

if test_case == UnblockSignerAcrossDisconnectCase::AtEnd {
Expand Down
48 changes: 29 additions & 19 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5500,32 +5500,42 @@ impl<SP: Deref> Channel<SP> where
fn get_last_revoke_and_ack<L: Deref>(&mut self, logger: &L) -> Option<msgs::RevokeAndACK> where L::Target: Logger {
debug_assert!(self.context.holder_commitment_point.transaction_number() <= INITIAL_COMMITMENT_NUMBER - 2);
self.context.holder_commitment_point.try_resolve_pending(&self.context.holder_signer, &self.context.secp_ctx, logger);
let per_commitment_secret = self.context.holder_signer.as_ref().release_commitment_secret(self.context.holder_commitment_point.transaction_number() + 2);
if let HolderCommitmentPoint::Available { transaction_number: _, current, next: _ } = self.context.holder_commitment_point {
let per_commitment_secret = self.context.holder_signer.as_ref()
.release_commitment_secret(self.context.holder_commitment_point.transaction_number() + 2).ok();
if let (HolderCommitmentPoint::Available { current, .. }, Some(per_commitment_secret)) =
(self.context.holder_commitment_point, per_commitment_secret) {
self.context.signer_pending_revoke_and_ack = false;
Some(msgs::RevokeAndACK {
return Some(msgs::RevokeAndACK {
channel_id: self.context.channel_id,
per_commitment_secret,
next_per_commitment_point: current,
#[cfg(taproot)]
next_local_nonce: None,
})
} else {
#[cfg(not(async_signing))] {
panic!("Holder commitment point must be Available when generating revoke_and_ack");
}
#[cfg(async_signing)] {
// Technically if we're at HolderCommitmentPoint::PendingNext,
// we have a commitment point ready to send in an RAA, however we
// choose to wait since if we send RAA now, we could get another
// CS before we have any commitment point available. Blocking our
// RAA here is a convenient way to make sure that post-funding
// we're only ever waiting on one commitment point at a time.
log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available",
&self.context.channel_id(), self.context.holder_commitment_point.transaction_number());
self.context.signer_pending_revoke_and_ack = true;
None
}
}
if !self.context.holder_commitment_point.is_available() {
log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available",
&self.context.channel_id(), self.context.holder_commitment_point.transaction_number());
}
if per_commitment_secret.is_none() {
log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment secret for {} is not available",
&self.context.channel_id(), self.context.holder_commitment_point.transaction_number(),
self.context.holder_commitment_point.transaction_number() + 2);
}
#[cfg(not(async_signing))] {
panic!("Holder commitment point and per commitment secret must be available when generating revoke_and_ack");
}
#[cfg(async_signing)] {
// Technically if we're at HolderCommitmentPoint::PendingNext,
// we have a commitment point ready to send in an RAA, however we
// choose to wait since if we send RAA now, we could get another
// CS before we have any commitment point available. Blocking our
// RAA here is a convenient way to make sure that post-funding
// we're only ever waiting on one commitment point at a time.
log_trace!(logger, "Last revoke-and-ack pending in channel {} for sequence {} because the next per-commitment point is not available",
&self.context.channel_id(), self.context.holder_commitment_point.transaction_number());
Comment on lines +5535 to +5536
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this log is redundant with the ones above

self.context.signer_pending_revoke_and_ack = true;
None
}
}

Expand Down
8 changes: 4 additions & 4 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,7 +1474,7 @@ fn test_fee_spike_violation_fails_htlc() {

let pubkeys = chan_signer.as_ref().pubkeys();
(pubkeys.revocation_basepoint, pubkeys.htlc_basepoint,
chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER),
chan_signer.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap(),
chan_signer.as_ref().get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx).unwrap(),
chan_signer.as_ref().pubkeys().funding_pubkey)
};
Expand Down Expand Up @@ -7860,15 +7860,15 @@ fn test_counterparty_raa_skip_no_crash() {

// Make signer believe we got a counterparty signature, so that it allows the revocation
keys.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1;
per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER);
per_commitment_secret = keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER).unwrap();

// Must revoke without gaps
keys.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1;
keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1);
keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1).unwrap();

keys.as_ecdsa().unwrap().get_enforcement_state().last_holder_commitment -= 1;
next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(),
&SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap());
&SecretKey::from_slice(&keys.as_ref().release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2).unwrap()).unwrap());
}

nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(),
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/sign/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,7 @@ pub trait ChannelSigner {
///
/// Note that the commitment number starts at `(1 << 48) - 1` and counts backwards.
// TODO: return a Result so we can signal a validation error
fn release_commitment_secret(&self, idx: u64) -> [u8; 32];
fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], ()>;

/// Validate the counterparty's signatures on the holder commitment transaction and HTLCs.
///
Expand Down Expand Up @@ -1361,8 +1361,8 @@ impl ChannelSigner for InMemorySigner {
Ok(PublicKey::from_secret_key(secp_ctx, &commitment_secret))
}

fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
chan_utils::build_commitment_secret(&self.commitment_seed, idx)
fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], ()> {
Ok(chan_utils::build_commitment_secret(&self.commitment_seed, idx))
}

fn validate_holder_commitment(
Expand Down
6 changes: 5 additions & 1 deletion lightning/src/util/test_channel_signer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,11 @@ impl ChannelSigner for TestChannelSigner {
self.inner.get_per_commitment_point(idx, secp_ctx)
}

fn release_commitment_secret(&self, idx: u64) -> [u8; 32] {
fn release_commitment_secret(&self, idx: u64) -> Result<[u8; 32], ()> {
#[cfg(test)]
if !self.is_signer_available(SignerOp::ReleaseCommitmentSecret) {
return Err(());
}
{
let mut state = self.state.lock().unwrap();
assert!(idx == state.last_holder_revoked_commitment || idx == state.last_holder_revoked_commitment - 1, "can only revoke the current or next unrevoked commitment - trying {}, last revoked {}", idx, state.last_holder_revoked_commitment);
Expand Down
Loading