Skip to content

Refactor async signing test utils to toggle specific method availability #3115

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

Merged
merged 3 commits into from
Jun 18, 2024
Merged

Refactor async signing test utils to toggle specific method availability #3115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8939e2
Allow toggling specific signing methods in test channel signer
alecchendev Jun 10, 2024
1fb2b8e
Refactor tests to use specific async signing ops
alecchendev Jun 17, 2024
21eeca4
Remove global availability logic for testing async signing
alecchendev Jun 11, 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
7 changes: 1 addition & 6 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,12 +396,7 @@ impl SignerProvider for KeyProvider {
let inner: InMemorySigner = ReadableArgs::read(&mut reader, self)?;
let state = self.make_enforcement_state_cell(inner.commitment_seed);

Ok(TestChannelSigner {
inner,
state,
disable_revocation_policy_check: false,
available: Arc::new(Mutex::new(true)),
})
Ok(TestChannelSigner::new_with_revoked(inner, state, false))
}

fn get_destination_script(&self, _channel_keys_id: [u8; 32]) -> Result<ScriptBuf, ()> {
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/chain/channelmonitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1924,9 +1924,9 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitor<Signer> {
}

#[cfg(test)]
pub fn do_signer_call<F: FnMut(&Signer) -> ()>(&self, mut f: F) {
let inner = self.inner.lock().unwrap();
f(&inner.onchain_tx_handler.signer);
pub fn do_mut_signer_call<F: FnMut(&mut Signer) -> ()>(&self, mut f: F) {
let mut inner = self.inner.lock().unwrap();
f(&mut inner.onchain_tx_handler.signer);
}
}

Expand Down
55 changes: 39 additions & 16 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 @@ -20,6 +20,7 @@ use crate::events::{Event, MessageSendEvent, MessageSendEventsProvider, ClosureR
use crate::ln::functional_test_utils::*;
use crate::ln::msgs::ChannelMessageHandler;
use crate::ln::channelmanager::{PaymentId, RecipientOnionFields};
use crate::util::test_channel_signer::SignerOp;

#[test]
fn test_async_commitment_signature_for_funding_created() {
Expand All @@ -43,7 +44,7 @@ fn test_async_commitment_signature_for_funding_created() {
// But! Let's make node[0]'s signer be unavailable: we should *not* broadcast a funding_created
// message...
let (temporary_channel_id, tx, _) = create_funding_transaction(&nodes[0], &nodes[1].node.get_our_node_id(), 100000, 42);
nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &temporary_channel_id, false);
nodes[0].disable_channel_signer_op(&nodes[1].node.get_our_node_id(), &temporary_channel_id, SignerOp::SignCounterpartyCommitment);
nodes[0].node.funding_transaction_generated(&temporary_channel_id, &nodes[1].node.get_our_node_id(), tx.clone()).unwrap();
check_added_monitors(&nodes[0], 0);

Expand All @@ -57,7 +58,7 @@ fn test_async_commitment_signature_for_funding_created() {
channels[0].channel_id
};

nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true);
nodes[0].enable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignCounterpartyCommitment);
nodes[0].node.signer_unblocked(Some((nodes[1].node.get_our_node_id(), chan_id)));

let mut funding_created_msg = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
Expand Down Expand Up @@ -98,7 +99,7 @@ fn test_async_commitment_signature_for_funding_signed() {

// Now let's make node[1]'s signer be unavailable while handling the `funding_created`. It should
// *not* broadcast a `funding_signed`...
nodes[1].set_channel_signer_available(&nodes[0].node.get_our_node_id(), &temporary_channel_id, false);
nodes[1].disable_channel_signer_op(&nodes[0].node.get_our_node_id(), &temporary_channel_id, SignerOp::SignCounterpartyCommitment);
nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
check_added_monitors(&nodes[1], 1);

Expand All @@ -111,7 +112,7 @@ fn test_async_commitment_signature_for_funding_signed() {
assert_eq!(channels.len(), 1, "expected one channel, not {}", channels.len());
channels[0].channel_id
};
nodes[1].set_channel_signer_available(&nodes[0].node.get_our_node_id(), &chan_id, true);
nodes[1].enable_channel_signer_op(&nodes[0].node.get_our_node_id(), &chan_id, SignerOp::SignCounterpartyCommitment);
nodes[1].node.signer_unblocked(Some((nodes[0].node.get_our_node_id(), chan_id)));

expect_channel_pending_event(&nodes[1], &nodes[0].node.get_our_node_id());
Expand Down Expand Up @@ -152,14 +153,14 @@ fn test_async_commitment_signature_for_commitment_signed() {

// 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.set_channel_signer_available(&src.node.get_our_node_id(), &chan_id, false);
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);

get_event_msg!(dst, MessageSendEvent::SendRevokeAndACK, src.node.get_our_node_id());

// Mark dst's signer as available and retry: we now expect to see dst's `commitment_signed`.
dst.set_channel_signer_available(&src.node.get_our_node_id(), &chan_id, true);
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)));

let events = dst.node.get_and_clear_pending_msg_events();
Expand Down Expand Up @@ -215,7 +216,7 @@ fn test_async_commitment_signature_for_funding_signed_0conf() {

// Now let's make node[1]'s signer be unavailable while handling the `funding_created`. It should
// *not* broadcast a `funding_signed`...
nodes[1].set_channel_signer_available(&nodes[0].node.get_our_node_id(), &temporary_channel_id, false);
nodes[1].disable_channel_signer_op(&nodes[0].node.get_our_node_id(), &temporary_channel_id, SignerOp::SignCounterpartyCommitment);
nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created_msg);
check_added_monitors(&nodes[1], 1);

Expand All @@ -230,7 +231,7 @@ fn test_async_commitment_signature_for_funding_signed_0conf() {
};

// At this point, we basically expect the channel to open like a normal zero-conf channel.
nodes[1].set_channel_signer_available(&nodes[0].node.get_our_node_id(), &chan_id, true);
nodes[1].enable_channel_signer_op(&nodes[0].node.get_our_node_id(), &chan_id, SignerOp::SignCounterpartyCommitment);
nodes[1].node.signer_unblocked(Some((nodes[0].node.get_our_node_id(), chan_id)));

let (funding_signed, channel_ready_1) = {
Expand Down Expand Up @@ -299,7 +300,7 @@ fn test_async_commitment_signature_for_peer_disconnect() {

// 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.set_channel_signer_available(&src.node.get_our_node_id(), &chan_id, false);
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);

Expand All @@ -314,7 +315,7 @@ fn test_async_commitment_signature_for_peer_disconnect() {
reconnect_nodes(reconnect_args);

// Mark dst's signer as available and retry: we now expect to see dst's `commitment_signed`.
dst.set_channel_signer_available(&src.node.get_our_node_id(), &chan_id, true);
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)));

{
Expand Down Expand Up @@ -366,7 +367,6 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
route_payment(&nodes[0], &[&nodes[1]], 1_000_000);
let error_message = "Channel force-closed";

nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, false);

if remote_commitment {
// Make the counterparty broadcast its latest commitment.
Expand All @@ -375,6 +375,8 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
check_closed_broadcast(&nodes[1], 1, true);
check_closed_event(&nodes[1], 1, ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true) }, false, &[nodes[0].node.get_our_node_id()], 100_000);
} else {
nodes[0].disable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignHolderCommitment);
nodes[0].disable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignHolderHtlcTransaction);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For convenience we'll probably want some wrapper methods that set/clear everything, no matter the internal representation.

// We'll connect blocks until the sender has to go onchain to time out the HTLC.
connect_blocks(&nodes[0], TEST_FINAL_CLTV + LATENCY_GRACE_PERIOD_BLOCKS + 1);

Expand All @@ -383,7 +385,8 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
assert!(nodes[0].chain_monitor.chain_monitor.get_and_clear_pending_events().is_empty());

// Mark it as available now, we should see the signed commitment transaction.
nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true);
nodes[0].enable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignHolderCommitment);
nodes[0].enable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignHolderHtlcTransaction);
get_monitor!(nodes[0], chan_id).signer_unblocked(nodes[0].tx_broadcaster, nodes[0].fee_estimator, &nodes[0].logger);
}

Expand All @@ -409,7 +412,13 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {

// Mark it as unavailable again to now test the HTLC transaction. We'll mine the commitment such
// that the HTLC transaction is retried.
nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, false);
let sign_htlc_op = if remote_commitment {
SignerOp::SignCounterpartyHtlcTransaction
} else {
SignerOp::SignHolderHtlcTransaction
};
nodes[0].disable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignHolderCommitment);
nodes[0].disable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, sign_htlc_op);
mine_transaction(&nodes[0], &commitment_tx);

check_added_monitors(&nodes[0], 1);
Expand All @@ -426,10 +435,12 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
if anchors && !remote_commitment {
handle_bump_htlc_event(&nodes[0], 1);
}
assert!(nodes[0].tx_broadcaster.txn_broadcast().is_empty());
let txn = nodes[0].tx_broadcaster.txn_broadcast();
assert!(txn.is_empty(), "expected no transaction to be broadcast, got {:?}", txn);

// Mark it as available now, we should see the signed HTLC transaction.
nodes[0].set_channel_signer_available(&nodes[1].node.get_our_node_id(), &chan_id, true);
nodes[0].enable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, SignerOp::SignHolderCommitment);
nodes[0].enable_channel_signer_op(&nodes[1].node.get_our_node_id(), &chan_id, sign_htlc_op);
get_monitor!(nodes[0], chan_id).signer_unblocked(nodes[0].tx_broadcaster, nodes[0].fee_estimator, &nodes[0].logger);

if anchors && !remote_commitment {
Expand All @@ -443,9 +454,21 @@ fn do_test_async_holder_signatures(anchors: bool, remote_commitment: bool) {
}

#[test]
fn test_async_holder_signatures() {
fn test_async_holder_signatures_no_anchors() {
do_test_async_holder_signatures(false, false);
}

#[test]
fn test_async_holder_signatures_remote_commitment_no_anchors() {
do_test_async_holder_signatures(false, true);
}

#[test]
fn test_async_holder_signatures_anchors() {
do_test_async_holder_signatures(true, false);
}

#[test]
fn test_async_holder_signatures_remote_commitment_anchors() {
do_test_async_holder_signatures(true, true);
}
4 changes: 2 additions & 2 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2118,8 +2118,8 @@ impl<SP: Deref> ChannelContext<SP> where SP::Target: SignerProvider {

/// Returns the holder signer for this channel.
#[cfg(test)]
pub fn get_signer(&self) -> &ChannelSignerType<SP> {
return &self.holder_signer
pub fn get_mut_signer(&mut self) -> &mut ChannelSignerType<SP> {
return &mut self.holder_signer
}

/// Only allowed immediately after deserialization if get_outbound_scid_alias returns 0,
Expand Down
74 changes: 52 additions & 22 deletions lightning/src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ use crate::util::errors::APIError;
use crate::util::logger::Logger;
use crate::util::scid_utils;
use crate::util::test_channel_signer::TestChannelSigner;
#[cfg(test)]
use crate::util::test_channel_signer::SignerOp;
use crate::util::test_utils;
use crate::util::test_utils::{panicking, TestChainMonitor, TestScorer, TestKeysInterface};
use crate::util::ser::{ReadableArgs, Writeable};
Expand Down Expand Up @@ -482,46 +484,74 @@ impl<'a, 'b, 'c> Node<'a, 'b, 'c> {
pub fn get_block_header(&self, height: u32) -> Header {
self.blocks.lock().unwrap()[height as usize].0.header
}
/// Changes the channel signer's availability for the specified peer and channel.

/// Toggles this node's signer to be available for the given signer operation.
/// This is useful for testing behavior for restoring an async signer that previously
/// could not return a signature immediately.
#[cfg(test)]
pub fn enable_channel_signer_op(&self, peer_id: &PublicKey, chan_id: &ChannelId, signer_op: SignerOp) {
self.set_channel_signer_ops(peer_id, chan_id, signer_op, true);
}

/// Toggles this node's signer to be unavailable, returning `Err` for the given signer operation.
/// This is useful for testing behavior for an async signer that cannot return a signature
/// immediately.
#[cfg(test)]
pub fn disable_channel_signer_op(&self, peer_id: &PublicKey, chan_id: &ChannelId, signer_op: SignerOp) {
self.set_channel_signer_ops(peer_id, chan_id, signer_op, false);
}

/// Changes the channel signer's availability for the specified peer, channel, and signer
/// operation.
///
/// When `available` is set to `true`, the channel signer will behave normally. When set to
/// `false`, the channel signer will act like an off-line remote signer and will return `Err` for
/// several of the signing methods. Currently, only `get_per_commitment_point` and
/// `release_commitment_secret` are affected by this setting.
/// For the specified signer operation, when `available` is set to `true`, the channel signer
/// will behave normally, returning `Ok`. When set to `false`, and the channel signer will
/// act like an off-line remote signer, returning `Err`. This applies to the signer in all
/// relevant places, i.e. the channel manager, chain monitor, and the keys manager.
#[cfg(test)]
pub fn set_channel_signer_available(&self, peer_id: &PublicKey, chan_id: &ChannelId, available: bool) {
fn set_channel_signer_ops(&self, peer_id: &PublicKey, chan_id: &ChannelId, signer_op: SignerOp, available: bool) {
use crate::sign::ChannelSigner;
log_debug!(self.logger, "Setting channel signer for {} as available={}", chan_id, available);

let per_peer_state = self.node.per_peer_state.read().unwrap();
let chan_lock = per_peer_state.get(peer_id).unwrap().lock().unwrap();
let mut chan_lock = per_peer_state.get(peer_id).unwrap().lock().unwrap();

let mut channel_keys_id = None;
if let Some(chan) = chan_lock.channel_by_id.get(chan_id).map(|phase| phase.context()) {
chan.get_signer().as_ecdsa().unwrap().set_available(available);
if let Some(chan) = chan_lock.channel_by_id.get_mut(chan_id).map(|phase| phase.context_mut()) {
let signer = chan.get_mut_signer().as_mut_ecdsa().unwrap();
if available {
signer.enable_op(signer_op);
} else {
signer.disable_op(signer_op);
}
channel_keys_id = Some(chan.channel_keys_id);
}

let mut monitor = None;
for (funding_txo, channel_id) in self.chain_monitor.chain_monitor.list_monitors() {
if *chan_id == channel_id {
monitor = self.chain_monitor.chain_monitor.get_monitor(funding_txo).ok();
}
}
let monitor = self.chain_monitor.chain_monitor.list_monitors().into_iter()
.find(|(_, channel_id)| *channel_id == *chan_id)
.and_then(|(funding_txo, _)| self.chain_monitor.chain_monitor.get_monitor(funding_txo).ok());
if let Some(monitor) = monitor {
monitor.do_signer_call(|signer| {
monitor.do_mut_signer_call(|signer| {
channel_keys_id = channel_keys_id.or(Some(signer.inner.channel_keys_id()));
signer.set_available(available)
if available {
signer.enable_op(signer_op);
} else {
signer.disable_op(signer_op);
}
});
}

let channel_keys_id = channel_keys_id.unwrap();
let mut unavailable_signers_ops = self.keys_manager.unavailable_signers_ops.lock().unwrap();
let entry = unavailable_signers_ops.entry(channel_keys_id).or_insert(new_hash_set());
if available {
self.keys_manager.unavailable_signers.lock().unwrap()
.remove(channel_keys_id.as_ref().unwrap());
entry.remove(&signer_op);
if entry.is_empty() {
unavailable_signers_ops.remove(&channel_keys_id);
}
} else {
self.keys_manager.unavailable_signers.lock().unwrap()
.insert(channel_keys_id.unwrap());
}
entry.insert(signer_op);
};
}
}

Expand Down
Loading
Loading