rpc: “addpeeraddress tried” return error on failure #28998

pull 0xB10C wants to merge 3 commits into bitcoin:master from 0xB10C:2023-12-addpeeraddress-return-error changing 2 files +51 −21
  1. 0xB10C commented at 2:21 pm on December 5, 2023: contributor

    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

  2. DrahtBot commented at 2:21 pm on December 5, 2023: contributor

    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.

  3. DrahtBot added the label RPC/REST/ZMQ on Dec 5, 2023
  4. in src/rpc/net.cpp:947 in cf54bc7aaa outdated 
    943@@ -944,7 +944,7 @@ static RPCHelpMan getnodeaddresses()
944 static RPCHelpMan addpeeraddress()
945 {
946     return RPCHelpMan{"addpeeraddress",
947-        "\nAdd the address of a potential peer to the address manager. This RPC is for testing only.\n",
948+        "\nAdd the address of a potential peer to an address manager table. This RPC is for testing only.\n",
    maflcko commented at 2:26 pm on December 5, 2023:

    nit: no need for those

    0        "Add the address of a potential peer to an address manager table. This RPC is for testing only.",
  5. in test/functional/rpc_net.py:509 in cf54bc7aaa outdated 
    505+                    self.log.debug(f"Failed to add {table_info['entries'][1]['address']} to {table_name} table")
506+                    # When trying to add an address to the tried table, it's first added to the
507+                    # new table and then moved to the tried table. If the move to the tried table
508+                    # fails, the address still remains in the new table.
509+                    if table_name == "tried" and result["error"] == "faild-adding-to-tried":
510+                        entry = table_info["entries"][1]
    maflcko commented at 2:30 pm on December 5, 2023:

    Is a copy the right way, or does it need to be removed from the expected tried table?

    Maybe add steps to reproduce the test code?

    0xB10C commented at 10:11 am on December 6, 2023:

    Is a copy the right way, or does it need to be removed from the expected tried table?

    Just removing the address from the expected tried table does not work. It’s still in the new table. So the question would rather be: “Do we need to retry adding the address to the tried table?”. I guess that’s debatable. We’re only running into this case very rarely when there’s a collision - all other runs test that this works. This should be solved with #29007.

    Maybe add steps to reproduce the test code?

    Yes, I think we should have that. Especially with typos like #28998 (review). Due to addrman non-determinism, this isn’t really doable. Again, #29007 helps here.

    0xB10C commented at 1:23 pm on March 12, 2024:
    Resolving this with the rebase on-top of #29007 as this code was removed in 2cc8ca19f4185490f30a49516c890b2289fbab71. The addresses are now added in seed_addrman() and (currently) can’t produce a collision.
  6. pablomartin4btc commented at 6:06 pm on December 5, 2023: member
    Concept ACK
  7. in test/functional/rpc_net.py:508 in cf54bc7aaa outdated 
    504                 else:
505+                    self.log.debug(f"Failed to add {table_info['entries'][1]['address']} to {table_name} table")
506+                    # When trying to add an address to the tried table, it's first added to the
507+                    # new table and then moved to the tried table. If the move to the tried table
508+                    # fails, the address still remains in the new table.
509+                    if table_name == "tried" and result["error"] == "faild-adding-to-tried":
    jonatack commented at 9:24 pm on December 5, 2023: 
    0                    if table_name == "tried" and result["error"] == "failed-adding-to-tried":

    (edit, seems this should be covered by a test in every case)

  8. in test/functional/rpc_net.py:499 in cf54bc7aaa outdated 
    494@@ -495,11 +495,20 @@ def check_getrawaddrman_entries(expected):
495             # added. Incrementing the port changes the position in the new table bucket (bucket
496             # stays the same) and changes both the bucket and the position in the tried table.
497             while True:
498-                if node.addpeeraddress(address=table_info["entries"][1]["address"], port=port, tried=table_name == "tried")["success"]:
499+                result = node.addpeeraddress(address=table_info["entries"][1]["address"], port=port, tried=table_name == "tried")
500+                if result["success"]:
    jonatack commented at 9:25 pm on December 5, 2023:

    Not sure, maybe test explicitly for True rather than just truthy

    0                if result["success"] is True:
  9. jonatack commented at 9:25 pm on December 5, 2023: contributor
    Concept ACK
  10. stratospher commented at 8:42 am on December 6, 2023: contributor
    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.
  11. 0xB10C commented at 10:48 am on December 6, 2023: contributor

    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 (review).

  12. 0xB10C marked this as a draft on Dec 6, 2023
  13. 0xB10C force-pushed on Dec 6, 2023
  14. amitiuttarwar commented at 7:52 pm on December 11, 2023: contributor

    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.

  15. DrahtBot added the label CI failed on Jan 16, 2024
  16. in test/functional/rpc_net.py:369 in 4f251842dd outdated 
    335@@ -336,7 +336,7 @@ def test_addpeeraddress(self):
336 
337         self.log.debug("Test that adding an already-present tried address to the new and tried tables fails")
338         for value in [True, False]:
339-            assert_equal(node.addpeeraddress(address="1.2.3.4", tried=value, port=8333), {"success": False})
340+            assert_equal(node.addpeeraddress(address="1.2.3.4", tried=value, port=8333), {"success": False, "error": "failed-adding-to-new"})
    sr-gi commented at 9:39 pm on February 7, 2024:

    This is a non-exhaustive case now. There are two potential error outcomes that can be triggered on demand, from which only one is being tested:

    • a) The address is already in new and we are trying to add it again (to any table)
    • b) The address is already in tried and we are trying to add it again (to any table)

    In this case, only b) is being tested: we do so both with and without the “try to add to tried” flag, but the reality is that the reason why the function is failing is the entry is already present in tried, even when we try to add it to new. Given how the addrman works, we cannot really try to add things to tried multiple times on demand, given it would need to be added to new first and that would fail.

    So I think the only way to properly test this would be to add two different addresses twice. e.g:

    0# Try that the same address cannot be added twice to `new`
1assert_equal(node.addpeeraddress(address="1.2.3.5", tried=False, port=8333), {"success": True})
2assert_equal(node.addpeeraddress(address="1.2.3.5", tried=False, port=8333), {"success": False, "error": "failed-adding-to-new"})
3# Try that the same address cannot be added twice to `tried`
4assert_equal(node.addpeeraddress(address="1.2.3.6", tried=True, port=8333), {"success": True})
5assert_equal(node.addpeeraddress(address="1.2.3.6", tried=True, port=8333), {"success": False, "error": "failed-adding-to-new"})
    0xB10C commented at 2:33 pm on March 12, 2024:
    Done!
  17. sr-gi commented at 9:44 pm on February 7, 2024: member

    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.

  18. 0xB10C commented at 9:35 am on February 9, 2024: contributor
    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.
  19. DrahtBot added the label Needs rebase on Mar 11, 2024
  20. 0xB10C force-pushed on Mar 12, 2024
  21. DrahtBot removed the label Needs rebase on Mar 12, 2024
  22. 0xB10C force-pushed on Mar 12, 2024
  23. 0xB10C commented at 2:47 pm on March 12, 2024: contributor

    Rebased on #29007, which allows us to check the failed-adding-to-tried case. I’ve also incorporated #28998#pullrequestreview-1868852542, 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 2cc8ca19f4185490f30a49516c890b2289fbab71) mocktime from test_addpeeraddress(). Since #29007, the test does not leak into test_getrawaddrman() anymore.

    This is ready for review again.

  24. 0xB10C marked this as ready for review on Mar 12, 2024
  25. 0xB10C renamed this:
    rpc: addpeeraddress tried return error on failure
    rpc: "addpeeraddress tried" return error on failure
    on Mar 12, 2024
  26. brunoerg commented at 4:11 pm on March 12, 2024: contributor

    Concept ACK

    I had this issue when writing #28869.

  27. DrahtBot removed the label CI failed on Mar 13, 2024
  28. in test/functional/rpc_net.py:373 in 76536d8345 outdated 
    379-        self.log.debug("Test that adding a second address, this time to the new table, succeeds")
380+            assert_equal(node.addpeeraddress(address="1.2.3.4", tried=value, port=8333), {"success": False, "error": "failed-adding-to-new"})
381+        assert_equal(len(node.getnodeaddresses(count=0)), 2)
382+
383+        self.log.debug("Test that adding an address, which collides with the address in tried table, fails")
384+        colliding_address = "1.2.5.45"  # grinded address that produrces a tried-table collision
    stratospher commented at 4:36 am on March 15, 2024:
    edc8cb64: nice one! nit: small typo - s/produrces/produces
  29. 0xB10C force-pushed on Mar 19, 2024
  30. 0xB10C commented at 2:40 pm on March 19, 2024: contributor
    Rebased with #29639 merged due to #29639 (review).
  31. in test/functional/rpc_net.py:362 in edc8cb64f9 outdated 
    362-            addrs = node.getnodeaddresses(count=0)  # getnodeaddresses re-runs the addrman checks
363-            assert_equal(len(addrs), 1)
364-            assert_equal(addrs[0]["address"], "1.2.3.4")
365-            assert_equal(addrs[0]["port"], 8333)
366+        addrman = node.getrawaddrman()
367+        assert_equal(len(list(addrman["new"].values())), 1)
    stratospher commented at 3:12 pm on March 19, 2024:

    edc8cb64:

    0        assert_equal(len(addrman["new"]), 1)
  32. in test/functional/rpc_net.py:346 in edc8cb64f9 outdated 
    339@@ -340,26 +340,52 @@ def test_addpeeraddress(self):
340         assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=-1)
341         assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].addpeeraddress, address="1.2.3.4", port=65536)
342 
343+        self.log.debug("Test that adding a valid address to the new table succeeds")
344+        assert_equal(node.addpeeraddress(address="1.0.0.0", tried=False, port=8333), {"success": True})
345+        addrman = node.getrawaddrman()
346+        assert_equal(len(list(addrman["tried"].values())), 0)
    stratospher commented at 3:13 pm on March 19, 2024:

    edc8cb64: why not?

    0        assert_equal(len(addrman["tried"]), 0)

    (left 1 more similar suggestion)

  33. stratospher commented at 3:21 pm on March 19, 2024: contributor

    tested ACK bb0f618.

    not touched in this PR but addpeerinfo in test_addpeeraddress doesn’t exist. 😳 if you retouch, do you want to include https://github.com/stratospher/bitcoin/commit/09c849e2f544934b121f5fdf62cb974b67b6b962 since it only affects addpeeraddress and getrawaddrman and is a simple fix?

  34. DrahtBot requested review from sr-gi on Mar 19, 2024
  35. DrahtBot requested review from jonatack on Mar 19, 2024
  36. DrahtBot requested review from pablomartin4btc on Mar 19, 2024
  37. DrahtBot requested review from amitiuttarwar on Mar 19, 2024
  38. DrahtBot requested review from brunoerg on Mar 19, 2024
  39. in src/rpc/net.cpp:964 in edc8cb64f9 outdated 
    959@@ -960,7 +960,8 @@ static RPCHelpMan addpeeraddress()
960         RPCResult{
961             RPCResult::Type::OBJ, "", "",
962             {
963-                {RPCResult::Type::BOOL, "success", "whether the peer address was successfully added to the address manager"},
964+                {RPCResult::Type::BOOL, "success", "whether the peer address was successfully added to the address manager table"},
965+                {RPCResult::Type::STR, "error", /*optional=*/true, "error description, if the address could not be added (and success is false)."},
    brunoerg commented at 4:25 pm on March 19, 2024:
    In edc8cb64f9aae6939183de276828991243927917: Is it necessary the “(and success is false)”? Isn’t it intuitive?
    0xB10C commented at 4:34 pm on March 19, 2024:
    dropping this.
  40. DrahtBot requested review from brunoerg on Mar 19, 2024
  41. rpc: "addpeeraddress tried" return error on failure 
    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.
    6205466512
  42. test: remove unused mocktime in test_addpeeraddress 
    Drops the mocktime added in fa4c6836c9366c3cc575cb386a397840d5f1aa57.
Setting the mocktime in test_addpeeraddress() isn't needed
anymore as it doesn't leak into test_getrawaddrman() anymore
(since 2cc8ca19f4185490f30a49516c890b2289fbab71).

test_getrawaddrman() clear's the addrman and sets it's own
mocktime.
    0d01f6f0c6
  43. test: fix test to ensure hidden RPC is present in detailed help 
    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().
    99954f914f
  44. 0xB10C force-pushed on Mar 19, 2024
  45. 0xB10C commented at 4:43 pm on March 19, 2024: contributor
    Addressed feedback and included https://github.com/stratospher/bitcoin/commit/09c849e2f544934b121f5fdf62cb974b67b6b962
  46. stratospher commented at 4:58 pm on March 19, 2024: contributor
    reACK 99954f9. 🚀
  47. brunoerg approved
  48. brunoerg commented at 7:09 pm on March 21, 2024: contributor
    utACK 99954f914f031c80aa53daa367fc049c4c55bdf3
  49. achow101 commented at 6:07 pm on March 22, 2024: member
    ACK 99954f914f031c80aa53daa367fc049c4c55bdf3
  50. achow101 merged this on Mar 22, 2024
  51. achow101 closed this on Mar 22, 2024


0xB10C DrahtBot maflcko pablomartin4btc jonatack stratospher amitiuttarwar sr-gi brunoerg achow101


amitiuttarwar jonatack sr-gi pablomartin4btc

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me