miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) #31713

pull hodlinator wants to merge 3 commits into bitcoin:master from hodlinator:2024/11/miniscript_ownership changing 4 files +266 −266
  1. hodlinator commented at 9:16 pm on January 22, 2025: contributor

    Removes one level of unnecessary indirection, which aided in finding one issue in #30866. Simplifies the code one step further than https://github.com/bitcoin/bitcoin/pull/30866/commits/09a1875ad8cddeb17c19af34b8282d37fed0937e belonging to aforementioned PR.

    No observed difference when running benchmarks: ExpandDescriptor/WalletIsMineDescriptors/WalletIsMineMigratedDescriptors/WalletLoadingDescriptors.

  2. DrahtBot commented at 9:16 pm on January 22, 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/31713.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK sipa

    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:

    • #31734 (miniscript: account for all StringType variants in Miniscriptdescriptor::ToString() by pythcoiner)

    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.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • Mow much higher -> How much higher [Typo in comment]
    • Constructed.emplace_back -> constructed.emplace_back [Typo in variable name]
    • //! Construct a miniscript node as a shared_ptr. -> This comment is inaccurate given the code changes, as Node is no longer represented by a shared_ptr. [Inaccurate comment]
  3. hodlinator commented at 9:20 pm on January 22, 2025: contributor

    CC @achow101 @darosior @brunoerg from parent PR to gauge their interest.

    It’s unfortunate that this PR touches many lines, so if there’s little support for the refactoring I’m fine with closing, or letting it linger for a while (I understand this is super-low priority). At least it helped find the issue mentioned at the top of the summary.

  4. darosior commented at 1:04 am on January 23, 2025: member

    CI failure looks unrelated. Could you s/script/miniscript/ in the PR title to make it clear to reviewer this does not touch consensus critical stuff?

    It’s unfortunate that this PR touches many lines

    It’s also not a part of the codebase where i expect much churn, so it probably lowers the bar for doing something like this.

  5. hodlinator renamed this:
    script refactor: Remove superfluous unique_ptr-indirection (#30866 follow-up)
    miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)
    on Jan 23, 2025
  6. hodlinator force-pushed on Jan 23, 2025
  7. maflcko commented at 8:33 am on January 23, 2025: member

    CI failure looks unrelated.

    I can’t seem to find it. It would be good to always report or at least leave a comment with the log to any CI failure. Otherwise, it will remain unfixed and will just happen on another pull later on.

  8. l0rinc commented at 8:59 am on January 23, 2025: contributor

    leave a comment with the log to any CI failure

    I think they meant the p2p_1p1c_network failure at https://github.com/bitcoin/bitcoin/actions/runs/12917231673/job/36023158007?pr=31713

  9. hodlinator commented at 9:05 am on January 23, 2025: contributor

    I think they meant the p2p_1p1c_network failure at https://github.com/bitcoin/bitcoin/actions/runs/12917231673/job/36023158007?pr=31713

    Yes, that’s the one, thanks for finding it @l0rinc!

    Could you s/script/miniscript/ in the PR title to make it clear to reviewer this does not touch consensus critical stuff?

    Updated PR title and commit message titles.

  10. darosior commented at 3:36 pm on January 23, 2025: member

    From a quick skim, this looks fine to me. I wonder if @sipa has any objection to not using pointers here.

    Let me see if we can get the conflicts in first and then do this when nothing else is conflicting anymore.

  11. sipa commented at 5:07 pm on January 23, 2025: member

    Very rough-skim Concept ACK.

    This will make the code diverge further from the c++ miniscript policy compiler code, but that’s a bullet we already took with the shared_ptr to unique_ptr change, and it doesn’t seem there is development happening on that anyway.

  12. in src/script/descriptor.cpp:1304 in 58e9c7ca89 outdated
    1300@@ -1301,31 +1301,31 @@ class StringMaker {
    1301 class MiniscriptDescriptor final : public DescriptorImpl
    1302 {
    1303 private:
    1304-    miniscript::NodeRef<uint32_t> m_node;
    1305+    const miniscript::Node<uint32_t> m_node;
    


    purpleKarrot commented at 4:02 pm on February 27, 2025:

    hodlinator commented at 3:51 pm on March 1, 2025:

    Thanks for having a look! const fields are the bane of making types movable, as in c7f1829b4bb3a579c65a99a62b42f64de946c0da of this very PR, so good to have another argument in that direction.

    Still think there’s some value in marking fields as const to collapse down the mental model of the code when moving is not necessary. Wish we had “const except on move”.

    But I’ve proven my point that it can now be made const. I’ll remove it on the next push.


    hodlinator commented at 2:41 pm on March 20, 2025:
    Adjusted in latest push, which is otherwise just a rebase.
  13. laanwj added the label Utils/log/libs on Mar 4, 2025
  14. DrahtBot added the label Needs rebase on Mar 20, 2025
  15. hodlinator force-pushed on Mar 20, 2025
  16. DrahtBot removed the label Needs rebase on Mar 20, 2025
  17. DrahtBot added the label Needs rebase on Apr 10, 2025
  18. hodlinator force-pushed on Apr 12, 2025
  19. DrahtBot removed the label Needs rebase on Apr 12, 2025
  20. DrahtBot added the label Needs rebase on Apr 29, 2025
  21. hodlinator force-pushed on Apr 30, 2025
  22. DrahtBot removed the label Needs rebase on Apr 30, 2025
  23. DrahtBot added the label CI failed on Apr 30, 2025
  24. DrahtBot commented at 10:33 am on April 30, 2025: contributor

    🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/41407555382 LLM reason (✨ experimental): The CI failure is caused by clang-tidy reporting errors related to functions being within a recursive call chain, indicating static analysis issues rather than build or test errors.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly 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.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  25. hodlinator force-pushed on Apr 30, 2025
  26. hodlinator force-pushed on Apr 30, 2025
  27. hodlinator force-pushed on Apr 30, 2025
  28. hodlinator force-pushed on Apr 30, 2025
  29. hodlinator force-pushed on Apr 30, 2025
  30. hodlinator force-pushed on Apr 30, 2025
  31. maflcko commented at 10:16 am on May 6, 2025: member
    If there is a clang-tidy bug, it could make sense to minimize the reproducer and report/fix it upstream
  32. hodlinator commented at 10:23 am on May 6, 2025: contributor

    If there is a clang-tidy bug, it could make sense to minimize the reproducer and report/fix it upstream

    Currently trying to just recreate the CI failure locally on NixOS instead of spamming subscribers of the PR (a bit involved due to clang-tidy depending on generated LLVM headers). I keep getting nerd-sniped into trying a more NixOS-native approach. Was able to iterate through env -i HOME="$HOME" PATH="$PATH" USER="$USER" bash -c 'MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh'.

    https://reviews.llvm.org/D72362 which added the recursion check does mention not supporting destructors.

    Edit: Using NOLINTBEGIN/-END rather than NOLINTNEXTLINE did the job. So it was just invalid use of clang-tidy, not a bug.

  33. miniscript doc: Remove mention of shared pointers
    Correct destructor implementation comment to no longer refer to shared pointers and also move it into the function body, in symmetry with Clone() right below.
    f622594ce0
  34. hodlinator force-pushed on May 7, 2025
  35. DrahtBot removed the label CI failed on May 7, 2025
  36. miniscript refactor: Make fields non-const
    Makes a lot of fields in miniscript.h non-const in order to allow move-operations in next commit.
    
    Also fixes adjacent comment typos.
    
    Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
    e9403fecf9
  37. miniscript refactor: Remove superfluous unique_ptr-indirection
    Functional parity is achieved through making Node move-able.
    
    (Also makes Node::subs no longer mutable).
    
    Unfortunately ~Node() now needs to have the recursion linter disabled, as it is unable to figure out that recursion stops 1 level down. Something with smart pointers must have been throwing it off somehow.
    8ccda63b6a
  38. in src/script/miniscript.h:420 in 33df8e9103 outdated
    413@@ -421,11 +414,11 @@ struct Ops {
    414  */
    415 struct SatInfo {
    416     //! Whether a canonical satisfaction/dissatisfaction is possible at all.
    417-    const bool valid;
    418+    bool valid;
    419     //! How much higher the stack size at start of execution can be compared to at the end.
    420-    const int32_t netdiff;
    421+    int32_t netdiff;
    422     //! Mow much higher the stack size can be during execution compared to at the end.
    


    maflcko commented at 12:58 pm on May 7, 2025:

    unrelated, but it looks like there are typos:

    Mow much -> How much [Typo in “Mow much”]

    RIPEMD10 -> RIPEMD160 [Incorrect algorithm name]


    hodlinator commented at 3:07 pm on May 7, 2025:
    Taken in latest push.
  39. hodlinator force-pushed on May 7, 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-05-10 15:12 UTC

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