Skip to content

Add support for opt_shutdown_anysegwit feature #780 #794

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 1 commit into from
Feb 25, 2021
Merged

Add support for opt_shutdown_anysegwit feature #780 #794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lightning-net-tokio/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ mod tests {
fn handle_funding_created(&self, _their_node_id: &PublicKey, _msg: &FundingCreated) {}
fn handle_funding_signed(&self, _their_node_id: &PublicKey, _msg: &FundingSigned) {}
fn handle_funding_locked(&self, _their_node_id: &PublicKey, _msg: &FundingLocked) {}
fn handle_shutdown(&self, _their_node_id: &PublicKey, _msg: &Shutdown) {}
fn handle_shutdown(&self, _their_node_id: &PublicKey, _their_features: &InitFeatures, _msg: &Shutdown) {}
fn handle_closing_signed(&self, _their_node_id: &PublicKey, _msg: &ClosingSigned) {}
fn handle_update_add_htlc(&self, _their_node_id: &PublicKey, _msg: &UpdateAddHTLC) {}
fn handle_update_fulfill_htlc(&self, _their_node_id: &PublicKey, _msg: &UpdateFulfillHTLC) {}
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 @@ -45,6 +45,7 @@ use std::ops::Deref;
#[cfg(any(test, feature = "fuzztarget"))]
use std::sync::Mutex;
use bitcoin::hashes::hex::ToHex;
use bitcoin::blockdata::opcodes::all::OP_PUSHBYTES_0;

#[cfg(test)]
pub struct ChannelValueStat {
Expand Down Expand Up @@ -737,15 +738,14 @@ impl<Signer: Sign> Channel<Signer> {
let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
match &msg.shutdown_scriptpubkey {
&OptionalField::Present(ref script) => {
// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. We enforce it while receiving shutdown msg
if script.is_p2pkh() || script.is_p2sh() || script.is_v0_p2wsh() || script.is_v0_p2wpkh() {
Some(script.clone())
// Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything
} else if script.len() == 0 {
if script.len() == 0 {
None
// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. Fail the channel
} else {
} else if is_unsupported_shutdown_script(&their_features, script) {
return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided a non-accepted scriptpubkey format. script: ({})", script.to_bytes().to_hex())));
} else {
Some(script.clone())
}
},
// Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel
Expand Down Expand Up @@ -1439,15 +1439,14 @@ impl<Signer: Sign> Channel<Signer> {
let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
match &msg.shutdown_scriptpubkey {
&OptionalField::Present(ref script) => {
// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. We enforce it while receiving shutdown msg
if script.is_p2pkh() || script.is_p2sh() || script.is_v0_p2wsh() || script.is_v0_p2wpkh() {
Some(script.clone())
// Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything
} else if script.len() == 0 {
if script.len() == 0 {
None
// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. Fail the channel
} else if is_unsupported_shutdown_script(&their_features, script) {
return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided a non-accepted scriptpubkey format. script: ({})", script.to_bytes().to_hex())));
} else {
return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided a non-accepted scriptpubkey format. scriptpubkey: ({})", script.to_bytes().to_hex())));
Some(script.clone())
}
},
// Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel
Expand Down Expand Up @@ -3066,7 +3065,7 @@ impl<Signer: Sign> Channel<Signer> {
})
}

pub fn shutdown<F: Deref>(&mut self, fee_estimator: &F, msg: &msgs::Shutdown) -> Result<(Option<msgs::Shutdown>, Option<msgs::ClosingSigned>, Vec<(HTLCSource, PaymentHash)>), ChannelError>
pub fn shutdown<F: Deref>(&mut self, fee_estimator: &F, their_features: &InitFeatures, msg: &msgs::Shutdown) -> Result<(Option<msgs::Shutdown>, Option<msgs::ClosingSigned>, Vec<(HTLCSource, PaymentHash)>), ChannelError>
where F::Target: FeeEstimator
{
if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
Expand All @@ -3085,14 +3084,7 @@ impl<Signer: Sign> Channel<Signer> {
}
assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0);

// BOLT 2 says we must only send a scriptpubkey of certain standard forms, which are up to
// 34 bytes in length, so don't let the remote peer feed us some super fee-heavy script.
if self.is_outbound() && msg.scriptpubkey.len() > 34 {
return Err(ChannelError::Close(format!("Got counterparty shutdown_scriptpubkey ({}) of absurd length from remote peer", msg.scriptpubkey.to_bytes().to_hex())));
}

//Check counterparty_shutdown_scriptpubkey form as BOLT says we must
if !msg.scriptpubkey.is_p2pkh() && !msg.scriptpubkey.is_p2sh() && !msg.scriptpubkey.is_v0_p2wpkh() && !msg.scriptpubkey.is_v0_p2wsh() {
if is_unsupported_shutdown_script(&their_features, &msg.scriptpubkey) {
return Err(ChannelError::Close(format!("Got a nonstandard scriptpubkey ({}) from remote peer", msg.scriptpubkey.to_bytes().to_hex())));
}

Expand Down Expand Up @@ -4217,6 +4209,24 @@ impl<Signer: Sign> Channel<Signer> {
}
}

fn is_unsupported_shutdown_script(their_features: &InitFeatures, script: &Script) -> bool {
// We restrain shutdown scripts to standards forms to avoid transactions not propagating on the p2p tx-relay network

// BOLT 2 says we must only send a scriptpubkey of certain standard forms,
// which for a a BIP-141-compliant witness program is at max 42 bytes in length.
// So don't let the remote peer feed us some super fee-heavy script.
Copy link

Choose a reason for hiding this comment

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

We restrain shutdown scripts for at least different reasons :
a) to avoid fee-heavy script costing us a lot if we're funder (fees are only paid by funder for now)
b) to avoid transactions not propagating on the p2p tx-relay network (e.g sending you a 20-of-20 CHECKMULTISIG scriptpubkey, too big)

I think the first one is clear enough but would be better to underscore b), standardness is a more restrictive notion than smallness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, this comment was already in place before this PR (see here). All I have done here is move it as a result of moving the validation code, and add a very small modification that you mentioned in this PR before.

If you want something different, can you be specific on the exact comment you want in the code? I can copy/paste what you said 👆 but not sure that's exactly what you want.

Copy link

Choose a reason for hiding this comment

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

I think the risk of non-standard transactions broadcasts should be mentioned, it's the reason why those checks should be applied independently of funder/fundee role.

"We restrain shutdown scripts to standards forms to avoid transactions not propagating on the p2p tx-relay network"

let is_script_too_long = script.len() > 42;
if is_script_too_long {
return true;
}

if their_features.supports_shutdown_anysegwit() && script.is_witness_program() && script.as_bytes()[0] != OP_PUSHBYTES_0.into_u8() {
return false;
}

return !script.is_p2pkh() && !script.is_p2sh() && !script.is_v0_p2wpkh() && !script.is_v0_p2wsh()
}

const SERIALIZATION_VERSION: u8 = 1;
const MIN_SERIALIZATION_VERSION: u8 = 1;

Expand Down
8 changes: 4 additions & 4 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2507,7 +2507,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
}
}

fn internal_shutdown(&self, counterparty_node_id: &PublicKey, msg: &msgs::Shutdown) -> Result<(), MsgHandleErrInternal> {
fn internal_shutdown(&self, counterparty_node_id: &PublicKey, their_features: &InitFeatures, msg: &msgs::Shutdown) -> Result<(), MsgHandleErrInternal> {
let (mut dropped_htlcs, chan_option) = {
let mut channel_state_lock = self.channel_state.lock().unwrap();
let channel_state = &mut *channel_state_lock;
Expand All @@ -2517,7 +2517,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
if chan_entry.get().get_counterparty_node_id() != *counterparty_node_id {
return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
}
let (shutdown, closing_signed, dropped_htlcs) = try_chan_entry!(self, chan_entry.get_mut().shutdown(&self.fee_estimator, &msg), channel_state, chan_entry);
let (shutdown, closing_signed, dropped_htlcs) = try_chan_entry!(self, chan_entry.get_mut().shutdown(&self.fee_estimator, &their_features, &msg), channel_state, chan_entry);
if let Some(msg) = shutdown {
channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown {
node_id: counterparty_node_id.clone(),
Expand Down Expand Up @@ -3351,9 +3351,9 @@ impl<Signer: Sign, M: Deref + Sync + Send, T: Deref + Sync + Send, K: Deref + Sy
let _ = handle_error!(self, self.internal_funding_locked(counterparty_node_id, msg), *counterparty_node_id);
}

fn handle_shutdown(&self, counterparty_node_id: &PublicKey, msg: &msgs::Shutdown) {
fn handle_shutdown(&self, counterparty_node_id: &PublicKey, their_features: &InitFeatures, msg: &msgs::Shutdown) {
let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
let _ = handle_error!(self, self.internal_shutdown(counterparty_node_id, msg), *counterparty_node_id);
let _ = handle_error!(self, self.internal_shutdown(counterparty_node_id, their_features, msg), *counterparty_node_id);
}

fn handle_closing_signed(&self, counterparty_node_id: &PublicKey, msg: &msgs::ClosingSigned) {
Expand Down
28 changes: 27 additions & 1 deletion lightning/src/ln/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ mod sealed {
StaticRemoteKey,
// Byte 2
,
// Byte 3
,
],
optional_features: [
// Byte 0
Expand All @@ -108,6 +110,8 @@ mod sealed {
VariableLengthOnion | PaymentSecret,
// Byte 2
BasicMPP,
// Byte 3
ShutdownAnySegwit,
],
});
define_context!(NodeContext {
Expand All @@ -118,6 +122,8 @@ mod sealed {
StaticRemoteKey,
// Byte 2
,
// Byte 3
,
],
optional_features: [
// Byte 0
Expand All @@ -126,6 +132,8 @@ mod sealed {
VariableLengthOnion | PaymentSecret,
// Byte 2
BasicMPP,
// Byte 3
ShutdownAnySegwit,
],
});
define_context!(ChannelContext {
Expand Down Expand Up @@ -252,6 +260,8 @@ mod sealed {
"Feature flags for `payment_secret`.");
define_feature!(17, BasicMPP, [InitContext, NodeContext],
"Feature flags for `basic_mpp`.");
define_feature!(27, ShutdownAnySegwit, [InitContext, NodeContext],
"Feature flags for `opt_shutdown_anysegwit`.");

#[cfg(test)]
define_context!(TestingContext {
Expand Down Expand Up @@ -549,6 +559,17 @@ impl<T: sealed::BasicMPP> Features<T> {
}
}

impl<T: sealed::ShutdownAnySegwit> Features<T> {
pub(crate) fn supports_shutdown_anysegwit(&self) -> bool {
<T as sealed::ShutdownAnySegwit>::supports_feature(&self.flags)
}
#[cfg(test)]
pub(crate) fn clear_shutdown_anysegwit(mut self) -> Self {
<T as sealed::ShutdownAnySegwit>::clear_bits(&mut self.flags);
self
}
}

impl<T: sealed::Context> Writeable for Features<T> {
fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
w.size_hint(self.flags.len() + 2);
Expand Down Expand Up @@ -619,6 +640,9 @@ mod tests {
assert!(!InitFeatures::known().requires_basic_mpp());
assert!(!NodeFeatures::known().requires_basic_mpp());

assert!(InitFeatures::known().supports_shutdown_anysegwit());
assert!(NodeFeatures::known().supports_shutdown_anysegwit());

let mut init_features = InitFeatures::known();
assert!(init_features.initial_routing_sync());
init_features.clear_initial_routing_sync();
Expand Down Expand Up @@ -657,10 +681,12 @@ mod tests {
// - option_data_loss_protect
// - var_onion_optin | static_remote_key (req) | payment_secret
// - basic_mpp
assert_eq!(node_features.flags.len(), 3);
// - opt_shutdown_anysegwit
assert_eq!(node_features.flags.len(), 4);
assert_eq!(node_features.flags[0], 0b00000010);
assert_eq!(node_features.flags[1], 0b10010010);
assert_eq!(node_features.flags[2], 0b00000010);
assert_eq!(node_features.flags[3], 0b00001000);
}

// Check that cleared flags are kept blank when converting back:
Expand Down
4 changes: 2 additions & 2 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 @@ -604,7 +604,7 @@ pub fn close_channel<'a, 'b, 'c>(outbound_node: &Node<'a, 'b, 'c>, inbound_node:
let (tx_a, tx_b);

node_a.close_channel(channel_id).unwrap();
node_b.handle_shutdown(&node_a.get_our_node_id(), &get_event_msg!(struct_a, MessageSendEvent::SendShutdown, node_b.get_our_node_id()));
node_b.handle_shutdown(&node_a.get_our_node_id(), &InitFeatures::known(), &get_event_msg!(struct_a, MessageSendEvent::SendShutdown, node_b.get_our_node_id()));

let events_1 = node_b.get_and_clear_pending_msg_events();
assert!(events_1.len() >= 1);
Expand All @@ -629,7 +629,7 @@ pub fn close_channel<'a, 'b, 'c>(outbound_node: &Node<'a, 'b, 'c>, inbound_node:
})
};

node_a.handle_shutdown(&node_b.get_our_node_id(), &shutdown_b);
node_a.handle_shutdown(&node_b.get_our_node_id(), &InitFeatures::known(), &shutdown_b);
let (as_update, bs_update) = if close_inbound_first {
assert!(node_a.get_and_clear_pending_msg_events().is_empty());
node_a.handle_closing_signed(&node_b.get_our_node_id(), &closing_signed_b.unwrap());
Expand Down
Loading