Skip to content

Commit bab1b49

Browse files
Antoine RiardTheBlueMatt
Antoine Riard
authored and
TheBlueMatt
committed
Implement DisconnectPeer and HandleOutboundMsgGenerationError events
Add test for DisconnectPeer event Update DisconnectPeer with optional ErrorMessage Manage error for funding_transaction_generated Add disconnect_socket to SocketDescriptor trait
1 parent fa889fc commit bab1b49

File tree

8 files changed

+289
-45
lines changed

8 files changed

+289
-45
lines changed

‎fuzz/fuzz_targets/full_stack_target.rs

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ use lightning::util::reset_rng_state;
2525
use secp256k1::key::{PublicKey,SecretKey};
2626
use secp256k1::Secp256k1;
2727

28+
use std::cell::RefCell;
2829
use std::collections::HashMap;
30+
use std::hash::Hash;
2931
use std::sync::Arc;
3032
use std::sync::atomic::{AtomicUsize,Ordering};
3133

@@ -104,15 +106,31 @@ impl BroadcasterInterface for TestBroadcaster {
104106
fn broadcast_transaction(&self, _tx: &Transaction) {}
105107
}
106108

107-
#[derive(Clone, PartialEq, Eq, Hash)]
108-
struct Peer {
109+
#[derive(Clone)]
110+
struct Peer<'a> {
109111
id: u8,
112+
peers_connected: &'a RefCell<[bool; 256]>,
110113
}
111-
impl SocketDescriptor for Peer {
114+
impl<'a> SocketDescriptor for Peer<'a> {
112115
fn send_data(&mut self, data: &Vec<u8>, write_offset: usize, _resume_read: bool) -> usize {
113116
assert!(write_offset < data.len());
114117
data.len() - write_offset
115118
}
119+
fn disconnect_socket(&mut self) {
120+
assert!(self.peers_connected.borrow()[self.id as usize]);
121+
self.peers_connected.borrow_mut()[self.id as usize] = false;
122+
}
123+
}
124+
impl<'a> PartialEq for Peer<'a> {
125+
fn eq(&self, other: &Self) -> bool {
126+
self.id == other.id
127+
}
128+
}
129+
impl<'a> Eq for Peer<'a> {}
130+
impl<'a> Hash for Peer<'a> {
131+
fn hash<H : std::hash::Hasher>(&self, h: &mut H) {
132+
self.id.hash(h)
133+
}
116134
}
117135

118136
#[inline]
@@ -158,12 +176,12 @@ pub fn do_test(data: &[u8]) {
158176
let channelmanager = ChannelManager::new(our_network_key, slice_to_be32(get_slice!(4)), get_slice!(1)[0] != 0, Network::Bitcoin, fee_est.clone(), monitor.clone(), watch.clone(), broadcast.clone()).unwrap();
159177
let router = Arc::new(Router::new(PublicKey::from_secret_key(&secp_ctx, &our_network_key).unwrap()));
160178

179+
let peers = RefCell::new([false; 256]);
161180
let handler = PeerManager::new(MessageHandler {
162181
chan_handler: channelmanager.clone(),
163182
route_handler: router.clone(),
164183
}, our_network_key);
165184

166-
let mut peers = [false; 256];
167185
let mut should_forward = false;
168186
let mut payments_received = Vec::new();
169187
let mut payments_sent = 0;
@@ -176,39 +194,39 @@ pub fn do_test(data: &[u8]) {
176194
0 => {
177195
let mut new_id = 0;
178196
for i in 1..256 {
179-
if !peers[i-1] {
197+
if !peers.borrow()[i-1] {
180198
new_id = i;
181199
break;
182200
}
183201
}
184202
if new_id == 0 { return; }
185-
peers[new_id - 1] = true;
186-
handler.new_outbound_connection(get_pubkey!(), Peer{id: (new_id - 1) as u8}).unwrap();
203+
peers.borrow_mut()[new_id - 1] = true;
204+
handler.new_outbound_connection(get_pubkey!(), Peer{id: (new_id - 1) as u8, peers_connected: &peers}).unwrap();
187205
},
188206
1 => {
189207
let mut new_id = 0;
190208
for i in 1..256 {
191-
if !peers[i-1] {
209+
if !peers.borrow()[i-1] {
192210
new_id = i;
193211
break;
194212
}
195213
}
196214
if new_id == 0 { return; }
197-
peers[new_id - 1] = true;
198-
handler.new_inbound_connection(Peer{id: (new_id - 1) as u8}).unwrap();
215+
peers.borrow_mut()[new_id - 1] = true;
216+
handler.new_inbound_connection(Peer{id: (new_id - 1) as u8, peers_connected: &peers}).unwrap();
199217
},
200218
2 => {
201219
let peer_id = get_slice!(1)[0];
202-
if !peers[peer_id as usize] { return; }
203-
peers[peer_id as usize] = false;
204-
handler.disconnect_event(&Peer{id: peer_id});
220+
if !peers.borrow()[peer_id as usize] { return; }
221+
peers.borrow_mut()[peer_id as usize] = false;
222+
handler.disconnect_event(&Peer{id: peer_id, peers_connected: &peers});
205223
},
206224
3 => {
207225
let peer_id = get_slice!(1)[0];
208-
if !peers[peer_id as usize] { return; }
209-
match handler.read_event(&mut Peer{id: peer_id}, get_slice!(get_slice!(1)[0]).to_vec()) {
226+
if !peers.borrow()[peer_id as usize] { return; }
227+
match handler.read_event(&mut Peer{id: peer_id, peers_connected: &peers}, get_slice!(get_slice!(1)[0]).to_vec()) {
210228
Ok(res) => assert!(!res),
211-
Err(_) => { peers[peer_id as usize] = false; }
229+
Err(_) => { peers.borrow_mut()[peer_id as usize] = false; }
212230
}
213231
},
214232
4 => {
@@ -231,7 +249,7 @@ pub fn do_test(data: &[u8]) {
231249
},
232250
5 => {
233251
let peer_id = get_slice!(1)[0];
234-
if !peers[peer_id as usize] { return; }
252+
if !peers.borrow()[peer_id as usize] { return; }
235253
let their_key = get_pubkey!();
236254
let chan_value = slice_to_be24(get_slice!(3)) as u64;
237255
if channelmanager.create_channel(their_key, chan_value, 0).is_err() { return; }

‎src/ln/channel.rs

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ macro_rules! secp_call {
328328
match $res {
329329
Ok(key) => key,
330330
//TODO: make the error a parameter
331-
Err(_) => return Err(HandleError{err: $err, action: Some(msgs::ErrorAction::DisconnectPeer{})})
331+
Err(_) => return Err(HandleError{err: $err, action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })})
332332
}
333333
};
334334
}
@@ -433,10 +433,10 @@ impl Channel {
433433

434434
fn check_remote_fee(fee_estimator: &FeeEstimator, feerate_per_kw: u32) -> Result<(), HandleError> {
435435
if (feerate_per_kw as u64) < fee_estimator.get_est_sat_per_vbyte(ConfirmationTarget::Background) * 250 {
436-
return Err(HandleError{err: "Peer's feerate much too low", action: Some(msgs::ErrorAction::DisconnectPeer{})});
436+
return Err(HandleError{err: "Peer's feerate much too low", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
437437
}
438438
if (feerate_per_kw as u64) > fee_estimator.get_est_sat_per_vbyte(ConfirmationTarget::HighPriority) * 375 { // 375 = 250 * 1.5x
439-
return Err(HandleError{err: "Peer's feerate much too high", action: Some(msgs::ErrorAction::DisconnectPeer{})});
439+
return Err(HandleError{err: "Peer's feerate much too high", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
440440
}
441441
Ok(())
442442
}
@@ -448,29 +448,29 @@ impl Channel {
448448
pub fn new_from_req(fee_estimator: &FeeEstimator, chan_keys: ChannelKeys, their_node_id: PublicKey, msg: &msgs::OpenChannel, user_id: u64, announce_publicly: bool) -> Result<Channel, HandleError> {
449449
// Check sanity of message fields:
450450
if msg.funding_satoshis >= MAX_FUNDING_SATOSHIS {
451-
return Err(HandleError{err: "funding value > 2^24", action: Some(msgs::ErrorAction::DisconnectPeer{})});
451+
return Err(HandleError{err: "funding value > 2^24", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
452452
}
453453
if msg.channel_reserve_satoshis > msg.funding_satoshis {
454-
return Err(HandleError{err: "Bogus channel_reserve_satoshis", action: Some(msgs::ErrorAction::DisconnectPeer{})});
454+
return Err(HandleError{err: "Bogus channel_reserve_satoshis", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
455455
}
456456
if msg.push_msat > (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 {
457-
return Err(HandleError{err: "push_msat more than highest possible value", action: Some(msgs::ErrorAction::DisconnectPeer{})});
457+
return Err(HandleError{err: "push_msat more than highest possible value", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
458458
}
459459
if msg.dust_limit_satoshis > msg.funding_satoshis {
460-
return Err(HandleError{err: "Peer never wants payout outputs?", action: Some(msgs::ErrorAction::DisconnectPeer{})});
460+
return Err(HandleError{err: "Peer never wants payout outputs?", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
461461
}
462462
if msg.htlc_minimum_msat >= (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 {
463-
return Err(HandleError{err: "Minimum htlc value is full channel value", action: Some(msgs::ErrorAction::DisconnectPeer{})});
463+
return Err(HandleError{err: "Minimum htlc value is full channel value", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
464464
}
465465
Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
466466
if msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT {
467-
return Err(HandleError{err: "They wanted our payments to be delayed by a needlessly long period", action: Some(msgs::ErrorAction::DisconnectPeer{})});
467+
return Err(HandleError{err: "They wanted our payments to be delayed by a needlessly long period", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
468468
}
469469
if msg.max_accepted_htlcs < 1 {
470-
return Err(HandleError{err: "0 max_accpted_htlcs makes for a useless channel", action: Some(msgs::ErrorAction::DisconnectPeer{})});
470+
return Err(HandleError{err: "0 max_accpted_htlcs makes for a useless channel", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
471471
}
472472
if (msg.channel_flags & 254) != 0 {
473-
return Err(HandleError{err: "unknown channel_flags", action: Some(msgs::ErrorAction::DisconnectPeer{})});
473+
return Err(HandleError{err: "unknown channel_flags", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
474474
}
475475

476476
// Convert things into internal flags and prep our state:

‎src/ln/channelmanager.rs

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -705,6 +705,16 @@ impl ChannelManager {
705705
/// Call this upon creation of a funding transaction for the given channel.
706706
/// Panics if a funding transaction has already been provided for this channel.
707707
pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_txo: OutPoint) {
708+
709+
macro_rules! add_pending_event {
710+
($event: expr) => {
711+
{
712+
let mut pending_events = self.pending_events.lock().unwrap();
713+
pending_events.push($event);
714+
}
715+
}
716+
}
717+
708718
let (chan, msg, chan_monitor) = {
709719
let mut channel_state = self.channel_state.lock().unwrap();
710720
match channel_state.by_id.remove(temporary_channel_id) {
@@ -713,10 +723,15 @@ impl ChannelManager {
713723
Ok(funding_msg) => {
714724
(chan, funding_msg.0, funding_msg.1)
715725
},
716-
Err(_e) => {
717-
//TODO: Push e to pendingevents
726+
Err(e) => {
727+
mem::drop(channel_state);
728+
add_pending_event!(events::Event::DisconnectPeer {
729+
node_id: chan.get_their_node_id(),
730+
msg: if let Some(msgs::ErrorAction::DisconnectPeer { msg } ) = e.action { msg } else { None },
731+
});
732+
718733
return;
719-
}
734+
},
720735
}
721736
},
722737
None => return
@@ -725,13 +740,10 @@ impl ChannelManager {
725740
if let Err(_e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
726741
unimplemented!(); // maybe remove from claimable_htlcs?
727742
}
728-
{
729-
let mut pending_events = self.pending_events.lock().unwrap();
730-
pending_events.push(events::Event::SendFundingCreated {
731-
node_id: chan.get_their_node_id(),
732-
msg: msg,
733-
});
734-
}
743+
add_pending_event!(events::Event::SendFundingCreated {
744+
node_id: chan.get_their_node_id(),
745+
msg: msg,
746+
});
735747

736748
let mut channel_state = self.channel_state.lock().unwrap();
737749
channel_state.by_id.insert(chan.channel_id(), chan);

‎src/ln/msgs.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,7 +379,9 @@ pub enum ErrorAction {
379379
msg: UpdateFailHTLC
380380
},
381381
/// The peer took some action which made us think they were useless. Disconnect them.
382-
DisconnectPeer,
382+
DisconnectPeer {
383+
msg: Option<ErrorMessage>
384+
},
383385
/// The peer did something harmless that we weren't able to process, just log and ignore
384386
IgnoreError,
385387
/// The peer did something incorrect. Tell them.

‎src/ln/peer_channel_encryptor.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ impl PeerChannelEncryptor {
147147

148148
let mut chacha = ChaCha20Poly1305RFC::new(key, &nonce, h);
149149
if !chacha.decrypt(&cyphertext[0..cyphertext.len() - 16], res, &cyphertext[cyphertext.len() - 16..]) {
150-
return Err(HandleError{err: "Bad MAC", action: Some(msgs::ErrorAction::DisconnectPeer{})});
150+
return Err(HandleError{err: "Bad MAC", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
151151
}
152152
Ok(())
153153
}
@@ -195,11 +195,11 @@ impl PeerChannelEncryptor {
195195
assert_eq!(act.len(), 50);
196196

197197
if act[0] != 0 {
198-
return Err(HandleError{err: "Unknown handshake version number", action: Some(msgs::ErrorAction::DisconnectPeer{})});
198+
return Err(HandleError{err: "Unknown handshake version number", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
199199
}
200200

201201
let their_pub = match PublicKey::from_slice(secp_ctx, &act[1..34]) {
202-
Err(_) => return Err(HandleError{err: "Invalid public key", action: Some(msgs::ErrorAction::DisconnectPeer{})}),
202+
Err(_) => return Err(HandleError{err: "Invalid public key", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })}),
203203
Ok(key) => key,
204204
};
205205

@@ -349,14 +349,14 @@ impl PeerChannelEncryptor {
349349
panic!("Requested act at wrong step");
350350
}
351351
if act_three[0] != 0 {
352-
return Err(HandleError{err: "Unknown handshake version number", action: Some(msgs::ErrorAction::DisconnectPeer{})});
352+
return Err(HandleError{err: "Unknown handshake version number", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
353353
}
354354

355355
let mut their_node_id = [0; 33];
356356
PeerChannelEncryptor::decrypt_with_ad(&mut their_node_id, 1, &temp_k2.unwrap(), &bidirectional_state.h, &act_three[1..50])?;
357357
self.their_node_id = Some(match PublicKey::from_slice(&self.secp_ctx, &their_node_id) {
358358
Ok(key) => key,
359-
Err(_) => return Err(HandleError{err: "Bad node_id from peer", action: Some(msgs::ErrorAction::DisconnectPeer{})}),
359+
Err(_) => return Err(HandleError{err: "Bad node_id from peer", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })}),
360360
});
361361

362362
let mut sha = Sha256::new();

0 commit comments

Comments
 (0)