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:
operator""_mst consteval (re-take) by hebasto)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.
Possible typos and grammar issues:
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.
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.
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;
const. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-constref
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.
🚧 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.
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.
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.
Also fixes adjacent comment typos.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
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.
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.
unrelated, but it looks like there are typos:
Mow much -> How much [Typo in “Mow much”]
RIPEMD10 -> RIPEMD160 [Incorrect algorithm name]