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

pull pythcoiner wants to merge 2 commits into bitcoin:master from pythcoiner:miniscriptdescriptor_to_string changing 2 files +41 −8
  1. pythcoiner commented at 4:52 pm on January 24, 2025: none

    In MiniscriptDescriptor::ToStringHelper() only the StringType::Private variant of the type argument was handled. This PR implements serializing w/ all variants of StringType & add a functional test for the descriptor triggering the related issue.

    Closes #31694: previously when calling listdescriptors RPC on a wallet containing a taproot descriptor w/ a (miniscript) taptree, origins of internal key & taptree were serialized w/ differents hardened derivation markers:

    • origin of the internal key were serialized w/ StringType::Normalized type (using h as marker)
    • origins of taptree keys were serialized w/ StringType::Private type (using ' as marker)

    Note: Origins in segwit (wsh()) miniscript descriptors were also serialized w/ StringType::Private type (' marker) and are now serialized w/ StringType::Normalized type (h marker).

  2. DrahtBot commented at 4:52 pm on January 24, 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/31734.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK 1440000bytes, brunoerg

    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:

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

    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. pythcoiner renamed this:
    Miniscriptdescriptor to string
    Account for all `StringType` variants in `Miniscriptdescriptor::ToString()`
    on Jan 24, 2025
  4. pythcoiner renamed this:
    Account for all `StringType` variants in `Miniscriptdescriptor::ToString()`
    miniscript: ccount for all `StringType` variants in `Miniscriptdescriptor::ToString()`
    on Jan 24, 2025
  5. DrahtBot added the label Descriptors on Jan 24, 2025
  6. pythcoiner renamed this:
    miniscript: ccount for all `StringType` variants in `Miniscriptdescriptor::ToString()`
    miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`
    on Jan 24, 2025
  7. pythcoiner force-pushed on Jan 24, 2025
  8. DrahtBot added the label CI failed on Jan 24, 2025
  9. DrahtBot commented at 4:58 pm on January 24, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36134804974

    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.

  10. pythcoiner marked this as a draft on Jan 25, 2025
  11. pythcoiner force-pushed on Jan 25, 2025
  12. pythcoiner force-pushed on Jan 25, 2025
  13. DrahtBot removed the label CI failed on Jan 25, 2025
  14. pythcoiner marked this as ready for review on Jan 25, 2025
  15. DrahtBot added the label CI failed on Jan 26, 2025
  16. DrahtBot removed the label CI failed on Jan 26, 2025
  17. in test/functional/wallet_listdescriptors.py:140 in 0bc213db76 outdated 
    136@@ -137,7 +137,7 @@ def run_test(self):
137         self.log.info('Test taproot descriptor do not have mixed hardened derivation marker')
138         node.createwallet(wallet_name='w5', descriptors=True, disable_private_keys=True)
139         wallet = node.get_wallet_rpc('w5')
140-        imp = wallet.importdescriptors([{
    hodlinator commented at 3:27 pm on January 29, 2025:
    In commit 0bc213db7686df9ccbd5ef76c5569181851d80a2: This change should be part of the other commit.
    pythcoiner commented at 2:54 am on January 30, 2025:
    done
  18. in src/script/descriptor.cpp:1291 in 0bc213db76 outdated 
    1285@@ -1286,20 +1286,31 @@ class StringMaker {
1286     const SigningProvider* m_arg;
1287     //! Keys contained in the Miniscript (a reference to DescriptorImpl::m_pubkey_args).
1288     const std::vector<std::unique_ptr<PubkeyProvider>>& m_pubkeys;
1289-    //! Whether to serialize keys as private or public.
1290-    bool m_private;
1291+    //! StringType to serialize keys
1292+    DescriptorImpl::StringType m_type;
1293+    std::unique_ptr<DescriptorCache> m_cache;
    hodlinator commented at 3:28 pm on January 29, 2025:

    Seems okay to store as raw pointer, if we take it as such as an argument into the StringMaker constructor?

    0    const DescriptorCache* m_cache;
    pythcoiner commented at 2:54 am on January 30, 2025:
    done
  19. in src/script/descriptor.cpp:1290 in 0bc213db76 outdated 
    1285@@ -1286,20 +1286,31 @@ class StringMaker {
1286     const SigningProvider* m_arg;
1287     //! Keys contained in the Miniscript (a reference to DescriptorImpl::m_pubkey_args).
1288     const std::vector<std::unique_ptr<PubkeyProvider>>& m_pubkeys;
1289-    //! Whether to serialize keys as private or public.
1290-    bool m_private;
1291+    //! StringType to serialize keys
1292+    DescriptorImpl::StringType m_type;
    hodlinator commented at 3:33 pm on January 29, 2025:

    nit: Could keep style with rest of the fields:

    0    const DescriptorImpl::StringType m_type;
    pythcoiner commented at 2:54 am on January 30, 2025:
    done
  20. in src/script/descriptor.cpp:1346 in 0bc213db76 outdated 
    1342@@ -1332,7 +1343,7 @@ class MiniscriptDescriptor final : public DescriptorImpl
1343     bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type,
1344                         const DescriptorCache* cache = nullptr) const override
1345     {
1346-        if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type == StringType::PRIVATE))) {
1347+        if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type))) {
    hodlinator commented at 3:42 pm on January 29, 2025:

    Should probably forward on the cache argument to the StringMaker.

    0        if (const auto res = m_node->ToString(StringMaker{arg, m_pubkey_args, type, cache})) {
    pythcoiner commented at 2:54 am on January 30, 2025:
    done
  21. hodlinator commented at 7:36 pm on January 29, 2025: contributor

    Code review 0bc213db7686df9ccbd5ef76c5569181851d80a2

    Thanks for taking on this issue! Not very familiar with this area but maybe I can assist in refining the PR anyway. Seems directionally correct to use h consistently for hardened derivation paths unless backwards compatibility is requested.

    PR title/summary

    PR title typo

    acount -> account

    Summary

    0- In `MiniscriptDescriptor::ToStringHelper()` only `StringType::Private` variant of `type` argument was accounted. This PR implement serializing w/ all variants of `StringType` & add a functionnal test for the descriptor triggereing the related issue.
1+ In `MiniscriptDescriptor::ToStringHelper()` only the `StringType::Private` variant of the `type` argument was handled. This PR implements serializing w/ all variants of `StringType` & add a functional test for the descriptor triggering the related issue.

    Maybe the summary could also include a brief explanation of why this change ends up fixing the issue?

    Commit order

    The commit order is wrong for our CI setup, adding the test before the fix. Our CI expects each commit to succeed, so the test needs to be added last (reviewers can drop the other commit or run with binaries from master in order to trigger test failures).

  22. pythcoiner renamed this:
    miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`
    miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`
    on Jan 29, 2025
  23. pythcoiner force-pushed on Jan 30, 2025
  24. pythcoiner commented at 2:55 am on January 30, 2025: none
    Thanks for the review @hodlinator! comments addressed & added more details to the summary
  25. 1440000bytes commented at 6:19 pm on February 1, 2025: none
    Concept ACK
  26. in src/script/descriptor.cpp:1297 in 498c47d931 outdated 
    1296-    StringMaker(const SigningProvider* arg LIFETIMEBOUND, const std::vector<std::unique_ptr<PubkeyProvider>>& pubkeys LIFETIMEBOUND, bool priv)
1297-        : m_arg(arg), m_pubkeys(pubkeys), m_private(priv) {}
1298+    StringMaker(const SigningProvider* arg LIFETIMEBOUND,
1299+                const std::vector<std::unique_ptr<PubkeyProvider>>& pubkeys LIFETIMEBOUND,
1300+                DescriptorImpl::StringType type,
1301+                const DescriptorCache* cache)
    hodlinator commented at 9:16 pm on February 1, 2025: 
    0                const DescriptorCache* cache LIFETIMEBOUND)

    Not sure how much it actually helps, especially on raw pointers, but at least LIFETIMEBOUND documents expectations, and is already used on the first arg which is also a raw pointer. See: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound

    pythcoiner commented at 8:14 am on February 6, 2025:
    done
  27. in test/functional/wallet_listdescriptors.py:141 in 498c47d931 outdated 
    133@@ -134,6 +134,26 @@ def run_test(self):
134         }
135         assert_equal(expected, wallet.listdescriptors())
136 
137+        self.log.info('Test taproot descriptor do not have mixed hardened derivation marker')
138+        node.createwallet(wallet_name='w5', descriptors=True, disable_private_keys=True)
139+        wallet = node.get_wallet_rpc('w5')
140+        wallet.importdescriptors([{
141+            'desc': 'tr([1dce71b2/48\'/1\'/0\'/2\']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48\'/1\'/0\'/2\']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#xhrh0cvn',
    hodlinator commented at 9:32 pm on February 1, 2025:

    nit: More readable if you think it’s tolerable to use double-quotes in this one case.

    0            'desc': "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#xhrh0cvn",
    hodlinator commented at 2:20 pm on February 5, 2025:
    nit: When debugging the test, it was annoying that both of the keys had identical beginnings ([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/), deriving them from different master keys with different derivation paths would help keep them apart.
    pythcoiner commented at 6:25 am on February 6, 2025:
    I go to the \' way instead because python linters often yell w/ double quotes but I’m fine w/ double quote if it’s fit the CI
    pythcoiner commented at 8:14 am on February 6, 2025:
    done
    hodlinator commented at 9:18 pm on February 6, 2025:

    I ran the test/lint/test_runner/ runner (COMMIT_RANGE="HEAD~3..HEAD" cargo run) on my suggestion (which runs ruff if it’s installed). It did not emit any errors, so I think you should be fine.

    Couldn’t find the precise rule against my suggestion, but Q003 seems like it would be in favor of it: https://docs.astral.sh/ruff/rules/#flake8-pytest-style-pt

    pythcoiner commented at 9:15 am on February 7, 2025:
    done
  28. brunoerg commented at 2:48 pm on February 3, 2025: contributor
    Concept ACK
  29. hodlinator commented at 3:05 pm on February 5, 2025: contributor

    Code review 498c47d9315ef79c107460b678f834d19cb9a53e

    Since last review

    git range-diff master 0bc213d 498c47d

    • Reordered commits and avoid touching new test code in 2 commits. ✅
    • Forwards on cache argument from calling function, changed types and const. ✅
    • Improved PR title and summary. ✅

    Concept

    Before the PR, first key origin/derivation path in the descriptor undergoes ' -> h conversion, but the second one does not. There seems to be some code in descriptor.cpp that deliberately tries to preserve apostrophes. I’m unsure whether the change breaks any existing requirements. My instinct says software depending on Bitcoin Core should handle h in all cases. But there may be cases where changing the format leads to errors, worst case leading to users having problems recovering funds. If we go ahead with this PR, it should probably have release notes (example of PR with release notes).

    Wallet and surrounding code is a beast that I don’t have enough familiarity with to produce a Concept A-C-K at this point. Main code being changed seems to be from bfb036756ad6e187fd6d3abfefe5804bb54a5c71 by @darosior and @sipa.

    TODO

    • Lacking LIFETIMEBOUND (see inline comment).
    • nit: PR summary typos: “differents hardened derivation marker” -> “different hardened derivation markers”
  30. pythcoiner force-pushed on Feb 6, 2025
  31. in src/script/descriptor.cpp:5 in a5c0516487 outdated 
    1@@ -2,6 +2,7 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5+#include "attributes.h"
    hodlinator commented at 9:06 pm on February 6, 2025:

    Causes linter failure:

    0[08:12:43.311] Please use bracket syntax includes ("#include <foo.h>") instead of quote syntax includes:
1[08:12:43.311] src/script/descriptor.cpp:#include "attributes.h"

    0#include <attributes.h>
    pythcoiner commented at 9:16 am on February 7, 2025:
    my bad, my clangd add it automatically and I don’t notice. I drop it as it seems redundant
  32. descriptor: account for all StringType in MiniscriptDescriptor::ToStringHelper() 975783cb79
  33. test: check listdescriptors do not return a mix of hardened derivation marker 28a4fcb03c
  34. pythcoiner force-pushed on Feb 7, 2025
  35. pythcoiner commented at 9:39 am on February 7, 2025: none
    tahnks for your time @hodlinator!


pythcoiner DrahtBot hodlinator 1440000bytes brunoerg

Labels
Descriptors

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-02-22 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me