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

hebasto commented at 7:53 PM on October 24, 2021: member

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

Th current implementation (04437ee721e66a7b76bef5ec2f88dd1efcd03b84) 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.

DrahtBot added the label RPC/REST/ZMQ on Oct 24, 2021

DrahtBot added the label Wallet on Oct 24, 2021

lsilva01 approved

lsilva01 commented at 10:44 PM on October 24, 2021: contributor

Code Review and Tested ACK 303ee60

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

laanwj commented at 11:23 AM on October 27, 2021: member

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 commented at 12:01 PM on October 27, 2021: member

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 commented at 5:01 PM on October 29, 2021: member

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

hebasto force-pushed on Oct 30, 2021

hebasto commented at 7:31 PM on October 30, 2021: member

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.

in src/wallet/wallet.cpp:2184 in 324172ee4f outdated

2182 | +            continue;
2183 | +        }
2184 | +        if (!spk_man_pair.second->GetOldestKeyPoolTime().has_value()) {
2185 | +            continue;
2186 | +        }
2187 | +        oldest_key = std::min(oldest_key.value(), spk_man_pair.second->GetOldestKeyPoolTime().value());

luke-jr commented at 12:44 AM on November 3, 2021:

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.

hebasto commented at 8:23 AM on November 3, 2021:

Thanks! Updated.

luke-jr changes_requested

hebasto force-pushed on Nov 3, 2021

hebasto commented at 8:23 AM on November 3, 2021: member

Updated 324172ee4f3a974a694b44d4c812d7c9a8e752d9 -> da5f8663b858d9bf027143666483ea5e05373344 (pr23348.02 -> pr23348.03, diff):

addressed @luke-jr's comment

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

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

No behavior change.

3e4f069d23

wallet: Make GetOldestKeyPoolTime return nullopt for blank wallets

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

ee03c782ba

hebasto force-pushed on Nov 3, 2021

hebasto commented at 8:39 AM on November 3, 2021: member

Updated da5f8663b858d9bf027143666483ea5e05373344 -> ee03c782ba61993d9e95fa499546cd14cee35445 (pr23348.03 -> pr23348.04, diff):

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

in src/wallet/wallet.cpp:2181 in ee03c782ba

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

luke-jr commented at 10:25 PM on November 3, 2021:

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

hebasto commented at 12:01 AM on November 4, 2021:

Couldn't you get a nullopt in here?

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

luke-jr commented at 1:13 AM on November 4, 2021:

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?

hebasto commented at 6:45 PM on November 7, 2021:

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

luke-jr changes_requested

lsilva01 approved

lsilva01 commented at 1:39 PM on November 4, 2021: contributor

re-ACK ee03c78

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

stratospher commented at 4:07 PM on November 10, 2021: contributor

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 commented at 3:02 AM on November 22, 2021: contributor

Code review ACK ee03c782ba61993d9e95fa499546cd14cee35445

meshcollider merged this on Nov 22, 2021

meshcollider closed this on Nov 22, 2021

hebasto deleted the branch on Nov 22, 2021

sidhujag referenced this in commit 1af44774c0 on Nov 22, 2021

sidhujag referenced this in commit d3a59ddc83 on Nov 23, 2021

DrahtBot locked this on Nov 22, 2022