rpc, wallet: Do not return "keypoololdest" for blank descriptor wallets #23348

hebasto · 2021-10-24T19:53:57Z

The "keypoololdest" field in the getwalletinfo RPC response should be used for legacy wallets only.

Th current implementation (04437ee) assumes that CWallet::GetOldestKeyPoolTime() always return 0 for descriptor wallets. This assumption is wrong for blank descriptor wallets, when m_spk_managers is empty. As a result:

$ src/bitcoin-cli -signet -rpcwallet=211024-d-DPK getwalletinfo
{
  "walletname": "211024-d-DPK",
  "walletversion": 169900,
  "format": "sqlite",
  "balance": 0.00000000,
  "unconfirmed_balance": 0.00000000,
  "immature_balance": 0.00000000,
  "txcount": 0,
  "keypoololdest": 9223372036854775807,
  "keypoolsize": 0,
  "keypoolsize_hd_internal": 0,
  "paytxfee": 0.00000000,
  "private_keys_enabled": false,
  "avoid_reuse": false,
  "scanning": false,
  "descriptors": true
}

This PR fixes this issue with direct checking of the WALLET_FLAG_DESCRIPTORS flag.

lsilva01

Code Review and Tested ACK 303ee60

nit: Maybe the comment in DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() can be updated.

laanwj · 2021-10-27T11:23:14Z

Concept ACK

Another solution would be to make DescriptorScriptPubKeyMan::GetOldestKeyPoolTime return std::numeric_limits<int64_t>::max() instead, then check for that instead of 0. I think a slight advantage of that would be that no special-case logic needs to be introduced into the RPC call.

hebasto · 2021-10-27T12:01:40Z

Another solution would be to make DescriptorScriptPubKeyMan::GetOldestKeyPoolTime return std::numeric_limits<int64_t>::max() instead, then check for that instead of 0. I think a slight advantage of that would be that no special-case logic needs to be introduced into the RPC call.

In that case, std::numeric_limits<int64_t>::max() will be used in three different places of the code. Won't it make the code a bit fragile for future changes? Maybe, std::optional could be a better approach, if we do not want to introduce special-case logic into the RPC call?

luke-jr · 2021-10-29T17:01:43Z

Agree with @laanwj's suggestion, and std::optional sounds like a good approach.

hebasto · 2021-10-30T19:31:19Z

Thanks to @lsilva01, @laanwj and @luke-jr for your reviews and suggestions!

Suggested an alternative implementation which is based on @laanwj's and @luke-jr's comments.

luke-jr · 2021-11-03T00:44:28Z

src/wallet/wallet.cpp

+    std::optional<int64_t> oldest_key = std::nullopt;
    for (const auto& spk_man_pair : m_spk_managers) {
-        oldestKey = std::min(oldestKey, spk_man_pair.second->GetOldestKeyPoolTime());
+        if (!oldest_key.has_value()) {
+            oldest_key = spk_man_pair.second->GetOldestKeyPoolTime();
+            continue;
+        }
+        if (!spk_man_pair.second->GetOldestKeyPoolTime().has_value()) {
+            continue;
+        }
+        oldest_key = std::min(oldest_key.value(), spk_man_pair.second->GetOldestKeyPoolTime().value());


This is a behaviour change: Previously, it would return 0 if any of the m_spk_managers returned 0. With the new semantics, it should prefer std::nullopt over a value.

If this change is intentional, please document why.

Thanks! Updated.

hebasto · 2021-11-03T08:23:33Z

Updated 324172e -> da5f866 (pr23348.02 -> pr23348.03, diff):

addressed @luke-jr's comment

This change gets rid of the magic number 0 in the DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() function. No behavior change.

This change suppress the "keypoololdest" field in the getwalletinfo RPC response for blank descriptor wallets.

hebasto · 2021-11-03T08:39:02Z

Updated da5f866 -> ee03c78 (pr23348.03 -> pr23348.04, diff):

split in two commits: a pure refactoring with no behavior change, and a bugfix.

luke-jr · 2021-11-03T22:25:58Z

src/wallet/wallet.cpp

+    std::optional<int64_t> oldest_key{std::numeric_limits<int64_t>::max()};
    for (const auto& spk_man_pair : m_spk_managers) {
-        oldestKey = std::min(oldestKey, spk_man_pair.second->GetOldestKeyPoolTime());
+        oldest_key = std::min(oldest_key, spk_man_pair.second->GetOldestKeyPoolTime());


Couldn't you get a nullopt in here? (If not, why change this block at all?)

Couldn't you get a nullopt in here?

Sorry, I did not get your question. What do you mean?

spk_man_pair.second->GetOldestKeyPoolTime() could return std::nullopt. Is it well-defined what std::min will do here, and is it the desired behaviour?

std::min() uses operator<, which is well-defined for std::optional.

lsilva01

re-ACK ee03c78

Tested std::min() with null value std::optional <int64_t> and it worked as expected.

stratospher

ACK ee03c78.

3e4f069
- The return type of all the GetOldestKeyPoolTime() is changed from int64_t to optional<int64_t>.
- DescriptorScriptPubKeyMan’s GetOldestKeyPoolTime() returns nullopt instead of 0 and changes were made in getwalletinfo() to handle this.
ee03c78
- In CWallet’s GetOldestKeyPoolTime(), If m_spk_managers is not present, nullopt is returned(previously it returned numeric_limits<int64_t>::max()) and handles the case for blank descriptor wallets.

meshcollider

Code review ACK ee03c78

…lank descriptor wallets ee03c78 wallet: Make GetOldestKeyPoolTime return nullopt for blank wallets (Hennadii Stepanov) 3e4f069 wallet, refactor: Make GetOldestKeyPoolTime return type std::optional (Hennadii Stepanov) Pull request description: The "keypoololdest" field in the `getwalletinfo` RPC response should be used for legacy wallets only. Th current implementation (04437ee) assumes that `CWallet::GetOldestKeyPoolTime()` always return `0` for descriptor wallets. This assumption is wrong for _blank_ descriptor wallets, when `m_spk_managers` is empty. As a result: ``` $ src/bitcoin-cli -signet -rpcwallet=211024-d-DPK getwalletinfo { "walletname": "211024-d-DPK", "walletversion": 169900, "format": "sqlite", "balance": 0.00000000, "unconfirmed_balance": 0.00000000, "immature_balance": 0.00000000, "txcount": 0, "keypoololdest": 9223372036854775807, "keypoolsize": 0, "keypoolsize_hd_internal": 0, "paytxfee": 0.00000000, "private_keys_enabled": false, "avoid_reuse": false, "scanning": false, "descriptors": true } ``` This PR fixes this issue with direct checking of the `WALLET_FLAG_DESCRIPTORS` flag. ACKs for top commit: lsilva01: re-ACK ee03c78 stratospher: ACK ee03c78. meshcollider: Code review ACK ee03c78 Tree-SHA512: 9852f9f8ed5c08c07507274d7714f039bbfda66da6df65cf98f67bf11a600167d0f7f872680c95775399477f4df9ba9fce80ec0cbe0adb7f2bb33c3bd65b15df

DrahtBot added RPC/REST/ZMQ Wallet labels Oct 24, 2021

lsilva01 approved these changes Oct 24, 2021

View reviewed changes

hebasto force-pushed the 211024-rpc-gwi branch from 303ee60 to 324172e Compare October 30, 2021 19:28

luke-jr suggested changes Nov 3, 2021

View reviewed changes

hebasto force-pushed the 211024-rpc-gwi branch from 324172e to da5f866 Compare November 3, 2021 08:13

hebasto added 2 commits November 3, 2021 10:35

wallet, refactor: Make GetOldestKeyPoolTime return type std::optional

3e4f069

This change gets rid of the magic number 0 in the DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() function. No behavior change.

wallet: Make GetOldestKeyPoolTime return nullopt for blank wallets

ee03c78

This change suppress the "keypoololdest" field in the getwalletinfo RPC response for blank descriptor wallets.

hebasto force-pushed the 211024-rpc-gwi branch from da5f866 to ee03c78 Compare November 3, 2021 08:36

luke-jr suggested changes Nov 3, 2021

View reviewed changes

lsilva01 approved these changes Nov 4, 2021

View reviewed changes

stratospher reviewed Nov 10, 2021

View reviewed changes

meshcollider reviewed Nov 22, 2021

View reviewed changes

meshcollider merged commit a42923c into bitcoin:master Nov 22, 2021

hebasto deleted the 211024-rpc-gwi branch November 22, 2021 06:00

stickies-v mentioned this pull request Mar 28, 2022

Connection via Bitcoin RPC fails in Bitcoin v23 rc2 sparrowwallet/sparrow#488

Closed

craigraw mentioned this pull request Mar 29, 2022

Connection via Bitcoin RPC fails in Bitcoin v23 rc2 bwt-dev/bwt#95

Closed

bitcoin locked and limited conversation to collaborators Nov 22, 2022

rpc, wallet: Do not return "keypoololdest" for blank descriptor wallets #23348

rpc, wallet: Do not return "keypoololdest" for blank descriptor wallets #23348

Uh oh!

Conversation

hebasto commented Oct 24, 2021

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 27, 2021

Uh oh!

luke-jr commented Oct 29, 2021

Uh oh!

hebasto commented Oct 30, 2021

Uh oh!

luke-jr Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 3, 2021

Uh oh!

hebasto commented Nov 3, 2021

Uh oh!

luke-jr Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

luke-jr Nov 4, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 7, 2021

Choose a reason for hiding this comment

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

laanwj commented Oct 27, 2021 •

edited

Loading