test, refactor: Embedded ASmap selected preparatory work #33026

pull fjahr wants to merge 4 commits into bitcoin:master from fjahr:2023-10-asmap-in-source-prep changing 7 files +79 −9
  1. fjahr commented at 9:55 pm on July 20, 2025: contributor

    This contains four commits from #28792 that can be easily reviewed and merged independently. I hope splitting this change off can make this part move a bit faster and reduce frequency of needed rebases for #28792.

    The commits in order:

    • Add additional unit test vectors to the asmap interpreter (written by sipa). This helps to ensure that the further refactors in #28792 don’t change behavior.
    • Modernizes the logging in util/asmap.cpp, I added this while touching the rest of the file all over anyway.
    • Fixes the behavior of the GetPathArg function when an path arg is given “1” (for example -asmap=1), which would be interpreted as a path but should be interpreted as using the are without a parameter which activates the fallback. There is no explicit test added here because this is tested implicitly in the tests added in #28792.
    • Adds an AutoFile::size helper function
  2. DrahtBot commented at 9:55 pm on July 20, 2025: contributor

    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/33026.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Stale ACK nervana21

    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:

    • #31868 ([IBD] specialize block serialization by l0rinc)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #28792 (Embed default ASMap as binary dump header file by fjahr)

    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. nervana21 commented at 4:02 am on July 24, 2025: contributor

    tACK 942d95d

    I thoroughly reviewed and understood all code changes. I ran the tests locally and they all passed.

  4. fanquake referenced this in commit 16f7b43b68 on Jul 24, 2025
  5. in src/util/asmap.cpp:203 in 942d95d3c6 outdated
    199@@ -200,13 +200,11 @@ std::vector<bool> DecodeAsmap(fs::path path)
    200     FILE *filestr = fsbridge::fopen(path, "rb");
    201     AutoFile file{filestr};
    202     if (file.IsNull()) {
    203-        LogPrintf("Failed to open asmap file from disk\n");
    


    fanquake commented at 10:22 am on July 24, 2025:
    In 9481c77420845316e825855ac589aa8a10bff057: Should drop the redundant \n on all lines.

    fjahr commented at 1:44 pm on July 24, 2025:
    done
  6. in src/common/args.cpp:276 in 942d95d3c6 outdated
    272@@ -273,7 +273,7 @@ fs::path ArgsManager::GetPathArg(std::string arg, const fs::path& default_value)
    273 {
    274     if (IsArgNegated(arg)) return fs::path{};
    275     std::string path_str = GetArg(arg, "");
    276-    if (path_str.empty()) return default_value;
    277+    if (path_str.empty() || path_str == "1") return default_value;
    


    fanquake commented at 10:25 am on July 24, 2025:
    In 74932c62f9627a2eee634b1dd16fe2582c258a74: Seems a bit awkaward to add an (undocumented) special case here for asmap? Can’t we just fail if the value given isn’t a valid path, or stop supporting this behaviour? cc @ryanofsky

    fjahr commented at 1:30 pm on July 24, 2025:
    I can drop this is other reviewers agree that it’s confusing. I personally think this is the more intuitive behavior for path args independently of asmap because our bool args all work with both -foo and -foo=1 afaik. If the user wants to use -asmap (or any path arg) as a bool then I think they would expect it to work the same as the other bool params. Fwiw, I discovered this because I wrote a functional test that covered -asmap=1 and I actually expected it work, only after it didn’t I added this.
  7. fjahr force-pushed on Jul 24, 2025
  8. DrahtBot added the label Needs rebase on Jul 25, 2025
  9. tests: add unit test vectors for asmap interpreter 5a63a5a1fd
  10. refactor: Modernize logging in util/asmap.cpp 70826bd4e7
  11. common: Use fallback in GetPathArg if the path is 1
    Otherwise -asmap=1, for example, would be interpreted as a path "1".
    054d79934f
  12. refactor: Add AutoFile::size 734737b593
  13. fjahr force-pushed on Jul 25, 2025
  14. DrahtBot removed the label Needs rebase on Jul 25, 2025
  15. DrahtBot added the label CI failed on Jul 26, 2025
  16. DrahtBot removed the label CI failed on Jul 26, 2025

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: 2025-08-03 00:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me