test: Verify unwelcome RPC clients are rejected before reading their requests #35866

pull winterrdog wants to merge 1 commits into bitcoin:master from winterrdog:test/verify-early-ip-rejection changing 1 files +59 −0

winterrdog commented at 2:03 PM on August 2, 2026: contributor

this is a follow-up PR from a suggestion in this comment

it adds a unit test that confirms that clients not permitted by -rpcallowip are rejected immediately after accept(), before any request bytes are read from the socket

specifically, the test checks that: no request is ever dispatched to the server's request handler, the connection is closed without any response to the client, no HTTPRemoteClient is ever registered for it, and the client's request bytes are left completely unread in the socket's receive buffer

test: verify disallowed RPC clients are rejected upon `accept()`

it adds a unit test verifying that clients not permitted by
`-rpcallowip` are rejected immediately after `accept()`, before any of
their request bytes are read from the socket.

Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>

89353cda70

DrahtBot added the label Tests on Aug 2, 2026
DrahtBot commented at 2:03 PM on August 2, 2026: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35866.



Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

ACK w0xlt, benediktrk

Concept ACK pinheadmz

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
pinheadmz commented at 2:46 PM on August 2, 2026: member

concept ACK, more tests are always good ;-) I'm curious to see the corecheck report to see if any new lines are covered
w0xlt commented at 4:02 PM on August 2, 2026: contributor

ACK 89353cda701d7704751b922ab545b86865736b16
DrahtBot requested review from pinheadmz on Aug 2, 2026
benediktrk commented at 7:02 PM on August 2, 2026: none

utACK 89353cda701d7704751b922ab545b86865736b16
benediktrk commented at 9:33 AM on August 3, 2026: none

tACK 89353cda701d7704751b922ab545b86865736b16

Tested locally on WSL/Ubuntu. Re-built branch using CMake and verified that the new unit test passes successfully (./bin/test_bitcoin -t "httpserver_tests").
m3dwards commented at 2:32 PM on August 3, 2026: contributor

This fails on the line BOOST_REQUIRE(server.InitHTTPAllowList()); on corecheck.dev but I'm not really sure why.

Error is: /tmp/bitcoin/src/test/httpserver_tests.cpp:798:26: error: no member named 'InitHTTPAllowList' in 'http_bitcoin::HTTPServer'
maflcko commented at 3:28 PM on August 3, 2026: member
@m3dwards corecheck uses git rebase --onto "$BASE_COMMIT" "$PR_MERGE_BASE". I presume to "backport" a pull request to the latest master coverage baseline? If yes, this has several issues:
- It doesn't work with subtree bumps, see also https://corecheck.dev/bitcoin/bitcoin/pulls/35781
- It doesn't generally work when commits are removed from a pull request that are required (as is the case here)
I think the correct approach would be to merge the pull request on top of the latest master coverage.

Of course this could mean that additional coverage is included in the diff, but then one could instead do: When a new push to master happens, schedule a new master coverage task before any pull coverage tasks.

If you want to avoid running a new coverage task on every master commit to reduce load, you could check and optionally schedule a new master coverage only when a pull request is pushed to.
winterrdog commented at 3:29 PM on August 3, 2026: contributor

Error is: /tmp/bitcoin/src/test/httpserver_tests.cpp:798:26: error: no member named 'InitHTTPAllowList' in 'http_bitcoin::HTTPServer'

the coverage report on corecheck is using an older master commit as its base: https://github.com/bitcoin/bitcoin/commit/9611a356035be531d62bfc40879f388d5dc359c4. that commit was merged before the changes that introduced HTTPServer::InitHTTPAllowList(). the method was added later in https://github.com/bitcoin/bitcoin/commit/556988790a7f961693a8fd93f73725baea66476a, so when the test is compiled against the older base, HTTPServer does not have that member, which explains the error

~~about why corecheck is using an older master commit here, i cannot tell why so. other later PRs seem to be using a newer base, though~~ - i know why, see the next comment below
winterrdog commented at 3:45 PM on August 3, 2026: contributor
It doesn't generally work when commits are removed from a pull request that are required (as is the case here)
correct!

i forgot to keep up with the upstream changes from master. i made this PR's branch off of an older base master (https://github.com/bitcoin/bitcoin/commit/556988790a7f961693a8fd93f73725baea66476a) commit on a local fork. the issue is squarely on me. i think a rebase on my local branch would fix this
winterrdog force-pushed on Aug 3, 2026
maflcko commented at 4:33 PM on August 3, 2026: member

no, it is not an issue with this pull. The prior commit compiled fine, you can reset to it. the issue is inside corecheck itself.
winterrdog commented at 5:19 PM on August 3, 2026: contributor

@maflcko

i had started resetting the branch, but i ran into an unexpected power outage midway and was offline for a while. i noticed the new corecheck runs are almost done now. since resetting the branch would likely trigger another run, do you still think it is worth rolling back to the old commit (89353cda701d7704751b922ab545b86865736b16) ?
maflcko commented at 5:40 PM on August 3, 2026: member

Should be fine to reset to avoid re-review. The next corecheck run should be passing as well.
winterrdog force-pushed on Aug 3, 2026