Repository navigation
node: wait for a --proxy seed answer before catch-up - #1102
Conversation
There was a problem hiding this comment.
Review
Requesting changes. await_proxy_seed_book blocks run_p2p until the shared book is non-empty and the peer snapshot has no AddrFetch session. Shutdown is the only other return. The call sits before catch-up and before the RPC handle is created, whenever --proxy queued seeds into an empty book (run.rs:684).
The PR says that if no answer comes, the existing awaiting_dialable path is unchanged. That path is unreachable from this wait: None is returned only from shutdown.cancelled(). A proxy that never answers re-queues the seeds every 60s and never opens catch-up or RPC. The old behavior exited catch-up incomplete in well under a second and returned Ok(()).
On this base the success condition is also unreachable for a normal seed reply. on_addrv2 still punish_disconnects an addr-fetch message with n > 1 and returns before learn_addrv2. v1 ADDR is not stored. A DNS seed's getaddr is that multi-address message, so the book stays empty and the re-queue repeats. The live book=1000 result in the PR body used unmerged #1101, which is not in this parent. Merging this alone turns the 60ms exit into a permanent wait.
| Severity | Location | Finding |
|---|---|---|
| High | crates/rbitcoin-node/src/run.rs:1634 |
No deadline. Success requires a non-empty book and no live addr-fetch session. On master a Core-sized getaddr never fills the book (#1101). A dead proxy never fills it either. |
| High | crates/rbitcoin-node/src/run.rs:1634 |
A one-address self-announcement can be stored while n == 1 leaves the session up. Once that session leaves the snapshot, the wait returns with only that row. The comment at run.rs:1608 treats "session ended" as "getaddr applied". On this base those are different events. Incomplete catch-up still returns Ok(()) at the clean-exit path. |
| Medium | crates/rbitcoin-node/src/run.rs:3036 |
proxy_seed_wait_requeues_until_an_answer_fills_the_book inserts an address from the dial channel. No session exists for that half, and nothing calls run_p2p, on_addrv2, or on_addr_list. Deleting the call at run.rs:684 and leaving the helper keeps the test green. The hold/unregister half does lock "a live AddrFetch session holds the wait". |
| Medium | crates/rbitcoin-node/src/run.rs:1625 |
Every retry calls queue_proxy_seed_addrfetch with no check for a live session or an in-flight dial. The poll reads snapshot(), so a re-queue that is connecting in the gap after the previous session unregistered can satisfy !book.is_empty() && !answering before the next session is registered. |
Please give the wait a bound that falls through to awaiting_dialable when no answer arrives, and stack it on the addr-fetch learn fix (or treat the book as filled only after a multi-address reply is actually learned). The re-queue, the hold while a session is registered, and the shutdown return are the right shape once that bound exists.
3e745d1 to
1b9167c
Compare
|
Thanks. Addressed in
Live, fresh signet through Tor |
An addr-fetch session (the --proxy DNS seeds, -seednode) that got more than one address back called punish_disconnect and returned before learn_addrv2, so the reply never reached AddrMan and the seed took a full ban score. The getaddr also never credited the token bucket, so even a learned reply would have kept one address. Follow Core ProcessMessage ADDR/ADDRV2: sending getaddr adds MAX_ADDR_TO_SEND tokens, the reply is learned, and the addr-fetch session then ends with request_disconnect and no misbehavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-up. A v1 addr reply to an addr-fetch getaddr was only a disconnect and never reached AddrMan; it is now converted and learned. An addr-fetch session no longer relays what it learns: Core sets m_getaddr_sent and does not relay a getaddr reply, and a run of one-address addrv2 messages fell through to the neighbor relay. learn_addrfetch_reply is the one path for both. The test checks at least 900 of the 1000 addresses are learned, that a wants_addrv2 neighbor gets nothing relayed, that a one-address self-announce is learned without ending the session, and that the v1 reply reaches the book. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With --proxy and an empty book, startup queues the DNS seeds as SOCKS addr-fetch dials and then reads its dial targets right away. No seed can answer in that time, so catch-up stayed incomplete and the node shut down about 60 ms in, saving no peers. Wait until the shared book holds an answer and no addr-fetch session is still live (a seed announces itself before its getaddr reply). Re-queue the seeds every 60 s until then. Shutdown ends the wait. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review follow-up. The wait had no deadline, so a proxy that never answered held run_p2p before catch-up and RPC for good. It now gives up after PROXY_SEED_WAIT (180 s) and the empty book takes the existing awaiting_dialable path. It also ended on "book non-empty and no addr-fetch session live", which a stored one-address self-announce can satisfy. PeerHub now counts addr-fetch getaddr replies learned (more than one address, bumped only in learn_addrfetch_reply), and the wait ends when that count rises. Seeds are re-queued only while no addr-fetch session is live. Stacked on the addr-fetch learn fix (reardencode#1101): without it no seed reply is learned and the count never rises. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1b9167c to
fb9ddd5
Compare
There was a problem hiding this comment.
Review
Reviewed 94620029 and fb9ddd59 on top of #1101 (507bda95). The last change request is addressed.
await_proxy_seed_book records addrfetch_replies() at entry and returns the book only when that counter has moved and the book is non-empty. note_addrfetch_reply runs only from learn_addrfetch_reply when n > 1, so a one-address self-announce does not end the wait. Re-queue runs every 60s only while no AddrFetch session is in the snapshot. After PROXY_SEED_WAIT (180s), and on shutdown, it returns None, and an empty book still takes awaiting_dialable. The signal handler is installed before this wait.
One residual, not blocking: the counter increments even when learn_addrv2 stores nothing (peer.rs around the n > 1 bump, using the pre-conversion list.len() for v1). A self-announce already in the book plus a multi-address message of undialable rows then satisfies replies() > start && !book.is_empty(). A Core seed reply that actually inserts rows is unaffected. The node test drives its own atomic and add_addr; the net test is what pins the real counter on a learned reply.
Problem
With
--proxyand an empty book,run_p2pqueues the DNS seeds as SOCKS addr-fetch dials and then reads its dial targets immediately. No seed can answer that fast, so catch-up is incomplete and the node exits about 60 ms in, saving 0 peers:Fix
await_proxy_seed_bookruns before catch-up:getaddrreply has been learned since the wait began.PeerHub::addrfetch_repliescounts those replies (more than one address, bumped only inlearn_addrfetch_reply), so a stored self-announce does not end the wait.PROXY_SEED_WAIT(180 s) and returnsNone, so the empty book takes the existingawaiting_dialablepath. Shutdown also returnsNone.Tests
proxy_seed_wait_ends_on_a_learned_reply_shutdown_or_deadline(crates/rbitcoin-node/tests/run.rs): a book filled only by a self-announce, with a live addr-fetch session, hits the deadline with no re-queue. With no session, it re-queues, and it returns once a reply is counted. Shutdown returnsNone.addrfetch_learns_the_getaddr_reply_then_disconnects(crates/rbitcoin-net/tests/peer/mod.rs) also pins the counter: 0 after a self-announce, 1 after theaddrv2reply, 2 after the v1 reply.run_p2pjourney for this path: the regtest harness (end_of_ibd.rs) has no DNS seeds, soproxy_seeds_queuedis never set there.--proxy: the first seed attempt got no answer, the wait re-queued at 60 s, andseed addrfetch filled the book (book=1000)followed, then catch-up.🤖 Generated with Claude Code