rpc: add hidden getrawaddrman RPC to list addrman table entries #28523

pull 0xB10C wants to merge 2 commits into bitcoin:master from 0xB10C:2023-09-verbose-getaddrmaninfo changing 6 files +239 −1
  1. 0xB10C commented at 5:48 PM on September 23, 2023: contributor

    Inspired by getaddrmaninfo (#27511), this adds a hidden/test-only getrawaddrman RPC. The RPC returns information on all addresses in the address manager new and tried tables. Addrman table contents can be used in tests and during development.

    The RPC result encodes the bucket and position, the internal location of addresses in the tables, in the address object's string key. This allows users to choose to consume or to ignore the location information. If the internals of the address manager implementation change, the location encoding might change too.

    getrawaddrman

EXPERIMENTAL warning: this call may be changed in future releases.

Returns information on all address manager entries for the new and tried tables.

Result:
{                                  (json object)
  "table" : {                      (json object) buckets with addresses in the address manager table ( new, tried )
    "bucket/position" : {          (json object) the location in the address manager table (<bucket>/<position>)
      "address" : "str",           (string) The address of the node
      "port" : n,                  (numeric) The port number of the node
      "network" : "str",           (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the address
      "services" : n,              (numeric) The services offered by the node
      "time" : xxx,                (numeric) The UNIX epoch time when the node was last seen
      "source" : "str",            (string) The address that relayed the address to us
      "source_network" : "str"     (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the source address
    },
    ...
  },
  ...
}

Examples:
> bitcoin-cli getrawaddrman
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getrawaddrman", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
  2. DrahtBot commented at 5:48 PM on September 23, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK willcl-ark, amitiuttarwar, stratospher, achow101
    Concept ACK ajtowns
    Stale ACK brunoerg

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28565 (rpc: getaddrmaninfo followups by stratospher)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label RPC/REST/ZMQ on Sep 23, 2023
  4. amitiuttarwar commented at 7:10 PM on September 23, 2023: contributor

    concept ACK. I think it makes sense to expose more info about the addrman internals to allow for better observations & tooling

  5. brunoerg commented at 9:43 AM on September 24, 2023: contributor

    Concept ACK

  6. in src/addrman.cpp:845 in 48dfc81e7e outdated 
     837 | @@ -838,6 +838,25 @@ std::vector<CAddress> AddrManImpl::GetAddr_(size_t max_addresses, size_t max_pct
 838 |      return addresses;
 839 |  }
 840 |  
 841 | +std::vector<std::tuple<int, int, AddrInfo>> AddrManImpl::GetEntries_(bool from_tried) const
 842 | +{
 843 | +    AssertLockHeld(cs);
 844 | +
 845 | +    const int bucket_count = (from_tried) ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT;
    brunoerg commented at 10:06 AM on September 24, 2023:

    nit:

        const int bucket_count = from_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT;
  7. in src/rpc/net.cpp:1054 in 48dfc81e7e outdated 
    1092 | +            },
1093 | +            RPCResult{"for verbose = true", RPCResult::Type::OBJ_DYN, "", "json object with network type, new_table and tried_table as keys",
1094 | +                {
1095 | +                    {RPCResult::Type::ELISION, "", "The same output as verbose = false"},
1096 | +                    {
1097 | +                        RPCResult::Type::ARR, "table", "list of addresses in a address manager table ( new_table, tried_table )",
    brunoerg commented at 11:41 AM on September 24, 2023: 
                            RPCResult::Type::ARR, "table", "list of addresses in the address manager table ( new_table, tried_table )",
  8. 0xB10C force-pushed on Sep 25, 2023
  9. 0xB10C commented at 12:24 PM on September 25, 2023: contributor

    I've also been hacking on a tool that visualizes addrman tables from the getaddrmaninfo verbose output in the browser. A wip version is on https://0xb10c.github.io/addrman-observer (github.com/0xB10C/addrman-observer). You can load dumps produced with bitcoin-cli getaddrmaninfo true > getaddrmaninfo.json (all processing happens locally).

  10. in src/rpc/net.cpp:1071 in da44fd3e49 outdated 
    1109 | +            },
1110 | +        },
1111 | +        RPCExamples{
1112 | +            HelpExampleCli("getaddrmaninfo", "")
1113 | +            + HelpExampleCli("getaddrmaninfo", "true")
1114 | +            + HelpExampleRpc("getaddrmaninfo", "")},
    brunoerg commented at 1:55 PM on September 26, 2023:

    In 56adcf42805b3007b66396754a9e5dcd0968d027: We could add the 'verbose' example for the RPC.

                + HelpExampleRpc("getaddrmaninfo", "")},
            + HelpExampleRpc("getaddrmaninfo", "true")}
    0xB10C commented at 5:03 PM on September 27, 2023:

    done!

  11. brunoerg approved
  12. brunoerg commented at 2:37 PM on September 26, 2023: contributor

    light ACK da44fd3e499ca53242332ef9cb2907ec31fce1e3

    I intend to do more tests soon. Just tested it on a recent clearnet node and worked as expected. Outputs: https://gist.github.com/brunoerg/e3643337b4c16923ee0e0f7e66bec4d9

  13. in test/functional/rpc_net.py:465 in da44fd3e49 outdated 
     415 | +        assert_equal(res["tried_table"][0]["services"], 9)
 416 | +        assert_equal(res["tried_table"][0]["source"], "1.2.3.4")
 417 | +        assert 0 <= res["tried_table"][0]["bucket"] < ADDRMAN_TRIED_BUCKET_COUNT
 418 | +        assert 0 <= res["tried_table"][0]["position"] < ADDRMAN_BUCKET_SIZE
 419 | +
 420 | +        self.log.debug("Add one new address to each addrman table")
    brunoerg commented at 2:39 PM on September 26, 2023:

    In da44fd3e499ca53242332ef9cb2907ec31fce1e3: I think we could addresses from other networks (e.g tor) to test it as well.

    0xB10C commented at 5:08 PM on September 27, 2023:

    will do if I re-touch the tests

  14. 0xB10C force-pushed on Sep 27, 2023
  15. 0xB10C force-pushed on Sep 27, 2023
  16. in src/rpc/net.cpp:1057 in 4307c8c90b outdated 
    1095 | +                    {RPCResult::Type::ELISION, "", "The same output as verbose = false"},
1096 | +                    {
1097 | +                        RPCResult::Type::ARR, "table", "list of addresses in the address manager table ( new_table, tried_table )",
1098 | +                        {
1099 | +                            {RPCResult::Type::OBJ, "", "an address manager table entry", {
1100 | +                                {RPCResult::Type::STR, "address", "the address"},
    ajtowns commented at 2:09 AM on September 28, 2023:

    getnodeaddresses separates this into address and port; does it make sense to be different here?

    getnodeaddresses also includes the last seen time which seems like it might be useful?

  17. in src/rpc/net.cpp:1060 in 4307c8c90b outdated 
    1098 | +                        {
1099 | +                            {RPCResult::Type::OBJ, "", "an address manager table entry", {
1100 | +                                {RPCResult::Type::STR, "address", "the address"},
1101 | +                                {RPCResult::Type::NUM, "services", "the services the node might support"},
1102 | +                                {RPCResult::Type::NUM, "bucket", "the address manager bucket the address is placed in"},
1103 | +                                {RPCResult::Type::NUM, "position", "the bucket position the address is placed in"},
    ajtowns commented at 2:33 AM on September 28, 2023:

    Could substantially reduce the size of the json output (by about 20%) by changing this to:

    {
  "new_table": {
    "0/2": {
      "address": "89.78.111.197:8333",
      "services": 1037,
      "source": "34.126.67.135"
    },
    "0/5": {
      "address": "13.228.211.83:8333",
      "services": 1033,
      "source": "34.126.67.135"
    }
}
    0xB10C commented at 10:06 AM on September 28, 2023:

    I'm not a fan of representing two integers as string in JSON key and having downstream figure out how to parse it. However, why not do

    "table": {
  bucket: {
    position: {
      "address": "13.228.211.83:8333",
      "services": 1033,
      "source": "34.126.67.135"
    },
  }
}

    where bucket and position are integer keys.

    0xB10C commented at 11:53 AM on September 28, 2023:

    TIL UniValue doesn't support integer keys - only string keys. Still going with this as it's about a 15% size reduction with somewhat empty addrman tables. I assume even more space savings on a full addrman tables, as the getaddrmaninfo verbose output would contain more bucket keys.

    $ ll getrawaddrman.json getaddrmaninfo_verbose.json -h
-rw-r--r-- 1 user group 761K Sep 28 13:37 getrawaddrman.json
-rw-r--r-- 1 user group 881K Sep 28 13:37 getaddrmaninfo_verbose.json
    ajtowns commented at 1:47 PM on September 28, 2023:

    JSON in general doesn't support integer keys.

    Not a strong opinion, but there's two advantages of "0/5": vs "0": {"5": -- one is that if you don't care about the bucket/position, you can just skip through the values directly (x.values() vs sum(x2.values() for x2 in x.values())), the other is that bucket+position are pretty internal arrangements; if we end up with a different layout in the future, we can just map that to a different string template (eg "0/5/2") and only the people who care about the internals have to even notice the change.

  18. in src/rpc/net.cpp:1083 in 4307c8c90b outdated 
    1121 | +            }
1122 | +
1123 | +            bool verbose = false;
1124 | +            if (!request.params[0].isNull()) {
1125 | +                verbose = request.params[0].get_bool();
1126 | +            }
    ajtowns commented at 2:44 AM on September 28, 2023:

    Add

    TMPL_INST(CheckRequiredOrDefault, bool, CHECK_NONFATAL(maybe_arg)->get_bool(););

    in rpc/util.cpp and this becomes const bool verbose = self.Arg<bool>(0); (see #28230)

    0xB10C commented at 9:59 AM on September 29, 2023:

    I choose to move to getrawaddrman instead of extending getaddrmaninfo with a verbose flag. #28523 (review). Resolving this as there's no need for it anymore.

  19. in src/rpc/net.cpp:1101 in 4307c8c90b outdated 
    1139 | +            obj.pushKV("new", node.addrman->Size(std::nullopt, true));
1140 | +            obj.pushKV("tried", node.addrman->Size(std::nullopt, false));
1141 | +            obj.pushKV("total", node.addrman->Size());
1142 | +            ret.pushKV("all_networks", obj);
1143 | +
1144 | +            if (verbose) {
    ajtowns commented at 2:50 AM on September 28, 2023:

    We have getmempoolinfo that gives statistics about the mempool and getrawmempool that gives the contents of the mempool; wouldn't it make more sense to mirror that arrangement and have two separate RPCs here as well?

  20. ajtowns commented at 2:52 AM on September 28, 2023: contributor

    Concept ACK

  21. in src/addrman.cpp:1221 in 4307c8c90b outdated 
    1217 | @@ -1199,6 +1218,15 @@ std::vector<CAddress> AddrManImpl::GetAddr(size_t max_addresses, size_t max_pct,
1218 |      return addresses;
1219 |  }
1220 |  
1221 | +std::vector<std::tuple<int, int, AddrInfo>> AddrManImpl::GetEntries(bool from_tried) const
    amitiuttarwar commented at 10:39 AM on September 28, 2023:

    I think it'd be nice to have some sort of named struct instead of using std::vector<std::tuple<int, int, AddrInfo>> several times, including from the callers. Especially since having two int fields means it's easy for callers to mix up.

    I wonder if it could make sense to use the AddressPosition struct defined in addrman.h. It's not a perfect mapping, but it's currently only used in the tests, and it seems fine to add the additional info from AddrInfo to it to pass to the RPC. that could generally be cleaner, but might be overkill.

    Alternatively just the bucket & position could be pulled out into a small struct (similar to NewTriedCount), and that can be used in this tuple.

  22. 0xB10C force-pushed on Sep 29, 2023
  23. 0xB10C commented at 9:56 AM on September 29, 2023: contributor

    Thanks for the helpful review comments @brunoerg @ajtowns @amitiuttarwar. I think I have addressed them all in the most recent push:

    • #28523 (review): I'm now adding an onion and an ipv6 address (instead of two ipv4) to the addrman tables. There are already two IPv4 addresses in the addrman during the test.
    • #28523 (review): Inspired by getnodeaddresses I added a time, port, network, and source_network field. This increases the response size. However, this is highly compressible if someone wants to store or serve it.
    • #28523 (review): adopted the <bucket>/<position> key. I've implemented both discussed approaches and found <bucket>/<position> to be better. See commit message for details.
    • #28523 (review): moved this out of getaddrmaninfo verbose and into a separate, hidden getrawaddrman RPC. This also means #27511 (review) is not included anymore.
    • #28523 (review): using AddressPosition in a std::pair<AddrInfo, AddressPosition> now. This should be easier to reason about than a std::tuple<int, int, AddrInfo>.

    I've also updated OP and the tool on https://0xb10c.github.io/addrman-observer.

  24. 0xB10C renamed this:
    rpc: add getaddrmaninfo verbose to list addrman table entries
    rpc: add hidden getrawaddrman RPC to list addrman table entries
    on Sep 29, 2023
  25. 0xB10C force-pushed on Sep 29, 2023
  26. DrahtBot added the label CI failed on Sep 29, 2023
  27. DrahtBot removed the label CI failed on Sep 29, 2023
  28. in src/addrman.cpp:848 in b7407bb775 outdated 
     843 | +    AssertLockHeld(cs);
 844 | +
 845 | +    const int bucket_count = from_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT;
 846 | +    std::vector<std::pair<AddrInfo, AddressPosition>> infos;
 847 | +    for (int bucket = 0; bucket < bucket_count; ++bucket) {
 848 | +        for (int position = 0; position < ADDRMAN_BUCKET_SIZE; position++) {
    stratospher commented at 1:48 PM on September 29, 2023:

    b7407bb: micro style nit, only if you retouch this commit for some reason - s/position++/++position

  29. in test/functional/rpc_net.py:405 in 45a84d19de outdated 
     400 | +        # It is hidden from general help, but its detailed help may be called directly.
 401 | +        assert "getrawaddrman" not in node.help()
 402 | +        assert "getrawaddrman" in node.help("getrawaddrman")
 403 | +
 404 | +        def check_addr_information(result, expected):
 405 | +            """Utillity to compare an getrawaddrman result entry to a expected entry"""
    stratospher commented at 2:33 PM on September 29, 2023:

    45a84d1:

                """Utility to compare a getrawaddrman result entry with an expected entry"""

    same suggestion for L416 too.

  30. stratospher commented at 3:30 PM on September 29, 2023: contributor

    tested ACK 45a84d1. Very cool to have!

  31. DrahtBot requested review from brunoerg on Sep 29, 2023
  32. in src/addrman_impl.h:266 in b7407bb775 outdated 
     262 | @@ -260,6 +263,8 @@ class AddrManImpl
 263 |  
 264 |      std::vector<CAddress> GetAddr_(size_t max_addresses, size_t max_pct, std::optional<Network> network) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 265 |  
 266 | +    std::vector<std::pair<AddrInfo, AddressPosition>> GetEntries_(bool use_tried) const EXCLUSIVE_LOCKS_REQUIRED(cs);
    amitiuttarwar commented at 5:18 PM on September 29, 2023:

    any reason from_tried became use_tried? I'm guessing to keep consistent with the param to GetEntry, but then could it make sense to just rename the param in the newly introduced GetEntries?

  33. in src/rpc/net.cpp:1098 in b7407bb775 outdated 
    1093 | +static RPCHelpMan getrawaddrman()
1094 | +{
1095 | +    return RPCHelpMan{
1096 | +        "getrawaddrman",
1097 | +        "\nReturns information on all address manager entries for the new and tried tables.\n"
1098 | +        "This RPC is for testing only.\n",
    amitiuttarwar commented at 5:37 PM on September 29, 2023:

    is it? 👀

    ajtowns commented at 7:03 PM on September 29, 2023:

    Perhaps it would be better to describe it as "EXPERIMENTAL warning: this call may be changed in future releases." like sendall ?

  34. in test/functional/rpc_net.py:476 in 45a84d19de outdated 
     471 | +            "address": "2803:0:1234:abcd::1",
 472 | +            "services": 9,
 473 | +            "network": "ipv6",
 474 | +            "source": "2803:0:1234:abcd::1",
 475 | +            "source_network": "ipv6",
 476 | +            "port": -1, # set once addpeeraddress is successful
    amitiuttarwar commented at 5:57 PM on September 29, 2023:

    flake8 complains: E261 at least two spaces before inline comment (here and below)

  35. in test/functional/rpc_net.py:494 in 45a84d19de outdated 
     489 | +            # There's a slight chance that the to-be-added address collides with an already
 490 | +            # present table entry. To avoid this, we increment the port until an address has been
 491 | +            # added. Incrementing the port changes the position in the new table bucket (bucket
 492 | +            # stays the same) and changes both the bucket and the position in the tried table.
 493 | +            while True:
 494 | +                if node.addpeeraddress(address=table_info["entries"][1]["address"], port=port, tried=table_name=="tried")["success"]:
    amitiuttarwar commented at 5:58 PM on September 29, 2023:

    flake8 complains: E225 missing whitespace around operator (for table_name=="tried" part)

  36. in test/functional/rpc_net.py:489 in 45a84d19de outdated 
     488 | +        for (table_name, table_info) in expected.items():
 489 | +            # There's a slight chance that the to-be-added address collides with an already
 490 | +            # present table entry. To avoid this, we increment the port until an address has been
 491 | +            # added. Incrementing the port changes the position in the new table bucket (bucket
 492 | +            # stays the same) and changes both the bucket and the position in the tried table.
 493 | +            while True:
    amitiuttarwar commented at 5:58 PM on September 29, 2023:

    good solution for the addrman collisions issue :)

  37. in src/addrman.h:175 in 45a84d19de outdated 
     168 | @@ -168,6 +169,15 @@ class AddrMan
 169 |       */
 170 |      std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const;
 171 |  
 172 | +    /**
 173 | +     * Returns a information-location pair for all addresses in an addrman table.
 174 | +     *
 175 | +     * @param[in] from_tried     If tried table entries should be returned. Otherwise, new table entries are returned.
    amitiuttarwar commented at 6:04 PM on September 29, 2023: 
         * [@param](/bitcoin-bitcoin/contributor/param/)[in] from_tried     Which table to return entries from (only selects from one).

    not sure if that's the best wording, but when I initially read this I was uncertain if true would be (new & tried) or just tried.

  38. in src/addrman.h:179 in 45a84d19de outdated 
     168 | @@ -168,6 +169,15 @@ class AddrMan
 169 |       */
 170 |      std::vector<CAddress> GetAddr(size_t max_addresses, size_t max_pct, std::optional<Network> network) const;
 171 |  
 172 | +    /**
 173 | +     * Returns a information-location pair for all addresses in an addrman table.
 174 | +     *
 175 | +     * @param[in] from_tried     If tried table entries should be returned. Otherwise, new table entries are returned.
 176 | +     *
 177 | +     * @return                   A vector consisting of pairs of AddrInfo and AddressPosition.
    amitiuttarwar commented at 6:10 PM on September 29, 2023:

    I believe if an addr has multiplicity > 1 (aka shows up on the new table multiple times), that this function will return it multiple times. Could consider adding to this documentation.

  39. amitiuttarwar commented at 6:14 PM on September 29, 2023: contributor

    tested ACK 45a84d19dec. All the review comments are small style/doc things. The functionality looks good.

  40. 0xB10C force-pushed on Sep 29, 2023
  41. 0xB10C commented at 10:04 PM on September 29, 2023: contributor

    Addresses the comments and nits. Thanks @amitiuttarwar, @stratospher, and @ajtowns.

  42. amitiuttarwar commented at 7:04 PM on September 30, 2023: contributor

    reACK 584e12d977b5c7dd57f4aafd984e38e64e85916a

  43. DrahtBot requested review from stratospher on Sep 30, 2023
  44. stratospher commented at 5:51 AM on October 1, 2023: contributor

    reACK 584e12d.

  45. DrahtBot removed review request from stratospher on Oct 1, 2023
  46. DrahtBot added the label CI failed on Oct 1, 2023
  47. in test/functional/rpc_net.py:33 in 584e12d977 outdated 
      25 | @@ -26,6 +26,11 @@
  26 |  )
  27 |  from test_framework.wallet import MiniWallet
  28 |  
  29 | +# Address manager size constants as defined in addrman_impl.h
  30 | +ADDRMAN_NEW_BUCKET_COUNT = 1 << 10
  31 | +ADDRMAN_TRIED_BUCKET_COUNT = 1 << 8
  32 | +ADDRMAN_BUCKET_SIZE = 1 << 6
  33 | +
    0xB10C commented at 9:54 AM on October 1, 2023:

    If I re-touch this, I'll move the constants to test/functional/test_framework/netutil.py as in #28176 (comment)

    ismaelsadeeq commented at 1:42 PM on October 2, 2023:

    #28176 was merged, you might want to move on it to avoid duplication. Or a follow up

  48. 0xB10C commented at 10:00 AM on October 1, 2023: contributor

    The red CI job with the failing wallet_upgradewallet.py --legacy-wallet test looks unrelated.

  49. DrahtBot removed the label CI failed on Oct 2, 2023
  50. willcl-ark commented at 10:34 AM on October 2, 2023: member

    This seems awfully slow to execute on my machine, taking about 30 seconds to return...

    It appears after some (light) investigation that the time is spent inside AddrmanTableToJSON: https://tmp.256k1.dev/23_10_bitcoin_getrawaddrman_split.svg

    The working call to (addrman) GetEntries is this sliver here, with all the rest of the time taken by processing the result to JSON:

    image

    If this is the best we can do then it might not be a blocker for me, as at least we aren't holding locks for all this time, but it does seem a shame to spend 30 seconds processing a near-instant internal call, especially for an RPC which could conceivably be used in a polling situation...

    Of course it's also possible that this is just something wrong with my system?

    Addrman numbers for reference:

    $ time /home/will/src/bitcoin/src/bitcoin-cli getaddrmaninfo
{
  "ipv4": {
    "new": 40403,
    "tried": 1623,
    "total": 42026
  },
  "ipv6": {
    "new": 9806,
    "tried": 0,
    "total": 9806
  },
  "onion": {
    "new": 11200,
    "tried": 818,
    "total": 12018
  },
  "i2p": {
    "new": 746,
    "tried": 107,
    "total": 853
  },
  "cjdns": {
    "new": 0,
    "tried": 0,
    "total": 0
  },
  "all_networks": {
    "new": 62155,
    "tried": 2548,
    "total": 64703
  }
}

________________________________________________________
Executed in    8.06 millis    fish           external
   usr time    4.12 millis  723.00 micros    3.39 millis
   sys time    3.58 millis  188.00 micros    3.39 millis
  51. 0xB10C force-pushed on Oct 2, 2023
  52. 0xB10C commented at 11:35 AM on October 2, 2023: contributor

    Thanks @willcl-ark for testing. I'm seeing about 6s for time bitcoin-cli getrawaddrman > /dev/null with similar sized addrman tables ( "new": 61469, "tried": 1157 vs "new": 62155, "tried": 2548) on a medium-spec machine. I wonder where the 5x increase in processing time on your side comes from. Can you reproduce this on a different machine?

    The flamegraph shows that were spending quite a bit of time in UniValue::pushKV() and UniValue::findKey(). This is O(n) for the number of addrman table entries (here mostly new table entries). The same problem was fixed for getrawmempool in #14984 by using __pushKV() (now called pushKVEnd()), which is O(1). We can use it here too, as our keys are unique.

    This reduces time bitcoin-cli getrawaddrman > /dev/null from 6s to 900ms for me. I've pushed this as I think this is generally a nice improvement. Would be curious to hear about the improvement on your side.

  53. willcl-ark commented at 12:05 PM on October 2, 2023: member

    Would be curious to hear about the improvement on your side.

    Thanks for the quick response. This certainly seems to give a good-enough improvement on my side:

    will@ubuntu in ~/src/bitcoin on  2023-09-verbose-getaddrmaninfo [$?] : C v16.0.6-clang : 🐍 3.8.17
₿ git log --oneline | head -n1; and time ./src/bitcoin-cli getrawaddrman > /dev/null
584e12d977 test: getrawaddrman RPC

________________________________________________________
Executed in   30.71 secs    fish           external
   usr time    1.04 secs  204.00 micros    1.04 secs
   sys time    0.07 secs   41.00 micros    0.07 secs


will@ubuntu in ~/src/bitcoin on  2023-09-verbose-getaddrmaninfo [$?] : C v16.0.6-clang : 🐍 3.8.17 took 30s
₿ git log --oneline | head -n1; and time ./src/bitcoin-cli getrawaddrman > /dev/null
2dc1a6d59e test: getrawaddrman RPC

________________________________________________________
Executed in    3.19 secs    fish           external
   usr time    1.09 secs  419.00 micros    1.09 secs
   sys time    0.04 secs   89.00 micros    0.04 secs
  54. willcl-ark commented at 12:25 PM on October 2, 2023: member

    tACK 2dc1a6d59e

    Nice introspection tool which is very useful for monitoring addrman buckets without the need for tracepoints.

  55. DrahtBot requested review from amitiuttarwar on Oct 2, 2023
  56. DrahtBot requested review from stratospher on Oct 2, 2023
  57. brunoerg approved
  58. brunoerg commented at 12:29 PM on October 2, 2023: contributor

    crACK 2dc1a6d59e9fed4297d10ae2b4b0b00084538348

  59. DrahtBot removed review request from stratospher on Oct 2, 2023
  60. DrahtBot requested review from stratospher on Oct 2, 2023
  61. rpc: getrawaddrman for addrman entries
    Exposing address manager table entries in a hidden RPC allows to introspect
addrman tables in tests and during development.

As response JSON object the following FORMAT1 is choosen:
{
  "table": {
    "<bucket>/<position>": { "address": "..", "port": .., ... },
    "<bucket>/<position>": { "address": "..", "port": .., ... },
    "<bucket>/<position>": { "address": "..", "port": .., ... },
    ...
  }
}

An alternative would be FORMAT2
{
  "table": {
    "bucket": {
      "position": { "address": "..", "port": .., ... },
      "position": { "address": "..", "port": .., ... },
      ..
    },
    "bucket": {
      "position": { "address": "..", "port": .., ... },
      ..
    },
  }
}

FORMAT1 and FORMAT2 have different encodings for the location of the
address in the address manager. While FORMAT2 might be easier to process
for downstream tools, it also mimics internal addrman mappings, which
might change at some point. Users not interested in the address location
can ignore the location key. They don't have to adapt to a new RPC
response format, when the internal addrman layout changes. Additionally,
FORMAT1 is also slightly easier to to iterate in downstream tools. The
RPC response-building implemenation complexcity is lower with FORMAT1
as we can more easily build a "<bucket>/<position>" key than a multiple
"bucket" objects with multiple "position" objects (FORMAT2).
    da384a286b
  62. DrahtBot removed review request from stratospher on Oct 2, 2023
  63. DrahtBot requested review from stratospher on Oct 2, 2023
  64. DrahtBot removed review request from stratospher on Oct 2, 2023
  65. DrahtBot requested review from stratospher on Oct 2, 2023
  66. test: getrawaddrman RPC
    Test that the getrawaddrman returns the addresses in the new and tried
tables. We can't check the buckets and positions as these are not
deterministic (yet).
    352d5eb2a9
  67. 0xB10C force-pushed on Oct 2, 2023
  68. 0xB10C commented at 1:48 PM on October 2, 2023: contributor

    With #28176 merged, I've rebased and now use the addrman table size constants in netutil.py in the test framework. This also resolves #28523 (review). Sorry for invalidating the ACKs again.

    Re-reviewers might want to use git range-diff 2dc1a6d...352d5eb.

  69. willcl-ark commented at 1:58 PM on October 2, 2023: member

    reACK 352d5eb2a9

    git range-diff shows the only difference since previous ACK @ 2dc1a6d is using the just-merged constants from netutil.py:

    <details> <summary>Details</summary>

    ₿ git range-diff 2dc1a6d...352d5eb
 -:  ---------- >  1:  63e90e1d3f test: check for specific disconnect reasons in p2p_blockfilters.py
 -:  ---------- >  2:  2ab7952bda test: add bip157 coverage for (start height > stop height) disconnect
 -:  ---------- >  3:  5671c15f45 blockstorage: Mark FindBlockPos as nodiscard
 -:  ---------- >  4:  f0207e0030 blockstorage: Return on fatal block file flush error
 -:  ---------- >  5:  d8041d4e04 blockstorage: Return on fatal undo file flush error
 -:  ---------- >  6:  925bb723ca [refactor] batch-add transactions to DisconnectedBlockTransactions
 -:  ---------- >  7:  59a35a7398 [bench] DisconnectedBlockTransactions
 -:  ---------- >  8:  79ce9f0aa4 add std::list to memusage
 -:  ---------- >  9:  2765d6f343 rewrite DisconnectedBlockTransactions as a list + map
 -:  ---------- > 10:  cf5f1faa03 MOVEONLY: DisconnectedBlockTransactions to its own file
 -:  ---------- > 11:  4313c77400 make DisconnectedBlockTransactions responsible for its own memory management
 -:  ---------- > 12:  fa389d902f refactor: Drop unused fclose() from BufferedFile
 -:  ---------- > 13:  9999b89cd3 Make BufferedFile to be a CAutoFile wrapper
 -:  ---------- > 14:  fa56c421be Return CAutoFile from BlockManager::Open*File()
 -:  ---------- > 15:  fa4a9c0f43 Remove unused GetType() from OverrideStream, CVectorWriter, SpanReader
 -:  ---------- > 16:  fa72f09d6f Remove CHashWriter type
 -:  ---------- > 17:  fac29a0ab1 Remove SER_GETHASH, hard-code client version in CKeyPool serialize
 -:  ---------- > 18:  96b3f2dbe4 test: add unit test coverage for Python ECDSA implementation
 -:  ---------- > 19:  12f7257b8f doc: Be vague instead of wrong about MALLOC_ARENA_MAX
 -:  ---------- > 20:  b3db8c9d5c rpc: bumpfee, improve doc for 'reduce_output' arg
 -:  ---------- > 21:  4660fc82a1 wallet: Check last block and conflict height are valid in MarkConflicted
 -:  ---------- > 22:  782701ce7d test: Test loading wallets with conflicts without a chain
 -:  ---------- > 23:  b0f5175c04 net: Drop v2 garbage authentication packet
 -:  ---------- > 24:  e3720bca39 net: Simplify v2 recv logic by decoupling AAD from state machine
 -:  ---------- > 25:  fa40b3ee22 test: Avoid test failure on Linux root without cap-add LINUX_IMMUTABLE
 -:  ---------- > 26:  b5a962564e tests: Use manual bumps instead of bumpfee for resendwallettransactions
 -:  ---------- > 27:  d9841a7ac6 Add make_secure_unique helper
 -:  ---------- > 28:  6ef405ddb1 key: don't allocate secure mem for null (invalid) key
 -:  ---------- > 29:  262ab8ef78 Add package evaluation fuzzer
 -:  ---------- > 30:  f9047771d6 lint: fix custom mypy cache dir setting
 -:  ---------- > 31:  380130d9d7 test: add coverage to feature_addrman.py
 -:  ---------- > 32:  d9b172cd00 doc: fix link to developer-notes.md file in multiprocess.md
 1:  7303dac506 = 33:  da384a286b rpc: getrawaddrman for addrman entries
 2:  2dc1a6d59e ! 34:  352d5eb2a9 test: getrawaddrman RPC
    @@ Commit message
         deterministic (yet).

      ## test/functional/rpc_net.py ##
    -@@ test/functional/rpc_net.py: from test_framework.util import (
    - )
    - from test_framework.wallet import MiniWallet
    +@@ test/functional/rpc_net.py: from itertools import product
    + import time

    -+# Address manager size constants as defined in addrman_impl.h
    -+ADDRMAN_NEW_BUCKET_COUNT = 1 << 10
    -+ADDRMAN_TRIED_BUCKET_COUNT = 1 << 8
    -+ADDRMAN_BUCKET_SIZE = 1 << 6
    -+
    -
    - def assert_net_servicesnames(servicesflag, servicenames):
    -     """Utility that checks if all flags are correctly decoded in
    + import test_framework.messages
    ++from test_framework.netutil import ADDRMAN_NEW_BUCKET_COUNT, ADDRMAN_TRIED_BUCKET_COUNT, ADDRMAN_BUCKET_SIZE
    + from test_framework.p2p import (
    +     P2PInterface,
    +     P2P_SERVICES,
     @@ test/functional/rpc_net.py: class NetTest(BitcoinTestFramework):
              self.test_addpeeraddress()
              self.test_sendmsgtopeer()

    </details>

  70. DrahtBot removed review request from stratospher on Oct 2, 2023
  71. DrahtBot requested review from stratospher on Oct 2, 2023
  72. DrahtBot requested review from brunoerg on Oct 2, 2023
  73. amitiuttarwar commented at 6:30 PM on October 2, 2023: contributor

    reACK 352d5eb2a9e

  74. DrahtBot removed review request from amitiuttarwar on Oct 2, 2023
  75. DrahtBot removed review request from stratospher on Oct 2, 2023
  76. DrahtBot requested review from stratospher on Oct 2, 2023
  77. stratospher commented at 3:24 AM on October 3, 2023: contributor

    reACK 352d5eb.

  78. DrahtBot removed review request from stratospher on Oct 3, 2023
  79. achow101 commented at 3:38 PM on October 3, 2023: member

    ACK 352d5eb2a9e89cff4a2815d94a9d81fcc20c4b2c

  80. achow101 merged this on Oct 3, 2023
  81. achow101 closed this on Oct 3, 2023
  82. in src/addrman.cpp:857 in 352d5eb2a9 
     852 | +                AddressPosition location = AddressPosition(
 853 | +                    from_tried,
 854 | +                    /*multiplicity_in=*/from_tried ? 1 : info.nRefCount,
 855 | +                    bucket,
 856 | +                    position);
 857 | +                infos.push_back(std::make_pair(info, location));
    maflcko commented at 8:55 AM on October 13, 2023:

    std::make_pair isn't needed, starting with C++11, you can use emplace_back. Fixed in https://github.com/bitcoin/bitcoin/pull/28583

  83. in test/functional/rpc_net.py:409 in 352d5eb2a9 
     404 | +            assert_equal(result["services"], expected["services"])
 405 | +            assert_equal(result["network"], expected["network"])
 406 | +            assert_equal(result["source"], expected["source"])
 407 | +            assert_equal(result["source_network"], expected["source_network"])
 408 | +            # To avoid failing on slow test runners, use a 10s vspan here.
 409 | +            assert_approx(result["time"], time.time(), vspan=10)
    maflcko commented at 8:57 AM on October 13, 2023: 
     test  2023-10-12T14:27:36.383000Z TestFramework (DEBUG): Test that the getrawaddrman contains information about the addresses added in a previous test 
 node1 2023-10-12T14:27:36.384588Z [http] [httpserver.cpp:307] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:37962 
 node1 2023-10-12T14:27:36.385003Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getrawaddrman user=__cookie__ 
 node1 2023-10-12T14:27:36.385345Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:36.385576Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.17ms) 
 node1 2023-10-12T14:27:36.988817Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:36.989107Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.17ms) 
 node1 2023-10-12T14:27:36.989261Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:36.989419Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.083728Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.094364Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (10.42ms) 
 node1 2023-10-12T14:27:37.096430Z [http] [httpserver.cpp:307] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:37962 
 node1 2023-10-12T14:27:37.096690Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getaddrmaninfo user=__cookie__ 
 node1 2023-10-12T14:27:37.096908Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.097184Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.21ms) 
 node1 2023-10-12T14:27:37.097247Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.097385Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.097434Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.097563Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.097606Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.097715Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.08ms) 
 node1 2023-10-12T14:27:37.097757Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.097854Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.07ms) 
 node1 2023-10-12T14:27:37.097883Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.175048Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (74.78ms) 
 node1 2023-10-12T14:27:37.179840Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.180187Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.27ms) 
 node1 2023-10-12T14:27:37.180243Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.180383Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.180466Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.180610Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.180652Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.180790Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.180850Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.180980Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.09ms) 
 node1 2023-10-12T14:27:37.181023Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.181160Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.181234Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.181363Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.09ms) 
 node1 2023-10-12T14:27:37.181406Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.181545Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.181605Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.181738Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.181780Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.181948Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.182008Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.182146Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.182187Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.182321Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.182393Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.182545Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.182588Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.182724Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.182786Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.182926Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.182975Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.183118Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.183181Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.183325Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
 node1 2023-10-12T14:27:37.183393Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.183544Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.183617Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.183772Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.12ms) 
 node1 2023-10-12T14:27:37.183812Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.183955Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.184068Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.184220Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.12ms) 
 node1 2023-10-12T14:27:37.184261Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.184404Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.184460Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.184607Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.184646Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.184790Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.184868Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.185043Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.14ms) 
 node1 2023-10-12T14:27:37.185086Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.185255Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.13ms) 
 node1 2023-10-12T14:27:37.185313Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.185483Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.185524Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.185671Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 node1 2023-10-12T14:27:37.185734Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.185883Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.12ms) 
 node1 2023-10-12T14:27:37.185922Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 node1 2023-10-12T14:27:37.186069Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 test  2023-10-12T14:27:37.273000Z TestFramework (ERROR): Assertion failed 
                                   Traceback (most recent call last):
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                       self.run_test()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_net.py", line 71, in run_test
                                       self.test_getrawaddrman()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_net.py", line 465, in test_getrawaddrman
                                       check_getrawaddrman_entries(expected)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_net.py", line 432, in check_getrawaddrman_entries
                                       check_addr_information(entry, expected_entry)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_net.py", line 411, in check_addr_information
                                       assert_approx(result["time"], time.time(), vspan=10)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 38, in assert_approx
                                       raise AssertionError("%s < [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
                                   AssertionError: 1697120845 < [1697120847.2721245..1697120867.2721245]
    0xB10C commented at 11:13 AM on October 13, 2023:

    What's a reasonable time range for slow test runners?

    maflcko commented at 11:18 AM on October 13, 2023:

    It seems the data is added in another test case, so I don't think an upper time can be specified, considering that it is possible that someone writes more test cases in the future and adds them in between the two test cases.

    maflcko commented at 12:06 PM on October 17, 2023:

    I think this check should either be removed, or made an exact check (via mocktime) or some other way.

    0xB10C commented at 9:05 AM on October 18, 2023:

    ref https://github.com/bitcoin/bitcoin/pull/28671

    0xB10C commented at 9:05 AM on October 18, 2023:

    ref https://github.com/bitcoin/bitcoin/pull/28671

  84. 0xB10C deleted the branch on Oct 13, 2023
  85. Frank-GER referenced this in commit ea2224fcf9 on Oct 13, 2023
  86. fanquake referenced this in commit f4049eaf08 on Oct 19, 2023
  87. Frank-GER referenced this in commit 22626bd0cf on Oct 21, 2023
  88. bitcoin locked this on Oct 17, 2024
Contributors
0xB10CDrahtBotamitiuttarwarbrunoergajtownsstratospherismaelsadeeqwillcl-arkachow101maflcko


brunoerg

Labels
RPC/REST/ZMQ
Linked (view graph)
#27511 rpc: Add test-only RPC getaddrmaninfo for new/tried table address count#28671 test: Fix failing time check in rpc_net.py
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: 2026-04-13 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me