contrib: Add asmap-tool #28793

fjahr · 2023-11-04T22:21:32Z

This adds asmap.py and asmap-tool.py from sipa's nextgen branch: https://github.com/sipa/asmap/tree/nextgen

The motivation is that we should maintain the tooling for de- and encoding asmap files within the bitcoin core repository because it is not possible to use an asmap file that is not encoded.

We already had an earlier version of asmap.py within the seeds contrib tools. The newer version only had a small amount of changes and is still compatible, so the old version is removed from contrib/seeds and the new version is made available to makeseeds.py.

DrahtBot · 2023-11-04T22:21:35Z

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
ACK	virtu, brunoerg, 0xB10C, achow101
Concept NACK	luke-jr
Concept ACK	Sjors

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:

#30008 (seeds: Pull additional nodes from my seeder and update fixed seeds by achow101)

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.

contrib/asmap/asmap-tool.py

brunoerg · 2023-11-06T21:23:50Z

Concept ACK

contrib/asmap/asmap-tool.py

brunoerg · 2023-11-06T21:35:27Z

contrib/asmap/asmap-tool.py

In fde0193e687ad50a01a191e14fb6c052b3534bc1: Is there any case of state not being None for any load_file usage?

Not right now; we could drop that for now. I think the idea was supporting loading an asmap, and then also loading a patch file on top of it (which could be created by diffing).

Dropped it for now

contrib/asmap/asmap-tool.py

luke-jr · 2023-11-08T16:28:57Z

Concept NACK

The motivation is that we should maintain the tooling for de- and encoding asmap files within the bitcoin core repository because it is not possible to use an asmap file that is not encoded.

That's not a reason. Ideally, the current repo should be split into several itself - but we have technical hurdles to get to that point. No reason to add more here "just because"

Sjors · 2023-11-09T01:19:59Z

I'm fine with adding another contrib script here. But we could also make a new repo under bitcoin-core. Perhaps the choice depends on whether this tool is mainly useful for users, developers or maintainers (bitcoin-core/bitcoin-maintainer-tools)?

fjahr · 2023-11-09T21:45:04Z

That's not a reason. Ideally, the current repo should be split into several itself - but we have technical hurdles to get to that point. No reason to add more here "just because"

@luke-jr It is a reason, but you don't have to agree with it ;) If we want to move the contrib scripts outside of the bitcoin repo that sounds like a pretty big change that would require some conceptual review that is outside of the scope of the asmap project. If there is wider conceptual agreement on this I am open to adapt the approach. In the meantime I think this approach here is not a step in the wrong direction either way. Putting the asmap-tool.py in a separate repo would mean we would still have the asmap.py file duplicated and outdated in contrib/seeds. So we would need to fix that anyway even if we start out by putting asmap-tool somewhere else. After this change we can still move contrib/asmap and contrib/seeds into another repo when there is agreement on that.

Perhaps the choice depends on whether this tool is mainly useful for users, developers or maintainers (bitcoin-core/bitcoin-maintainer-tools)?

@Sjors All of the above. Users who want to build and use their own asmap file need to encode it for bitcoin core to accept it. Developers who want to test asmap functionality will need to encode and possibly also decode asmap files. Maintainers who build a release including an asmap file will need to encode the file as well if they want to reproduce the asmap file that will be embedded.

Sjors · 2024-01-09T12:00:16Z

Concept ACK on just adding these 150 lines of Python here, and lower the barrier for people to verify these things.

There's still some work in progress on fixing determinism, see asmap/asmap-data#6, but getting close!

fjahr · 2024-01-13T14:58:43Z

There's still some work in progress on fixing determinism, see fjahr/asmap-data#6, but getting close!

This should be fixed now, the issue appears to have been the ordering of sets after combining/diffing them. The results are now explicitly sorted. On my system, I could replicate the issue by using different python versions and it was fixed with this change.

Credits to @sipa for pointing me in the right direction.

contrib/asmap/asmap-tool.py

virtu · 2024-04-22T08:42:48Z

contrib/seeds/makeseeds.py

nit: I searched other scripts in contrib for appending to sys.path to find out if there's precedent one could follow:

testgen/gen_key_io_test_vectors.py 14:sys.path.append(os.path.join(os.path.dirname(__file__), '../../test/functional') message-capture/message-capture-parser.py 16:sys.path.append(os.path.join(os.path.dirname(__file__), '../../test/functional'))

Thanks for this hint but I think usage of pathlib is usually preferred now because it is more modern. So I will keep this as is unless there are more pros to this change that I have overlooked.

fjahr · 2024-04-25T15:20:57Z

Addressed comments from @virtu , thanks for reviewing!

Co-authored-by: Pieter Wuille <[email protected]>

DrahtBot · 2024-04-25T15:27:20Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24258778716}

virtu · 2024-04-29T08:22:30Z

ACK 6abe772

Tested decoding and encoding some demo ASMaps.

Also made sure encoding yields a reproducible ordering by decoding, shuf'ling, and re-encoded an ASMap and making sure the resulting encoding was identical to the original one.

brunoerg

ACK 6abe772

I've been using this for a while but did some new tests to ensure.

I've tested encoding/decoding as well as the diff command with a very old file (1 year ago) and a newest one to test big changes - e.g.: # 1296418840 (2^30.27) IPv4 addresses changed; 8741211000446919691919144517632603 (2^112.75) IPv6 addresses changed).. I think code could be clearer but lgtm to merge as is.

0xB10C · 2024-05-08T11:35:42Z

ACK 6abe772

Did some light code review & some testing of encode -> decode -> encode with kartograf generated final_results.txt file, diff, and the handling of invalid text and mutated binary input files.

I don't have a particular strong opinion on having this here or e.g. a bitcoin-core repository.

achow101 · 2024-05-09T15:57:16Z

ACK 6abe772

DrahtBot added the Scripts and tools label Nov 4, 2023

fjahr mentioned this pull request Nov 4, 2023

Embedded ASMap data Tracking Issue #28794

Closed

4 tasks

fjahr force-pushed the 2023-11-asmap-tool-nextgen branch from 2f7db67 to 14ff893 Compare November 4, 2023 22:50

DrahtBot added CI failed and removed CI failed labels Nov 4, 2023

sipa reviewed Nov 5, 2023

View reviewed changes

contrib/asmap/asmap-tool.py Outdated Show resolved Hide resolved

fjahr force-pushed the 2023-11-asmap-tool-nextgen branch from 14ff893 to fde0193 Compare November 6, 2023 20:12

brunoerg reviewed Nov 6, 2023

View reviewed changes

contrib/asmap/asmap-tool.py Outdated Show resolved Hide resolved

brunoerg reviewed Nov 6, 2023

View reviewed changes

contrib/asmap/asmap-tool.py Outdated Show resolved Hide resolved

brunoerg reviewed Nov 6, 2023

View reviewed changes

sipa reviewed Nov 7, 2023

View reviewed changes

contrib/asmap/asmap-tool.py Outdated Show resolved Hide resolved

fjahr force-pushed the 2023-11-asmap-tool-nextgen branch 2 times, most recently from 980e2a3 to 423dbf8 Compare November 9, 2023 22:28

DrahtBot mentioned this pull request Nov 14, 2023

contrib: add test for bucketing with asmap #28869

Closed

DrahtBot added the Needs rebase label Nov 17, 2023

fjahr force-pushed the 2023-11-asmap-tool-nextgen branch from 423dbf8 to a86ffab Compare November 17, 2023 12:43

DrahtBot removed the Needs rebase label Nov 17, 2023

fjahr mentioned this pull request Jan 7, 2024

Add 1704463200_asmap.dat asmap/asmap-data#6

Merged

fjahr force-pushed the 2023-11-asmap-tool-nextgen branch 2 times, most recently from 47d4339 to 66620b9 Compare January 13, 2024 14:55

fjahr mentioned this pull request Jan 31, 2024

Add 1706536800_asmap.dat asmap/asmap-data#10

Merged

fjahr force-pushed the 2023-11-asmap-tool-nextgen branch from 66620b9 to 798ff1d Compare March 2, 2024 22:16

fjahr mentioned this pull request Mar 19, 2024

Add 1710770400_asmap.dat asmap/asmap-data#13

Merged

virtu reviewed Apr 22, 2024

View reviewed changes

fjahr force-pushed the 2023-11-asmap-tool-nextgen branch from 798ff1d to 856a2da Compare April 25, 2024 15:19

contrib: Add asmap-tool

6abe772

Co-authored-by: Pieter Wuille <[email protected]>

fjahr force-pushed the 2023-11-asmap-tool-nextgen branch from 856a2da to 6abe772 Compare April 25, 2024 15:27

DrahtBot added CI failed and removed CI failed labels Apr 25, 2024

DrahtBot requested review from Sjors and brunoerg April 29, 2024 08:22

DrahtBot mentioned this pull request May 1, 2024

seeds: Pull additional nodes from my seeder and update fixed seeds #30008

Merged

brunoerg approved these changes May 2, 2024

View reviewed changes

achow101 merged commit ceb1e07 into bitcoin:master May 9, 2024

jurraca mentioned this pull request Aug 22, 2024

Add ASmap run 1724248800 asmap/asmap-data#16

Merged

bitcoin locked and limited conversation to collaborators May 9, 2025

contrib: Add asmap-tool #28793

contrib: Add asmap-tool #28793

Uh oh!

Conversation

fjahr commented Nov 4, 2023

Uh oh!

DrahtBot commented Nov 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

brunoerg commented Nov 6, 2023

Uh oh!

Uh oh!

Uh oh!

brunoerg Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

sipa Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

fjahr Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luke-jr commented Nov 8, 2023

Uh oh!

Sjors commented Nov 9, 2023

Uh oh!

fjahr commented Nov 9, 2023

Uh oh!

Sjors commented Jan 9, 2024

Uh oh!

fjahr commented Jan 13, 2024

Uh oh!

Uh oh!

Uh oh!

virtu Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

fjahr Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

fjahr commented Apr 25, 2024

Uh oh!

DrahtBot commented Apr 25, 2024

Uh oh!

virtu commented Apr 29, 2024

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

0xB10C commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented May 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Nov 4, 2023 •

edited

Loading

0xB10C commented May 8, 2024 •

edited

Loading