contrib: Add asmap-tool

[fjahr]

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]

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.

[brunoerg]

Concept ACK

[brunoerg]

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

[sipa]

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

[fjahr]

Dropped it for now

[luke-jr]

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]

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]

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]

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 fjahr/asmap-data#6, but getting close!

[fjahr]

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.

[virtu]

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

[fjahr]

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]

Addressed comments from @virtu , thanks for reviewing!

[DrahtBot]

🚧 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]

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]

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]

ACK 6abe772