Skip to content

Commit 101c09f

Browse files
TheBlueMattTheBlueMatt
authored
Merge pull request #2213 from benthecarman/error-sign-provider-addrs
Allow get_shutdown_scriptpubkey and get_destination_script to return an Error
2 parents 0e8da58 + 0b8bdbf commit 101c09f

File tree

8 files changed

+76
-34
lines changed

8 files changed

+76
-34
lines changed

‎fuzz/src/chanmon_consistency.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -259,18 +259,18 @@ impl SignerProvider for KeyProvider {
259259
})
260260
}
261261

262-
fn get_destination_script(&self) -> Script {
262+
fn get_destination_script(&self) -> Result<Script, ()> {
263263
let secp_ctx = Secp256k1::signing_only();
264264
let channel_monitor_claim_key = SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, self.node_secret[31]]).unwrap();
265265
let our_channel_monitor_claim_key_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &channel_monitor_claim_key).serialize());
266-
Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_monitor_claim_key_hash[..]).into_script()
266+
Ok(Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_monitor_claim_key_hash[..]).into_script())
267267
}
268268

269-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
269+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()> {
270270
let secp_ctx = Secp256k1::signing_only();
271271
let secret_key = SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, self.node_secret[31]]).unwrap();
272272
let pubkey_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &secret_key).serialize());
273-
ShutdownScript::new_p2wpkh(&pubkey_hash)
273+
Ok(ShutdownScript::new_p2wpkh(&pubkey_hash))
274274
}
275275
}
276276

‎fuzz/src/full_stack.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -376,18 +376,18 @@ impl SignerProvider for KeyProvider {
376376
))
377377
}
378378

379-
fn get_destination_script(&self) -> Script {
379+
fn get_destination_script(&self) -> Result<Script, ()> {
380380
let secp_ctx = Secp256k1::signing_only();
381381
let channel_monitor_claim_key = SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap();
382382
let our_channel_monitor_claim_key_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &channel_monitor_claim_key).serialize());
383-
Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_monitor_claim_key_hash[..]).into_script()
383+
Ok(Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&our_channel_monitor_claim_key_hash[..]).into_script())
384384
}
385385

386-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
386+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()> {
387387
let secp_ctx = Secp256k1::signing_only();
388388
let secret_key = SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1]).unwrap();
389389
let pubkey_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &secret_key).serialize());
390-
ShutdownScript::new_p2wpkh(&pubkey_hash)
390+
Ok(ShutdownScript::new_p2wpkh(&pubkey_hash))
391391
}
392392
}
393393

‎fuzz/src/onion_message.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,9 +141,9 @@ impl SignerProvider for KeyProvider {
141141

142142
fn read_chan_signer(&self, _data: &[u8]) -> Result<EnforcingSigner, DecodeError> { unreachable!() }
143143

144-
fn get_destination_script(&self) -> Script { unreachable!() }
144+
fn get_destination_script(&self) -> Result<Script, ()> { unreachable!() }
145145

146-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript { unreachable!() }
146+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()> { unreachable!() }
147147
}
148148

149149
#[cfg(test)]

‎lightning/src/chain/keysinterface.rs

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -543,15 +543,21 @@ pub trait SignerProvider {
543543

544544
/// Get a script pubkey which we send funds to when claiming on-chain contestable outputs.
545545
///
546+
/// If this function returns an error, this will result in a channel failing to open.
547+
///
546548
/// This method should return a different value each time it is called, to avoid linking
547549
/// on-chain funds across channels as controlled to the same user.
548-
fn get_destination_script(&self) -> Script;
550+
fn get_destination_script(&self) -> Result<Script, ()>;
549551

550552
/// Get a script pubkey which we will send funds to when closing a channel.
551553
///
554+
/// If this function returns an error, this will result in a channel failing to open or close.
555+
/// In the event of a failure when the counterparty is initiating a close, this can result in a
556+
/// channel force close.
557+
///
552558
/// This method should return a different value each time it is called, to avoid linking
553559
/// on-chain funds across channels as controlled to the same user.
554-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript;
560+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()>;
555561
}
556562

557563
/// A simple implementation of [`WriteableEcdsaChannelSigner`] that just keeps the private keys in memory.
@@ -1366,12 +1372,12 @@ impl SignerProvider for KeysManager {
13661372
InMemorySigner::read(&mut io::Cursor::new(reader), self)
13671373
}
13681374

1369-
fn get_destination_script(&self) -> Script {
1370-
self.destination_script.clone()
1375+
fn get_destination_script(&self) -> Result<Script, ()> {
1376+
Ok(self.destination_script.clone())
13711377
}
13721378

1373-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
1374-
ShutdownScript::new_p2wpkh_from_pubkey(self.shutdown_pubkey.clone())
1379+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()> {
1380+
Ok(ShutdownScript::new_p2wpkh_from_pubkey(self.shutdown_pubkey.clone()))
13751381
}
13761382
}
13771383

@@ -1461,11 +1467,11 @@ impl SignerProvider for PhantomKeysManager {
14611467
self.inner.read_chan_signer(reader)
14621468
}
14631469

1464-
fn get_destination_script(&self) -> Script {
1470+
fn get_destination_script(&self) -> Result<Script, ()> {
14651471
self.inner.get_destination_script()
14661472
}
14671473

1468-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
1474+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()> {
14691475
self.inner.get_shutdown_scriptpubkey()
14701476
}
14711477
}

‎lightning/src/ln/channel.rs

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -986,7 +986,10 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
986986
secp_ctx.seeded_randomize(&entropy_source.get_secure_random_bytes());
987987

988988
let shutdown_scriptpubkey = if config.channel_handshake_config.commit_upfront_shutdown_pubkey {
989-
Some(signer_provider.get_shutdown_scriptpubkey())
989+
match signer_provider.get_shutdown_scriptpubkey() {
990+
Ok(scriptpubkey) => Some(scriptpubkey),
991+
Err(_) => return Err(APIError::ChannelUnavailable { err: "Failed to get shutdown scriptpubkey".to_owned()}),
992+
}
990993
} else { None };
991994

992995
if let Some(shutdown_scriptpubkey) = &shutdown_scriptpubkey {
@@ -995,6 +998,11 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
995998
}
996999
}
9971000

1001+
let destination_script = match signer_provider.get_destination_script() {
1002+
Ok(script) => script,
1003+
Err(_) => return Err(APIError::ChannelUnavailable { err: "Failed to get destination script".to_owned()}),
1004+
};
1005+
9981006
let temporary_channel_id = entropy_source.get_secure_random_bytes();
9991007

10001008
Ok(Channel {
@@ -1021,7 +1029,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
10211029

10221030
holder_signer,
10231031
shutdown_scriptpubkey,
1024-
destination_script: signer_provider.get_destination_script(),
1032+
destination_script,
10251033

10261034
cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
10271035
cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
@@ -1333,7 +1341,10 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
13331341
} else { None };
13341342

13351343
let shutdown_scriptpubkey = if config.channel_handshake_config.commit_upfront_shutdown_pubkey {
1336-
Some(signer_provider.get_shutdown_scriptpubkey())
1344+
match signer_provider.get_shutdown_scriptpubkey() {
1345+
Ok(scriptpubkey) => Some(scriptpubkey),
1346+
Err(_) => return Err(ChannelError::Close("Failed to get upfront shutdown scriptpubkey".to_owned())),
1347+
}
13371348
} else { None };
13381349

13391350
if let Some(shutdown_scriptpubkey) = &shutdown_scriptpubkey {
@@ -1342,6 +1353,11 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
13421353
}
13431354
}
13441355

1356+
let destination_script = match signer_provider.get_destination_script() {
1357+
Ok(script) => script,
1358+
Err(_) => return Err(ChannelError::Close("Failed to get destination script".to_owned())),
1359+
};
1360+
13451361
let mut secp_ctx = Secp256k1::new();
13461362
secp_ctx.seeded_randomize(&entropy_source.get_secure_random_bytes());
13471363

@@ -1368,7 +1384,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
13681384

13691385
holder_signer,
13701386
shutdown_scriptpubkey,
1371-
destination_script: signer_provider.get_destination_script(),
1387+
destination_script,
13721388

13731389
cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
13741390
cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
@@ -4355,7 +4371,10 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
43554371
Some(_) => false,
43564372
None => {
43574373
assert!(send_shutdown);
4358-
let shutdown_scriptpubkey = signer_provider.get_shutdown_scriptpubkey();
4374+
let shutdown_scriptpubkey = match signer_provider.get_shutdown_scriptpubkey() {
4375+
Ok(scriptpubkey) => scriptpubkey,
4376+
Err(_) => return Err(ChannelError::Close("Failed to get shutdown scriptpubkey".to_owned())),
4377+
};
43594378
if !shutdown_scriptpubkey.is_compatible(their_features) {
43604379
return Err(ChannelError::Close(format!("Provided a scriptpubkey format not accepted by peer: {}", shutdown_scriptpubkey)));
43614380
}
@@ -6062,7 +6081,10 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
60626081
let update_shutdown_script = match self.shutdown_scriptpubkey {
60636082
Some(_) => false,
60646083
None if !chan_closed => {
6065-
let shutdown_scriptpubkey = signer_provider.get_shutdown_scriptpubkey();
6084+
let shutdown_scriptpubkey = match signer_provider.get_shutdown_scriptpubkey() {
6085+
Ok(scriptpubkey) => scriptpubkey,
6086+
Err(_) => return Err(APIError::ChannelUnavailable { err: "Failed to get shutdown scriptpubkey".to_owned() }),
6087+
};
60666088
if !shutdown_scriptpubkey.is_compatible(their_features) {
60676089
return Err(APIError::IncompatibleShutdownScript { script: shutdown_scriptpubkey.clone() });
60686090
}
@@ -7087,17 +7109,17 @@ mod tests {
70877109

70887110
fn read_chan_signer(&self, _data: &[u8]) -> Result<Self::Signer, DecodeError> { panic!(); }
70897111

7090-
fn get_destination_script(&self) -> Script {
7112+
fn get_destination_script(&self) -> Result<Script, ()> {
70917113
let secp_ctx = Secp256k1::signing_only();
70927114
let channel_monitor_claim_key = SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap();
70937115
let channel_monitor_claim_key_hash = WPubkeyHash::hash(&PublicKey::from_secret_key(&secp_ctx, &channel_monitor_claim_key).serialize());
7094-
Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&channel_monitor_claim_key_hash[..]).into_script()
7116+
Ok(Builder::new().push_opcode(opcodes::all::OP_PUSHBYTES_0).push_slice(&channel_monitor_claim_key_hash[..]).into_script())
70957117
}
70967118

7097-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
7119+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()> {
70987120
let secp_ctx = Secp256k1::signing_only();
70997121
let channel_close_key = SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap();
7100-
ShutdownScript::new_p2wpkh_from_pubkey(PublicKey::from_secret_key(&secp_ctx, &channel_close_key))
7122+
Ok(ShutdownScript::new_p2wpkh_from_pubkey(PublicKey::from_secret_key(&secp_ctx, &channel_close_key)))
71017123
}
71027124
}
71037125

‎lightning/src/ln/channelmanager.rs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1854,6 +1854,10 @@ where
18541854
/// Raises [`APIError::APIMisuseError`] when `channel_value_satoshis` > 2**24 or `push_msat` is
18551855
/// greater than `channel_value_satoshis * 1k` or `channel_value_satoshis < 1000`.
18561856
///
1857+
/// Raises [`APIError::ChannelUnavailable`] if the channel cannot be opened due to failing to
1858+
/// generate a shutdown scriptpubkey or destination script set by
1859+
/// [`SignerProvider::get_shutdown_scriptpubkey`] or [`SignerProvider::get_destination_script`].
1860+
///
18571861
/// Note that we do not check if you are currently connected to the given peer. If no
18581862
/// connection is available, the outbound `open_channel` message may fail to send, resulting in
18591863
/// the channel eventually being silently forgotten (dropped on reload).
@@ -2098,6 +2102,11 @@ where
20982102
///
20992103
/// May generate a [`SendShutdown`] message event on success, which should be relayed.
21002104
///
2105+
/// Raises [`APIError::ChannelUnavailable`] if the channel cannot be closed due to failing to
2106+
/// generate a shutdown scriptpubkey or destination script set by
2107+
/// [`SignerProvider::get_shutdown_scriptpubkey`]. A force-closure may be needed to close the
2108+
/// channel.
2109+
///
21012110
/// [`ChannelConfig::force_close_avoidance_max_fee_satoshis`]: crate::util::config::ChannelConfig::force_close_avoidance_max_fee_satoshis
21022111
/// [`Background`]: crate::chain::chaininterface::ConfirmationTarget::Background
21032112
/// [`Normal`]: crate::chain::chaininterface::ConfirmationTarget::Normal
@@ -2122,6 +2131,11 @@ where
21222131
///
21232132
/// May generate a [`SendShutdown`] message event on success, which should be relayed.
21242133
///
2134+
/// Raises [`APIError::ChannelUnavailable`] if the channel cannot be closed due to failing to
2135+
/// generate a shutdown scriptpubkey or destination script set by
2136+
/// [`SignerProvider::get_shutdown_scriptpubkey`]. A force-closure may be needed to close the
2137+
/// channel.
2138+
///
21252139
/// [`ChannelConfig::force_close_avoidance_max_fee_satoshis`]: crate::util::config::ChannelConfig::force_close_avoidance_max_fee_satoshis
21262140
/// [`Background`]: crate::chain::chaininterface::ConfirmationTarget::Background
21272141
/// [`Normal`]: crate::chain::chaininterface::ConfirmationTarget::Normal

‎lightning/src/ln/shutdown_tests.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -669,7 +669,7 @@ fn test_unsupported_anysegwit_shutdown_script() {
669669
let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
670670

671671
// Check that using an unsupported shutdown script fails and a supported one succeeds.
672-
let supported_shutdown_script = chanmon_cfgs[1].keys_manager.get_shutdown_scriptpubkey();
672+
let supported_shutdown_script = chanmon_cfgs[1].keys_manager.get_shutdown_scriptpubkey().unwrap();
673673
let unsupported_shutdown_script =
674674
ShutdownScript::new_witness_program(WitnessVersion::V16, &[0, 40]).unwrap();
675675
chanmon_cfgs[1].keys_manager

‎lightning/src/util/test_utils.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,8 @@ impl SignerProvider for OnlyReadsKeysInterface {
180180
))
181181
}
182182

183-
fn get_destination_script(&self) -> Script { unreachable!(); }
184-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript { unreachable!(); }
183+
fn get_destination_script(&self) -> Result<Script, ()> { Err(()) }
184+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()> { Err(()) }
185185
}
186186

187187
pub struct TestChainMonitor<'a> {
@@ -809,14 +809,14 @@ impl SignerProvider for TestKeysInterface {
809809
))
810810
}
811811

812-
fn get_destination_script(&self) -> Script { self.backing.get_destination_script() }
812+
fn get_destination_script(&self) -> Result<Script, ()> { self.backing.get_destination_script() }
813813

814-
fn get_shutdown_scriptpubkey(&self) -> ShutdownScript {
814+
fn get_shutdown_scriptpubkey(&self) -> Result<ShutdownScript, ()> {
815815
match &mut *self.expectations.lock().unwrap() {
816816
None => self.backing.get_shutdown_scriptpubkey(),
817817
Some(expectations) => match expectations.pop_front() {
818818
None => panic!("Unexpected get_shutdown_scriptpubkey"),
819-
Some(expectation) => expectation.returns,
819+
Some(expectation) => Ok(expectation.returns),
820820
},
821821
}
822822
}

0 commit comments

Comments
 (0)