docs(adr): add ADR-023 P2P connection gater blocklist#3268
docs(adr): add ADR-023 P2P connection gater blocklist#3268
Conversation
Defines the full TTLConnectionGater feature: TTL-based peer blocklist with config/dynamic block sources, startup reconciliation, background sweep, and Prometheus metrics for blocklist observability. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @auricom's task in 2m 40s —— View job Code Review
SummaryThe net effect of this PR (after all 3 commits) is a single changed file:
This is a deliberate regression in filtering capability, presumably as a step toward a future TTL-based gater (per the comment referencing ADR-023 Option C). The changes are reasonable in intent but leave several issues behind. Issues Found🔴 High — Dead code:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an ADR proposing a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/adr/adr-023-p2p-gater-ttl.md`:
- Around line 83-84: The ADR currently contains conflicting default TTLs for
dynamic blocks ("24 hour" vs "1h"); pick the intended canonical default (e.g.,
1h or 24h), replace all occurrences of the wrong value in the ADR text (search
for the phrases "a default TTL of **24 hour**" and "1h"), and ensure the
documented behavior for dynamic blocks and any related Prometheus metrics
exposition text is updated to reference the chosen TTL consistently (also update
the other affected section that repeats the value).
- Around line 222-225: Add a blank line immediately before the fenced code block
that contains the Go snippet so the markdown has an empty line preceding the
triple backticks; specifically adjust the block that shows
metrics.BlockedPeer.With("peer_id", id.String(), "source",
string(record.Source)).Set(0) to be separated from the preceding paragraph by
one empty line to satisfy markdownlint MD031.
- Around line 95-97: The fenced code block containing "/p2p/gater/ttl/<peerID>
→ {"expires_at": <unix-seconds>, "reason": "<string>", "source":
"<config|dynamic>"}" should include a language identifier (e.g., "text") to
satisfy markdownlint MD040; update the triple-backtick fence that wraps that
line to use ```text so the block is explicitly labeled.
- Around line 183-185: The fenced code block showing the p2p gater log should
include a language identifier to satisfy markdownlint MD040; replace the opening
backticks ``` with ```text for the block that contains "p2p gater loaded: 3
permanent blocks (config), 1 dynamic block (expires in 47m), 0 expired entries
pruned" so the example is marked as plain text.
- Around line 23-24: The incident description contains awkward phrasing and
hyphenation: replace the phrase "binary builders nodes" with "binary builder
nodes" (or "binary builders' nodes" if possessive was intended) and change
"latency prone" to the hyphenated form "latency-prone" so the sentence reads
clearly (e.g., "...caused every binary builder node to be rejected by the local
gater... and the node fell back to unstable and latency-prone DA-only sync.").
Ensure both corrections are applied in the ADR document where the original
sentence appears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 301e3de7-c46a-4865-a091-d3760589d73d
📒 Files selected for processing (1)
docs/adr/adr-023-p2p-gater-ttl.md
| a fullnode accumulated stale block entries, which caused every binary builders nodes to be rejected by the | ||
| local gater. Header sync never initialized, and the node fell back to unstable and latency prone DA-only sync. |
There was a problem hiding this comment.
Tighten wording for readability in the incident description.
There’s a grammar/readability issue (binary builders nodes) and hyphenation should be latency-prone.
Proposed doc fix
-a fullnode accumulated stale block entries, which caused every binary builders nodes to be rejected by the
-local gater. Header sync never initialized, and the node fell back to unstable and latency prone DA-only sync.
+a fullnode accumulated stale block entries, which caused binary-builder nodes to be rejected by the
+local gater. Header sync never initialized, and the node fell back to unstable, latency-prone DA-only sync.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| a fullnode accumulated stale block entries, which caused every binary builders nodes to be rejected by the | |
| local gater. Header sync never initialized, and the node fell back to unstable and latency prone DA-only sync. | |
| a fullnode accumulated stale block entries, which caused binary-builder nodes to be rejected by the | |
| local gater. Header sync never initialized, and the node fell back to unstable, latency-prone DA-only sync. |
🧰 Tools
🪛 LanguageTool
[grammar] ~24-~24: Use a hyphen to join words.
Context: ...e node fell back to unstable and latency prone DA-only sync. Two distinct catego...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/adr-023-p2p-gater-ttl.md` around lines 23 - 24, The incident
description contains awkward phrasing and hyphenation: replace the phrase
"binary builders nodes" with "binary builder nodes" (or "binary builders' nodes"
if possessive was intended) and change "latency prone" to the hyphenated form
"latency-prone" so the sentence reads clearly (e.g., "...caused every binary
builder node to be rejected by the local gater... and the node fell back to
unstable and latency-prone DA-only sync."). Ensure both corrections are applied
in the ADR document where the original sentence appears.
| a default TTL of **24 hour** for dynamic blocks, permanent status for config-sourced blocks, | ||
| and Prometheus metrics exposing the full blocklist state. |
There was a problem hiding this comment.
Resolve conflicting default TTL values in the ADR.
The document defines two different defaults for dynamic block TTL (24 hour vs 1h). This creates implementation ambiguity and risks misconfiguration.
Proposed doc fix
-Implement **Option C**: a `TTLConnectionGater` wrapper around `BasicConnectionGater` with
-a default TTL of **24 hour** for dynamic blocks, permanent status for config-sourced blocks,
+Implement **Option C**: a `TTLConnectionGater` wrapper around `BasicConnectionGater` with
+a default TTL of **1h** for dynamic blocks, permanent status for config-sourced blocks,
and Prometheus metrics exposing the full blocklist state.Also applies to: 232-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/adr-023-p2p-gater-ttl.md` around lines 83 - 84, The ADR currently
contains conflicting default TTLs for dynamic blocks ("24 hour" vs "1h"); pick
the intended canonical default (e.g., 1h or 24h), replace all occurrences of the
wrong value in the ADR text (search for the phrases "a default TTL of **24
hour**" and "1h"), and ensure the documented behavior for dynamic blocks and any
related Prometheus metrics exposition text is updated to reference the chosen
TTL consistently (also update the other affected section that repeats the
value).
| ``` | ||
| /p2p/gater/ttl/<peerID> → {"expires_at": <unix-seconds>, "reason": "<string>", "source": "<config|dynamic>"} | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to this fenced code block.
This triggers markdownlint MD040.
Proposed doc fix
-```
+```text
/p2p/gater/ttl/<peerID> → {"expires_at": <unix-seconds>, "reason": "<string>", "source": "<config|dynamic>"}</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 95-95: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/adr/adr-023-p2p-gater-ttl.md around lines 95 - 97, The fenced code
block containing "/p2p/gater/ttl/ → {"expires_at": ,
"reason": "", "source": "<config|dynamic>"}" should include a language
identifier (e.g., "text") to satisfy markdownlint MD040; update the
triple-backtick fence that wraps that line to use ```text so the block is
explicitly labeled.
</details>
<!-- fingerprinting:phantom:triton:hawk:076a2914-53eb-4a56-a71c-b7031c85b262 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ``` | ||
| p2p gater loaded: 3 permanent blocks (config), 1 dynamic block (expires in 47m), 0 expired entries pruned | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the log example fence.
This also triggers markdownlint MD040.
Proposed doc fix
-```
+```text
p2p gater loaded: 3 permanent blocks (config), 1 dynamic block (expires in 47m), 0 expired entries pruned</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/adr/adr-023-p2p-gater-ttl.md` around lines 183 - 185, The fenced code
block showing the p2p gater log should include a language identifier to satisfy
markdownlint MD040; replace the opening backticks ``` with ```text for the block
that contains "p2p gater loaded: 3 permanent blocks (config), 1 dynamic block
(expires in 47m), 0 expired entries pruned" so the example is marked as plain
text.
| On unblock, set the gauge to 0: | ||
| ```go | ||
| metrics.BlockedPeer.With("peer_id", id.String(), "source", string(record.Source)).Set(0) | ||
| ``` |
There was a problem hiding this comment.
Insert a blank line before the fenced code block.
The current layout violates markdownlint MD031 (blanks-around-fences).
Proposed doc fix
On unblock, set the gauge to 0:
+
```go
metrics.BlockedPeer.With("peer_id", id.String(), "source", string(record.Source)).Set(0)</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 223-223: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/adr/adr-023-p2p-gater-ttl.md around lines 222 - 225, Add a blank line
immediately before the fenced code block that contains the Go snippet so the
markdown has an empty line preceding the triple backticks; specifically adjust
the block that shows metrics.BlockedPeer.With("peer_id", id.String(), "source",
string(record.Source)).Set(0) to be separated from the preceding paragraph by
one empty line to satisfy markdownlint MD031.
</details>
<!-- fingerprinting:phantom:triton:hawk:076a2914-53eb-4a56-a71c-b7031c85b262 -->
<!-- This is an auto-generated comment by CodeRabbit -->
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3268 +/- ##
==========================================
+ Coverage 62.48% 62.57% +0.08%
==========================================
Files 122 122
Lines 13012 13014 +2
==========================================
+ Hits 8131 8143 +12
+ Misses 3995 3990 -5
+ Partials 886 881 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/p2p/client.go`:
- Around line 85-89: The call to conngater.NewBasicConnectionGater currently
constructs a MapDatastore (datastore.NewMapDatastore()) even though the comments
state bans should be ephemeral; change the call to pass nil instead so
BasicConnectionGater uses its in-memory maps only—update the invocation of
NewBasicConnectionGater to pass nil rather than datastore.NewMapDatastore()
(reference: NewBasicConnectionGater).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Stale blocklist entries in the Badger datastore were causing the edennet-2 incident (#3267): fullnodes rejected every binary builder peer, header sync never initialized, and nodes fell back to DA-only sync. Replace the persistent BasicConnectionGater with a no-op variant: - Nil datastore → purely in-memory, nothing survives restart - Removed from libp2p host → no connection-level filtering - Removed setupBlockedPeers / setupAllowedPeers → nothing ever blocks - Instance kept only to satisfy go-header's Exchange API requirement Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
auricom
force-pushed
the
docs/adr-023-p2p-gater-blocklist
branch
from
10bfb60 to
0e137ca
Compare
The no-op gater implementation resolves the edennet-2 incident (#3267) without the TTL wrapper described in the ADR. The ADR is no longer relevant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Defines the full TTLConnectionGater feature: TTL-based peer blocklist with config/dynamic block sources, startup reconciliation, background sweep, and Prometheus metrics for blocklist observability.
Overview
Summary by CodeRabbit
Documentation
Chores