log the direct source IP and node_id of NewBlockHashes of new PoW blocks - #3583
log the direct source IP and node_id of NewBlockHashes of new PoW blocks#3583Hecate2 wants to merge 4 commits into
Conversation
Add the ability to log the direct source IP and NodeId for each received NewBlockHashes message. This feature is controlled by the log_block_source config flag (default: false) and is designed for deployment on public bootnodes to track block propagation patterns. Changes: - Add get_peer_addr() method to NetworkContext trait - Implement get_peer_addr() in network service to look up session address - Add peer_addr field to sync message Context struct - Populate peer_addr when dispatching messages - Log [BLOCK_SOURCE] with block hash, source NodeId and IP when enabled - Add log_block_source config option in ProtocolConfiguration and configuration.rs
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| .map(|s| s.as_str()) | ||
| .unwrap_or("unknown"); | ||
| for hash in &self.block_hashes { | ||
| info!( |
There was a problem hiding this comment.
WARNING: Per-hash info!() logging may produce very high log volume on a busy bootnode
A single NewBlockHashes message can carry many hashes, and a bootnode with many connected peers can receive a large number of these messages per second. When log_block_source is true, this loop emits one info! line per hash per message, which could produce tens of thousands of log lines per second under load — potentially causing I/O saturation or log-file bloat.
Consider logging a single aggregated line per message instead, e.g.:
| info!( | |
| info!( |
Alternatively, restructure to log once per message rather than once per hash:
let hashes_str: Vec<String> = self.block_hashes.iter()
.map(|h| format!("{:#x}", h))
.collect();
info!(
"[BLOCK_SOURCE] hashes=[{}] from_node={} from_addr={}",
hashes_str.join(","), ctx.node_id, peer_addr
);This reduces the log volume proportionally to the average number of hashes per message.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Review NotesThis review covers changes since commit The latest commit ("use simple impl") removes the
Files Reviewed (1 file)
Previous Review Summaries (4 snapshots, latest commit 49f0ab7)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 49f0ab7)Status: No Issues Found | Recommendation: Merge Incremental Review NotesThis review covers changes since commit The latest commit ("use simple impl") reverts the
Files Reviewed (3 files)
Previous review (commit 9cd735f)Status: No Issues Found | Recommendation: Merge Incremental Review NotesThis review covers changes since commit This commit is a clean performance-focused refactor of
The previously flagged WARNING (per-hash Files Reviewed (1 file)
Previous review (commit 7388f07)Status: No Issues Found | Recommendation: Merge Incremental Review NotesThis review covers changes since commit The previous WARNING (per-hash
Per-hash logging for new/unknown hashes remains intentional behavior (first-seen propagation tracking). The duplicate ratio is bounded by the short window before header download, as documented in the new code comments. No new issues were found in the incremental diff. Files Reviewed (1 file)
Previous review (commit 66f4665)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files)
Reviewed by claude-sonnet-4.6 · Input: 9 · Output: 2.2K · Cached: 127.1K |
Replace the custom BlockSourceTracker with approximate first-seen deduplication that reuses the existing block_header_by_hash lookup. Only block hashes whose headers are not yet downloaded generate a [BLOCK_SOURCE] log entry. This avoids adding any new data structure, consuming zero additional memory and zero extra disk I/O. Trade-offs documented in new_block_hashes.rs: - Not a strict first-seen guarantee: multiple peers propagating the same hash within the header-download window (ms to seconds) each generate a log entry. In practice this window is small. - After the header is cached, all subsequent NewBlockHashes for the same block are silently suppressed, which is the desired behavior. Changes: - Remove BlockSourceTracker struct and its unit tests - Remove block_source_tracker field from SynchronizationProtocolHandler - Remove log_block_source_capacity config option - Reuse block_header_by_hash filter for dedup in NewBlockHashes::handle - Unify the header existence check so logging and request filtering share a single lookup
|
Changed to reuse the original |
49f0ab7 to
758bb18
Compare
1 participant
Add the ability to log the direct source IP and NodeId for each received NewBlockHashes message. This feature is controlled by the log_block_source config flag (default: false). To track PoW block propagation patterns.
Changes:
This change is