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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31713.
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.
Reviewers, this pull request conflicts with the following ones:
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.
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.
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.
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.
Makes a lot of fields in miniscript.h non-const in order to allow move-operations in next commit.
Functional parity is achieved through making Node move-able.
(Also makes Node::subs no longer mutable).
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.
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
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.
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.