refactor: remove unused param from legacy pubkey interface #27274

Bushstar commented at 8:23 AM on March 17, 2023: contributor

Unused param present in legacy pubkey manager interface. This param will not be used and should be removed to prevent unintended usage.

DrahtBot commented at 8:23 AM on March 17, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, furszy
Stale ACK	achow101

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:

#25297 (wallet: group independent db writes on single batched db transaction by furszy)

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 Refactoring on Mar 17, 2023

maflcko added the label Wallet on Mar 17, 2023

in src/wallet/scriptpubkeyman.cpp:1392 in dcceb622a7 outdated

1384 | @@ -1385,9 +1385,10 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, co
1385 |      WalletLogPrintf("keypool return %d\n", nIndex);
1386 |  }
1387 |  
1388 | -bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type, bool internal)
1389 | +bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type)
1390 |  {
1391 |      assert(type != OutputType::BECH32M);
1392 | +    const bool internal{};
1393 |      if (!CanGetAddresses(internal)) {

Sjors commented at 11:58 AM on March 17, 2023:

The following style is more consistent with what we do in other places:

if (!CanGetAddresses(/*internal*/=false)) {

Bushstar commented at 12:34 PM on March 17, 2023:

There's three uses of internal so defining a local bool seemed cleaner. Update all three locations in the style you linked or leave it as it is?

Bushstar commented at 2:15 PM on March 17, 2023:

Updated.

Sjors commented at 12:10 PM on March 17, 2023: member

Concept ACK

Bushstar force-pushed on Mar 17, 2023

achow101 commented at 9:00 PM on March 17, 2023: member

ACK e1b0353e04e90badeb6d0cfe498001ec7a4174ac

in src/wallet/scriptpubkeyman.cpp:1391 in e1b0353e04 outdated

1384 | @@ -1385,21 +1385,21 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t nIndex, bool fInternal, co
1385 |      WalletLogPrintf("keypool return %d\n", nIndex);
1386 |  }
1387 |  
1388 | -bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type, bool internal)
1389 | +bool LegacyScriptPubKeyMan::GetKeyFromPool(CPubKey& result, const OutputType type)
1390 |  {
1391 |      assert(type != OutputType::BECH32M);
1392 | -    if (!CanGetAddresses(internal)) {
1393 | +    if (!CanGetAddresses(/* internal */ false)) {

fanquake commented at 12:15 PM on March 19, 2023:

    if (!CanGetAddresses(/*internal=*/ false)) {

Bushstar commented at 12:51 PM on March 19, 2023:

Updated as per the suggestion.

Bushstar force-pushed on Mar 19, 2023

fanquake commented at 11:29 AM on March 20, 2023: member

        if (!ReserveKeyFromKeyPool(nIndex, keypool, /*internal=*/ false) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
                                                    ^
./wallet/scriptpubkeyman.h:353:73: note: 'fRequestedInternal' declared here
    bool ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
                                                                        ^
clang-tidy-14 --use-color -p=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/scriptpubkeyman.cpp
wallet/scriptpubkeyman.cpp:1399:53: error: argument name 'internal' in comment does not match parameter name 'fRequestedInternal' [bugprone-argument-comment,-warnings-as-errors]

refactor: remove unused param from legacy pubkey 1869310f3c

Bushstar force-pushed on Mar 20, 2023

in src/wallet/scriptpubkeyman.cpp:1399 in 1869310f3c

1397 |      CKeyPool keypool;
1398 |      {
1399 |          LOCK(cs_KeyStore);
1400 |          int64_t nIndex;
1401 | -        if (!ReserveKeyFromKeyPool(nIndex, keypool, internal) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
1402 | +        if (!ReserveKeyFromKeyPool(nIndex, keypool, /*fRequestedInternal=*/ false) && !m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {

Sjors commented at 10:20 AM on March 30, 2023:

I'm guessing that just dropping the space will fix the CI issue?

/*fRequestedInternal=*/false

Bushstar commented at 10:44 AM on March 30, 2023:

CI is passing now after rename internal to fRequestedInternal. CI checks below appear all green to me.

Sjors commented at 11:57 AM on March 30, 2023:

Oh you're right, I missed the last push.

In the majority of cases we don't use a space after =*/, but since the linter doesn't care, it's not worth touching the PR for imo.

Sjors approved

Sjors commented at 12:05 PM on March 30, 2023: member

ACK 1869310f3cfa4ab26b5090d8a4002eefdc84870e

DrahtBot requested review from achow101 on Mar 30, 2023

furszy approved

furszy commented at 1:58 PM on March 31, 2023: member

ACK 1869310f3cfa4ab26b5090d8a4002eefdc84870e

fanquake merged this on Mar 31, 2023

fanquake closed this on Mar 31, 2023

sidhujag referenced this in commit 91c02adaa9 on Apr 1, 2023

bitcoin locked this on Mar 30, 2024