Skip to content

Commit 4e859a4

Browse files
obycodeobycode
authored
Merge pull request #5720 from stacks-network/chore/add-drop-reason-to-p2p-logs
Add DropReason and DropPeer structs to pass reason string to logs about Dropping a Neighbor
2 parents 4dadde0 + db4f14c commit 4e859a4

File tree

17 files changed

+814
-295
lines changed

17 files changed

+814
-295
lines changed

‎CHANGELOG.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,16 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
77

88
## [Unreleased]
99

10-
### Added
10+
## Added
1111

1212
- Add `vrf_seed` to the `/v3/sortitions` rpc endpoint
1313

1414
### Changed
1515

1616
- Miner will stop waiting for signatures on a block if the Stacks tip advances (causing the block it had proposed to be invalid).
17+
- Logging improvements:
18+
- P2P logs now includes a reason for dropping a peer or neighbor
19+
- Improvements to how a PeerAddress is logged (human readable format vs hex)
1720

1821
### Fixed
1922

‎stacks-common/src/types/net.rs

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -118,15 +118,12 @@ impl PeerAddress {
118118
)
119119
}
120120

121-
/// Convert to SocketAddr
122-
pub fn to_socketaddr(&self, port: u16) -> SocketAddr {
121+
/// Convert to IpAddr
122+
pub fn to_ipaddr(&self) -> IpAddr {
123123
if self.is_ipv4() {
124-
SocketAddr::new(
125-
IpAddr::V4(Ipv4Addr::new(
126-
self.0[12], self.0[13], self.0[14], self.0[15],
127-
)),
128-
port,
129-
)
124+
IpAddr::V4(Ipv4Addr::new(
125+
self.0[12], self.0[13], self.0[14], self.0[15],
126+
))
130127
} else {
131128
let addr_words: [u16; 8] = [
132129
((self.0[0] as u16) << 8) | (self.0[1] as u16),
@@ -138,23 +135,25 @@ impl PeerAddress {
138135
((self.0[12] as u16) << 8) | (self.0[13] as u16),
139136
((self.0[14] as u16) << 8) | (self.0[15] as u16),
140137
];
141-
142-
SocketAddr::new(
143-
IpAddr::V6(Ipv6Addr::new(
144-
addr_words[0],
145-
addr_words[1],
146-
addr_words[2],
147-
addr_words[3],
148-
addr_words[4],
149-
addr_words[5],
150-
addr_words[6],
151-
addr_words[7],
152-
)),
153-
port,
154-
)
138+
IpAddr::V6(Ipv6Addr::new(
139+
addr_words[0],
140+
addr_words[1],
141+
addr_words[2],
142+
addr_words[3],
143+
addr_words[4],
144+
addr_words[5],
145+
addr_words[6],
146+
addr_words[7],
147+
))
155148
}
156149
}
157150

151+
/// Convert to SocketAddr
152+
pub fn to_socketaddr(&self, port: u16) -> SocketAddr {
153+
let ip_addr = self.to_ipaddr();
154+
SocketAddr::new(ip_addr, port)
155+
}
156+
158157
/// Convert from socket address
159158
pub fn from_socketaddr(addr: &SocketAddr) -> PeerAddress {
160159
PeerAddress::from_ip(&addr.ip())
@@ -228,6 +227,10 @@ impl PeerAddress {
228227
pub fn to_bin(&self) -> String {
229228
to_bin(&self.0)
230229
}
230+
231+
pub fn pretty_print(&self) -> String {
232+
self.to_ipaddr().to_string()
233+
}
231234
}
232235

233236
/// Peer address variants for the Host: header

‎stackslib/src/net/download/epoch2x.rs

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ use std::sync::mpsc::{
2222
sync_channel, Receiver, RecvError, RecvTimeoutError, SyncSender, TryRecvError, TrySendError,
2323
};
2424

25+
use p2p::DropSource;
2526
use rand::seq::SliceRandom;
2627
use rand::{thread_rng, RngCore};
2728
use stacks_common::types::chainstate::{BlockHeaderHash, PoxId, SortitionId, StacksBlockId};
@@ -233,7 +234,7 @@ pub struct BlockDownloader {
233234
/// statistics on peers' data-plane endpoints
234235
pub(crate) dead_peers: Vec<usize>,
235236
pub(crate) broken_peers: Vec<usize>,
236-
broken_neighbors: Vec<NeighborKey>, // disconnect peers who report invalid block inventories too
237+
broken_neighbors: Vec<DropNeighbor>, // disconnect peers who report invalid block inventories too
237238

238239
pub(crate) blocked_urls: HashMap<UrlString, u64>, // URLs that chronically don't work, and when we can try them again
239240

@@ -500,7 +501,13 @@ impl BlockDownloader {
500501
{
501502
info!("Invalid block from {:?} ({:?}): did not ask for block {}/{}", &block_key.neighbor, &block_key.data_url, block_key.consensus_hash, block.block_hash());
502503
self.broken_peers.push(event_id);
503-
self.broken_neighbors.push(block_key.neighbor.clone());
504+
self.broken_neighbors.push(DropNeighbor {
505+
key: block_key.neighbor.clone(),
506+
reason: DropReason::BrokenConnection(
507+
"Remote neighbor sent an invalid block".into(),
508+
),
509+
source: DropSource::BlockDownloaderGetBlocks,
510+
});
504511
} else {
505512
// got the block
506513
debug!(
@@ -519,14 +526,25 @@ impl BlockDownloader {
519526
// the fact that we asked this peer means that it's block inv indicated
520527
// it was present, so the absence is the mark of a broken peer
521528
self.broken_peers.push(event_id);
522-
self.broken_neighbors.push(block_key.neighbor.clone());
529+
self.broken_neighbors.push(DropNeighbor {
530+
key: block_key.neighbor.clone(),
531+
reason: DropReason::BrokenConnection(
532+
"Remote neighbor was missing an expected block"
533+
.into(),
534+
),
535+
source: DropSource::BlockDownloaderGetBlocks,
536+
});
523537
}
524538
Err(e) => {
525539
info!("Error decoding response from remote neighbor {:?} (at {}): {:?}", &block_key.neighbor, &block_key.data_url, &e;
526540
"consensus_hash" => %block_key.consensus_hash
527541
);
528542
self.broken_peers.push(event_id);
529-
self.broken_neighbors.push(block_key.neighbor.clone());
543+
self.broken_neighbors.push(DropNeighbor {
544+
key: block_key.neighbor.clone(),
545+
reason: DropReason::BrokenConnection(format!("Error occurred decoding block response from neighbor: {e}")),
546+
source: DropSource::BlockDownloaderGetBlocks
547+
});
530548
}
531549
}
532550
}
@@ -632,7 +650,11 @@ impl BlockDownloader {
632650
"consensus_hash" => %block_key.consensus_hash
633651
);
634652
self.broken_peers.push(event_id);
635-
self.broken_neighbors.push(block_key.neighbor.clone());
653+
self.broken_neighbors.push(DropNeighbor {
654+
key: block_key.neighbor.clone(),
655+
reason: DropReason::BrokenConnection("Remote neighbor sent an unexpected zero-length microblock stream".into()),
656+
source: DropSource::BlockDownloaderGetMicroblocks
657+
});
636658
} else {
637659
// have microblocks (but we don't know yet if they're well-formed)
638660
debug!(
@@ -664,7 +686,11 @@ impl BlockDownloader {
664686
"consensus_hash" => %block_key.consensus_hash
665687
);
666688
self.broken_peers.push(event_id);
667-
self.broken_neighbors.push(block_key.neighbor.clone());
689+
self.broken_neighbors.push(DropNeighbor {
690+
key: block_key.neighbor.clone(),
691+
reason: DropReason::BrokenConnection(format!("Error occurred decoding microblock response from neighbor: {e}")),
692+
source: DropSource::BlockDownloaderGetMicroblocks
693+
});
668694
}
669695
}
670696
}
@@ -890,7 +916,7 @@ impl BlockDownloader {
890916
}
891917

892918
/// Clear out broken peers that told us they had blocks, but didn't serve them.
893-
fn clear_broken_peers(&mut self) -> (Vec<usize>, Vec<NeighborKey>) {
919+
fn clear_broken_peers(&mut self) -> (Vec<usize>, Vec<DropNeighbor>) {
894920
// remove dead/broken peers
895921
let mut disconnect = vec![];
896922
let mut disconnect_neighbors = vec![];
@@ -2341,7 +2367,7 @@ impl PeerNetwork {
23412367
Vec<(ConsensusHash, StacksBlock, u64)>,
23422368
Vec<(ConsensusHash, Vec<StacksMicroblock>, u64)>,
23432369
Vec<usize>,
2344-
Vec<NeighborKey>,
2370+
Vec<DropNeighbor>,
23452371
),
23462372
net_error,
23472373
> {

‎stackslib/src/net/download/nakamoto/download_state_machine.rs

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ use crate::net::inv::epoch2x::InvState;
6363
use crate::net::inv::nakamoto::{NakamotoInvStateMachine, NakamotoTenureInv};
6464
use crate::net::neighbors::rpc::NeighborRPC;
6565
use crate::net::neighbors::NeighborComms;
66-
use crate::net::p2p::{CurrentRewardSet, PeerNetwork};
66+
use crate::net::p2p::{CurrentRewardSet, DropReason, DropSource, PeerNetwork};
6767
use crate::net::server::HttpPeer;
6868
use crate::net::{Error as NetError, Neighbor, NeighborAddress, NeighborKey};
6969
use crate::util_lib::db::{DBConn, Error as DBError};
@@ -1204,7 +1204,12 @@ impl NakamotoDownloadStateMachine {
12041204
"Downloader for {} failed; this peer is dead: {:?}",
12051205
&naddr, &e
12061206
);
1207-
neighbor_rpc.add_dead(network, naddr);
1207+
neighbor_rpc.add_dead(
1208+
network,
1209+
naddr,
1210+
DropReason::DeadConnection(format!("Failed to send download request: {e}")),
1211+
DropSource::NakamotoDownloadStateMachine,
1212+
);
12081213
continue;
12091214
};
12101215
}
@@ -1236,12 +1241,24 @@ impl NakamotoDownloadStateMachine {
12361241
) {
12371242
Ok(blocks_opt) => blocks_opt,
12381243
Err(NetError::StaleView) => {
1239-
neighbor_rpc.add_dead(network, &naddr);
1244+
neighbor_rpc.add_dead(
1245+
network,
1246+
&naddr,
1247+
DropReason::DeadConnection("Stale view".into()),
1248+
DropSource::NakamotoDownloadStateMachine,
1249+
);
12401250
continue;
12411251
}
12421252
Err(e) => {
12431253
debug!("Failed to handle next download response from unconfirmed downloader for {:?} in state {:?}: {:?}", &naddr, &downloader.state, &e);
1244-
neighbor_rpc.add_dead(network, &naddr);
1254+
neighbor_rpc.add_dead(
1255+
network,
1256+
&naddr,
1257+
DropReason::DeadConnection(format!(
1258+
"Failed to handle next download response: {e}"
1259+
)),
1260+
DropSource::NakamotoDownloadStateMachine,
1261+
);
12451262
continue;
12461263
}
12471264
};

‎stackslib/src/net/download/nakamoto/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ use crate::net::inv::epoch2x::InvState;
152152
use crate::net::inv::nakamoto::{NakamotoInvStateMachine, NakamotoTenureInv};
153153
use crate::net::neighbors::rpc::NeighborRPC;
154154
use crate::net::neighbors::NeighborComms;
155-
use crate::net::p2p::PeerNetwork;
155+
use crate::net::p2p::{DropReason, PeerNetwork};
156156
use crate::net::server::HttpPeer;
157157
use crate::net::{Error as NetError, Neighbor, NeighborAddress, NeighborKey};
158158
use crate::util_lib::db::{DBConn, Error as DBError};
@@ -236,11 +236,11 @@ impl PeerNetwork {
236236
};
237237

238238
for broken in block_downloader.neighbor_rpc.take_broken() {
239-
self.deregister_and_ban_neighbor(&broken);
239+
self.deregister_and_ban_neighbor(&broken.key, broken.reason, broken.source);
240240
}
241241

242242
for dead in block_downloader.neighbor_rpc.take_dead() {
243-
self.deregister_neighbor(&dead);
243+
self.deregister_neighbor(&dead.key, dead.reason, dead.source);
244244
}
245245

246246
self.block_downloader_nakamoto = Some(block_downloader);

‎stackslib/src/net/download/nakamoto/tenure_downloader.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ use crate::net::inv::epoch2x::InvState;
5757
use crate::net::inv::nakamoto::{NakamotoInvStateMachine, NakamotoTenureInv};
5858
use crate::net::neighbors::rpc::NeighborRPC;
5959
use crate::net::neighbors::NeighborComms;
60-
use crate::net::p2p::{CurrentRewardSet, PeerNetwork};
60+
use crate::net::p2p::{CurrentRewardSet, DropReason, DropSource, PeerNetwork};
6161
use crate::net::server::HttpPeer;
6262
use crate::net::{Error as NetError, Neighbor, NeighborAddress, NeighborKey};
6363
use crate::util_lib::db::{DBConn, Error as DBError};
@@ -745,7 +745,12 @@ impl NakamotoTenureDownloader {
745745

746746
let Some(peerhost) = NeighborRPC::get_peer_host(network, &self.naddr) else {
747747
// no conversation open to this neighbor
748-
neighbor_rpc.add_dead(network, &self.naddr);
748+
neighbor_rpc.add_dead(
749+
network,
750+
&self.naddr,
751+
DropReason::DeadConnection("No authenticated connection open".into()),
752+
DropSource::NakamotoTenureDownloader,
753+
);
749754
return Err(NetError::PeerNotConnected);
750755
};
751756

0 commit comments

Comments
 (0)