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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/28792.

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

Reviewers, this pull request conflicts with the following ones:

• #33026 (test, refactor: Embedded ASmap selected preparatory work by fjahr)
• #31868 ([IBD] specialize block serialization by l0rinc)
• #30595 (kernel: Introduce initial C header API by TheCharlatan)
• #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
• #17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
• #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
• #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
• #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

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.

Would it make sense to only check in the asmap.dat file into the repository, and then have the build system convert it to a .h at compile time (similar to the approach used in the src/test/data directory)? That would mean there is also a directly-manipulable file in the repository (that can be used with asmap-tool etc).

How much data would this add per year to the repo, assuming updates happen?

@maflcko The asmap file added here is around 1.6 MiB, so assuming we update for every 6 months, probably a bit over 3 MiB per year (gzip compresses asmap files only ~5%).

Why is the option of loading it from a file even in dev builds, not considered?

Would it make sense to only check in the asmap.dat file into the repository, and then have the build system convert it to a .h at compile time (similar to the approach used in the src/test/data directory)? That would mean there is also a directly-manipulable file in the repository (that can be used with asmap-tool etc).

Yeah, that would work as well. I have drafted that approach here: fjahr@0f9109f Details can change of course (like keeping the raw file in init) but if people prefer that approach, I will pull this into here.

Why is the option of loading it from a file even in dev builds, not considered?

It is considered. Loading from a file is the only option for dev builds if we go the other route mentioned in the description: "The alternative approach is not having the data in the source code but only adding it during the build phase of a release." But this also comes with the aforementioned downsides.

A general question about this approach: do most contemporary OSes handle loading large amounts of (potentially) unused data into physical memory well-enough, that they would (likely) not load this extra 1.5-3MB of data in the case that the -asmap is not being used? Perhaps this will be handled by the demand paging system? If not, it seems to be a bit of a shame to increase binary size by 20% in all cases.

It seems to make most sense to me to take the path suggested by @sipa, including the .dat file in the repo, and having it converted to a .h at compile time. It might also be nice to have a configuration flag to disable the embedded header creation for faster builds.

I am picking this back up since we have come a long way in terms of tooling outside of Bitcoin Core and lately I was just waiting for cmake and v28 feature freeze. I will suggest an early merge in the cycle for v29 so more people are encouraged to use one steady file over a longer period of time.

I am now using the suggested approach of building the header file at compile time and I added the functionality for both autotools and cmake (the autotools one has existed for a while, I can also remove it if people think it's unnecessary). Of course this is rebased to make cmake available. First time I am editing the cmake build system so there are probably things to improve... And there is also a command for asmap-tool.py to generate the header file.

I am adding the latest (1724248800) asmap file from asmap-data, the corresponding creation process can be reviewed in the issue and PR.

I have not added a config option to skip the asmap header file but I will look into that next.

I am now using the suggested approach of building the header file at compile time

I think the suggestion was to not include the header itself in that case.

the autotools one has existed for a while, I can also remove it if people think it's unnecessary

Maybe keep it in until #30664 is merged.

Rebased and I shaved off 4 easy to review commits from here and put them into #33026. Maybe splitting this off helps to move things along a bit faster.

🚧 At least one of the CI tasks failed.
_{Task no wallet, libbitcoinkernel: https://github.com/bitcoin/bitcoin/runs/46745894874
LLM reason (✨ experimental): Build failure caused by incorrect constructor invocation of 'NetGroupManager' in test code.}

Updated the map to that of the latest run: asmap/asmap-data#30