rpc: getdescriptorinfo also returns normalized descriptor

araujo88 commented at 7:41 am on February 7, 2024: contributor

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.

DrahtBot commented at 7:41 am on February 7, 2024: contributor

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.

DrahtBot added the label RPC/REST/ZMQ on Feb 7, 2024

DrahtBot added the label CI failed on Feb 7, 2024

brunoerg commented at 1:45 pm on February 7, 2024: contributor

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

S3RK commented at 8:06 am on February 8, 2024: contributor

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

araujo88 force-pushed on Feb 8, 2024

araujo88 commented at 0:29 am on February 9, 2024: contributor

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):

0-        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)
1+        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 commented at 2:59 am on February 9, 2024: member

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):
0-        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)
1+        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.

araujo88 commented at 4:25 am on February 9, 2024: contributor

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):
0-        self.test_desc("pkh(tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1h/2)", isrange=False, issolvable=True, hasprivatekeys=False)
1+        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 commented at 2:05 am on February 11, 2024: contributor

utACK 9cf7092474f9b8faaa59fce0a6c26a4df705266c.

DrahtBot requested review from S3RK on Feb 11, 2024

luke-jr commented at 3:45 pm on February 22, 2024: member

For the case of hardened derivation path…

…it’s documented herein as working :\

araujo88 commented at 7:56 pm on February 22, 2024: contributor

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.

rpc: getdescriptorinfo also returns normalized descriptor 21f1d55084

araujo88 force-pushed on Feb 26, 2024

DrahtBot removed the label CI failed on Feb 26, 2024

in test/functional/rpc_getdescriptorinfo.py:27 in 21f1d55084

19@@ -20,9 +20,11 @@ def set_test_params(self):
20         self.wallet_names = []
21 
22     def test_desc(self, desc, isrange, issolvable, hasprivatekeys):
23+        expected_normalized_desc_with_checksum = descsum_create(desc)
24         info = self.nodes[0].getdescriptorinfo(desc)
25         assert_equal(info, self.nodes[0].getdescriptorinfo(descsum_create(desc)))
26         assert_equal(info['descriptor'], descsum_create(desc))
27+        assert_equal(info['normalizeddescriptor'], expected_normalized_desc_with_checksum)

S3RK commented at 8:03 am on February 26, 2024:

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

araujo88 commented at 8:35 am on February 26, 2024:

Isn’t this a hardened path derivation?

https://github.com/bitcoin/bitcoin/blob/21f1d55084582c8a44509942d08d54f5c59ebd08/test/functional/rpc_getdescriptorinfo.py#L60

S3RK commented at 8:44 am on February 26, 2024:

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 added the label CI failed on Jun 13, 2024

DrahtBot removed the label CI failed on Jun 14, 2024

DrahtBot added the label Needs rebase on Aug 28, 2024

DrahtBot commented at 4:40 pm on August 28, 2024: contributor

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

rpc: getdescriptorinfo also returns normalized descriptor #29396

Code Coverage

Reviews

Conflicts