rpc: getdescriptorinfo also returns normalized descriptor #29396

leo-aa88 · 2024-02-07T07:41:51Z

This pull request introduces enhancements to the getdescriptorinfo RPC command, specifically by incorporating the functionality to return the normalized_descriptor alongside the original descriptor information. This improvement addresses the need for a standardized format that facilitates the use of descriptors in public key operations, especially important for scenarios requiring the derivation of addresses without access to private keys.

Related issue: #29320

DrahtBot · 2024-02-07T07:41:56Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	S3RK
Stale ACK	epiccurious

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:

#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

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.

brunoerg · 2024-02-07T13:45:15Z

test  2024-02-07T08:26:22.085000Z TestFramework (ERROR): JSONRPC error 
                                 Traceback (most recent call last):
                                   File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                     self.run_test()
                                   File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_bumpfee.py", line 100, in run_test
                                     test_watchonly_psbt(self, peer_node, rbf_node, dest_address)
                                   File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_bumpfee.py", line 575, in test_watchonly_psbt
                                     pub_rec_desc = rbf_node.getdescriptorinfo(priv_rec_desc)["descriptor"]
                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                   File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
                                     return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                   File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in __call__
                                     raise JSONRPCException(response['error'], status)
                                 test_framework.authproxy.JSONRPCException: Internal bug detected: RPC call "getdescriptorinfo" returned incorrect type:
                                 {
                                     "normalized_descriptor": "key returned that was not in doc"
                                 }
                                 Bitcoin Core v26.99.0-504d50bfa3bb-dirty
                                 Please report this issue here: https://github.com/bitcoin/bitcoin/issues
                                  (-1)

S3RK · 2024-02-08T08:06:33Z

Concept ACK. To fix the error you need to declare new response field. (see around line no. 183)

leo-aa88 · 2024-02-09T00:29:50Z

I'm getting an error in one the functional tests cases in test/functional/rpc_getdescriptorinfo.py.

A fix would be changing the derivation path from 1/2h (hardened) to 1/2 (non-hardened):

-        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)
+        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/2)", isrange=False, issolvable=True, hasprivatekeys=False)

I'm not sure if this change would go against the objectives of the test.

achow101 · 2024-02-09T02:59:51Z

I'm getting an error in one the functional tests cases in test/functional/rpc_getdescriptorinfo.py.

A fix would be changing the derivation path from 1/2h (hardened) to 1/2 (non-hardened):
-        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)
+        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/2)", isrange=False, issolvable=True, hasprivatekeys=False)
I'm not sure if this change would go against the objectives of the test.

You are adding a new field, existing tests should not fail. That means either your test changes to test the new field are incorrect, or your code is incorrect.

leo-aa88 · 2024-02-09T04:25:54Z

I'm getting an error in one the functional tests cases in test/functional/rpc_getdescriptorinfo.py.
A fix would be changing the derivation path from 1/2h (hardened) to 1/2 (non-hardened):
-        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)
+        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/2)", isrange=False, issolvable=True, hasprivatekeys=False)
I'm not sure if this change would go against the objectives of the test.
You are adding a new field, existing tests should not fail. That means either your test changes to test the new field are incorrect, or your code is incorrect.

For the case of hardened derivation path, I guess that instead of throwing a JSONRPCError exception, it could simply leave the normalized descriptor field as empty?

epiccurious · 2024-02-11T02:05:26Z

utACK 9cf7092.

luke-jr · 2024-02-22T15:45:05Z

For the case of hardened derivation path...

...it's documented herein as working :\

leo-aa88 · 2024-02-22T19:56:50Z

For the case of hardened derivation path...

...it's documented herein as working :\

I'm not sure on how to handle this case. I've looked through the docs but it remains unclear if I should adapt the test suite to encompass the new functionality or if the issue is within the new code itself.

S3RK · 2024-02-26T08:03:16Z

test/functional/rpc_getdescriptorinfo.py

        info = self.nodes[0].getdescriptorinfo(desc)
        assert_equal(info, self.nodes[0].getdescriptorinfo(descsum_create(desc)))
        assert_equal(info['descriptor'], descsum_create(desc))
+        assert_equal(info['normalizeddescriptor'], expected_normalized_desc_with_checksum)


This test is never called with a hardened path derivation, so it actually never checks the new logic

Isn't this a hardened path derivation?

bitcoin/test/functional/rpc_getdescriptorinfo.py

Line 60 in 21f1d55

self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)

ah, yes. Missed that. Still "descriptor" and "normalizeddescriptor" are always the same, so it's still not clear why the new field is needed.

DrahtBot · 2024-08-28T16:40:52Z

🐙 This pull request conflicts with the target branch and needs rebase.

achow101 · 2024-10-15T14:20:54Z

Are you still working on this?

DrahtBot · 2025-01-13T10:09:55Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko · 2025-01-13T11:27:06Z

Closing for now. The issue remains open (#29320) and discussion can continue there. This can also be picked up again, a fresh pull can be created, or it can be re-opened.

DrahtBot added RPC/REST/ZMQ CI failed labels Feb 7, 2024

DrahtBot mentioned this pull request Feb 7, 2024

descriptors: Be able to specify change and receiving in a single descriptor string #22838

Merged

leo-aa88 force-pushed the rpc/getdescriptorinfo-also-returns-normalized-descriptor branch from 53d47c0 to 9cf7092 Compare February 8, 2024 21:44

DrahtBot requested a review from S3RK February 11, 2024 02:05

rpc: getdescriptorinfo also returns normalized descriptor

21f1d55

leo-aa88 force-pushed the rpc/getdescriptorinfo-also-returns-normalized-descriptor branch from 62e7e78 to 21f1d55 Compare February 26, 2024 03:15

DrahtBot removed the CI failed label Feb 26, 2024

S3RK reviewed Feb 26, 2024

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Jun 13, 2024

DrahtBot added the Needs rebase label Aug 28, 2024

maflcko closed this Jan 13, 2025

rpc: getdescriptorinfo also returns normalized descriptor #29396

rpc: getdescriptorinfo also returns normalized descriptor #29396

Uh oh!

Conversation

leo-aa88 commented Feb 7, 2024

Uh oh!

DrahtBot commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

brunoerg commented Feb 7, 2024

Uh oh!

S3RK commented Feb 8, 2024

Uh oh!

leo-aa88 commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Feb 9, 2024

Uh oh!

leo-aa88 commented Feb 9, 2024

Uh oh!

epiccurious commented Feb 11, 2024

Uh oh!

luke-jr commented Feb 22, 2024

Uh oh!

leo-aa88 commented Feb 22, 2024

Uh oh!

S3RK Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

leo-aa88 Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

S3RK Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 28, 2024

Uh oh!

achow101 commented Oct 15, 2024

Uh oh!

DrahtBot commented Jan 13, 2025

Uh oh!

maflcko commented Jan 13, 2025

Uh oh!

Uh oh!

DrahtBot commented Feb 7, 2024 •

edited

Loading

leo-aa88 commented Feb 9, 2024 •

edited

Loading