Skip to content

Commit 6ce3713

Browse files
wvanlintwvanlint
committed
Review comments
1 parent e67e8b8 commit 6ce3713

File tree

3 files changed

+250
-57
lines changed

3 files changed

+250
-57
lines changed

‎lightning/src/ln/channel.rs

+164-6
Original file line numberDiff line numberDiff line change
@@ -1944,16 +1944,28 @@ impl<Signer: ChannelSigner> ChannelContext<Signer> {
19441944
res
19451945
}
19461946

1947-
/// Returns transaction if there is pending funding transaction that is yet to broadcast
1948-
pub fn unbroadcasted_funding(&self) -> Option<Transaction> {
1947+
fn map_unbroadcasted_funding<F, R>(&self, f: F) -> Option<R>
1948+
where F: Fn(&Transaction) -> R {
19491949
if self.channel_state & ChannelState::FundingCreated as u32 != 0 ||
19501950
self.channel_state & ChannelState::WaitingForBatch as u32 != 0 {
1951-
self.funding_transaction.clone()
1951+
self.funding_transaction.as_ref().map(f)
19521952
} else {
19531953
None
19541954
}
19551955
}
19561956

1957+
/// Returns the transaction if there is a pending funding transaction that is yet to be
1958+
/// broadcast.
1959+
pub fn unbroadcasted_funding(&self) -> Option<Transaction> {
1960+
self.map_unbroadcasted_funding(|tx| tx.clone())
1961+
}
1962+
1963+
/// Returns the transaction ID if there is a pending funding transaction that is yet to be
1964+
/// broadcast.
1965+
pub fn unbroadcasted_funding_txid(&self) -> Option<Txid> {
1966+
self.map_unbroadcasted_funding(|tx| tx.txid())
1967+
}
1968+
19571969
/// Gets the latest commitment transaction and any dependent transactions for relay (forcing
19581970
/// shutdown of this channel - no more calls into this Channel may be made afterwards except
19591971
/// those explicitly stated to be allowed after shutdown completes, eg some simple getters).
@@ -2613,7 +2625,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
26132625

26142626
let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
26152627

2616-
if non_shutdown_state == ChannelState::FundingSent as u32 {
2628+
if non_shutdown_state & !(ChannelState::WaitingForBatch as u32) == ChannelState::FundingSent as u32 {
26172629
self.context.channel_state |= ChannelState::TheirChannelReady as u32;
26182630
} else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurChannelReady as u32) {
26192631
self.context.channel_state = ChannelState::ChannelReady as u32 | (self.context.channel_state & MULTI_STATE_FLAGS);
@@ -4565,7 +4577,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
45654577
pub fn is_awaiting_initial_mon_persist(&self) -> bool {
45664578
if !self.is_awaiting_monitor_update() { return false; }
45674579
if self.context.channel_state &
4568-
!(ChannelState::TheirChannelReady as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32)
4580+
!(ChannelState::TheirChannelReady as u32 | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32 | ChannelState::WaitingForBatch as u32)
45694581
== ChannelState::FundingSent as u32 {
45704582
// If we're not a 0conf channel, we'll be waiting on a monitor update with only
45714583
// FundingSent set, though our peer could have sent their channel_ready.
@@ -4645,6 +4657,8 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
46454657
return None;
46464658
}
46474659

4660+
// Note that we don't include ChannelState::WaitingForBatch as we don't want to send
4661+
// channel_ready until the entire batch is ready.
46484662
let non_shutdown_state = self.context.channel_state & (!MULTI_STATE_FLAGS);
46494663
let need_commitment_update = if non_shutdown_state == ChannelState::FundingSent as u32 {
46504664
self.context.channel_state |= ChannelState::OurChannelReady as u32;
@@ -7477,7 +7491,7 @@ mod tests {
74777491
use crate::ln::PaymentHash;
74787492
use crate::ln::channelmanager::{self, HTLCSource, PaymentId};
74797493
use crate::ln::channel::InitFeatures;
7480-
use crate::ln::channel::{Channel, InboundHTLCOutput, OutboundV1Channel, InboundV1Channel, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator, commit_tx_fee_msat};
7494+
use crate::ln::channel::{Channel, ChannelState, InboundHTLCOutput, OutboundV1Channel, InboundV1Channel, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator, commit_tx_fee_msat};
74817495
use crate::ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS, MIN_THEIR_CHAN_RESERVE_SATOSHIS};
74827496
use crate::ln::features::ChannelTypeFeatures;
74837497
use crate::ln::msgs::{ChannelUpdate, DecodeError, UnsignedChannelUpdate, MAX_VALUE_MSAT};
@@ -8952,4 +8966,148 @@ mod tests {
89528966
);
89538967
assert!(res.is_err());
89548968
}
8969+
8970+
#[test]
8971+
fn test_waiting_for_batch() {
8972+
let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
8973+
let logger = test_utils::TestLogger::new();
8974+
let secp_ctx = Secp256k1::new();
8975+
let seed = [42; 32];
8976+
let network = Network::Testnet;
8977+
let best_block = BestBlock::from_network(network);
8978+
let chain_hash = genesis_block(network).header.block_hash();
8979+
let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
8980+
8981+
let mut config = UserConfig::default();
8982+
// Set trust_own_funding_0conf while ensuring we don't send channel_ready for a
8983+
// channel in a batch before all channels are ready.
8984+
config.channel_handshake_limits.trust_own_funding_0conf = true;
8985+
8986+
// Create a channel from node a to node b that will be part of batch funding.
8987+
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
8988+
let mut node_a_chan = OutboundV1Channel::<EnforcingSigner>::new(
8989+
&feeest,
8990+
&&keys_provider,
8991+
&&keys_provider,
8992+
node_b_node_id,
8993+
&channelmanager::provided_init_features(&config),
8994+
10000000,
8995+
100000,
8996+
42,
8997+
&config,
8998+
0,
8999+
42,
9000+
).unwrap();
9001+
9002+
let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
9003+
let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
9004+
let mut node_b_chan = InboundV1Channel::<EnforcingSigner>::new(
9005+
&feeest,
9006+
&&keys_provider,
9007+
&&keys_provider,
9008+
node_b_node_id,
9009+
&channelmanager::provided_channel_type_features(&config),
9010+
&channelmanager::provided_init_features(&config),
9011+
&open_channel_msg,
9012+
7,
9013+
&config,
9014+
0,
9015+
&&logger,
9016+
42,
9017+
).unwrap();
9018+
9019+
// Allow node b to send a 0conf channel_ready.
9020+
node_b_chan.set_0conf();
9021+
9022+
let accept_channel_msg = node_b_chan.accept_inbound_channel(0);
9023+
node_a_chan.accept_channel(
9024+
&accept_channel_msg,
9025+
&config.channel_handshake_limits,
9026+
&channelmanager::provided_init_features(&config),
9027+
).unwrap();
9028+
9029+
// Fund the channel with a batch funding transaction.
9030+
let output_script = node_a_chan.context.get_funding_redeemscript();
9031+
let tx = Transaction {
9032+
version: 1,
9033+
lock_time: PackedLockTime::ZERO,
9034+
input: Vec::new(),
9035+
output: vec![
9036+
TxOut {
9037+
value: 10000000, script_pubkey: output_script.clone(),
9038+
},
9039+
TxOut {
9040+
value: 10000000, script_pubkey: Builder::new().into_script(),
9041+
},
9042+
]};
9043+
let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
9044+
let (mut node_a_chan, funding_created_msg) = node_a_chan.get_funding_created(
9045+
tx.clone(),
9046+
funding_outpoint,
9047+
&&logger,
9048+
).map_err(|_| ()).unwrap();
9049+
let (mut node_b_chan, funding_signed_msg, _) = node_b_chan.funding_created(
9050+
&funding_created_msg,
9051+
best_block,
9052+
&&keys_provider,
9053+
&&logger,
9054+
).map_err(|_| ()).unwrap();
9055+
let node_b_updates = node_b_chan.monitor_updating_restored(
9056+
&&logger,
9057+
&&keys_provider,
9058+
chain_hash,
9059+
&config,
9060+
0,
9061+
);
9062+
9063+
// Receive funding_signed, but the channel will be configured to hold sending channel_ready and
9064+
// broadcasting the funding transaction until the batch is ready.
9065+
let _ = node_a_chan.funding_signed(
9066+
&funding_signed_msg,
9067+
best_block,
9068+
&&keys_provider,
9069+
true,
9070+
&&logger,
9071+
).unwrap();
9072+
let node_a_updates = node_a_chan.monitor_updating_restored(
9073+
&&logger,
9074+
&&keys_provider,
9075+
chain_hash,
9076+
&config,
9077+
0,
9078+
);
9079+
// Our channel_ready shouldn't be sent yet, even with trust_own_funding_0conf set,
9080+
// as the funding transaction depends on all channels in the batch becoming ready.
9081+
assert!(node_a_updates.channel_ready.is_none());
9082+
assert!(node_a_updates.funding_broadcastable.is_none());
9083+
assert_eq!(
9084+
node_a_chan.context.channel_state,
9085+
ChannelState::FundingSent as u32 |
9086+
ChannelState::WaitingForBatch as u32,
9087+
);
9088+
9089+
// It is possible to receive a 0conf channel_ready from the remote node.
9090+
node_a_chan.channel_ready(
9091+
&node_b_updates.channel_ready.unwrap(),
9092+
&&keys_provider,
9093+
chain_hash,
9094+
&config,
9095+
&best_block,
9096+
&&logger,
9097+
).unwrap();
9098+
assert_eq!(
9099+
node_a_chan.context.channel_state,
9100+
ChannelState::FundingSent as u32 |
9101+
ChannelState::WaitingForBatch as u32 |
9102+
ChannelState::TheirChannelReady as u32,
9103+
);
9104+
9105+
// Clear the ChannelState::WaitingForBatch only when called by ChannelManager.
9106+
node_a_chan.set_batch_ready();
9107+
assert_eq!(
9108+
node_a_chan.context.channel_state,
9109+
ChannelState::FundingSent as u32 |
9110+
ChannelState::TheirChannelReady as u32,
9111+
);
9112+
}
89559113
}

‎lightning/src/ln/channelmanager.rs

+45-48
Original file line numberDiff line numberDiff line change
@@ -1766,9 +1766,9 @@ macro_rules! update_maps_on_chan_removal {
17661766
// If the channel was part of a batch funding transaction, all channels in that
17671767
// batch are affected.
17681768
let v1_funding_batch_states = $self.v1_funding_batch_states.read().unwrap();
1769-
$channel_context.unbroadcasted_funding().map(|tx| {
1770-
if v1_funding_batch_states.contains_key(&tx.txid()) {
1771-
$self.v1_funding_batches_to_be_closed.lock().unwrap().push(tx.txid());
1769+
$channel_context.unbroadcasted_funding_txid().map(|txid| {
1770+
if v1_funding_batch_states.contains_key(&txid) {
1771+
$self.v1_funding_batches_to_be_closed.lock().unwrap().push(txid);
17721772
}
17731773
})
17741774
}}
@@ -1920,8 +1920,8 @@ macro_rules! handle_monitor_update_completion {
19201920
let mut completed_batch = None;
19211921
{
19221922
let v1_funding_batch_states = $self.v1_funding_batch_states.read().unwrap();
1923-
let batch_state_key_value = $chan.context.unbroadcasted_funding()
1924-
.and_then(|tx| v1_funding_batch_states.get_key_value(&tx.txid()));
1923+
let batch_state_key_value = $chan.context.unbroadcasted_funding_txid()
1924+
.and_then(|txid| v1_funding_batch_states.get_key_value(&txid));
19251925
if let Some((txid, batch_state)) = batch_state_key_value {
19261926
let mut batch_state = batch_state.lock().unwrap();
19271927
batch_state.insert(
@@ -3575,42 +3575,41 @@ where
35753575
/// If there is an error, all channels in the batch are to be considered closed.
35763576
pub fn batch_funding_transaction_generated(&self, temporary_channels: &[(&[u8; 32], &PublicKey)], funding_transaction: Transaction) -> Result<(), APIError> {
35773577
let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self);
3578+
let mut result = Ok(());
35783579

35793580
for inp in funding_transaction.input.iter() {
35803581
if inp.witness.is_empty() {
3581-
return Err(APIError::APIMisuseError {
3582+
result = result.and(Err(APIError::APIMisuseError {
35823583
err: "Funding transaction must be fully signed and spend Segwit outputs".to_owned()
3583-
});
3584+
}));
35843585
}
35853586
}
3587+
if funding_transaction.output.len() > u16::max_value() as usize {
3588+
result = result.and(Err(APIError::APIMisuseError {
3589+
err: "Transaction had more than 2^16 outputs, which is not supported".to_owned()
3590+
}));
3591+
}
35863592
{
35873593
let height = self.best_block.read().unwrap().height();
35883594
// Transactions are evaluated as final by network mempools if their locktime is strictly
35893595
// lower than the next block height. However, the modules constituting our Lightning
35903596
// node might not have perfect sync about their blockchain views. Thus, if the wallet
35913597
// module is ahead of LDK, only allow one more block of headroom.
35923598
if !funding_transaction.input.iter().all(|input| input.sequence == Sequence::MAX) && LockTime::from(funding_transaction.lock_time).is_block_height() && funding_transaction.lock_time.0 > height + 1 {
3593-
return Err(APIError::APIMisuseError {
3599+
result = result.and(Err(APIError::APIMisuseError {
35943600
err: "Funding transaction absolute timelock is non-final".to_owned()
3595-
});
3601+
}));
35963602
}
35973603
}
35983604

35993605
let is_batch_funding = temporary_channels.len() > 1;
36003606
let v1_funding_batch_state = RefCell::new(HashMap::new());
3601-
let mut result = Ok(());
36023607
for (temporary_channel_id, counterparty_node_id) in temporary_channels {
36033608
result = result.and_then(|_| self.funding_transaction_generated_intern(
36043609
temporary_channel_id,
36053610
counterparty_node_id,
36063611
funding_transaction.clone(),
36073612
|chan, tx| {
3608-
if tx.output.len() > u16::max_value() as usize {
3609-
return Err(APIError::APIMisuseError {
3610-
err: "Transaction had more than 2^16 outputs, which is not supported".to_owned()
3611-
});
3612-
}
3613-
36143613
let mut output_index = None;
36153614
let expected_spk = chan.context.get_funding_redeemscript().to_v0_p2wsh();
36163615
for (idx, outp) in tx.output.iter().enumerate() {
@@ -3634,36 +3633,34 @@ where
36343633
})
36353634
);
36363635
}
3637-
if is_batch_funding {
3638-
if let Err(ref e) = result {
3639-
// Remaining channels in the batch need to be removed on any error.
3640-
let e = format!("Error in batch transaction funding: {:?}", e);
3641-
let per_peer_state = self.per_peer_state.read().unwrap();
3642-
for (temporary_channel_id, counterparty_node_id) in temporary_channels {
3643-
per_peer_state.get(counterparty_node_id)
3644-
.map(|peer_state_mutex| peer_state_mutex.lock().unwrap())
3645-
.and_then(|mut peer_state| peer_state.outbound_v1_channel_by_id.remove(*temporary_channel_id))
3646-
.map(|chan| {
3647-
update_maps_on_chan_removal!(self, &chan.context);
3648-
self.issue_channel_close_events(&chan.context, ClosureReason::ProcessingError { err: e.clone() });
3649-
});
3650-
}
3651-
for (channel_id, counterparty_node_id) in v1_funding_batch_state.borrow().keys() {
3652-
per_peer_state.get(counterparty_node_id)
3653-
.map(|peer_state_mutex| peer_state_mutex.lock().unwrap())
3654-
.and_then(|mut peer_state| peer_state.channel_by_id.remove(channel_id))
3655-
.map(|chan| {
3656-
update_maps_on_chan_removal!(self, &chan.context);
3657-
self.issue_channel_close_events(&chan.context, ClosureReason::ProcessingError { err: e.clone() });
3658-
});
3659-
}
3660-
} else {
3661-
// Initialize the state of the batch.
3662-
self.v1_funding_batch_states.write().unwrap().insert(
3663-
funding_transaction.txid(),
3664-
Mutex::new(v1_funding_batch_state.into_inner()),
3665-
);
3636+
if let Err(ref e) = result {
3637+
// Remaining channels need to be removed on any error.
3638+
let e = format!("Error in transaction funding: {:?}", e);
3639+
let per_peer_state = self.per_peer_state.read().unwrap();
3640+
for (temporary_channel_id, counterparty_node_id) in temporary_channels {
3641+
per_peer_state.get(counterparty_node_id)
3642+
.map(|peer_state_mutex| peer_state_mutex.lock().unwrap())
3643+
.and_then(|mut peer_state| peer_state.outbound_v1_channel_by_id.remove(*temporary_channel_id))
3644+
.map(|chan| {
3645+
update_maps_on_chan_removal!(self, &chan.context);
3646+
self.issue_channel_close_events(&chan.context, ClosureReason::ProcessingError { err: e.clone() });
3647+
});
3648+
}
3649+
for (channel_id, counterparty_node_id) in v1_funding_batch_state.borrow().keys() {
3650+
per_peer_state.get(counterparty_node_id)
3651+
.map(|peer_state_mutex| peer_state_mutex.lock().unwrap())
3652+
.and_then(|mut peer_state| peer_state.channel_by_id.remove(channel_id))
3653+
.map(|chan| {
3654+
update_maps_on_chan_removal!(self, &chan.context);
3655+
self.issue_channel_close_events(&chan.context, ClosureReason::ProcessingError { err: e.clone() });
3656+
});
36663657
}
3658+
} else if is_batch_funding {
3659+
// Initialize the state of the batch.
3660+
self.v1_funding_batch_states.write().unwrap().insert(
3661+
funding_transaction.txid(),
3662+
Mutex::new(v1_funding_batch_state.into_inner()),
3663+
);
36673664
}
36683665
result
36693666
}
@@ -5718,8 +5715,8 @@ where
57185715
let peer_state = &mut *peer_state_lock;
57195716
match peer_state.channel_by_id.entry(msg.channel_id) {
57205717
hash_map::Entry::Occupied(mut chan) => {
5721-
let is_batch_funding = chan.get().context.unbroadcasted_funding()
5722-
.map(|tx| self.v1_funding_batch_states.read().unwrap().contains_key(&tx.txid()))
5718+
let is_batch_funding = chan.get().context.unbroadcasted_funding_txid()
5719+
.map(|txid| self.v1_funding_batch_states.read().unwrap().contains_key(&txid))
57235720
.unwrap_or(false);
57245721
let monitor = try_chan_entry!(self,
57255722
chan.get_mut().funding_signed(&msg, best_block, &self.signer_provider, is_batch_funding, &self.logger), chan);
@@ -7408,7 +7405,7 @@ where
74087405
let peer_state = &mut *peer_state_lock;
74097406

74107407
peer_state.channel_by_id.retain(|_, chan| {
7411-
if chan.context.unbroadcasted_funding().is_some() {
7408+
if !chan.context.is_funding_initiated() {
74127409
// It is possible to have persisted the monitor upon funding_signed
74137410
// but not have broadcast the transaction, especially for batch funding.
74147411
// The monitor should be moved to the correct state.

0 commit comments

Comments
 (0)