Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions smite-ir/src/mutators/operation_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ fn mutate_operation(op: &mut Operation, rng: &mut impl Rng) -> bool {
| Operation::RecvAcceptChannel
| Operation::RecvFundingSigned
| Operation::RecvChannelReady
| Operation::RecvShutdown
| Operation::BroadcastTransaction
| Operation::LookupShortChannelId => {
unreachable!("is_param_mutable returned true for {op:?}")
Expand Down
21 changes: 20 additions & 1 deletion smite-ir/src/operation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,17 @@ pub enum Operation {
/// point unknown) and its funding transaction has enough confirmations for
/// the target to have sent `channel_ready`.
RecvChannelReady,
/// Receive and parse the target's `shutdown` in reply to ours.
/// Produces the target's `scriptpubkey` (`Bytes`), or empty `Bytes` if no
/// `shutdown` was received.
///
/// This is a no-op unless the channel is tracked and the target has not
/// replied yet. A `warning` the target may send instead of replying is
/// accepted.
///
/// Inputs (1):
/// 0: `SentShutdown` from the `SendShutdown` being answered
RecvShutdown,
/// Mines the given number of blocks on the Bitcoin network.
MineBlocks(u8),
/// Sign wallet inputs of the transaction and broadcast it via `bitcoin-cli`.
Expand Down Expand Up @@ -556,6 +567,7 @@ impl fmt::Display for Operation {
Self::RecvAcceptChannel => write!(f, "RecvAcceptChannel"),
Self::RecvFundingSigned => write!(f, "RecvFundingSigned"),
Self::RecvChannelReady => write!(f, "RecvChannelReady()"),
Self::RecvShutdown => write!(f, "RecvShutdown"),
Self::MineBlocks(v) => write!(f, "MineBlocks({v})"),
Self::BroadcastTransaction => write!(f, "BroadcastTransaction"),
Self::LookupShortChannelId => write!(f, "LookupShortChannelId"),
Expand All @@ -579,7 +591,9 @@ impl Operation {
Self::LoadForwardingFee(_) => Some(VariableType::ForwardingFee),
Self::LoadU16(_) => Some(VariableType::U16),
Self::LoadU8(_) => Some(VariableType::U8),
Self::LoadBytes(_) | Self::LoadShutdownScript(_) => Some(VariableType::Bytes),
Self::LoadBytes(_) | Self::LoadShutdownScript(_) | Self::RecvShutdown => {
Some(VariableType::Bytes)
}
Self::LoadFeatures(_) | Self::LoadChannelType(_) => Some(VariableType::Features),
Self::LoadPrivateKey(_) => Some(VariableType::PrivateKey),
Self::LoadChannelId(_) | Self::RecvFundingSigned => Some(VariableType::ChannelId),
Expand Down Expand Up @@ -718,6 +732,7 @@ impl Operation {
],
Self::RecvAcceptChannel => vec![VariableType::SentOpenChannel],
Self::RecvFundingSigned => vec![VariableType::SentFundingCreated],
Self::RecvShutdown => vec![VariableType::SentShutdown],
Self::BroadcastTransaction | Self::LookupShortChannelId => {
vec![VariableType::FundingTransaction]
}
Expand Down Expand Up @@ -763,6 +778,7 @@ impl Operation {
| Self::SendShutdown
| Self::RecvFundingSigned
| Self::RecvChannelReady
| Self::RecvShutdown
| Self::MineBlocks(_)
| Self::BroadcastTransaction
| Self::LookupShortChannelId => vec![],
Expand Down Expand Up @@ -812,6 +828,7 @@ impl Operation {
| Self::RecvAcceptChannel
| Self::RecvFundingSigned
| Self::RecvChannelReady
| Self::RecvShutdown
| Self::MineBlocks(_)
| Self::BroadcastTransaction => true,
}
Expand Down Expand Up @@ -866,6 +883,7 @@ impl Operation {
| Self::RecvAcceptChannel
| Self::RecvFundingSigned
| Self::RecvChannelReady
| Self::RecvShutdown
| Self::MineBlocks(_)
| Self::BroadcastTransaction
| Self::LookupShortChannelId => false,
Expand Down Expand Up @@ -919,6 +937,7 @@ impl Operation {
| Self::RecvAcceptChannel
| Self::RecvFundingSigned
| Self::RecvChannelReady
| Self::RecvShutdown
| Self::BroadcastTransaction
| Self::LookupShortChannelId => false,
}
Expand Down
7 changes: 6 additions & 1 deletion smite-ir/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,7 @@ fn display_send_and_recv_channel_ready_program() {
}

#[test]
fn display_send_shutdown_program() {
fn display_send_and_recv_shutdown_program() {
let instructions = vec![
Instruction {
operation: Operation::LoadChannelId([0xcd; 32]),
Expand All @@ -584,6 +584,10 @@ fn display_send_shutdown_program() {
operation: Operation::SendShutdown,
inputs: vec![0, 1],
},
Instruction {
operation: Operation::RecvShutdown,
inputs: vec![2],
},
];

let program = Program { instructions };
Expand All @@ -596,6 +600,7 @@ fn display_send_shutdown_program() {
format!("v0 = LoadChannelId(0x{cid_hex})"),
format!("v1 = LoadShutdownScript(P2wpkh(0x{spk_hex}))"),
"v2 = SendShutdown(v0, v1)".into(),
"v3 = RecvShutdown(v2)".into(),
];
assert_eq!(lines.len(), expected.len(), "line count mismatch");
for (i, (got, want)) in lines.iter().zip(expected.iter()).enumerate() {
Expand Down
6 changes: 3 additions & 3 deletions smite-ir/src/variable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
//! The serialized program stores data only in [`Operation`] literals.

use bitcoin::secp256k1::PublicKey;
use smite::bolt::{AcceptChannel, ChannelId, OpenChannel, ShortChannelId};
use smite::bolt::{AcceptChannel, ChannelId, OpenChannel, ShortChannelId, Shutdown};
use smite::channel_tx::FundingTransaction;

const CHAIN_HASH_SIZE: usize = 32;
Expand Down Expand Up @@ -60,7 +60,7 @@ pub enum Variable {
SentFundingCreated,
/// `shutdown` has been sent, so the counterparty's `shutdown` may now be
/// received.
SentShutdown,
SentShutdown(Shutdown),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the first affine variable to contain actual data, and I don't think we really need to do this.

We could make RecvShutdown like RecvChannelReady and record our shutdown script in the channel state when we send shutdown. Then no matter which shutdown we get a response for, we can look it's channel up in the channel state to get our script.

}

impl Variable {
Expand Down Expand Up @@ -88,7 +88,7 @@ impl Variable {
Self::FundingTransaction(_) => VariableType::FundingTransaction,
Self::SentOpenChannel => VariableType::SentOpenChannel,
Self::SentFundingCreated => VariableType::SentFundingCreated,
Self::SentShutdown => VariableType::SentShutdown,
Self::SentShutdown(_) => VariableType::SentShutdown,
}
}
}
Expand Down
106 changes: 100 additions & 6 deletions smite-scenarios/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use smite::bolt::{
AcceptChannel, AnnouncementSignatures, ChannelAnnouncement, ChannelId, ChannelReady,
ChannelReadyTlvs, ChannelUpdate, Features, FromMessage, FundingCreated, FundingSigned, Message,
MessageType, NodeAnnouncement, OpenChannel, OpenChannelTlvs, Pong, ShortChannelId, Shutdown,
TemporaryChannelId,
TemporaryChannelId, is_standard_shutdown_script,
};
use smite::channel_tx::{
ChannelConfig, ChannelPartyConfig, ChannelState, FundingTransaction, HolderIdentity, Side,
Expand Down Expand Up @@ -484,14 +484,14 @@ impl<C: Connection, B: BitcoinRpc, R: TargetRpc> Executor<C, B, R> {

Operation::SendShutdown => {
let sd = build_shutdown(&variables, &instr.inputs);
let encoded = Message::Shutdown(sd).encode();
let encoded = Message::Shutdown(sd.clone()).encode();
log::debug!(
"[{:?}] SendShutdown: {} bytes",
start.elapsed(),
encoded.len()
);
self.conn.send_message(&encoded)?;
Some(Variable::SentShutdown)
Some(Variable::SentShutdown(sd))
}

Operation::RecvAcceptChannel => {
Expand Down Expand Up @@ -538,6 +538,40 @@ impl<C: Connection, B: BitcoinRpc, R: TargetRpc> Executor<C, B, R> {
None
}

Operation::RecvShutdown => {
let Variable::SentShutdown(sent) = consume_affine(
&mut variables,
instr.inputs[0],
instr.operation.input_types()[0],
) else {
unreachable!("consume_affine checked the variable type");
};
// TODO: we only expect `shutdown` when all HTLCs are resolved, else this is a
// no-op.
let reply = if is_shutdown_expected(&self.channel_states, sent.channel_id) {
log::debug!("[{:?}] RecvShutdown: waiting", start.elapsed());
let reply = recv_shutdown_reply(
&mut self.conn,
&sent,
&self.context.negotiated_features,
)?;
log::debug!("[{:?}] RecvShutdown: received", start.elapsed());
reply
} else {
None
};
match reply {
Some(sd) => {
self.channel_states
.get_mut(&sent.channel_id)
.expect("is_shutdown_expected guarantees a tracked channel")
.counterparty_shutdown_received = true;
Some(Variable::Bytes(sd.scriptpubkey))
}
None => Some(Variable::Bytes(Vec::new())),
}
Comment on lines +551 to +572

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: this code is over-complicated. If recv_shutdown returns the spk instead, we get:

Suggested change
let reply = if is_shutdown_expected(&self.channel_states, sent.channel_id) {
log::debug!("[{:?}] RecvShutdown: waiting", start.elapsed());
let reply = recv_shutdown_reply(
&mut self.conn,
&sent,
&self.context.negotiated_features,
)?;
log::debug!("[{:?}] RecvShutdown: received", start.elapsed());
reply
} else {
None
};
match reply {
Some(sd) => {
self.channel_states
.get_mut(&sent.channel_id)
.expect("is_shutdown_expected guarantees a tracked channel")
.counterparty_shutdown_received = true;
Some(Variable::Bytes(sd.scriptpubkey))
}
None => Some(Variable::Bytes(Vec::new())),
}
if is_shutdown_expected(&self.channel_states, sent.channel_id) {
log::debug!("[{:?}] RecvShutdown: waiting", start.elapsed());
let spk = recv_shutdown(
&mut self.conn,
&sent,
&self.context.negotiated_features,
)?;
log::debug!("[{:?}] RecvShutdown: received", start.elapsed());
Some(Variable::Bytes(spk))
} else {
Some(Variable::Bytes(Vec::new()))
};

}

Operation::MineBlocks(v) => {
// Clear the private mempool and mine the requested blocks,
// adding those transactions to the first block.
Expand Down Expand Up @@ -681,8 +715,12 @@ define_resolver!(
);

/// Consumes an affine variable, leaving its slot void so it cannot be used
/// again.
fn consume_affine(variables: &mut [Option<Variable>], index: usize, expected: VariableType) {
/// again, and returns it.
fn consume_affine(
variables: &mut [Option<Variable>],
index: usize,
expected: VariableType,
) -> Variable {
assert!(
expected.is_affine(),
"consume_affine called with non-affine type {expected:?}; voiding the slot would break later reads"
Expand All @@ -691,7 +729,9 @@ fn consume_affine(variables: &mut [Option<Variable>], index: usize, expected: Va
if actual != expected {
type_mismatch(index, expected, actual);
}
variables[index] = None;
variables[index]
.take()
.expect("resolve checked the slot is not void")
}

// -- Operation handlers --
Expand Down Expand Up @@ -1247,6 +1287,60 @@ fn is_channel_ready_expected(
})
}

/// Returns `true` if the target still owes us a `shutdown` response on the given channel.
fn is_shutdown_expected(
channel_states: &HashMap<ChannelId, ChannelState>,
channel_id: ChannelId,
) -> bool {
// TODO: we don't know for sure if the target will reply because if a target didn't reply with
// `channel_ready` yet, it MAY reply with `shutdown` (but doesn't have to)
Comment on lines +1295 to +1296

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we also check for this case? We could just check whether the PCP exists in the channel state.

We would avoid some 1s timeouts on targets that don't reply, but lose some edge case coverage on targets that do reply. WDYT?

// TODO: once the target has replied, a duplicate `shutdown` from it goes unread here
channel_states
.get(&channel_id)
.is_some_and(|state| !state.counterparty_shutdown_received)
}

/// Receives the target's reply to our `shutdown`, or `None` if it sent a
/// `warning` for our channel instead, which BOLT 2 allows when our
/// `scriptpubkey` is non-standard.
///
/// # Errors
///
/// Returns [`ExecuteError::UnexpectedMessage`] if the received message is
/// neither a `shutdown` nor such a `warning`, or is a `shutdown` for another
/// channel. That may answer a `shutdown` we sent there earlier, which we can't
/// check against the `shutdown` we sent on this channel.
fn recv_shutdown_reply(
conn: &mut impl Connection,
sent: &Shutdown,
negotiated_features: &Features,
) -> Result<Option<Shutdown>, ExecuteError> {
let may_warn = !is_standard_shutdown_script(&sent.scriptpubkey, negotiated_features);
match recv_non_ping(conn, RECV_IDLE_TIMEOUT)? {
Message::Shutdown(sd) if sd.channel_id == sent.channel_id => Ok(Some(sd)),
Message::Shutdown(sd) => {
log::debug!(
"received shutdown on {} while waiting on {}",
sd.channel_id,
sent.channel_id
);
Comment on lines +1322 to +1326

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes sense to flag this as an error -- we can handle any shutdown message by keeping the necessary info in channel state and looking it up by channel_id.

Err(ExecuteError::UnexpectedMessage {
expected: MessageType::SHUTDOWN,
got: MessageType::SHUTDOWN,
})
}
Message::Warning(w)
if may_warn && (w.channel_id == sent.channel_id || w.channel_id == ChannelId::ALL) =>
{
Ok(None)
}
Comment on lines +1332 to +1336

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes sense to handle this warning. It looks like every impl except LDK will disconnect us after sending warning anyway, and LDK acts as if the shutdown never came.

We could just stop execution like we usually do on warnings.

other => Err(ExecuteError::UnexpectedMessage {
expected: MessageType::SHUTDOWN,
got: other.msg_type(),
}),
}
}

/// Records a sent `open_channel`, keyed by `temporary_channel_id`, so the
/// funding flow can build commitments from the values actually put on the wire.
///
Expand Down
Loading
Loading