rpc: "addpeeraddress tried" return error on failure #28998

0xB10C · 2023-12-05T14:21:54Z

When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only addpeeraddress tried=true RPC would return { "success": true }. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue #28964.

This is fixed by new returning { "success": false, "error": "..." } for failed tried table additions. Since the address remaining in the new table can't be removed (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort), an error message is returned. This indicates to a user why the RPC failed and allows accounting for the extra address in the new table. This is done in the functional test for the getrawaddrman RPC.

Fixes #28964

DrahtBot · 2023-12-05T14:21:57Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stratospher, brunoerg, achow101
Concept ACK	pablomartin4btc, jonatack, amitiuttarwar
Approach ACK	sr-gi

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

src/rpc/net.cpp

test/functional/rpc_net.py

pablomartin4btc

Concept ACK

jonatack

Concept ACK

test/functional/rpc_net.py

stratospher · 2023-12-06T08:42:07Z

Concept ACK. it's nice to clearly define success/failure paths for addpeeraddress. hopefully after #29007, these rare collisions won't happen since a fixed nKey can be used.

0xB10C · 2023-12-06T10:48:25Z

Maybe get #29007 in first. ~~This allows us to remove the getrawaddrman-test retry logic (possibly do it there). Makes the getrawaddrman test simpler.~~ edit: I just noticed that 29007 already does that!

I've addressed the minor comments here, but I think it makes sense to draft this PR for now until #29007 is in. With #29007 we can test the collision case when addpeeraddress tried returns {"success": false, "error": "failed-adding-to-tried"} as discussed in #28998 (comment).

amitiuttarwar · 2023-12-11T19:52:23Z

approach ACK

(the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort)

since right now the only way addresses are kicked off is triggered by a collision, removing addresses from addrman would require introducing new test-only code paths. in addition to being a bigger effort, it would increase the surface area of potential issues in the future. eg. if someone tried to use it with mainnet code. when I was learning addrman, it took a while for me to wrap my head around the only-removed-by-collision logic (how it works, the reasoning, etc.), and I find it valuable for the tests to encourage devs familiarizing with the pattern that's used in the wild.

this is to provide some context behind my approach ACK. the solution proposed here is scoped to the RPC code, which is appropriate for improving the currently misleading RPC interface.

sr-gi

Approach ACK

It is a pity that "failed-adding-to-tried" cannot be tested on demand, however, this at least gets rid of the intermittent failure when a collision is found in tried.

I think the tests can be made more exhaustive though, check comment inline.

test/functional/rpc_net.py

0xB10C · 2024-02-09T09:35:05Z

Thanks for the review @sr-gi! You might want to take a look at #29007 first - this allows us to also test failed-adding-to-tried. Once this is merged, I'll continue to work on this PR.

0xB10C · 2024-03-12T14:47:00Z

Rebased on #29007, which allows us to check the failed-adding-to-tried case. I've also incorporated #28998 (review), and, as I was touching it anyway, replaced the log-assertions assert_debug_log on CheckAddrman: new X, tried Y, total Z started with getaddrmaninfo and getrawaddrman calls (which weren't available when the test was added). I've kept the existing getnodeaddresses calls, as these re-run the addrman checks.

Also, as I'm touching it already, I've dropped the now unused (since 2cc8ca1) mocktime from test_addpeeraddress(). Since #29007, the test does not leak into test_getrawaddrman() anymore.

This is ready for review again.

brunoerg · 2024-03-12T16:11:14Z

Concept ACK

I had this issue when writing #28869.

0xB10C · 2024-03-19T14:40:06Z

Rebased with #29639 merged due to #29639 (comment).

stratospher

tested ACK bb0f618.

not touched in this PR but addpeerinfo in test_addpeeraddress doesn't exist. 😳
if you retouch, do you want to include stratospher@09c849e since it only affects addpeeraddress and getrawaddrman and is a simple fix?

test/functional/rpc_net.py

src/rpc/net.cpp

When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue 28964. This is fixed by returning `{ "success": false, "error": "failed-adding-to-tried" }` for failed tried table additions. Since the address remaining in the new table can't be removed (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort), an error message is returned. This indicates to a user why the RPC failed and allows accounting for the extra address in the new table. Also: To check the number of addresses in each addrman table, the addrman checks were re-run and the log output of this check was asserted. Ideally, logs shouldn't be used as an interface in automated tests. To avoid asserting the logs, use the getaddrmaninfo and getrawaddrman RPCs (which weren't implemented when the test was added). Removing the "getnodeaddress" calls would also remove the addrman checks from the test, which could reduce the test coverage. To avoid this, these are kept.

Drops the mocktime added in fa4c683. Setting the mocktime in test_addpeeraddress() isn't needed anymore as it doesn't leak into test_getrawaddrman() anymore (since 2cc8ca1). test_getrawaddrman() clear's the addrman and sets it's own mocktime.

current check to make sure that detailed help for hidden RPC is displayed won't work because the assertion isn't sufficient. Even if unknown RPCs are passed, RPC names would still be present in node.help().

0xB10C · 2024-03-19T16:43:38Z

Addressed feedback and included stratospher@09c849e

stratospher

reACK 99954f9. 🚀

brunoerg

utACK 99954f9

achow101 · 2024-03-22T18:07:13Z

ACK 99954f9

99954f9 test: fix test to ensure hidden RPC is present in detailed help (stratospher) 0d01f6f test: remove unused mocktime in test_addpeeraddress (0xb10c) 6205466 rpc: "addpeeraddress tried" return error on failure (0xb10c) Pull request description: When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue bitcoin#28964. This is fixed by new returning `{ "success": false, "error": "..." }` for failed tried table additions. Since the address remaining in the new table can't be removed (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort), an error message is returned. This indicates to a user why the RPC failed and allows accounting for the extra address in the new table. This is done in the functional test for the `getrawaddrman` RPC. Fixes bitcoin#28964 ACKs for top commit: achow101: ACK 99954f9 stratospher: reACK 99954f9. 🚀 brunoerg: utACK 99954f9 Tree-SHA512: 2f1299410c0582ebc2071271ba789a8abed905f9a510821f77afbcf2a555ec31397578ea55cbcd162fb828be27afedd3246c7b13ad8883f2f745bb8e04364a76

99954f9 test: fix test to ensure hidden RPC is present in detailed help (stratospher) 0d01f6f test: remove unused mocktime in test_addpeeraddress (0xb10c) 6205466 rpc: "addpeeraddress tried" return error on failure (0xb10c) Pull request description: When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue bitcoin#28964. This is fixed by new returning `{ "success": false, "error": "..." }` for failed tried table additions. Since the address remaining in the new table can't be removed (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort), an error message is returned. This indicates to a user why the RPC failed and allows accounting for the extra address in the new table. This is done in the functional test for the `getrawaddrman` RPC. Fixes bitcoin#28964 ACKs for top commit: achow101: ACK 99954f9 stratospher: reACK 99954f9. 🚀 brunoerg: utACK 99954f9 Tree-SHA512: 2f1299410c0582ebc2071271ba789a8abed905f9a510821f77afbcf2a555ec31397578ea55cbcd162fb828be27afedd3246c7b13ad8883f2f745bb8e04364a76 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

- Add test for adding valid address to new table first - Add critical test case for 'failed-adding-to-tried' error path - Add test for already-present new address failing with 'failed-adding-to-new' - Add collision test case with grinded address 1.2.5.45 - Fix test expectations to match actual addrman state These changes complete the Bitcoin backport by including all the test coverage for the new error handling paths in addpeeraddress RPC.

DrahtBot added the RPC/REST/ZMQ label Dec 5, 2023

0xB10C mentioned this pull request Dec 5, 2023

rpc_net.py intemittent failure / assert_equal(len(getrawaddrman[table_name]), len(table_info["entries"])) #28964

Closed

maflcko reviewed Dec 5, 2023

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

test/functional/rpc_net.py Outdated Show resolved Hide resolved

pablomartin4btc reviewed Dec 5, 2023

View reviewed changes

jonatack reviewed Dec 5, 2023

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

test/functional/rpc_net.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 6, 2023

test: create deterministic addrman in the functional tests #29007

Merged

0xB10C marked this pull request as draft December 6, 2023 10:48

0xB10C force-pushed the 2023-12-addpeeraddress-return-error branch from cf54bc7 to 4f25184 Compare December 6, 2023 11:09

DrahtBot added the CI failed label Jan 16, 2024

sr-gi reviewed Feb 7, 2024

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Mar 11, 2024

0xB10C force-pushed the 2023-12-addpeeraddress-return-error branch from 4f25184 to fbb02be Compare March 12, 2024 13:13

DrahtBot removed the Needs rebase label Mar 12, 2024

0xB10C force-pushed the 2023-12-addpeeraddress-return-error branch from fbb02be to 4477a37 Compare March 12, 2024 14:32

0xB10C marked this pull request as ready for review March 12, 2024 14:47

0xB10C changed the title ~~rpc: addpeeraddress tried return error on failure~~ rpc: "addpeeraddress tried" return error on failure Mar 12, 2024

0xB10C mentioned this pull request Mar 12, 2024

test: #29007 follow ups #29636

Merged

brunoerg mentioned this pull request Mar 12, 2024

feature_asmap.py intermittent fail ( AssertionError: [node 0] Expected messages "['CheckAddrman: new 2, tried 2, total 4 started', 'CheckAddrman: completed']" does not partially match log:) #29634

Closed

DrahtBot mentioned this pull request Mar 12, 2024

test: fix intermittent failures with test=addrman #29639

Merged

DrahtBot removed the CI failed label Mar 13, 2024

0xB10C force-pushed the 2023-12-addpeeraddress-return-error branch from 4477a37 to bb0f618 Compare March 19, 2024 14:39

stratospher reviewed Mar 19, 2024

View reviewed changes

test/functional/rpc_net.py Outdated Show resolved Hide resolved

test/functional/rpc_net.py Outdated Show resolved Hide resolved

test/functional/rpc_net.py Outdated Show resolved Hide resolved

DrahtBot requested review from amitiuttarwar, brunoerg, jonatack, pablomartin4btc and sr-gi March 19, 2024 15:21

brunoerg reviewed Mar 19, 2024

View reviewed changes

src/rpc/net.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from brunoerg March 19, 2024 16:25

0xB10C and others added 3 commits March 19, 2024 17:38

test: fix test to ensure hidden RPC is present in detailed help

99954f9

current check to make sure that detailed help for hidden RPC is displayed won't work because the assertion isn't sufficient. Even if unknown RPCs are passed, RPC names would still be present in node.help().

0xB10C force-pushed the 2023-12-addpeeraddress-return-error branch from bb0f618 to 99954f9 Compare March 19, 2024 16:43

stratospher reviewed Mar 19, 2024

View reviewed changes

brunoerg approved these changes Mar 21, 2024

View reviewed changes

achow101 merged commit 2795e89 into bitcoin:master Mar 22, 2024

brunoerg mentioned this pull request Mar 27, 2024

contrib: add test for bucketing with asmap #28869

Closed

0xB10C deleted the 2023-12-addpeeraddress-return-error branch February 18, 2025 15:38

DashCoreAutoGuix mentioned this pull request Jul 31, 2025

Merge bitcoin/bitcoin#28998: rpc: "addpeeraddress tried" return error on failure DashCoreAutoGuix/dash#734

Closed

rpc: "addpeeraddress tried" return error on failure #28998

rpc: "addpeeraddress tried" return error on failure #28998

Conversation

0xB10C commented Dec 5, 2023

Uh oh!

DrahtBot commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stratospher commented Dec 6, 2023

Uh oh!

0xB10C commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitiuttarwar commented Dec 11, 2023

Uh oh!

sr-gi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

0xB10C commented Feb 9, 2024

Uh oh!

0xB10C commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoerg commented Mar 12, 2024

Uh oh!

0xB10C commented Mar 19, 2024

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xB10C commented Mar 19, 2024

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Mar 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DrahtBot commented Dec 5, 2023 •

edited

Loading

0xB10C commented Dec 6, 2023 •

edited

Loading

0xB10C commented Mar 12, 2024 •

edited

Loading