test, refactor: Embedded ASmap selected preparatory work #33026

pull fjahr wants to merge 3 commits into bitcoin:master from fjahr:2023-10-asmap-in-source-prep changing 6 files +78 −8
  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.
    • 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
    ACK sipa
    Stale ACK nervana21, achow101

    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.

    sipa commented at 1:28 pm on October 14, 2025:
    Sounds like we need to agree on what approach to take here before this can move forward?

    fjahr commented at 1:41 pm on October 14, 2025:
    There has been some further discussion on this in #33386 and I wasn’t able to address it yet. I will drop the commit from here and open two separate PRs because there doesn’t seem to be a clear outcome of the discussion yet. One adding the commit here and improving docs and the other splitting asmap= into asmap=/asmapfile= which is certainly the cleaner alternative but changes behavior and it’s a bit messy to redefine behavior of asmap= just slightly.
  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. fjahr force-pushed on Jul 25, 2025
  12. DrahtBot removed the label Needs rebase on Jul 25, 2025
  13. DrahtBot added the label CI failed on Jul 26, 2025
  14. DrahtBot removed the label CI failed on Jul 26, 2025
  15. sipa commented at 3:30 pm on August 8, 2025: member
    utACK 734737b5930df7cebab83cf0dbe5fd390143f2be
  16. achow101 commented at 8:27 pm on August 27, 2025: member
    ACK 734737b5930df7cebab83cf0dbe5fd390143f2be
  17. refactor: Add AutoFile::size 5bb456c92b
  18. fjahr force-pushed on Oct 14, 2025
  19. fjahr commented at 1:43 pm on October 14, 2025: contributor
    No changes except for dropping the contentious change to args behavior.
  20. sipa commented at 1:46 pm on October 14, 2025: member
    ACK 5bb456c92bbd27ebbba1773832b051968f162b8a
  21. DrahtBot requested review from achow101 on Oct 14, 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-10-17 06:13 UTC

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