rpc: sort listdescriptors result #25931

pull Sjors wants to merge 1 commits into bitcoin:master from Sjors:2022/08/sort-listdescriptors changing 1 files +38 −13

Sjors commented at 6:16 PM on August 25, 2022: member

This puts receive and change descriptors directly below each other.

The change would be simpler if UniValue arrays were sortable.
DrahtBot added the label RPC/REST/ZMQ on Aug 25, 2022
theStack commented at 9:39 PM on August 25, 2022: contributor

Concept ACK

Just a few days ago while playing around with descriptor wallet stuff I thought that a sorted output order for listdescriptors would be more convenient 👍
brunoerg commented at 12:33 AM on August 27, 2022: contributor

Concept ACK
w0xlt approved
w0xlt commented at 6:34 AM on August 27, 2022: contributor

Maybe there could be a well-defined ordering for descriptors (instead of string ordering), but this solution is better than the current code.

ACK https://github.com/bitcoin/bitcoin/pull/25931/commits/4996ae3affa3a3b39c5ec53e74c9e4435b9b871a
kristapsk commented at 8:16 PM on August 27, 2022: contributor

Concept ACK
S3RK commented at 7:14 AM on August 29, 2022: contributor

Concept ACK

in src/wallet/rpc/backup.cpp:1789 in 4996ae3aff outdated

1783 | @@ -1784,25 +1784,40 @@ RPCHelpMan listdescriptors()
1784 |  
1785 |      LOCK(wallet->cs_wallet);
1786 |  
1787 | -    UniValue descriptors(UniValue::VARR);
1788 |      const auto active_spk_mans = wallet->GetActiveScriptPubKeyMans();
1789 | +
1790 | +    std::vector<std::tuple<std::string, WalletDescriptor, bool, std::optional<bool>>> wallet_descriptors;

S3RK commented at 7:14 AM on August 29, 2022:

nit: I find tuples a bit hard to grok. Could we just pass a comparator to GetAllScriptPubKeyMans?

Sjors force-pushed on Aug 29, 2022
Sjors commented at 10:20 AM on August 29, 2022: member

@S3RK I refactored the PR to use a struct (with trivial types) instead of a tuple. This should also make it easier for a future refactor to move some of this stuff out of the RPC and into the wallet (e.g. so that the GUI can also display a list of descriptors).
S3RK commented at 6:41 AM on August 30, 2022: contributor

Code review ACK edbd1aa43f940910f31037770e386e4cf48ab92f
in src/wallet/rpc/backup.cpp:1793 in edbd1aa43f outdated
```
1789 | +
1790 | +    struct WalletDescInfo {
1791 | +        std::string descriptor;
1792 | +        uint64_t creation_time;
1793 | +        bool active;
1794 | +        std::optional<bool> internal;
```
furszy commented at 1:10 PM on August 30, 2022:

this field doesn't need to be an optional. It will always have a value.

S3RK commented at 6:56 AM on August 31, 2022:

when a descriptor is not active we can't determine whether it's internal or not

Sjors commented at 8:38 AM on August 31, 2022:

Indeed, this is why IsInternalScriptPubKeyMan returns std::optional<bool>. We list both active and inactive descriptors.

furszy commented at 4:58 PM on August 31, 2022:

k gotcha, nvm then.
in src/wallet/rpc/backup.cpp:1817 in edbd1aa43f outdated
```
1821 | +            descriptor,
1822 | +            wallet_descriptor.creation_time,
1823 | +            active_spk_mans.count(desc_spk_man) != 0,
1824 | +            wallet->IsInternalScriptPubKeyMan(desc_spk_man),
1825 | +            is_range ? std::optional(std::make_pair(wallet_descriptor.range_start, wallet_descriptor.range_end)) : std::nullopt,
1826 | +            is_range ? wallet_descriptor.next_index : 0
```
furszy commented at 1:13 PM on August 30, 2022:

I don't think that this is needed. if descriptor is ranged, next_index should always be 0.

S3RK commented at 6:58 AM on August 31, 2022:

next_index defines the index at which we will derive next address for this descriptor. What should it be always 0?

Sjors commented at 8:39 AM on August 31, 2022:

next_index is initialized to zero (see wallet_util.h) so I dropped the ternary operator.

furszy commented at 4:56 PM on August 31, 2022:

next_index defines the index at which we will derive next address for this descriptor. What should it be always 0? @S3RK I missed two letters there, wanted to say "if descriptor isn't ranged".
Sjors force-pushed on Aug 31, 2022
rpc: sort listdescriptors result 50996241f2
Sjors force-pushed on Aug 31, 2022
achow101 commented at 4:28 PM on August 31, 2022: member

ACK 50996241f2b0eefeaab4fcd11b9730fa2dc107ae
furszy approved
furszy commented at 5:10 PM on August 31, 2022: member

utACK 50996241
Sjors commented at 7:13 PM on August 31, 2022: member

CI failure is fixed in #25963, not worth rebasing.
w0xlt approved
w0xlt commented at 7:19 PM on August 31, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/25931/commits/50996241f2b0eefeaab4fcd11b9730fa2dc107ae
S3RK commented at 6:27 AM on September 1, 2022: contributor

reACK 50996241f2b0eefeaab4fcd11b9730fa2dc107ae
achow101 merged this on Sep 1, 2022
achow101 closed this on Sep 1, 2022
sidhujag referenced this in commit 5e49d67556 on Sep 1, 2022
Sjors deleted the branch on Sep 2, 2022
fanquake added this to the milestone 24.0 on Sep 15, 2022
theStack referenced this in commit d99af861d0 on Sep 22, 2022
MarcoFalke referenced this in commit eeac05aa22 on Sep 27, 2022
sidhujag referenced this in commit 9bebfdb7fd on Sep 27, 2022
bitcoin locked this on Sep 15, 2023

Contributors

Labels

Milestone
24.0

Linked (view graph)

#26156 test: check that `listdescriptors` descriptor strings are sorted