Skip to content

Commit 50df4cf

Browse files
authored
Merge pull request #644 from joemphilips/improve_error_message
Improve error message.
2 parents 5a1c0cc + 407e306 commit 50df4cf

13 files changed

+387
-334
lines changed

lightning/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,3 +28,4 @@ features = ["bitcoinconsensus"]
2828

2929
[dev-dependencies]
3030
hex = "0.3"
31+
regex = "0.1.80"

lightning/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
extern crate bitcoin;
2222
#[cfg(test)] extern crate hex;
23+
#[cfg(test)] extern crate regex;
2324

2425
#[macro_use]
2526
pub mod util;

lightning/src/ln/channel.rs

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

lightning/src/ln/channelmanager.rs

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

lightning/src/ln/functional_test_utils.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -972,8 +972,8 @@ pub fn route_over_limit<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_rou
972972
}
973973

974974
let (_, our_payment_hash) = get_payment_preimage_hash!(origin_node);
975-
unwrap_send_err!(origin_node.node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
976-
assert_eq!(err, "Cannot send value that would put us over the max HTLC value in flight our peer will accept"));
975+
unwrap_send_err!(origin_node.node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { ref err },
976+
assert!(err.contains("Cannot send value that would put us over the max HTLC value in flight our peer will accept")));
977977
}
978978

979979
pub fn send_payment<'a, 'b, 'c>(origin: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64, expected_value: u64) {

lightning/src/ln/functional_tests.rs

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

lightning/src/ln/msgs.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@ pub enum ErrorAction {
461461
/// An Err type for failure to process messages.
462462
pub struct LightningError {
463463
/// A human-readable message describing the error
464-
pub err: &'static str,
464+
pub err: String,
465465
/// The action which should be taken against the offending peer.
466466
pub action: ErrorAction,
467467
}
@@ -701,7 +701,7 @@ impl fmt::Display for DecodeError {
701701

702702
impl fmt::Debug for LightningError {
703703
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
704-
f.write_str(self.err)
704+
f.write_str(self.err.as_str())
705705
}
706706
}
707707

lightning/src/ln/onion_utils.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,11 @@ pub(super) fn build_onion_payloads(path: &Vec<RouteHop>, total_msat: u64, paymen
146146
});
147147
cur_value_msat += hop.fee_msat;
148148
if cur_value_msat >= 21000000 * 100000000 * 1000 {
149-
return Err(APIError::RouteError{err: "Channel fees overflowed?!"});
149+
return Err(APIError::RouteError{err: "Channel fees overflowed?"});
150150
}
151151
cur_cltv += hop.cltv_expiry_delta as u32;
152152
if cur_cltv >= 500000000 {
153-
return Err(APIError::RouteError{err: "Channel CLTV overflowed?!"});
153+
return Err(APIError::RouteError{err: "Channel CLTV overflowed?"});
154154
}
155155
last_short_channel_id = hop.short_channel_id;
156156
}

lightning/src/ln/peer_channel_encryptor.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use bitcoin::secp256k1;
1111

1212
use util::chacha20poly1305rfc::ChaCha20Poly1305RFC;
1313
use util::byte_utils;
14+
use bitcoin::hashes::hex::ToHex;
1415

1516
/// Maximum Lightning message data length according to
1617
/// [BOLT-8](https://github.com/lightningnetwork/lightning-rfc/blob/v1.0/08-transport.md#lightning-message-specification)
@@ -144,7 +145,7 @@ impl PeerChannelEncryptor {
144145

145146
let mut chacha = ChaCha20Poly1305RFC::new(key, &nonce, h);
146147
if !chacha.decrypt(&cyphertext[0..cyphertext.len() - 16], res, &cyphertext[cyphertext.len() - 16..]) {
147-
return Err(LightningError{err: "Bad MAC", action: msgs::ErrorAction::DisconnectPeer{ msg: None }});
148+
return Err(LightningError{err: "Bad MAC".to_owned(), action: msgs::ErrorAction::DisconnectPeer{ msg: None }});
148149
}
149150
Ok(())
150151
}
@@ -198,11 +199,11 @@ impl PeerChannelEncryptor {
198199
assert_eq!(act.len(), 50);
199200

200201
if act[0] != 0 {
201-
return Err(LightningError{err: "Unknown handshake version number", action: msgs::ErrorAction::DisconnectPeer{ msg: None }});
202+
return Err(LightningError{err: format!("Unknown handshake version number {}", act[0]), action: msgs::ErrorAction::DisconnectPeer{ msg: None }});
202203
}
203204

204205
let their_pub = match PublicKey::from_slice(&act[1..34]) {
205-
Err(_) => return Err(LightningError{err: "Invalid public key", action: msgs::ErrorAction::DisconnectPeer{ msg: None }}),
206+
Err(_) => return Err(LightningError{err: format!("Invalid public key {}", &act[1..34].to_hex()), action: msgs::ErrorAction::DisconnectPeer{ msg: None }}),
206207
Ok(key) => key,
207208
};
208209

@@ -335,14 +336,14 @@ impl PeerChannelEncryptor {
335336
panic!("Requested act at wrong step");
336337
}
337338
if act_three[0] != 0 {
338-
return Err(LightningError{err: "Unknown handshake version number", action: msgs::ErrorAction::DisconnectPeer{ msg: None }});
339+
return Err(LightningError{err: format!("Unknown handshake version number {}", act_three[0]), action: msgs::ErrorAction::DisconnectPeer{ msg: None }});
339340
}
340341

341342
let mut their_node_id = [0; 33];
342343
PeerChannelEncryptor::decrypt_with_ad(&mut their_node_id, 1, &temp_k2.unwrap(), &bidirectional_state.h, &act_three[1..50])?;
343344
self.their_node_id = Some(match PublicKey::from_slice(&their_node_id) {
344345
Ok(key) => key,
345-
Err(_) => return Err(LightningError{err: "Bad node_id from peer", action: msgs::ErrorAction::DisconnectPeer{ msg: None }}),
346+
Err(_) => return Err(LightningError{err: format!("Bad node_id from peer, {}", &their_node_id.to_hex()), action: msgs::ErrorAction::DisconnectPeer{ msg: None }}),
346347
});
347348

348349
let mut sha = Sha256::engine();

lightning/src/routing/network_graph.rs

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ use std::sync::atomic::{AtomicUsize, Ordering};
2222
use std::collections::BTreeMap;
2323
use std::collections::btree_map::Entry as BtreeEntry;
2424
use std::ops::Deref;
25+
use bitcoin::hashes::hex::ToHex;
2526

2627
/// Receives and validates network updates from peers,
2728
/// stores authentic and relevant data as a network graph.
@@ -74,7 +75,7 @@ macro_rules! secp_verify_sig {
7475
( $secp_ctx: expr, $msg: expr, $sig: expr, $pubkey: expr ) => {
7576
match $secp_ctx.verify($msg, $sig, $pubkey) {
7677
Ok(_) => {},
77-
Err(_) => return Err(LightningError{err: "Invalid signature from remote node", action: ErrorAction::IgnoreError}),
78+
Err(_) => return Err(LightningError{err: "Invalid signature from remote node".to_owned(), action: ErrorAction::IgnoreError}),
7879
}
7980
};
8081
}
@@ -86,7 +87,7 @@ impl<C: Deref + Sync + Send, L: Deref + Sync + Send> RoutingMessageHandler for N
8687

8788
fn handle_channel_announcement(&self, msg: &msgs::ChannelAnnouncement) -> Result<bool, LightningError> {
8889
if msg.contents.node_id_1 == msg.contents.node_id_2 || msg.contents.bitcoin_key_1 == msg.contents.bitcoin_key_2 {
89-
return Err(LightningError{err: "Channel announcement node had a channel with itself", action: ErrorAction::IgnoreError});
90+
return Err(LightningError{err: "Channel announcement node had a channel with itself".to_owned(), action: ErrorAction::IgnoreError});
9091
}
9192

9293
let checked_utxo = match self.chain_monitor.get_chain_utxo(msg.contents.chain_hash, msg.contents.short_channel_id) {
@@ -97,7 +98,7 @@ impl<C: Deref + Sync + Send, L: Deref + Sync + Send> RoutingMessageHandler for N
9798
.push_opcode(opcodes::all::OP_PUSHNUM_2)
9899
.push_opcode(opcodes::all::OP_CHECKMULTISIG).into_script().to_v0_p2wsh();
99100
if script_pubkey != expected_script {
100-
return Err(LightningError{err: "Channel announcement keys didn't match on-chain script", action: ErrorAction::IgnoreError});
101+
return Err(LightningError{err: format!("Channel announcement key ({}) didn't match on-chain script ({})", script_pubkey.to_hex(), expected_script.to_hex()), action: ErrorAction::IgnoreError});
101102
}
102103
//TODO: Check if value is worth storing, use it to inform routing, and compare it
103104
//to the new HTLC max field in channel_update
@@ -108,10 +109,10 @@ impl<C: Deref + Sync + Send, L: Deref + Sync + Send> RoutingMessageHandler for N
108109
false
109110
},
110111
Err(ChainError::NotWatched) => {
111-
return Err(LightningError{err: "Channel announced on an unknown chain", action: ErrorAction::IgnoreError});
112+
return Err(LightningError{err: format!("Channel announced on an unknown chain ({})", msg.contents.chain_hash.encode().to_hex()), action: ErrorAction::IgnoreError});
112113
},
113114
Err(ChainError::UnknownTx) => {
114-
return Err(LightningError{err: "Channel announced without corresponding UTXO entry", action: ErrorAction::IgnoreError});
115+
return Err(LightningError{err: "Channel announced without corresponding UTXO entry".to_owned(), action: ErrorAction::IgnoreError});
115116
},
116117
};
117118
let result = self.network_graph.write().unwrap().update_channel_from_announcement(msg, checked_utxo, Some(&self.secp_ctx));
@@ -522,11 +523,11 @@ impl NetworkGraph {
522523
}
523524

524525
match self.nodes.get_mut(&msg.contents.node_id) {
525-
None => Err(LightningError{err: "No existing channels for node_announcement", action: ErrorAction::IgnoreError}),
526+
None => Err(LightningError{err: "No existing channels for node_announcement".to_owned(), action: ErrorAction::IgnoreError}),
526527
Some(node) => {
527528
if let Some(node_info) = node.announcement_info.as_ref() {
528529
if node_info.last_update >= msg.contents.timestamp {
529-
return Err(LightningError{err: "Update older than last processed update", action: ErrorAction::IgnoreError});
530+
return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreError});
530531
}
531532
}
532533

@@ -588,7 +589,7 @@ impl NetworkGraph {
588589
Self::remove_channel_in_nodes(&mut self.nodes, &entry.get(), msg.contents.short_channel_id);
589590
*entry.get_mut() = chan_info;
590591
} else {
591-
return Err(LightningError{err: "Already have knowledge of channel", action: ErrorAction::IgnoreError})
592+
return Err(LightningError{err: "Already have knowledge of channel".to_owned(), action: ErrorAction::IgnoreError})
592593
}
593594
},
594595
BtreeEntry::Vacant(entry) => {
@@ -656,13 +657,13 @@ impl NetworkGraph {
656657
let chan_was_enabled;
657658

658659
match self.channels.get_mut(&msg.contents.short_channel_id) {
659-
None => return Err(LightningError{err: "Couldn't find channel for update", action: ErrorAction::IgnoreError}),
660+
None => return Err(LightningError{err: "Couldn't find channel for update".to_owned(), action: ErrorAction::IgnoreError}),
660661
Some(channel) => {
661662
macro_rules! maybe_update_channel_info {
662663
( $target: expr, $src_node: expr) => {
663664
if let Some(existing_chan_info) = $target.as_ref() {
664665
if existing_chan_info.last_update >= msg.contents.timestamp {
665-
return Err(LightningError{err: "Update older than last processed update", action: ErrorAction::IgnoreError});
666+
return Err(LightningError{err: "Update older than last processed update".to_owned(), action: ErrorAction::IgnoreError});
666667
}
667668
chan_was_enabled = existing_chan_info.enabled;
668669
} else {

lightning/src/routing/router.rs

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -165,11 +165,11 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, targ
165165
// TODO: Obviously *only* using total fee cost sucks. We should consider weighting by
166166
// uptime/success in using a node in the past.
167167
if *target == *our_node_id {
168-
return Err(LightningError{err: "Cannot generate a route to ourselves", action: ErrorAction::IgnoreError});
168+
return Err(LightningError{err: "Cannot generate a route to ourselves".to_owned(), action: ErrorAction::IgnoreError});
169169
}
170170

171171
if final_value_msat > 21_000_000 * 1_0000_0000 * 1000 {
172-
return Err(LightningError{err: "Cannot generate a route of more value than all existing satoshis", action: ErrorAction::IgnoreError});
172+
return Err(LightningError{err: "Cannot generate a route of more value than all existing satoshis".to_owned(), action: ErrorAction::IgnoreError});
173173
}
174174

175175
// We do a dest-to-source Dijkstra's sorting by each node's distance from the destination
@@ -209,7 +209,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, targ
209209
first_hop_targets.insert(chan.remote_network_id, (short_channel_id, chan.counterparty_features.clone()));
210210
}
211211
if first_hop_targets.is_empty() {
212-
return Err(LightningError{err: "Cannot route when there are no outbound routes away from us", action: ErrorAction::IgnoreError});
212+
return Err(LightningError{err: "Cannot route when there are no outbound routes away from us".to_owned(), action: ErrorAction::IgnoreError});
213213
}
214214
}
215215

@@ -374,7 +374,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, targ
374374

375375
let new_entry = match dist.remove(&res.last().unwrap().pubkey) {
376376
Some(hop) => hop.3,
377-
None => return Err(LightningError{err: "Failed to find a non-fee-overflowing path to the given destination", action: ErrorAction::IgnoreError}),
377+
None => return Err(LightningError{err: "Failed to find a non-fee-overflowing path to the given destination".to_owned(), action: ErrorAction::IgnoreError}),
378378
};
379379
res.last_mut().unwrap().fee_msat = new_entry.fee_msat;
380380
res.last_mut().unwrap().cltv_expiry_delta = new_entry.cltv_expiry_delta;
@@ -395,7 +395,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, targ
395395
}
396396
}
397397

398-
Err(LightningError{err: "Failed to find a path to the given destination", action: ErrorAction::IgnoreError})
398+
Err(LightningError{err: "Failed to find a path to the given destination".to_owned(), action: ErrorAction::IgnoreError})
399399
}
400400

401401
#[cfg(test)]
@@ -881,7 +881,7 @@ mod tests {
881881
assert_eq!(route.paths[0][0].fee_msat, 200);
882882
assert_eq!(route.paths[0][0].cltv_expiry_delta, (13 << 8) | 1);
883883
assert_eq!(route.paths[0][0].node_features.le_flags(), &vec![0b11]); // it should also override our view of their features
884-
assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::new()); // No feature flags will meet the relevant-to-channel conversion
884+
assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::<u8>::new()); // No feature flags will meet the relevant-to-channel conversion
885885

886886
assert_eq!(route.paths[0][1].pubkey, node3);
887887
assert_eq!(route.paths[0][1].short_channel_id, 13);
@@ -945,7 +945,7 @@ mod tests {
945945
assert_eq!(route.paths[0][0].fee_msat, 200);
946946
assert_eq!(route.paths[0][0].cltv_expiry_delta, (13 << 8) | 1);
947947
assert_eq!(route.paths[0][0].node_features.le_flags(), &vec![0b11]); // it should also override our view of their features
948-
assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::new()); // No feature flags will meet the relevant-to-channel conversion
948+
assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::<u8>::new()); // No feature flags will meet the relevant-to-channel conversion
949949

950950
assert_eq!(route.paths[0][1].pubkey, node3);
951951
assert_eq!(route.paths[0][1].short_channel_id, 13);
@@ -1008,7 +1008,7 @@ mod tests {
10081008
assert_eq!(route.paths[0][0].fee_msat, 200);
10091009
assert_eq!(route.paths[0][0].cltv_expiry_delta, (13 << 8) | 1);
10101010
assert_eq!(route.paths[0][0].node_features.le_flags(), &vec![0b11]);
1011-
assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::new()); // No feature flags will meet the relevant-to-channel conversion
1011+
assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::<u8>::new()); // No feature flags will meet the relevant-to-channel conversion
10121012

10131013
assert_eq!(route.paths[0][1].pubkey, node3);
10141014
assert_eq!(route.paths[0][1].short_channel_id, 13);
@@ -1082,8 +1082,8 @@ mod tests {
10821082
assert_eq!(route.paths[0][4].short_channel_id, 8);
10831083
assert_eq!(route.paths[0][4].fee_msat, 100);
10841084
assert_eq!(route.paths[0][4].cltv_expiry_delta, 42);
1085-
assert_eq!(route.paths[0][4].node_features.le_flags(), &Vec::new()); // We dont pass flags in from invoices yet
1086-
assert_eq!(route.paths[0][4].channel_features.le_flags(), &Vec::new()); // We can't learn any flags from invoices, sadly
1085+
assert_eq!(route.paths[0][4].node_features.le_flags(), &Vec::<u8>::new()); // We dont pass flags in from invoices yet
1086+
assert_eq!(route.paths[0][4].channel_features.le_flags(), &Vec::<u8>::new()); // We can't learn any flags from invoices, sadly
10871087

10881088
// Simple test with outbound channel to 4 to test that last_hops and first_hops connect
10891089
let our_chans = vec![channelmanager::ChannelDetails {
@@ -1105,14 +1105,14 @@ mod tests {
11051105
assert_eq!(route.paths[0][0].fee_msat, 0);
11061106
assert_eq!(route.paths[0][0].cltv_expiry_delta, (8 << 8) | 1);
11071107
assert_eq!(route.paths[0][0].node_features.le_flags(), &vec![0b11]);
1108-
assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::new()); // No feature flags will meet the relevant-to-channel conversion
1108+
assert_eq!(route.paths[0][0].channel_features.le_flags(), &Vec::<u8>::new()); // No feature flags will meet the relevant-to-channel conversion
11091109

11101110
assert_eq!(route.paths[0][1].pubkey, node7);
11111111
assert_eq!(route.paths[0][1].short_channel_id, 8);
11121112
assert_eq!(route.paths[0][1].fee_msat, 100);
11131113
assert_eq!(route.paths[0][1].cltv_expiry_delta, 42);
1114-
assert_eq!(route.paths[0][1].node_features.le_flags(), &Vec::new()); // We dont pass flags in from invoices yet
1115-
assert_eq!(route.paths[0][1].channel_features.le_flags(), &Vec::new()); // We can't learn any flags from invoices, sadly
1114+
assert_eq!(route.paths[0][1].node_features.le_flags(), &Vec::<u8>::new()); // We dont pass flags in from invoices yet
1115+
assert_eq!(route.paths[0][1].channel_features.le_flags(), &Vec::<u8>::new()); // We can't learn any flags from invoices, sadly
11161116

11171117
last_hops[0].fees.base_msat = 1000;
11181118

@@ -1147,8 +1147,8 @@ mod tests {
11471147
assert_eq!(route.paths[0][3].short_channel_id, 10);
11481148
assert_eq!(route.paths[0][3].fee_msat, 100);
11491149
assert_eq!(route.paths[0][3].cltv_expiry_delta, 42);
1150-
assert_eq!(route.paths[0][3].node_features.le_flags(), &Vec::new()); // We dont pass flags in from invoices yet
1151-
assert_eq!(route.paths[0][3].channel_features.le_flags(), &Vec::new()); // We can't learn any flags from invoices, sadly
1150+
assert_eq!(route.paths[0][3].node_features.le_flags(), &Vec::<u8>::new()); // We dont pass flags in from invoices yet
1151+
assert_eq!(route.paths[0][3].channel_features.le_flags(), &Vec::<u8>::new()); // We can't learn any flags from invoices, sadly
11521152

11531153
// ...but still use 8 for larger payments as 6 has a variable feerate
11541154
let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &node7, None, &last_hops, 2000, 42, Arc::clone(&logger)).unwrap();
@@ -1188,7 +1188,7 @@ mod tests {
11881188
assert_eq!(route.paths[0][4].short_channel_id, 8);
11891189
assert_eq!(route.paths[0][4].fee_msat, 2000);
11901190
assert_eq!(route.paths[0][4].cltv_expiry_delta, 42);
1191-
assert_eq!(route.paths[0][4].node_features.le_flags(), &Vec::new()); // We dont pass flags in from invoices yet
1192-
assert_eq!(route.paths[0][4].channel_features.le_flags(), &Vec::new()); // We can't learn any flags from invoices, sadly
1191+
assert_eq!(route.paths[0][4].node_features.le_flags(), &Vec::<u8>::new()); // We dont pass flags in from invoices yet
1192+
assert_eq!(route.paths[0][4].channel_features.le_flags(), &Vec::<u8>::new()); // We can't learn any flags from invoices, sadly
11931193
}
11941194
}

lightning/src/util/errors.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ pub enum APIError {
99
/// are documented, but generally indicates some precondition of a function was violated.
1010
APIMisuseError {
1111
/// A human-readable error message
12-
err: &'static str
12+
err: String
1313
},
1414
/// Due to a high feerate, we were unable to complete the request.
1515
/// For example, this may be returned if the feerate implies we cannot open a channel at the
@@ -31,7 +31,7 @@ pub enum APIError {
3131
/// peer, channel at capacity, channel shutting down, etc.
3232
ChannelUnavailable {
3333
/// A human-readable error message
34-
err: &'static str
34+
err: String
3535
},
3636
/// An attempt to call add/update_monitor returned an Err (ie you did this!), causing the
3737
/// attempted action to fail.

0 commit comments

Comments
 (0)