Skip to content

fix(network): remove every address book entry for a banned peer IP - #11173

Merged
mergify[bot] merged 3 commits into
ZcashFoundation:mainfrom
jiehuo100net:fix/11134-banned-peer-zombie-address-book
Aug 5, 2026
Merged

fix(network): remove every address book entry for a banned peer IP#11173
mergify[bot] merged 3 commits into
ZcashFoundation:mainfrom
jiehuo100net:fix/11134-banned-peer-zombie-address-book

Conversation

@jiehuo100net

@jiehuo100net jiehuo100net commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Motivation

Closes #11134.

After a peer IP is banned, an address book entry for that same IP on a different port could
survive the ban and then stay at the front of the reconnection order for the lifetime of the
process, producing a burst of attempted to add a banned peer addr to address book warnings on
every crawl cycle. This was observed on Mainnet shortly after NU6.3 activation, when the wave of
misbehavior bans made it easy to hit.

Solution

Three changes in zebra-network, matching the three defects in the issue:

  1. AddressBook::update() scans the whole book when removing a banned IP's entries.
    by_addr is an OrderedMap keyed by reconnection order, not grouped by IP, so entries for one
    IP can be separated by entries for other IPs. The previous
    skip_while(ip != banned).take_while(ip == banned) scan stops at the first entry for a
    different IP, leaving any later same-IP entry behind. Replaced with a filter over all keys.

  2. reconnection_peers() skips banned IPs. A surviving entry was selected as a candidate on
    every crawl; the ban check at the top of update() then rejected the resulting UpdateAttempt,
    so the entry was never marked AttemptPending and its position never changed. Filtering
    bans_by_ip here means a banned IP cannot be handed out even if an entry does reach the book.

  3. The rejected-change log moved from warn to debug. Remote peers can gossip a banned
    address at any time, so the volume of this message was effectively chosen by them.

Tests

One regression test in zebra-network/src/address_book/tests/vectors.rs,
ban_removes_every_entry_for_the_banned_ip. It builds a book where an unrelated IP sorts between
two entries that share the banned IP, and asserts that ordering before doing anything else —
with contiguous entries the test would pass against the old code and prove nothing. After the ban
it asserts the book holds exactly the unrelated entry, and that reconnection_peers() yields
exactly the unrelated address.

Negative controls, reverting the real implementation rather than reimplementing it, one change at a
time:

Implementation state ban_removes_every_entry_for_the_banned_ip
Both changes (this PR) pass
Change 1 reverted FAIL
Change 2 reverted pass
Both reverted (main) FAIL

Change 1 is pinned directly by the test. Change 2 is defence in depth: with change 1 in place the
surviving-entry state is not reachable through update(), so no test can isolate it. That is why
the two earlier tests are now one — the second could only fail when the first also failed.

Gates run locally on this branch, all in debug:

  • cargo fmt --all -- --check — clean
  • cargo clippy --workspace --all-targets -- -D warnings — clean, for both the default-release-binaries
    and the proptest-impl lightwalletd-grpc-tests zebra-checkpoints feature sets
  • cargo clippy -p zebra-network --all-targets -- -D warnings and cargo build -p zebra-network,
    for each of the four feature combinations CI runs per crate — clean
  • cargo test -p zebra-network --lib — 216 passed, 0 failed (217 before the two tests became one)
  • .github/scripts/validate-pr-changelogs.sh <merge-base> HEAD true fix false — exit 0

Specifications & References

Follow-up Work

The issue's third defect also has a structural half that this PR does not touch: when update()
returns None, CandidateSet::next() returns before the
MIN_OUTBOUND_PEER_CONNECTION_INTERVAL sleep, so queued crawler demand drains in a tight loop.
With the two changes above, a ban no longer drives that loop. The general fix belongs with the
CandidateSet/AddressBook refactor in #1976, as the issue notes.

AI Disclosure

  • No AI tools were used in this PR
  • AI tools were used: Claude Code, for the code, the tests, and this description. All of it was
    reviewed and verified by hand, including the negative-control runs in the table above.

PR Checklist

  • The PR title follows conventional commits format: type(scope): description
  • The PR follows the contribution guidelines.
  • This change was discussed in an issue or with the team beforehand.
  • The solution is tested.
  • The documentation and changelogs are up to date.

`by_addr` is ordered by reconnection order, not grouped by IP, so entries
for one IP can be separated by entries for other IPs. The ban path scanned
it with `skip_while(ip != banned).take_while(ip == banned)`, which stops at
the first entry for a different IP, so an entry for the banned IP on another
port could survive the ban.

Such an entry could never leave the book: `reconnection_peers()` did not
filter banned IPs, so it was selected as a candidate on every crawl, the ban
check in `update()` rejected the resulting `UpdateAttempt`, and its state
therefore never changed. It stayed at the front of the reconnection order
until the node restarted, logging a warning on every cycle.

Scan the whole book when removing a banned IP's entries, and skip banned IPs
in `reconnection_peers()` so a surviving entry cannot be selected. Log a
change rejected for a banned IP at `debug` instead of `warn`: remote peers
can gossip a banned address at any time, so they were choosing the level of
that message.
oxarbitrage
oxarbitrage previously approved these changes Aug 4, 2026

@oxarbitrage oxarbitrage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good, thanks!

Verbosity can be reduced a bit, i left some comments.

Comment thread zebra-network/src/address_book.rs Outdated
Comment thread zebra-network/src/address_book.rs Outdated
Comment thread zebra-network/CHANGELOG.md Outdated
Comment thread zebra-network/src/address_book/tests/vectors.rs Outdated
@mergify mergify Bot added the queued label Aug 5, 2026
@mergify

mergify Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-08-05 15:05 UTC · Rule: batched · triggered by rule move to any queue if GitHub Rulesets are satisfied
  • Checks passed · on draft merge queue: checking main (da29357) and #11173 together #11190
  • Merged2026-08-05 15:51 UTC · at fd2dc730c5806d1a84bde73e586361077fc5afcc · merge

This pull request spent 46 minutes 18 seconds in the queue, including 45 minutes 25 seconds running CI.

Required conditions to merge

@mergify
mergify Bot merged commit 9487509 into ZcashFoundation:main Aug 5, 2026
114 of 118 checks passed
@mergify mergify Bot removed the queued label Aug 5, 2026
jvff added a commit that referenced this pull request Aug 6, 2026
Link the banned-peer behavior to its implementation PR #11173 so the
release notes identify the reviewed change.
jvff added a commit that referenced this pull request Aug 6, 2026
Link the banned-peer logging change to its implementation PR #11173
so crate consumers can find its review context.
jvff added a commit that referenced this pull request Aug 6, 2026
Link the banned-peer behavior fix to its implementation PR #11173 so
crate consumers can find the implementation and tests.
jvff added a commit that referenced this pull request Aug 10, 2026
Link the banned-peer behavior to its implementation PR #11173 so the
release notes identify the reviewed change.
jvff added a commit that referenced this pull request Aug 10, 2026
Link the banned-peer logging change to its implementation PR #11173
so crate consumers can find its review context.
jvff added a commit that referenced this pull request Aug 10, 2026
Link the banned-peer behavior fix to its implementation PR #11173 so
crate consumers can find the implementation and tests.
jvff added a commit that referenced this pull request Aug 10, 2026
Link the banned-peer behavior to its implementation PR #11173 so the
release notes identify the reviewed change.
jvff added a commit that referenced this pull request Aug 10, 2026
Link the banned-peer logging change to its implementation PR #11173
so crate consumers can find its review context.
jvff added a commit that referenced this pull request Aug 10, 2026
Link the banned-peer behavior fix to its implementation PR #11173 so
crate consumers can find the implementation and tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Banned peer leaves a "zombie" address-book entry that spams attempted to add a banned peer addr WARNs every crawl cycle

2 participants