Skip to content

Commit 0572957

Browse files
tnulltnull
committed
Hold PeerState in an RwLock rather than a Mutex
Previously, we would hold the `PeerState` in a `Mutex`, which disallows concurrent read-only operations. Here we switch to an `RwLock` making this possible.
1 parent b8b1ef3 commit 0572957

File tree

6 files changed

+179
-179
lines changed

6 files changed

+179
-179
lines changed

‎lightning/src/ln/channelmanager.rs

Lines changed: 151 additions & 151 deletions
Large diffs are not rendered by default.

‎lightning/src/ln/functional_test_utils.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ impl<'a, 'b, 'c> Node<'a, 'b, 'c> {
491491
log_debug!(self.logger, "Setting channel signer for {} as available={}", chan_id, available);
492492

493493
let per_peer_state = self.node.per_peer_state.read().unwrap();
494-
let chan_lock = per_peer_state.get(peer_id).unwrap().lock().unwrap();
494+
let chan_lock = per_peer_state.get(peer_id).unwrap().write().unwrap();
495495

496496
let mut channel_keys_id = None;
497497
if let Some(chan) = chan_lock.channel_by_id.get(chan_id).map(|phase| phase.context()) {
@@ -930,7 +930,7 @@ macro_rules! get_channel_ref {
930930
($node: expr, $counterparty_node: expr, $per_peer_state_lock: ident, $peer_state_lock: ident, $channel_id: expr) => {
931931
{
932932
$per_peer_state_lock = $node.node.per_peer_state.read().unwrap();
933-
$peer_state_lock = $per_peer_state_lock.get(&$counterparty_node.node.get_our_node_id()).unwrap().lock().unwrap();
933+
$peer_state_lock = $per_peer_state_lock.get(&$counterparty_node.node.get_our_node_id()).unwrap().write().unwrap();
934934
$peer_state_lock.channel_by_id.get_mut(&$channel_id).unwrap()
935935
}
936936
}
@@ -2005,8 +2005,8 @@ pub fn do_commitment_signed_dance(node_a: &Node<'_, '_, '_>, node_b: &Node<'_, '
20052005

20062006
let node_a_per_peer_state = node_a.node.per_peer_state.read().unwrap();
20072007
let mut number_of_msg_events = 0;
2008-
for (cp_id, peer_state_mutex) in node_a_per_peer_state.iter() {
2009-
let peer_state = peer_state_mutex.lock().unwrap();
2008+
for (cp_id, peer_state) in node_a_per_peer_state.iter() {
2009+
let peer_state = peer_state.write().unwrap();
20102010
let cp_pending_msg_events = &peer_state.pending_msg_events;
20112011
number_of_msg_events += cp_pending_msg_events.len();
20122012
if cp_pending_msg_events.len() == 1 {
@@ -2834,7 +2834,7 @@ pub fn pass_claimed_payment_along_route<'a, 'b, 'c, 'd>(args: ClaimAlongRouteArg
28342834
let (base_fee, prop_fee) = {
28352835
let per_peer_state = $node.node.per_peer_state.read().unwrap();
28362836
let peer_state = per_peer_state.get(&$prev_node.node.get_our_node_id())
2837-
.unwrap().lock().unwrap();
2837+
.unwrap().write().unwrap();
28382838
let channel = peer_state.channel_by_id.get(&next_msgs.as_ref().unwrap().0.channel_id).unwrap();
28392839
if let Some(prev_config) = channel.context().prev_config() {
28402840
(prev_config.forwarding_fee_base_msat as u64,
@@ -3402,7 +3402,7 @@ pub fn get_announce_close_broadcast_events<'a, 'b, 'c>(nodes: &Vec<Node<'a, 'b,
34023402
macro_rules! get_channel_value_stat {
34033403
($node: expr, $counterparty_node: expr, $channel_id: expr) => {{
34043404
let peer_state_lock = $node.node.per_peer_state.read().unwrap();
3405-
let chan_lock = peer_state_lock.get(&$counterparty_node.node.get_our_node_id()).unwrap().lock().unwrap();
3405+
let chan_lock = peer_state_lock.get(&$counterparty_node.node.get_our_node_id()).unwrap().write().unwrap();
34063406
let chan = chan_lock.channel_by_id.get(&$channel_id).map(
34073407
|phase| if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
34083408
).flatten().unwrap();

‎lightning/src/ln/functional_tests.rs

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -702,7 +702,7 @@ fn test_update_fee_that_funder_cannot_afford() {
702702
// needed to sign the new commitment tx and (2) sign the new commitment tx.
703703
let (local_revocation_basepoint, local_htlc_basepoint, local_funding) = {
704704
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
705-
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
705+
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
706706
let local_chan = chan_lock.channel_by_id.get(&chan.2).map(
707707
|phase| if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
708708
).flatten().unwrap();
@@ -713,7 +713,7 @@ fn test_update_fee_that_funder_cannot_afford() {
713713
};
714714
let (remote_delayed_payment_basepoint, remote_htlc_basepoint,remote_point, remote_funding) = {
715715
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
716-
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
716+
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().write().unwrap();
717717
let remote_chan = chan_lock.channel_by_id.get(&chan.2).map(
718718
|phase| if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
719719
).flatten().unwrap();
@@ -730,7 +730,7 @@ fn test_update_fee_that_funder_cannot_afford() {
730730

731731
let res = {
732732
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
733-
let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
733+
let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
734734
let local_chan = local_chan_lock.channel_by_id.get(&chan.2).map(
735735
|phase| if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
736736
).flatten().unwrap();
@@ -1432,7 +1432,7 @@ fn test_fee_spike_violation_fails_htlc() {
14321432
// needed to sign the new commitment tx and (2) sign the new commitment tx.
14331433
let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point, local_funding) = {
14341434
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
1435-
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
1435+
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
14361436
let local_chan = chan_lock.channel_by_id.get(&chan.2).map(
14371437
|phase| if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
14381438
).flatten().unwrap();
@@ -1448,7 +1448,7 @@ fn test_fee_spike_violation_fails_htlc() {
14481448
};
14491449
let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_point, remote_funding) = {
14501450
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
1451-
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
1451+
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().write().unwrap();
14521452
let remote_chan = chan_lock.channel_by_id.get(&chan.2).map(
14531453
|phase| if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
14541454
).flatten().unwrap();
@@ -1479,7 +1479,7 @@ fn test_fee_spike_violation_fails_htlc() {
14791479

14801480
let res = {
14811481
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
1482-
let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
1482+
let local_chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
14831483
let local_chan = local_chan_lock.channel_by_id.get(&chan.2).map(
14841484
|phase| if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
14851485
).flatten().unwrap();
@@ -3239,7 +3239,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
32393239
// The dust limit applied to HTLC outputs considers the fee of the HTLC transaction as
32403240
// well, so HTLCs at exactly the dust limit will not be included in commitment txn.
32413241
nodes[2].node.per_peer_state.read().unwrap().get(&nodes[1].node.get_our_node_id())
3242-
.unwrap().lock().unwrap().channel_by_id.get(&chan_2.2).unwrap().context().holder_dust_limit_satoshis * 1000
3242+
.unwrap().write().unwrap().channel_by_id.get(&chan_2.2).unwrap().context().holder_dust_limit_satoshis * 1000
32433243
} else { 3000000 };
32443244

32453245
let (_, first_payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value);
@@ -5212,7 +5212,7 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
52125212
assert_eq!(get_local_commitment_txn!(nodes[3], chan_2_3.2)[0].output.len(), 2);
52135213

52145214
let ds_dust_limit = nodes[3].node.per_peer_state.read().unwrap().get(&nodes[2].node.get_our_node_id())
5215-
.unwrap().lock().unwrap().channel_by_id.get(&chan_2_3.2).unwrap().context().holder_dust_limit_satoshis;
5215+
.unwrap().write().unwrap().channel_by_id.get(&chan_2_3.2).unwrap().context().holder_dust_limit_satoshis;
52165216
// 0th HTLC:
52175217
let (_, payment_hash_1, ..) = route_payment(&nodes[0], &[&nodes[2], &nodes[3], &nodes[4]], ds_dust_limit*1000); // not added < dust limit + HTLC tx fee
52185218
// 1st HTLC:
@@ -6347,7 +6347,7 @@ fn test_update_add_htlc_bolt2_sender_exceed_max_htlc_num_and_htlc_id_increment()
63476347
let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
63486348
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 0);
63496349
let max_accepted_htlcs = nodes[1].node.per_peer_state.read().unwrap().get(&nodes[0].node.get_our_node_id())
6350-
.unwrap().lock().unwrap().channel_by_id.get(&chan.2).unwrap().context().counterparty_max_accepted_htlcs as u64;
6350+
.unwrap().write().unwrap().channel_by_id.get(&chan.2).unwrap().context().counterparty_max_accepted_htlcs as u64;
63516351

63526352
// Fetch a route in advance as we will be unable to once we're unable to send.
63536353
let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[1], 100000);
@@ -6418,7 +6418,7 @@ fn test_update_add_htlc_bolt2_receiver_check_amount_received_more_than_min() {
64186418
let htlc_minimum_msat: u64;
64196419
{
64206420
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
6421-
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
6421+
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
64226422
let channel = chan_lock.channel_by_id.get(&chan.2).unwrap();
64236423
htlc_minimum_msat = channel.context().get_holder_htlc_minimum_msat();
64246424
}
@@ -7024,7 +7024,7 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) {
70247024
let chan =create_announced_chan_between_nodes(&nodes, 0, 1);
70257025

70267026
let bs_dust_limit = nodes[1].node.per_peer_state.read().unwrap().get(&nodes[0].node.get_our_node_id())
7027-
.unwrap().lock().unwrap().channel_by_id.get(&chan.2).unwrap().context().holder_dust_limit_satoshis;
7027+
.unwrap().write().unwrap().channel_by_id.get(&chan.2).unwrap().context().holder_dust_limit_satoshis;
70287028

70297029
// We route 2 dust-HTLCs between A and B
70307030
let (_, payment_hash_1, ..) = route_payment(&nodes[0], &[&nodes[1]], bs_dust_limit*1000);
@@ -7117,7 +7117,7 @@ fn do_test_sweep_outbound_htlc_failure_update(revoked: bool, local: bool) {
71177117
let chan = create_announced_chan_between_nodes(&nodes, 0, 1);
71187118

71197119
let bs_dust_limit = nodes[1].node.per_peer_state.read().unwrap().get(&nodes[0].node.get_our_node_id())
7120-
.unwrap().lock().unwrap().channel_by_id.get(&chan.2).unwrap().context().holder_dust_limit_satoshis;
7120+
.unwrap().write().unwrap().channel_by_id.get(&chan.2).unwrap().context().holder_dust_limit_satoshis;
71217121

71227122
let (_payment_preimage_1, dust_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], bs_dust_limit*1000);
71237123
let (_payment_preimage_2, non_dust_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
@@ -7796,7 +7796,7 @@ fn test_counterparty_raa_skip_no_crash() {
77967796
let next_per_commitment_point;
77977797
{
77987798
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
7799-
let mut guard = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
7799+
let mut guard = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
78007800
let keys = guard.channel_by_id.get_mut(&channel_id).map(
78017801
|phase| if let ChannelPhase::Funded(chan) = phase { Some(chan) } else { None }
78027802
).flatten().unwrap().get_signer();
@@ -9227,7 +9227,7 @@ fn test_duplicate_chan_id() {
92279227

92289228
let funding_created = {
92299229
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
9230-
let mut a_peer_state = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
9230+
let mut a_peer_state = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
92319231
// Once we call `get_funding_created` the channel has a duplicate channel_id as
92329232
// another channel in the ChannelManager - an invalid state. Thus, we'd panic later when we
92339233
// try to create another channel. Instead, we drop the channel entirely here (leaving the
@@ -9942,7 +9942,7 @@ fn do_test_max_dust_htlc_exposure(dust_outbound_balance: bool, exposure_breach_e
99429942

99439943
let (dust_buffer_feerate, max_dust_htlc_exposure_msat) = {
99449944
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
9945-
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
9945+
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
99469946
let chan = chan_lock.channel_by_id.get(&channel_id).unwrap();
99479947
(chan.context().get_dust_buffer_feerate(None) as u64,
99489948
chan.context().get_max_dust_htlc_exposure_msat(&LowerBoundedFeeEstimator(nodes[0].fee_estimator)))
@@ -10440,7 +10440,7 @@ fn test_remove_expired_outbound_unfunded_channels() {
1044010440
// Asserts the outbound channel has been removed from a nodes[0]'s peer state map.
1044110441
let check_outbound_channel_existence = |should_exist: bool| {
1044210442
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
10443-
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
10443+
let chan_lock = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
1044410444
assert_eq!(chan_lock.channel_by_id.contains_key(&temp_channel_id), should_exist);
1044510445
};
1044610446

@@ -10491,7 +10491,7 @@ fn test_remove_expired_inbound_unfunded_channels() {
1049110491
// Asserts the inbound channel has been removed from a nodes[1]'s peer state map.
1049210492
let check_inbound_channel_existence = |should_exist: bool| {
1049310493
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
10494-
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().lock().unwrap();
10494+
let chan_lock = per_peer_state.get(&nodes[0].node.get_our_node_id()).unwrap().write().unwrap();
1049510495
assert_eq!(chan_lock.channel_by_id.contains_key(&temp_channel_id), should_exist);
1049610496
};
1049710497

‎lightning/src/ln/onion_route_tests.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,7 @@ fn test_onion_failure() {
515515

516516
let short_channel_id = channels[1].0.contents.short_channel_id;
517517
let amt_to_forward = nodes[1].node.per_peer_state.read().unwrap().get(&nodes[2].node.get_our_node_id())
518-
.unwrap().lock().unwrap().channel_by_id.get(&channels[1].2).unwrap()
518+
.unwrap().write().unwrap().channel_by_id.get(&channels[1].2).unwrap()
519519
.context().get_counterparty_htlc_minimum_msat() - 1;
520520
let mut bogus_route = route.clone();
521521
let route_len = bogus_route.paths[0].hops.len();

‎lightning/src/ln/payment_tests.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -796,7 +796,7 @@ fn do_retry_with_no_persist(confirm_before_reload: bool) {
796796
{
797797
let per_peer_state = nodes[1].node.per_peer_state.read().unwrap();
798798
let mut peer_state = per_peer_state.get(&nodes[2].node.get_our_node_id())
799-
.unwrap().lock().unwrap();
799+
.unwrap().write().unwrap();
800800
let mut channel = peer_state.channel_by_id.get_mut(&chan_id_2).unwrap();
801801
let mut new_config = channel.context().config();
802802
new_config.forwarding_fee_base_msat += 100_000;

‎lightning/src/ln/reorg_tests.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
258258

259259
{
260260
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
261-
let peer_state = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
261+
let peer_state = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
262262
assert_eq!(peer_state.channel_by_id.len(), 1);
263263
assert_eq!(nodes[0].node.short_to_chan_info.read().unwrap().len(), 2);
264264
}
@@ -294,7 +294,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
294294

295295
{
296296
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
297-
let peer_state = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
297+
let peer_state = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
298298
assert_eq!(peer_state.channel_by_id.len(), 0);
299299
assert_eq!(nodes[0].node.short_to_chan_info.read().unwrap().len(), 0);
300300
}
@@ -340,7 +340,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
340340

341341
{
342342
let per_peer_state = nodes[0].node.per_peer_state.read().unwrap();
343-
let peer_state = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().lock().unwrap();
343+
let peer_state = per_peer_state.get(&nodes[1].node.get_our_node_id()).unwrap().write().unwrap();
344344
assert_eq!(peer_state.channel_by_id.len(), 0);
345345
assert_eq!(nodes[0].node.short_to_chan_info.read().unwrap().len(), 0);
346346
}

0 commit comments

Comments
 (0)