contrib: Add asmap-tool #28793

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2023-11-asmap-tool-nextgen changing 4 files +176 −3
  1. fjahr commented at 10:21 pm on November 4, 2023: contributor

    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.

  2. DrahtBot commented at 10:21 pm on November 4, 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
    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.

  3. DrahtBot added the label Scripts and tools on Nov 4, 2023
  4. fjahr force-pushed on Nov 4, 2023
  5. DrahtBot added the label CI failed on Nov 4, 2023
  6. DrahtBot removed the label CI failed on Nov 4, 2023
  7. in contrib/asmap/asmap-tool.py:113 in 14ff89353f outdated 
    108+                               help="output text file; default is stdout")
109+
110+    parser_diff = subparsers.add_parser("diff", help="compute the difference between two asmap files")
111+    parser_diff.add_argument('-i', '--ignore-unassigned', dest="ignore_unassigned", default=False, action="store_true",
112+                             help="ignore unassigned ranges in the first input (useful when second input is filled)")
113+    parser_diff.add_argument('-u', '--unified', dest="unified", default=False, action="store_true",
    sipa commented at 0:42 am on November 5, 2023:
    This was never implemented, it should probably be dropped.
    fjahr commented at 8:13 pm on November 6, 2023:
    done
  8. fjahr force-pushed on Nov 6, 2023
  9. brunoerg commented at 9:23 pm on November 6, 2023: contributor
    Concept ACK
  10. in contrib/asmap/asmap-tool.py:16 in fde0193e68 outdated 
    11+
12+def load_file(input_file, state=None):
13+    try:
14+        contents = input_file.read()
15+    except OSError as err:
16+        sys.exit("Input file '%s' cannot be read: %s." % (input_file.name, err.strerror))
    brunoerg commented at 9:25 pm on November 6, 2023:
    In fde0193e687ad50a01a191e14fb6c052b3534bc1: perhaps using f-strings in all similar cases?
    fjahr commented at 10:20 pm on November 9, 2023:
    done
  11. in contrib/asmap/asmap-tool.py:146 in fde0193e68 outdated 
    141+                print("# %s was AS%i" % (net, old_asn))
142+            elif old_asn == 0:
143+                print("%s AS%i # was unassigned" % (net, new_asn))
144+            else:
145+                print("%s AS%i # was AS%i" % (net, new_asn, old_asn))
146+        print("# %i (2^%f) IPv4 addresses changed; %i (2^%f) IPv6 addresses changed" % (ipv4_changed, math.log2(ipv4_changed), ipv6_changed, math.log2(ipv6_changed)))
    brunoerg commented at 9:29 pm on November 6, 2023:

    from https://github.com/sipa/asmap/pull/9/

    0        print(
1            "# %i%s IPv4 addresses changed; %i%s IPv6 addresses changed"
2            % (
3                ipv4_changed,
4                "" if ipv4_changed == 0 else " (2^%.2f)" % math.log2(ipv4_changed),
5                ipv6_changed,
6                "" if ipv6_changed == 0 else " (2^%.2f)" % math.log2(ipv6_changed),
7            )
8        )
    fjahr commented at 10:20 pm on November 9, 2023:
    Taken, but reformatted further with f-strings
  12. in contrib/asmap/asmap-tool.py:12 in fde0193e68 outdated 
     7+import ipaddress
 8+import math
 9+
10+import asmap
11+
12+def load_file(input_file, state=None):
    brunoerg commented at 9:35 pm on November 6, 2023:
    In fde0193e687ad50a01a191e14fb6c052b3534bc1: Is there any case of state not being None for any load_file usage?
    sipa commented at 9:46 pm on November 7, 2023:
    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 commented at 10:29 pm on November 9, 2023:
    Dropped it for now
  13. in contrib/asmap/asmap-tool.py:104 in fde0193e68 outdated 
     99+
100+    parser_decode = subparsers.add_parser("decode", help="convert asmap data to text format")
101+    parser_decode.add_argument('-f', '--fill', dest="fill", default=False, action="store_true",
102+                               help="permit reassigning undefined network ranges arbitrarily to reduce length")
103+    parser_decode.add_argument('-n', '--nonoverlapping', dest="overlapping", default=True, action="store_false",
104+                               help="output strictly non-overallping network ranges (increases output size)")
    sipa commented at 9:46 pm on November 7, 2023:
    Nit: typo “overallping” (I love criticizing my own code…)
    fjahr commented at 10:21 pm on November 9, 2023:
    done
  14. luke-jr commented at 4:28 pm on November 8, 2023: member

    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”

  15. Sjors commented at 1:19 am on November 9, 2023: member
    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)?
  16. fjahr commented at 9:45 pm on November 9, 2023: contributor

    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.

  17. fjahr force-pushed on Nov 9, 2023
  18. fjahr force-pushed on Nov 9, 2023
  19. DrahtBot added the label Needs rebase on Nov 17, 2023
  20. fjahr force-pushed on Nov 17, 2023
  21. DrahtBot removed the label Needs rebase on Nov 17, 2023
  22. Sjors commented at 12:00 pm on January 9, 2024: member

    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 https://github.com/fjahr/asmap-data/pull/6, but getting close!

  23. fjahr force-pushed on Jan 13, 2024
  24. fjahr force-pushed on Jan 13, 2024
  25. fjahr commented at 2:58 pm on January 13, 2024: contributor

    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.

  26. fjahr force-pushed on Mar 2, 2024
  27. in contrib/asmap/asmap-tool.py:2 in 798ff1ddb9 outdated 
    0@@ -0,0 +1,155 @@
1+# Copyright (c) 2022 Pieter Wuille
    virtu commented at 8:31 am on April 22, 2024: 
    0#!/usr/bin/env python3
1# Copyright (c) 2022 Pieter Wuille
    fjahr commented at 3:19 pm on April 25, 2024:
    done
  28. in contrib/asmap/asmap-tool.py:55 in 798ff1ddb9 outdated 
    50+                txt_error = f"invalid network '{prefix}'"
51+                entries = None
52+                break
53+            entries.append((asmap.net_to_prefix(net), int(asn[2:])))
54+    if entries is not None and bin_asmap is not None and len(contents) > 0:
55+        sys.exit("Input file '%s' is ambiguous." % input_file.name)
    virtu commented at 8:35 am on April 22, 2024:
    The last non-f-string left, I think.
    fjahr commented at 3:20 pm on April 25, 2024:
    done
  29. in contrib/seeds/makeseeds.py:18 in 798ff1ddb9 outdated 
    14 import sys
15 from typing import Union
16 
17-from asmap import ASMap, net_to_prefix
18+asmap_dir = Path(__file__).parent.parent / "asmap"
19+sys.path.append(str(asmap_dir))
    virtu commented at 8:42 am on April 22, 2024:

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

    0testgen/gen_key_io_test_vectors.py
114:sys.path.append(os.path.join(os.path.dirname(__file__), '../../test/functional')
2message-capture/message-capture-parser.py
316:sys.path.append(os.path.join(os.path.dirname(__file__), '../../test/functional'))
    fjahr commented at 3:20 pm on April 25, 2024:
    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.
  30. fjahr force-pushed on Apr 25, 2024
  31. fjahr commented at 3:20 pm on April 25, 2024: contributor
    Addressed comments from @virtu , thanks for reviewing!
  32. contrib: Add asmap-tool 
    Co-authored-by: Pieter Wuille <pieter@wuille.net>
    6abe772a17
  33. fjahr force-pushed on Apr 25, 2024
  34. DrahtBot commented at 3:27 pm on April 25, 2024: contributor

    🚧 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

  35. DrahtBot added the label CI failed on Apr 25, 2024
  36. DrahtBot removed the label CI failed on Apr 26, 2024
  37. virtu commented at 8:22 am on April 29, 2024: contributor

    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.

  38. DrahtBot requested review from Sjors on Apr 29, 2024
  39. DrahtBot requested review from brunoerg on Apr 29, 2024
  40. brunoerg approved
  41. brunoerg commented at 1:31 pm on May 2, 2024: contributor

    ACK 6abe772a17e09fe96e68cd3311280d5a30f6378b

    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.

  42. 0xB10C commented at 11:35 am on May 8, 2024: contributor

    ACK 6abe772a17e09fe96e68cd3311280d5a30f6378b

    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.

  43. achow101 commented at 3:57 pm on May 9, 2024: member
    ACK 6abe772a17e09fe96e68cd3311280d5a30f6378b
  44. achow101 merged this on May 9, 2024
  45. achow101 closed this on May 9, 2024


fjahr DrahtBot sipa brunoerg luke-jr Sjors virtu 0xB10C achow101


Sjors

Labels
Scripts and tools

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me