rpc: addpeeraddress: throw on invalid IP #33430

pull john-moffett wants to merge 1 commits into bitcoin:master from john-moffett:rpc-addpeeraddress-error changing 2 files +22 −17

john-moffett commented at 6:52 PM on September 18, 2025: contributor

Right now we return an opaque {"success" : false} in addpeeraddress for an empty or invalid IP. This changes it to throw RPC_CLIENT_INVALID_IP_OR_SUBNET with the error message Invalid IP address. Tests updated to match.

rpc: addpeeraddress: throw on invalid IP

Throw RPC_CLIENT_INVALID_IP_OR_SUBNET when LookupHost(addr, false) fails
in addpeeraddress. This aligns with setban/addconnection and avoids the
opaque {"success": false} result for input errors. The JSON {success,
error?} object remains for addrman outcomes only. Update test to match.

316a0c5132

DrahtBot added the label RPC/REST/ZMQ on Sep 18, 2025
DrahtBot commented at 6:52 PM on September 18, 2025: 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/33430.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK vasild, sipa, pablomartin4btc, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
vasild approved
vasild commented at 12:22 PM on September 23, 2025: contributor

ACK 316a0c513278d53cb25f42ea502d20691962aad6

As an alternative, we could use the error field in the response instead of throwing an exception. The error field is currently not set in master if an invalid IP address is provided.
sipa commented at 12:47 PM on September 23, 2025: member

utACK 316a0c513278d53cb25f42ea502d20691962aad6
pablomartin4btc commented at 3:10 PM on September 24, 2025: member
tACK 316a0c513278d53cb25f42ea502d20691962aad6

master:
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc addpeeraddress "" 8883
{
  "success": false
}
```
This branch:
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc addpeeraddress "" 8883
error code: -30
error message:
Invalid IP address
```
I agree with @vasild with the suggested alternative, not strong opinion.

Would it be useful for the user to differentiate from an empty address vs a provided invalid IP (which obviously includes an empty value) in the error message?
john-moffett commented at 4:04 PM on September 24, 2025: contributor

Thanks, all! @vasild

As an alternative, we could use the error field in the response instead of throwing an exception.

I tried to use the same pattern as setban does, which I think has a clean boundary between an invalid input and a problem that occurred despite valid input. To me, that category separation makes sense. @pablomartin4btc

Would it be useful for the user to differentiate from an empty address vs a provided invalid IP (which obviously includes an empty value) in the error message?

My gut says it's probably overkill, but I'm certainly happy to change it if others disagree.
BrunoSampaioDev commented at 12:15 AM on September 25, 2025: none

LGTM
achow101 commented at 10:37 PM on September 25, 2025: member

ACK 316a0c513278d53cb25f42ea502d20691962aad6
achow101 merged this on Sep 25, 2025
achow101 closed this on Sep 25, 2025