contrib: add test for bucketing with asmap

brunoerg commented at 11:40 pm on November 13, 2023: contributor

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 commented at 11:40 pm on November 13, 2023: contributor

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
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.

DrahtBot added the label Tests on Nov 13, 2023

brunoerg commented at 11:41 pm on November 13, 2023: contributor

cc: @fjahr

brunoerg force-pushed on Nov 14, 2023

DrahtBot added the label CI failed on Nov 14, 2023

fjahr commented at 10:59 am on November 14, 2023: contributor

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 commented at 12:01 pm on November 14, 2023: contributor

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 commented at 12:07 pm on November 14, 2023: member

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 commented at 6:26 pm on November 14, 2023: contributor

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 on Nov 14, 2023

DrahtBot added the label Needs rebase on Nov 17, 2023

brunoerg force-pushed on Nov 17, 2023

brunoerg renamed this:
~~test: add stress test for bucketing with asmap~~
contrib: add test for bucketing with asmap
on Nov 17, 2023

brunoerg commented at 6:44 pm on November 17, 2023: contributor

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.

brunoerg force-pushed on Nov 17, 2023

DrahtBot removed the label CI failed on Nov 17, 2023

DrahtBot removed the label Needs rebase on Nov 17, 2023

fjahr commented at 4:48 pm on November 23, 2023: contributor

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 force-pushed on Nov 24, 2023

brunoerg marked this as ready for review on Nov 24, 2023

brunoerg commented at 1:10 pm on November 24, 2023: contributor

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

brunoerg force-pushed on Dec 7, 2023

brunoerg commented at 9:00 pm on December 7, 2023: contributor

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

DrahtBot added the label CI failed on Jan 16, 2024

brunoerg force-pushed on Jan 22, 2024

brunoerg commented at 4:56 pm on January 22, 2024: contributor

Rebased

DrahtBot removed the label CI failed on Jan 22, 2024

fjahr commented at 12:06 pm on January 24, 2024: contributor

Code looks good. I have been testing this with the asmap.dat file from https://github.com/fjahr/asmap-data/pull/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 commented at 9:40 pm on February 2, 2024: contributor

Code looks good. I have been testing this with the asmap.dat file from https://github.com/fjahr/asmap-data/pull/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?

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

fjahr commented at 2:29 pm on February 18, 2024: contributor

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 on Feb 19, 2024

brunoerg commented at 5:14 pm on February 19, 2024: contributor

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

Thanks, @fjahr.

DrahtBot added the label CI failed on Feb 19, 2024

fjahr commented at 4:39 pm on February 21, 2024: contributor

tACK facfc2676b95949fa814a7686334d85d99e52519

CI failure seems unrelated…

contrib: add test for bucketing with asmap 7e4562ed13

brunoerg force-pushed on Mar 27, 2024

brunoerg commented at 6:32 pm on March 27, 2024: contributor

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

DrahtBot removed the label CI failed on Mar 28, 2024

contrib: add test for bucketing with asmap #28869

Code Coverage

Reviews

Conflicts