Fix get_data/get_client_filters livelock in tx and filter out protocols.#5
Open
echennells wants to merge 1 commit into
Open
Fix get_data/get_client_filters livelock in tx and filter out protocols.#5echennells wants to merge 1 commit into
echennells wants to merge 1 commit into
Conversation
handle_receive_get_data and handle_receive_get_filters drive the first send synchronously and then return false to drop the channel subscription. On a request that completes synchronously (a get_data carrying no transaction items, or a get_filters yielding an empty ancestry) the send reaches its terminal branch and resubscribes via SUBSCRIBE_CHANNEL while still inside the channel subscriber notification that invoked the handler. The channel subscriber is a std::list iterated in place; the re-entrant subscribe appends to that list, and the handler's false return advances the iterator into the appended element, re-invoking the handler on the same message indefinitely and pegging the channel strand (the author's flagged "BUGBUG: registration race"). Post the initial send so it runs on a later strand cycle, outside the subscriber notification. The subscription is still dropped on receipt, the terminal resubscribe still rides the completion of the last asynchronous send, and only the synchronous-completion path is deferred.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Fixes the re-entrant livelock in
protocol_transaction_out_106(get_data) and its siblingprotocol_filter_out_70015(get_client_filters) by posting the initial send drive, rather than removing the subscription-drop backpressure. This is the corrected approach after the earlier direction (drop the resubscribe and stay subscribed) was rejected upstream as a memory-exhaustion DoS.Root cause
handle_receive_get_data/handle_receive_get_filterscallsend_transaction/send_filtersynchronously, thenreturn falseto drop the channel subscription (backpressure: don't accept another request of this type while one is in flight). The re-arm happens later, when the send walk reaches its terminal branch and callsSUBSCRIBE_CHANNEL.The channel subscriber (
network::unsubscriber) stores handlers in astd::listthatnotify()iterates in place: for each handler,false→erase(it)(advance),true→++it.subscribe()is an unconditionalpush_back.For a request that completes synchronously — a
get_datawith no transaction-type items, or aget_filterswhose ancestry is empty — the send reaches its terminal branch and resubscribes while still inside thenotify()that invoked the handler. The re-entrantpush_backappends to the list being iterated; the handler'sfalsereturn thenerase-advances the iterator straight into the just-appended handler, which re-fires on the same message and re-appends — an infinite loop that pegs the channel strand at 100% CPU. This is the spot the author flagged with// BUGBUG: registration race.Fix
Post the first
send_transaction/send_filtercall so it runs on a later strand cycle, outside the subscriber notification:Everything else is unchanged: the handler still
return falses on receipt (subscription dropped while processing), and the terminal resubscribe still rides the completion of the last asynchronousSEND. Only the synchronous-completion path is deferred, which is the sole path that re-enterednotify().Why not "stay subscribed" (
return true+ delete resubscribe)That removes tx/filter's only backpressure. Because
SENDcompletions are asynchronous, a peer firing repeated requests would spawn unbounded concurrent send walks (each retaining the request pointer) → memory-exhaustion DoS.protocol_block_out_106can safely stay subscribed only because it substitutes an explicit bounded accumulator (backlog_capped atmax_inventory, single idle-guarded drain); tx/filter have no such structure, so drop-and-resubscribe is their throttle and must be preserved.Preserves the intended design
SEND's completion — no strand gap after the last send.Note: deferring the first drive opens a one-strand-turn window in which the subscription is absent, so a request pipelined into that window is dropped. This is strictly narrower than the subscriber gap the multi-item path already has across its entire async send chain, and is consistent with the one-request-at-a-time design (a peer feeds the next request after the prior response); it is not a new behavior class.
Trigger / severity
Remote, but gated:
protocol_transaction_out_106is attached only on relay channels where the peer negotiated bip37 (session_peer.ippattach_protocols);protocol_filter_out_70015only when bip157 filters are enabled and negotiated. Deterministically reproduced on testnet3 (a singleget_dataof block-only inventory pegs a worker thread).Testing
getdatano longer pegs a core; tx serving unchanged (byte-identical response, repeatable); getdata-flood does not accumulate concurrent send walks (the DoS case not previously exercised).Staging PR on the fork for CI + testrig iteration; not for upstream in this form.