rpc: add hidden getrawaddrman RPC to list addrman table entries

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.

 0getrawaddrman
 1
 2EXPERIMENTAL warning: this call may be changed in future releases.
 3
 4Returns information on all address manager entries for the new and tried tables.
 5
 6Result:
 7{                                  (json object)
 8  "table" : {                      (json object) buckets with addresses in the address manager table ( new, tried )
 9    "bucket/position" : {          (json object) the location in the address manager table (<bucket>/<position>)
10      "address" : "str",           (string) The address of the node
11      "port" : n,                  (numeric) The port number of the node
12      "network" : "str",           (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the address
13      "services" : n,              (numeric) The services offered by the node
14      "time" : xxx,                (numeric) The UNIX epoch time when the node was last seen
15      "source" : "str",            (string) The address that relayed the address to us
16      "source_network" : "str"     (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the source address
17    },
18    ...
19  },
20  ...
21}
22
23Examples:
24> bitcoin-cli getrawaddrman
25> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getrawaddrman", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

DrahtBot commented at 5:48 pm on September 23, 2023: contributor

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

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.

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.

DrahtBot added the label RPC/REST/ZMQ on Sep 23, 2023

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

brunoerg commented at 9:43 am on September 24, 2023: contributor

Concept ACK

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:

0    const int bucket_count = from_tried ? ADDRMAN_TRIED_BUCKET_COUNT : ADDRMAN_NEW_BUCKET_COUNT;

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:

0                        RPCResult::Type::ARR, "table", "list of addresses in the address manager table ( new_table, tried_table )",

0xB10C force-pushed on Sep 25, 2023

0xB10C commented at 12:24 pm on September 25, 2023: contributor

addressed @brunoerg’s review
included #27511 (review) as I’m touching the line anyway

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

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.

0            + HelpExampleRpc("getaddrmaninfo", "")},
1            + HelpExampleRpc("getaddrmaninfo", "true")}

0xB10C commented at 5:03 pm on September 27, 2023:

done!

brunoerg approved

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

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

0xB10C force-pushed on Sep 27, 2023

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?

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:

 0{
 1  "new_table": {
 2    "0/2": {
 3      "address": "89.78.111.197:8333",
 4      "services": 1037,
 5      "source": "34.126.67.135"
 6    },
 7    "0/5": {
 8      "address": "13.228.211.83:8333",
 9      "services": 1033,
10      "source": "34.126.67.135"
11    }
12}

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

0"table": {
1  bucket: {
2    position: {
3      "address": "13.228.211.83:8333",
4      "services": 1033,
5      "source": "34.126.67.135"
6    },
7  }
8}

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.

0$ ll getrawaddrman.json getaddrmaninfo_verbose.json -h
1-rw-r--r-- 1 user group 761K Sep 28 13:37 getrawaddrman.json
2-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.

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

0TMPL_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.

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?

ajtowns commented at 2:52 am on September 28, 2023: contributor

Concept ACK

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.

0xB10C force-pushed on Sep 29, 2023

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.

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

0xB10C force-pushed on Sep 29, 2023

DrahtBot added the label CI failed on Sep 29, 2023

DrahtBot removed the label CI failed on Sep 29, 2023

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

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:

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

same suggestion for L416 too.

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

tested ACK 45a84d1. Very cool to have!

DrahtBot requested review from brunoerg on Sep 29, 2023

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?

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 ?

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)

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)

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 :)

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:

0     * [@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.

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.

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.

0xB10C force-pushed on Sep 29, 2023

0xB10C commented at 10:04 pm on September 29, 2023: contributor

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

amitiuttarwar commented at 7:04 pm on September 30, 2023: contributor

reACK 584e12d977b5c7dd57f4aafd984e38e64e85916a

DrahtBot requested review from stratospher on Sep 30, 2023

stratospher commented at 5:51 am on October 1, 2023: contributor

reACK 584e12d.

DrahtBot removed review request from stratospher on Oct 1, 2023

DrahtBot added the label CI failed on Oct 1, 2023

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

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.

DrahtBot removed the label CI failed on Oct 2, 2023

willcl-ark commented at 10:34 am on October 2, 2023: contributor

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:

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:

 0$ time /home/will/src/bitcoin/src/bitcoin-cli getaddrmaninfo
 1{
 2  "ipv4": {
 3    "new": 40403,
 4    "tried": 1623,
 5    "total": 42026
 6  },
 7  "ipv6": {
 8    "new": 9806,
 9    "tried": 0,
10    "total": 9806
11  },
12  "onion": {
13    "new": 11200,
14    "tried": 818,
15    "total": 12018
16  },
17  "i2p": {
18    "new": 746,
19    "tried": 107,
20    "total": 853
21  },
22  "cjdns": {
23    "new": 0,
24    "tried": 0,
25    "total": 0
26  },
27  "all_networks": {
28    "new": 62155,
29    "tried": 2548,
30    "total": 64703
31  }
32}
33
34________________________________________________________
35Executed in    8.06 millis    fish           external
36   usr time    4.12 millis  723.00 micros    3.39 millis
37   sys time    3.58 millis  188.00 micros    3.39 millis

0xB10C force-pushed on Oct 2, 2023

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.

willcl-ark commented at 12:05 pm on October 2, 2023: contributor

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:

 0will@ubuntu in ~/src/bitcoin on  2023-09-verbose-getaddrmaninfo [$?] : C v16.0.6-clang : 🐍 3.8.17
 1₿ git log --oneline | head -n1; and time ./src/bitcoin-cli getrawaddrman > /dev/null
 2584e12d977 test: getrawaddrman RPC
 3
 4________________________________________________________
 5Executed in   30.71 secs    fish           external
 6   usr time    1.04 secs  204.00 micros    1.04 secs
 7   sys time    0.07 secs   41.00 micros    0.07 secs
 8
 9
10will@ubuntu in ~/src/bitcoin on  2023-09-verbose-getaddrmaninfo [$?] : C v16.0.6-clang : 🐍 3.8.17 took 30s
11₿ git log --oneline | head -n1; and time ./src/bitcoin-cli getrawaddrman > /dev/null
122dc1a6d59e test: getrawaddrman RPC
13
14________________________________________________________
15Executed in    3.19 secs    fish           external
16   usr time    1.09 secs  419.00 micros    1.09 secs
17   sys time    0.04 secs   89.00 micros    0.04 secs

willcl-ark commented at 12:25 pm on October 2, 2023: contributor

tACK 2dc1a6d59e

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

DrahtBot requested review from amitiuttarwar on Oct 2, 2023

DrahtBot requested review from stratospher on Oct 2, 2023

brunoerg approved

brunoerg commented at 12:29 pm on October 2, 2023: contributor

crACK 2dc1a6d59e9fed4297d10ae2b4b0b00084538348

DrahtBot removed review request from stratospher on Oct 2, 2023

DrahtBot requested review from stratospher on Oct 2, 2023

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

DrahtBot removed review request from stratospher on Oct 2, 2023

DrahtBot requested review from stratospher on Oct 2, 2023

DrahtBot removed review request from stratospher on Oct 2, 2023

DrahtBot requested review from stratospher on Oct 2, 2023

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

0xB10C force-pushed on Oct 2, 2023

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.

willcl-ark commented at 1:58 pm on October 2, 2023: contributor

reACK 352d5eb2a9

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

 0₿ git range-diff 2dc1a6d...352d5eb
 1 -:  ---------- >  1:  63e90e1d3f test: check for specific disconnect reasons in p2p_blockfilters.py
 2 -:  ---------- >  2:  2ab7952bda test: add bip157 coverage for (start height > stop height) disconnect
 3 -:  ---------- >  3:  5671c15f45 blockstorage: Mark FindBlockPos as nodiscard
 4 -:  ---------- >  4:  f0207e0030 blockstorage: Return on fatal block file flush error
 5 -:  ---------- >  5:  d8041d4e04 blockstorage: Return on fatal undo file flush error
 6 -:  ---------- >  6:  925bb723ca [refactor] batch-add transactions to DisconnectedBlockTransactions
 7 -:  ---------- >  7:  59a35a7398 [bench] DisconnectedBlockTransactions
 8 -:  ---------- >  8:  79ce9f0aa4 add std::list to memusage
 9 -:  ---------- >  9:  2765d6f343 rewrite DisconnectedBlockTransactions as a list + map
10 -:  ---------- > 10:  cf5f1faa03 MOVEONLY: DisconnectedBlockTransactions to its own file
11 -:  ---------- > 11:  4313c77400 make DisconnectedBlockTransactions responsible for its own memory management
12 -:  ---------- > 12:  fa389d902f refactor: Drop unused fclose() from BufferedFile
13 -:  ---------- > 13:  9999b89cd3 Make BufferedFile to be a CAutoFile wrapper
14 -:  ---------- > 14:  fa56c421be Return CAutoFile from BlockManager::Open*File()
15 -:  ---------- > 15:  fa4a9c0f43 Remove unused GetType() from OverrideStream, CVectorWriter, SpanReader
16 -:  ---------- > 16:  fa72f09d6f Remove CHashWriter type
17 -:  ---------- > 17:  fac29a0ab1 Remove SER_GETHASH, hard-code client version in CKeyPool serialize
18 -:  ---------- > 18:  96b3f2dbe4 test: add unit test coverage for Python ECDSA implementation
19 -:  ---------- > 19:  12f7257b8f doc: Be vague instead of wrong about MALLOC_ARENA_MAX
20 -:  ---------- > 20:  b3db8c9d5c rpc: bumpfee, improve doc for 'reduce_output' arg
21 -:  ---------- > 21:  4660fc82a1 wallet: Check last block and conflict height are valid in MarkConflicted
22 -:  ---------- > 22:  782701ce7d test: Test loading wallets with conflicts without a chain
23 -:  ---------- > 23:  b0f5175c04 net: Drop v2 garbage authentication packet
24 -:  ---------- > 24:  e3720bca39 net: Simplify v2 recv logic by decoupling AAD from state machine
25 -:  ---------- > 25:  fa40b3ee22 test: Avoid test failure on Linux root without cap-add LINUX_IMMUTABLE
26 -:  ---------- > 26:  b5a962564e tests: Use manual bumps instead of bumpfee for resendwallettransactions
27 -:  ---------- > 27:  d9841a7ac6 Add make_secure_unique helper
28 -:  ---------- > 28:  6ef405ddb1 key: don't allocate secure mem for null (invalid) key
29 -:  ---------- > 29:  262ab8ef78 Add package evaluation fuzzer
30 -:  ---------- > 30:  f9047771d6 lint: fix custom mypy cache dir setting
31 -:  ---------- > 31:  380130d9d7 test: add coverage to feature_addrman.py
32 -:  ---------- > 32:  d9b172cd00 doc: fix link to developer-notes.md file in multiprocess.md
33 1:  7303dac506 = 33:  da384a286b rpc: getrawaddrman for addrman entries
34 2:  2dc1a6d59e ! 34:  352d5eb2a9 test: getrawaddrman RPC
35    @@ Commit message
36         deterministic (yet).
37
38      ## test/functional/rpc_net.py ##
39    -@@ test/functional/rpc_net.py: from test_framework.util import (
40    - )
41    - from test_framework.wallet import MiniWallet
42    +@@ test/functional/rpc_net.py: from itertools import product
43    + import time
44
45    -+# Address manager size constants as defined in addrman_impl.h
46    -+ADDRMAN_NEW_BUCKET_COUNT = 1 << 10
47    -+ADDRMAN_TRIED_BUCKET_COUNT = 1 << 8
48    -+ADDRMAN_BUCKET_SIZE = 1 << 6
49    -+
50    -
51    - def assert_net_servicesnames(servicesflag, servicenames):
52    -     """Utility that checks if all flags are correctly decoded in
53    + import test_framework.messages
54    ++from test_framework.netutil import ADDRMAN_NEW_BUCKET_COUNT, ADDRMAN_TRIED_BUCKET_COUNT, ADDRMAN_BUCKET_SIZE
55    + from test_framework.p2p import (
56    +     P2PInterface,
57    +     P2P_SERVICES,
58     @@ test/functional/rpc_net.py: class NetTest(BitcoinTestFramework):
59              self.test_addpeeraddress()
60              self.test_sendmsgtopeer()

DrahtBot removed review request from stratospher on Oct 2, 2023

DrahtBot requested review from stratospher on Oct 2, 2023

DrahtBot requested review from brunoerg on Oct 2, 2023

amitiuttarwar commented at 6:30 pm on October 2, 2023: contributor

reACK 352d5eb2a9e

DrahtBot removed review request from amitiuttarwar on Oct 2, 2023

DrahtBot removed review request from stratospher on Oct 2, 2023

DrahtBot requested review from stratospher on Oct 2, 2023

stratospher commented at 3:24 am on October 3, 2023: contributor

reACK 352d5eb.

DrahtBot removed review request from stratospher on Oct 3, 2023

achow101 commented at 3:38 pm on October 3, 2023: member

ACK 352d5eb2a9e89cff4a2815d94a9d81fcc20c4b2c

achow101 merged this on Oct 3, 2023

achow101 closed this on Oct 3, 2023

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

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:

 0 test  2023-10-12T14:27:36.383000Z TestFramework (DEBUG): Test that the getrawaddrman contains information about the addresses added in a previous test 
 1 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 
 2 node1 2023-10-12T14:27:36.385003Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getrawaddrman user=__cookie__ 
 3 node1 2023-10-12T14:27:36.385345Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 4 node1 2023-10-12T14:27:36.385576Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.17ms) 
 5 node1 2023-10-12T14:27:36.988817Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 6 node1 2023-10-12T14:27:36.989107Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.17ms) 
 7 node1 2023-10-12T14:27:36.989261Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
 8 node1 2023-10-12T14:27:36.989419Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
 9 node1 2023-10-12T14:27:37.083728Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
10 node1 2023-10-12T14:27:37.094364Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (10.42ms) 
11 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 
12 node1 2023-10-12T14:27:37.096690Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getaddrmaninfo user=__cookie__ 
13 node1 2023-10-12T14:27:37.096908Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
14 node1 2023-10-12T14:27:37.097184Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.21ms) 
15 node1 2023-10-12T14:27:37.097247Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
16 node1 2023-10-12T14:27:37.097385Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
17 node1 2023-10-12T14:27:37.097434Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
18 node1 2023-10-12T14:27:37.097563Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
19 node1 2023-10-12T14:27:37.097606Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
20 node1 2023-10-12T14:27:37.097715Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.08ms) 
21 node1 2023-10-12T14:27:37.097757Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
22 node1 2023-10-12T14:27:37.097854Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.07ms) 
23 node1 2023-10-12T14:27:37.097883Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
24 node1 2023-10-12T14:27:37.175048Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (74.78ms) 
25 node1 2023-10-12T14:27:37.179840Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
26 node1 2023-10-12T14:27:37.180187Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.27ms) 
27 node1 2023-10-12T14:27:37.180243Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
28 node1 2023-10-12T14:27:37.180383Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
29 node1 2023-10-12T14:27:37.180466Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
30 node1 2023-10-12T14:27:37.180610Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
31 node1 2023-10-12T14:27:37.180652Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
32 node1 2023-10-12T14:27:37.180790Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
33 node1 2023-10-12T14:27:37.180850Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
34 node1 2023-10-12T14:27:37.180980Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.09ms) 
35 node1 2023-10-12T14:27:37.181023Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
36 node1 2023-10-12T14:27:37.181160Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
37 node1 2023-10-12T14:27:37.181234Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
38 node1 2023-10-12T14:27:37.181363Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.09ms) 
39 node1 2023-10-12T14:27:37.181406Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
40 node1 2023-10-12T14:27:37.181545Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
41 node1 2023-10-12T14:27:37.181605Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
42 node1 2023-10-12T14:27:37.181738Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
43 node1 2023-10-12T14:27:37.181780Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
44 node1 2023-10-12T14:27:37.181948Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
45 node1 2023-10-12T14:27:37.182008Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
46 node1 2023-10-12T14:27:37.182146Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
47 node1 2023-10-12T14:27:37.182187Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
48 node1 2023-10-12T14:27:37.182321Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
49 node1 2023-10-12T14:27:37.182393Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
50 node1 2023-10-12T14:27:37.182545Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
51 node1 2023-10-12T14:27:37.182588Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
52 node1 2023-10-12T14:27:37.182724Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
53 node1 2023-10-12T14:27:37.182786Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
54 node1 2023-10-12T14:27:37.182926Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
55 node1 2023-10-12T14:27:37.182975Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
56 node1 2023-10-12T14:27:37.183118Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
57 node1 2023-10-12T14:27:37.183181Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
58 node1 2023-10-12T14:27:37.183325Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.10ms) 
59 node1 2023-10-12T14:27:37.183393Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
60 node1 2023-10-12T14:27:37.183544Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
61 node1 2023-10-12T14:27:37.183617Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
62 node1 2023-10-12T14:27:37.183772Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.12ms) 
63 node1 2023-10-12T14:27:37.183812Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
64 node1 2023-10-12T14:27:37.183955Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
65 node1 2023-10-12T14:27:37.184068Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
66 node1 2023-10-12T14:27:37.184220Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.12ms) 
67 node1 2023-10-12T14:27:37.184261Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
68 node1 2023-10-12T14:27:37.184404Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
69 node1 2023-10-12T14:27:37.184460Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
70 node1 2023-10-12T14:27:37.184607Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
71 node1 2023-10-12T14:27:37.184646Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
72 node1 2023-10-12T14:27:37.184790Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
73 node1 2023-10-12T14:27:37.184868Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
74 node1 2023-10-12T14:27:37.185043Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.14ms) 
75 node1 2023-10-12T14:27:37.185086Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
76 node1 2023-10-12T14:27:37.185255Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.13ms) 
77 node1 2023-10-12T14:27:37.185313Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
78 node1 2023-10-12T14:27:37.185483Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
79 node1 2023-10-12T14:27:37.185524Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
80 node1 2023-10-12T14:27:37.185671Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
81 node1 2023-10-12T14:27:37.185734Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
82 node1 2023-10-12T14:27:37.185883Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.12ms) 
83 node1 2023-10-12T14:27:37.185922Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: new 1, tried 1, total 2 started 
84 node1 2023-10-12T14:27:37.186069Z [httpworker.1] [logging/timer.h:58] [Log] [addrman] CheckAddrman: completed (0.11ms) 
85 test  2023-10-12T14:27:37.273000Z TestFramework (ERROR): Assertion failed 
86                                   Traceback (most recent call last):
87                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
88                                       self.run_test()
89                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_net.py", line 71, in run_test
90                                       self.test_getrawaddrman()
91                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_net.py", line 465, in test_getrawaddrman
92                                       check_getrawaddrman_entries(expected)
93                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_net.py", line 432, in check_getrawaddrman_entries
94                                       check_addr_information(entry, expected_entry)
95                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_net.py", line 411, in check_addr_information
96                                       assert_approx(result["time"], time.time(), vspan=10)
97                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 38, in assert_approx
98                                       raise AssertionError("%s < [%s..%s]" % (str(v), str(vexp - vspan), str(vexp + vspan)))
99                                   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

0xB10C deleted the branch on Oct 13, 2023

Frank-GER referenced this in commit ea2224fcf9 on Oct 13, 2023

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

Reviews

Conflicts