You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Enable native peer-reputation observation by default in Bor. Normal bor server startup activates scoring; --peer-reputation=false or [p2p] peer-reputation = false opts out. Existing TOML/HCL configurations inherit the enabled default when the setting is omitted. The bounded ledger measures invalid deliveries and excessive traffic and exposes per-reason counters and peer snapshots without changing serving decisions or peer selection.
The observer reuses ETH decoding/body checks, txpool signature/KZG outcomes and WIT strike decisions. Completed block-body replies record the actual served hashes and byte sizes. Request accounting covers headers, bodies, receipts and pooled transactions. Transaction replies are excluded from unsolicited-volume scoring only when their peer, request ID and returned hashes match an outstanding fetcher request. Replayed and unmatched replies still count as traffic without being labeled invalid solely for being unmatched. Correlation runs on the fetcher loop before request cleanup, avoiding concurrent access to request state.
Block announcements, transaction announcements and completed body downloads have separate bounded repetition histories. Transaction-hash churn therefore cannot erase block-download history. Each peer retains at most 128 entries per family and the ledger retains at most 1,024 peers.
Scoring uses six ten-second buckets, the strongest reason in each bucket, and a cap of 100. Request-only peers within allowances have zero risk. Excess request volume, body-serving volume and repeated-serving evidence contribute at most 20 per bucket; three qualifying buckets give 60 and five give 100. Silence adds no penalty and announcements do not cancel negative evidence. Invalid-data evidence has weight 60. Existing downloader failures and disconnect/jail notifications have zero weight. The repository documentation and dashboard are excluded from this PR; the design is maintained in Confluence.
Executed tests
Focused peer-policy tests pass with -race across p2p/peerpolicy, eth/fetcher, eth/protocols/eth and eth. Coverage includes all five ETH request handlers, request-only allowance boundaries and expiry, simultaneous serving reasons, mixed-family repetition history, request-ID/hash correlation, replayed replies and unchanged invalid-transaction classification.
Full race-enabled suites passed for p2p/peerpolicy, eth/fetcher and eth/protocols/eth. The first full eth run failed TestSendTx with a delegated-account error mismatch. That test then passed 20 isolated race-enabled runs on both the PR and clean develop (755ae8b81843). Full eth race reruns passed on both as well. The initial failure remains recorded rather than being treated as a clean first run.
Lint and git diff --check pass.
Incremental Diffguard against 886a55cf94e5 passes: 24/26 mutations killed; tier-1 logic 11/11 and tier-2 semantic 4/4. Two reported tier-3 survivors remain in its output. Manual source-overlay checks of both reported deletions caused the corresponding new tests to fail, demonstrating that removal of delivery observation or correlation guards is detected when those tests run directly.
Warm-ledger observation benchmark: 130–144 ns/op, 0 B/op and 0 allocations/op on Apple M5 Pro. This measures ledger bookkeeping, not end-to-end network propagation.
Default startup/opt-out/configuration were validated in the preceding changes. Earlier repository-wide race runs had failures in unrelated packages and a downloader timeout; repository-wide CI, Kurtosis and live-network calibration are not claimed green by these scoped checks.
Rollout notes
This remains an observation-first draft. Scores and hypothetical actions are enabled by default. It does not execute score-based throttling or jailing, change peer slots, or grant contribution credit. Existing protocol validation, static/trusted treatment and legacy enforcement remain active. There are no wire-protocol, consensus or dependency changes.
Shared serving reservations, designated-BP headroom, contribution scheduling, complete SNAP/WIT and header/receipt serving accounting, and unification of enforcement ownership remain prerequisites for score-driven enforcement. The broader design's additive weights and serving-work budgets are not active in this conservative observation model. No dashboard is included.
Keep this PR in draft while CI and review continue. No deployment or live monitoring change has been made.
❌ Patch coverage is 95.84416% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.79%. Comparing base (755ae8b) to head (28570c3).
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
This PR touches security-sensitive p2p/eth protocol code across 39 files, and inline comments already flag two real gaps in the new peer-reputation scoring. Beyond those, I also checked tx_fetcher.go's early KZG-abort path in Enqueue: observeValidation reports byte/item counts for the full batch passed in, not just the prefix processed before the break, but that total reflects bytes actually received off the wire, so it's not an inflation bug.
Extended reasoning...
The diff adds a new p2p/peerpolicy reputation subsystem wired into eth protocol handlers, the downloader, and the tx fetcher — security-relevant surface for peer DoS/eclipse scoring, though enabled only as shadow observation with no enforcement yet. Two confirmed findings (a missing GetPooledTransactionsMsg case in messageFamily, and a shared per-peer hash LRU that lets mixed announcement/serving traffic evict each other's dedup state) are posted inline. Given the size, protocol-handler surface, and open findings, a human should also review; I additionally traced the KZG-early-exit path in tx_fetcher.go and confirmed the reported byte/item totals reflect real wire traffic rather than being an inflation artifact.
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this PR again after the latest commit and didn't find any new bugs. Both issues I flagged on the previous version are now fixed: messageFamily includes GetPooledTransactionsMsg so pooled-tx request volume is scored, and the per-peer hash LRU is now partitioned per Family (tracker.go) so transaction-hash churn can no longer evict block-announcement/body-serving repetition history. Given the size of this change (41 files, a new p2p/peerpolicy subsystem wired into downloader/fetcher/protocol handlers) and that it's peer-triggered, security-sensitive code, a human look is still worthwhile before merging.
Extended reasoning...
The latest commit (40ce6c3) fixes both issues raised in my prior review: messageFamily() now classifies GetPooledTransactionsMsg as a Requests family member (eth/protocols/eth/observation.go:55), and peerRecord.hashes was changed from a single shared LRU to a per-Family array of LRUs (p2p/peerpolicy/tracker.go), resolving the cross-family eviction problem. No new findings surfaced in this run (exit reason dry_streak). Given the overall scope (new peer-reputation subsystem touching p2p, eth/protocols/eth, downloader, fetcher, and config plumbing) and that it processes peer-supplied data, deferring to a human for final sign-off is still prudent despite no outstanding automated findings.
The reason will be displayed to describe this comment to others. Learn more.
I reviewed this PR again and found no new bugs. The two issues I flagged in earlier reviews (GetPooledTransactionsMsg missing from request-volume accounting, and a shared per-peer hash LRU that let tx-hash churn evict block-download repetition history) have both been fixed since. This remains a large change introducing a new peer-reputation/scoring system enabled by default across eth/p2p, so a human look is still worthwhile.
What was reviewed:
Request/response observation wiring (eth/protocols/eth/observation*.go) and the per-family hash LRUs and scoring windows in p2p/peerpolicy/tracker.go
WIT2 strike-to-risk severity and the Tracker's process-wide mutex on the hot inbound-message path — examined, not raised as blocking for this observation-only (non-enforcing) rollout
Default-enable CLI/config plumbing and the tests/bor Heimdall-injection changes
Extended reasoning...
This is the third pass on a large PR (43 files) that adds a new p2p/peerpolicy scoring package and wires peer-reputation observation into eth/protocols/eth, eth/downloader and eth/fetcher, enabled by default — directly the kind of peer-scoring/externally-triggerable surface the repo's p2p-security.md calls out for extra scrutiny. Both issues I previously flagged inline (missing GetPooledTransactionsMsg classification, shared hash LRU across evidence families) are confirmed fixed in the current diff against tracker.go and observation.go. No new bugs were found this run, but the size, default-on rollout, and security-sensitive scoring logic still warrant a human's sign-off rather than automated approval.
This review covers commit b4c7058, which is no longer the latest commit on this pull request; later commits are not covered by it.
The reason will be displayed to describe this comment to others. Learn more.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Transaction traffic received while transaction processing is disabled bypasses volume observation, including unmatched or replayed replies.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Record transaction deliveries during initial sync before early returns
eth/protocols/eth/observation.go:58
Transaction delivery messages fall through to Other, relying on the tx fetcher to classify them after correlation. However, both transaction handlers return before decoding or calling the fetcher while AcceptTxs() is false (handlers.go:738-740 and handlers.go:769-771). During initial sync, unsolicited broadcasts and unmatched/replayed replies therefore contribute no transaction-volume evidence, despite the stated requirement that only genuinely matched replies are excluded. Add an observation path for these early-return deliveries while preserving fetcher correlation once transaction processing is enabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Summary
Enable native peer-reputation observation by default in Bor. Normal
bor serverstartup activates scoring;--peer-reputation=falseor[p2p] peer-reputation = falseopts out. Existing TOML/HCL configurations inherit the enabled default when the setting is omitted. The bounded ledger measures invalid deliveries and excessive traffic and exposes per-reason counters and peer snapshots without changing serving decisions or peer selection.The observer reuses ETH decoding/body checks, txpool signature/KZG outcomes and WIT strike decisions. Completed block-body replies record the actual served hashes and byte sizes. Request accounting covers headers, bodies, receipts and pooled transactions. Transaction replies are excluded from unsolicited-volume scoring only when their peer, request ID and returned hashes match an outstanding fetcher request. Replayed and unmatched replies still count as traffic without being labeled invalid solely for being unmatched. Correlation runs on the fetcher loop before request cleanup, avoiding concurrent access to request state.
Block announcements, transaction announcements and completed body downloads have separate bounded repetition histories. Transaction-hash churn therefore cannot erase block-download history. Each peer retains at most 128 entries per family and the ledger retains at most 1,024 peers.
Scoring uses six ten-second buckets, the strongest reason in each bucket, and a cap of 100. Request-only peers within allowances have zero risk. Excess request volume, body-serving volume and repeated-serving evidence contribute at most 20 per bucket; three qualifying buckets give 60 and five give 100. Silence adds no penalty and announcements do not cancel negative evidence. Invalid-data evidence has weight 60. Existing downloader failures and disconnect/jail notifications have zero weight. The repository documentation and dashboard are excluded from this PR; the design is maintained in Confluence.
Executed tests
-raceacrossp2p/peerpolicy,eth/fetcher,eth/protocols/ethandeth. Coverage includes all five ETH request handlers, request-only allowance boundaries and expiry, simultaneous serving reasons, mixed-family repetition history, request-ID/hash correlation, replayed replies and unchanged invalid-transaction classification.p2p/peerpolicy,eth/fetcherandeth/protocols/eth. The first fullethrun failedTestSendTxwith a delegated-account error mismatch. That test then passed 20 isolated race-enabled runs on both the PR and cleandevelop(755ae8b81843). Fullethrace reruns passed on both as well. The initial failure remains recorded rather than being treated as a clean first run.git diff --checkpass.886a55cf94e5passes: 24/26 mutations killed; tier-1 logic 11/11 and tier-2 semantic 4/4. Two reported tier-3 survivors remain in its output. Manual source-overlay checks of both reported deletions caused the corresponding new tests to fail, demonstrating that removal of delivery observation or correlation guards is detected when those tests run directly.Rollout notes
This remains an observation-first draft. Scores and hypothetical actions are enabled by default. It does not execute score-based throttling or jailing, change peer slots, or grant contribution credit. Existing protocol validation, static/trusted treatment and legacy enforcement remain active. There are no wire-protocol, consensus or dependency changes.
Shared serving reservations, designated-BP headroom, contribution scheduling, complete SNAP/WIT and header/receipt serving accounting, and unification of enforcement ownership remain prerequisites for score-driven enforcement. The broader design's additive weights and serving-work budgets are not active in this conservative observation model. No dashboard is included.
Keep this PR in draft while CI and review continue. No deployment or live monitoring change has been made.