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.

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.

cc: @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?

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.

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"?

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.

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.

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.

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

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

Rebased

Code looks good. I have been testing this with the asmap.dat file from asmap/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 ?

Code looks good. I have been testing this with the asmap.dat file from asmap/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()

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!

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

Thanks, @fjahr.

tACK facfc2676b95949fa814a7686334d85d99e52519

CI failure seems unrelated...

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

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Closing for now.