Skip to content

Commit fa6f891

Browse files
author
Antoine Riard
committed
Drop Result for ChannelMessageHandler methods
Simplify interfaces between ChannelMessageHandler and PeerManager, by switching all ChannelMessageHandler errors to HandleError sent internally instead of being return. With further refactors in Router and PeerChannelEncryptor, errors management on the PeerManager-side won't be splitted between try_potential_handleerror and HandleError processing. Inside ChannelManager, we now log MsgHandleErrInternal and send ErrorAction to PeerManager. On a high-level, it should allow client using API to be more flexible by polling events instead of waiting function call returns. We also update handle_error macro to take channel_state_lock from caller which should avoid some deadlock potential for some edges cases.
1 parent 62d69e0 commit fa6f891

8 files changed

+1012
-852
lines changed

‎fuzz/src/chanmon_consistency.rs

+30-24
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ use lightning::ln::channelmonitor;
2929
use lightning::ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, HTLCUpdate};
3030
use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, ChannelManagerReadArgs};
3131
use lightning::ln::router::{Route, RouteHop};
32-
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, ErrorAction, LightningError, UpdateAddHTLC, LocalFeatures};
32+
use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, UpdateAddHTLC, LocalFeatures, ErrorAction};
3333
use lightning::util::enforcing_trait_impls::EnforcingChannelKeys;
3434
use lightning::util::events;
3535
use lightning::util::logger::Logger;
@@ -252,7 +252,7 @@ pub fn do_test(data: &[u8]) {
252252
} else { panic!("Wrong event type"); }
253253
};
254254

255-
$dest.handle_open_channel(&$source.get_our_node_id(), LocalFeatures::new(), &open_channel).unwrap();
255+
$dest.handle_open_channel(&$source.get_our_node_id(), LocalFeatures::new(), &open_channel);
256256
let accept_channel = {
257257
let events = $dest.get_and_clear_pending_msg_events();
258258
assert_eq!(events.len(), 1);
@@ -261,7 +261,7 @@ pub fn do_test(data: &[u8]) {
261261
} else { panic!("Wrong event type"); }
262262
};
263263

264-
$source.handle_accept_channel(&$dest.get_our_node_id(), LocalFeatures::new(), &accept_channel).unwrap();
264+
$source.handle_accept_channel(&$dest.get_our_node_id(), LocalFeatures::new(), &accept_channel);
265265
{
266266
let events = $source.get_and_clear_pending_events();
267267
assert_eq!(events.len(), 1);
@@ -282,7 +282,7 @@ pub fn do_test(data: &[u8]) {
282282
msg.clone()
283283
} else { panic!("Wrong event type"); }
284284
};
285-
$dest.handle_funding_created(&$source.get_our_node_id(), &funding_created).unwrap();
285+
$dest.handle_funding_created(&$source.get_our_node_id(), &funding_created);
286286

287287
let funding_signed = {
288288
let events = $dest.get_and_clear_pending_msg_events();
@@ -291,7 +291,7 @@ pub fn do_test(data: &[u8]) {
291291
msg.clone()
292292
} else { panic!("Wrong event type"); }
293293
};
294-
$source.handle_funding_signed(&$dest.get_our_node_id(), &funding_signed).unwrap();
294+
$source.handle_funding_signed(&$dest.get_our_node_id(), &funding_signed);
295295

296296
{
297297
let events = $source.get_and_clear_pending_events();
@@ -330,7 +330,7 @@ pub fn do_test(data: &[u8]) {
330330
if let events::MessageSendEvent::SendFundingLocked { ref node_id, ref msg } = event {
331331
for node in $nodes.iter() {
332332
if node.get_our_node_id() == *node_id {
333-
node.handle_funding_locked(&$nodes[idx].get_our_node_id(), msg).unwrap();
333+
node.handle_funding_locked(&$nodes[idx].get_our_node_id(), msg);
334334
}
335335
}
336336
} else { panic!("Wrong event type"); }
@@ -381,16 +381,6 @@ pub fn do_test(data: &[u8]) {
381381
let mut node_c_ser = VecWriter(Vec::new());
382382
nodes[2].write(&mut node_c_ser).unwrap();
383383

384-
macro_rules! test_err {
385-
($res: expr) => {
386-
match $res {
387-
Ok(()) => {},
388-
Err(LightningError { action: ErrorAction::IgnoreError, .. }) => { },
389-
_ => { $res.unwrap() },
390-
}
391-
}
392-
}
393-
394384
macro_rules! test_return {
395385
() => { {
396386
assert_eq!(nodes[0].list_channels().len(), 1);
@@ -470,7 +460,7 @@ pub fn do_test(data: &[u8]) {
470460
assert!(update_fee.is_none());
471461
for update_add in update_add_htlcs {
472462
if !$corrupt_forward {
473-
test_err!(dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), &update_add));
463+
dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), &update_add);
474464
} else {
475465
// Corrupt the update_add_htlc message so that its HMAC
476466
// check will fail and we generate a
@@ -479,33 +469,33 @@ pub fn do_test(data: &[u8]) {
479469
let mut msg_ser = update_add.encode();
480470
msg_ser[1000] ^= 0xff;
481471
let new_msg = UpdateAddHTLC::read(&mut Cursor::new(&msg_ser)).unwrap();
482-
test_err!(dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), &new_msg));
472+
dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), &new_msg);
483473
}
484474
}
485475
for update_fulfill in update_fulfill_htlcs {
486-
test_err!(dest.handle_update_fulfill_htlc(&nodes[$node].get_our_node_id(), &update_fulfill));
476+
dest.handle_update_fulfill_htlc(&nodes[$node].get_our_node_id(), &update_fulfill);
487477
}
488478
for update_fail in update_fail_htlcs {
489-
test_err!(dest.handle_update_fail_htlc(&nodes[$node].get_our_node_id(), &update_fail));
479+
dest.handle_update_fail_htlc(&nodes[$node].get_our_node_id(), &update_fail);
490480
}
491481
for update_fail_malformed in update_fail_malformed_htlcs {
492-
test_err!(dest.handle_update_fail_malformed_htlc(&nodes[$node].get_our_node_id(), &update_fail_malformed));
482+
dest.handle_update_fail_malformed_htlc(&nodes[$node].get_our_node_id(), &update_fail_malformed);
493483
}
494-
test_err!(dest.handle_commitment_signed(&nodes[$node].get_our_node_id(), &commitment_signed));
484+
dest.handle_commitment_signed(&nodes[$node].get_our_node_id(), &commitment_signed);
495485
}
496486
}
497487
},
498488
events::MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
499489
for dest in nodes.iter() {
500490
if dest.get_our_node_id() == *node_id {
501-
test_err!(dest.handle_revoke_and_ack(&nodes[$node].get_our_node_id(), msg));
491+
dest.handle_revoke_and_ack(&nodes[$node].get_our_node_id(), msg);
502492
}
503493
}
504494
},
505495
events::MessageSendEvent::SendChannelReestablish { ref node_id, ref msg } => {
506496
for dest in nodes.iter() {
507497
if dest.get_our_node_id() == *node_id {
508-
test_err!(dest.handle_channel_reestablish(&nodes[$node].get_our_node_id(), msg));
498+
dest.handle_channel_reestablish(&nodes[$node].get_our_node_id(), msg);
509499
}
510500
}
511501
},
@@ -516,6 +506,14 @@ pub fn do_test(data: &[u8]) {
516506
// Can be generated due to a payment forward being rejected due to a
517507
// channel having previously failed a monitor update
518508
},
509+
events::MessageSendEvent::HandleError { action, .. } => {
510+
// Can be generated at any processing step to send back an error, disconnect
511+
// peer or just ignore
512+
match action {
513+
ErrorAction::IgnoreError => {},
514+
_ => panic!("Expected IgnoreError")
515+
}
516+
},
519517
_ => panic!("Unhandled message event"),
520518
}
521519
}
@@ -532,6 +530,7 @@ pub fn do_test(data: &[u8]) {
532530
events::MessageSendEvent::SendChannelReestablish { .. } => {},
533531
events::MessageSendEvent::SendFundingLocked { .. } => {},
534532
events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {},
533+
events::MessageSendEvent::HandleError { .. } => {},
535534
_ => panic!("Unhandled message event"),
536535
}
537536
}
@@ -544,6 +543,12 @@ pub fn do_test(data: &[u8]) {
544543
events::MessageSendEvent::SendChannelReestablish { .. } => {},
545544
events::MessageSendEvent::SendFundingLocked { .. } => {},
546545
events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {},
546+
events::MessageSendEvent::HandleError { action, .. } => {
547+
match action {
548+
ErrorAction::IgnoreError => {},
549+
_ => panic!("Expected IgnoreError")
550+
}
551+
}
547552
_ => panic!("Unhandled message event"),
548553
}
549554
}
@@ -565,6 +570,7 @@ pub fn do_test(data: &[u8]) {
565570
},
566571
events::MessageSendEvent::SendFundingLocked { .. } => false,
567572
events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => false,
573+
events::MessageSendEvent::HandleError { .. } => false,
568574
_ => panic!("Unhandled message event"),
569575
};
570576
if push { msg_sink.push(event); }

0 commit comments

Comments
 (0)