contrib: add test for bucketing with asmap

[brunoerg]

This PR adds a Python script to test the addrman bucketing logic using asmap. You should run this test using your own asmap file (./contrib/asmap/test_bucketing.py --asmap=path/to/asmap --num_asns=1000).

How it works?

• Read the asmap file
• From --num_asns=N: Get N unique ASNs and their respective ranges.
• For 1/3 of the ASNs: it will try to add 1000 addresses from each ASN into the "new" table.
• For 2/3 of the ASNs: it will try to add 1 address from each ASN into the "new" table.

I'm first opening it as a draft to seek concept acks and perhaps more ideas to include here.

[DrahtBot]

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	fjahr

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

Conflicts

No conflicts as of last run.

[brunoerg]

cc: @fjahr

[fjahr]

I would like it more if you keep the asmap.py in seeds because ultimately I want to move it from there to contrib/asmap in #28793 and I think the primary function of that code is as a tool, not as a test lib.

And if this is not being run as part of the functional test suite (which I only understood just now), then I am not sure it belongs there. Maybe there is precedent for something similar but to me it feels more like this is something comparable to test_utxo_snapshots.sh? I think the dependencies to the test framework could be removed and then the script could be further parameterized, allowing the stress test to be run on even different numbers with different asmap files, depending on what scenario the tester wants to run.

I think it could be running as part of suite as well though, invoked with the asmap file fixture that we have there? I guess that also depends on how long the test takes to run.

So I am currently unsure if this is more valuable as a devtool or as a functional test. I slightly tend towards devtool at the moment. @virtu do you have some feedback on whether this would be/would have been helpful for you as a tool for your simulations?

[brunoerg]

And if this is not being run as part of the functional test suite (which I only understood just now), then I am not sure it belongs there. Maybe there is precedent for something similar but to me it feels more like this is something comparable to test_utxo_snapshots.sh?

I agree. I'm inclined to move it to contrib instead of being a functional test. This way we could be more free to test stuff without having to be caution with CI.

[fanquake]

The idea of this test is "stressing" the addrman using asmap.

Can you clarify what this meant to do? What is it stressing, where do the magic numbers "2000, 1/3, 2/3" come from, what is expected to fail under this "stress"?

[brunoerg]

Can you clarify what this meant to do? What is it stressing, where do the magic numbers "2000, 1/3, 2/3" come from, what is expected to fail under this "stress"?

Sure! "Stressing" is basically test bucketing logic in a situation that we try to add in the new table:

• Addresses from more distinct ASs than the number of available buckets (2000 is a little bit less than 2x the number of buckets. I chose this number to not make the test slower, however, if we migrate this test to contrib, we can remove this limitation and use all the distincts ASN from the file).
• Many addresses from the same AS (e.g. 1000) - especially because when adding a new address to an occupied position of a bucket, it will not replace the existing entry, expect for some specific cases.

[brunoerg]

Force-pushed changing the approach - PR description has been updated:

• Moved it to contrib
• Now it's possible to specify the asmap file and the number of unique ASNs to be used in the test
• When adding an address into new table, check the log: LogPrint(BCLog::ADDRMAN, "Added %s%s to new[%i][%i]\n". It ensures that Core is mapping the addresses correctly according to the ASN.

[fjahr]

Concept ACK

I think this could use some documentation on top of the file on how the script is intended to be used, see the text in test_utxo_snapshots.sh.

[brunoerg]

@fjahr, nice idea. Force-pushed adding it.

[brunoerg]

Force-pushed addressing ASMap health check. cc: @fjahr

[brunoerg]

Rebased

[fjahr]

Code looks good. I have been testing this with the asmap.dat file from fjahr/asmap-data#6 and I noticed that after adding 60-70 entries the program slows down a lot and sometimes takes over a minute to add the next address. Have you seen this as well and do you know why this is @brunoerg ?

[brunoerg]

Code looks good. I have been testing this with the asmap.dat file from fjahr/asmap-data#6 and I noticed that after adding 60-70 entries the program slows down a lot and sometimes takes over a minute to add the next address. Have you seen this as well and do you know why this is @brunoerg ?

Just checked it, @fjahr. I think assert_debug_log is slowing it down. Can you please check with the following diff?

diff --git a/contrib/asmap/test_bucketing.py b/contrib/asmap/test_bucketing.py
index 575ce72ad3..f1033d66bc 100755
--- a/contrib/asmap/test_bucketing.py
+++ b/contrib/asmap/test_bucketing.py
@@ -78,11 +78,10 @@ class AsmapBucketingTest(BitcoinTestFramework):
                     addr_port = f"{addr}:8333" if type(ipaddress.ip_address(addr)) is ipaddress.IPv4Address else f"[{addr}]:8333"
                     log_msg = f"Added {addr_port} mapped to AS{asn} to new"
                     try:
-                        with node.assert_debug_log([log_msg]):
-                            if node.addpeeraddress(address=str(addr), tried=False, port=8333)["success"]:
-                                added = True
-                                self.log.info(f"added {addr} (ASN {asn}) to the new table")
-                                asns.append(asn)
+                        if node.addpeeraddress(address=str(addr), tried=False, port=8333)["success"]:
+                            added = True
+                            self.log.info(f"added {addr} (ASN {asn}) to the new table")
+                            asns.append(asn)
                     except AssertionError as e:
                         if added:
                             assert f"{log_msg}[" in str(e)
@@ -91,12 +90,6 @@ class AsmapBucketingTest(BitcoinTestFramework):
         addrman_info = node.getaddrmaninfo()
         assert_equal(len_entries, addrman_info["all_networks"]["new"])
 
-        raw_addrman = node.getrawaddrman()
-        self.log.info("Check addrman is successfully loaded after restarting")
-        with node.assert_debug_log([f"ASMap Health Check: {len_entries} clearnet peers are mapped to {NUM_ASNS} ASNs with 0 peers being unmapped"]):
-            self.restart_node(0, extra_args=self.args)
-        assert_equal(raw_addrman, node.getrawaddrman())
-
 
 if __name__ == '__main__':
     AsmapBucketingTest().main()

[fjahr]

Just checked it, @fjahr. I think assert_debug_log is slowing it down. Can you please check with the following diff?

Yeah, with that change it doesn't slow down anymore!

[brunoerg]

Force-pushed changing the code to make it faster. Removed the assert_debug_log when adding an address into addrman.

Thanks, @fjahr.

[fjahr]

tACK facfc26

CI failure seems unrelated...

[brunoerg]

Rebased and since addpeeraddress is now reliable (#28998), we can use its return to count the entries.