Skip to content

Commit 64d3f58

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 edab29e commit 64d3f58

8 files changed

+921
-851
lines changed

‎fuzz/src/chanmon_consistency.rs

+21-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};
3333
use lightning::util::events;
3434
use lightning::util::logger::Logger;
3535
use lightning::util::config::UserConfig;
@@ -251,7 +251,7 @@ pub fn do_test(data: &[u8]) {
251251
} else { panic!("Wrong event type"); }
252252
};
253253

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

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

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

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

383-
macro_rules! test_err {
384-
($res: expr) => {
385-
match $res {
386-
Ok(()) => {},
387-
Err(LightningError { action: ErrorAction::IgnoreError, .. }) => { },
388-
_ => { $res.unwrap() },
389-
}
390-
}
391-
}
392-
393383
macro_rules! test_return {
394384
() => { {
395385
assert_eq!(nodes[0].list_channels().len(), 1);
@@ -469,7 +459,7 @@ pub fn do_test(data: &[u8]) {
469459
assert!(update_fee.is_none());
470460
for update_add in update_add_htlcs {
471461
if !$corrupt_forward {
472-
test_err!(dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), &update_add));
462+
dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), &update_add);
473463
} else {
474464
// Corrupt the update_add_htlc message so that its HMAC
475465
// check will fail and we generate a
@@ -478,33 +468,33 @@ pub fn do_test(data: &[u8]) {
478468
let mut msg_ser = update_add.encode();
479469
msg_ser[1000] ^= 0xff;
480470
let new_msg = UpdateAddHTLC::read(&mut Cursor::new(&msg_ser)).unwrap();
481-
test_err!(dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), &new_msg));
471+
dest.handle_update_add_htlc(&nodes[$node].get_our_node_id(), &new_msg);
482472
}
483473
}
484474
for update_fulfill in update_fulfill_htlcs {
485-
test_err!(dest.handle_update_fulfill_htlc(&nodes[$node].get_our_node_id(), &update_fulfill));
475+
dest.handle_update_fulfill_htlc(&nodes[$node].get_our_node_id(), &update_fulfill);
486476
}
487477
for update_fail in update_fail_htlcs {
488-
test_err!(dest.handle_update_fail_htlc(&nodes[$node].get_our_node_id(), &update_fail));
478+
dest.handle_update_fail_htlc(&nodes[$node].get_our_node_id(), &update_fail);
489479
}
490480
for update_fail_malformed in update_fail_malformed_htlcs {
491-
test_err!(dest.handle_update_fail_malformed_htlc(&nodes[$node].get_our_node_id(), &update_fail_malformed));
481+
dest.handle_update_fail_malformed_htlc(&nodes[$node].get_our_node_id(), &update_fail_malformed);
492482
}
493-
test_err!(dest.handle_commitment_signed(&nodes[$node].get_our_node_id(), &commitment_signed));
483+
dest.handle_commitment_signed(&nodes[$node].get_our_node_id(), &commitment_signed);
494484
}
495485
}
496486
},
497487
events::MessageSendEvent::SendRevokeAndACK { ref node_id, ref msg } => {
498488
for dest in nodes.iter() {
499489
if dest.get_our_node_id() == *node_id {
500-
test_err!(dest.handle_revoke_and_ack(&nodes[$node].get_our_node_id(), msg));
490+
dest.handle_revoke_and_ack(&nodes[$node].get_our_node_id(), msg);
501491
}
502492
}
503493
},
504494
events::MessageSendEvent::SendChannelReestablish { ref node_id, ref msg } => {
505495
for dest in nodes.iter() {
506496
if dest.get_our_node_id() == *node_id {
507-
test_err!(dest.handle_channel_reestablish(&nodes[$node].get_our_node_id(), msg));
497+
dest.handle_channel_reestablish(&nodes[$node].get_our_node_id(), msg);
508498
}
509499
}
510500
},
@@ -515,6 +505,10 @@ pub fn do_test(data: &[u8]) {
515505
// Can be generated due to a payment forward being rejected due to a
516506
// channel having previously failed a monitor update
517507
},
508+
events::MessageSendEvent::HandleError { .. } => {
509+
// Can be generated at any processing step to send back an error, disconnect
510+
// peer or just ignore
511+
},
518512
_ => panic!("Unhandled message event"),
519513
}
520514
}
@@ -531,6 +525,7 @@ pub fn do_test(data: &[u8]) {
531525
events::MessageSendEvent::SendChannelReestablish { .. } => {},
532526
events::MessageSendEvent::SendFundingLocked { .. } => {},
533527
events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {},
528+
events::MessageSendEvent::HandleError { .. } => {},
534529
_ => panic!("Unhandled message event"),
535530
}
536531
}
@@ -543,6 +538,7 @@ pub fn do_test(data: &[u8]) {
543538
events::MessageSendEvent::SendChannelReestablish { .. } => {},
544539
events::MessageSendEvent::SendFundingLocked { .. } => {},
545540
events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => {},
541+
events::MessageSendEvent::HandleError { .. } => {},
546542
_ => panic!("Unhandled message event"),
547543
}
548544
}
@@ -564,6 +560,7 @@ pub fn do_test(data: &[u8]) {
564560
},
565561
events::MessageSendEvent::SendFundingLocked { .. } => false,
566562
events::MessageSendEvent::PaymentFailureNetworkUpdate { .. } => false,
563+
events::MessageSendEvent::HandleError { .. } => false,
567564
_ => panic!("Unhandled message event"),
568565
};
569566
if push { msg_sink.push(event); }

0 commit comments

Comments
 (0)