LegacyScriptPubKeyMan code cleanups #17381

ryanofsky commented at 4:04 PM on November 5, 2019: member

This PR implements suggested code cleanups from #17300 and #17304 review comments

Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp

This also fixes unused variable warnings in rpcdump.cpp

b07b07cd87

Pass CTxDestination to ScriptPubKeyMan::GetMetadata

Pass CTxDestination instead of more ambiguous uint160 hash value. This is more
type safe and more efficient since it avoids doing map lookups that will always
fail and were not done previously before
a18edd7b383d667b15b6d4b87aa3a055a9fa5051 from
https://github.com/bitcoin/bitcoin/pull/17304

Change suggested by Andrew Chow <achow101-github@achow101.com> in
https://github.com/bitcoin/bitcoin/pull/17304#discussion_r340345745 and
https://github.com/bitcoin/bitcoin/pull/17381#issuecomment-549994944

4a0abf694e

Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method

Previous discussion https://github.com/bitcoin/bitcoin/pull/17304#discussion_r340307903

491a599b37

Clean up nested scope in GetReservedDestination

Suggested https://github.com/bitcoin/bitcoin/pull/17304#discussion_r341194391
by Gregory Sanders <gsanders87@gmail.com>

Reason for keeping the `return true` `return false` verbosity is that more code
will be added after the ReserveKeyFromKeyPool() call before returning.

bfd826a675

Add missing SetupGeneration error handling in EncryptWallet

Suggested https://github.com/bitcoin/bitcoin/pull/17304#discussion_r341286026
by me

05b224a175

ryanofsky added this to the "PRs" column in a project

ryanofsky force-pushed on Nov 5, 2019

fanquake added the label Wallet on Nov 5, 2019

in src/wallet/rpcwallet.cpp:3769 in 1c93192ae6 outdated

3765 | @@ -3766,14 +3766,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
3766 |      ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan();
3767 |      if (spk_man) {
3768 |          CKeyID key_id = GetKeyForDestination(*provider, dest);

Sjors commented at 5:34 PM on November 5, 2019:

Maybe add a comment or assertion that only LegacyScriptPubKeyMan should use key_id.

Sjors approved

Sjors commented at 5:42 PM on November 5, 2019: member

Code review ACK ~5ed6daf4b703b6811d7b9f366edba35042de31f7~ 5b3a0b4

ryanofsky force-pushed on Nov 5, 2019

ryanofsky commented at 6:32 PM on November 5, 2019: member

Updated 5ed6daf4b703b6811d7b9f366edba35042de31f7 -> 5b3a0b4fbebcb84c6ab83df40c00be075d5ef28b (pr/keyman-cleanup2.2 -> pr/keyman-cleanup2.3, compare) with requested code comment

achow101 commented at 8:01 PM on November 5, 2019: member

I'm not sure that the GetMetadata change is really the right direction. What I was thinking was more of passing in the scriptPubKey directly (or maybe a CTxDestination?) and pulling from it the key id or script id via the Solver.

Everything else looks good.

DrahtBot commented at 8:21 PM on November 5, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17373 (wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet by achow101)
#17283 (rpc: improve getaddressinfo test coverage, help, code docs by jonatack)
#17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
#16528 ([WIP] Native Descriptor Wallets (take 2) 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.

ryanofsky force-pushed on Nov 5, 2019

ryanofsky referenced this in commit e7c2161f90 on Nov 5, 2019

ryanofsky force-pushed on Nov 5, 2019

ryanofsky commented at 9:15 PM on November 5, 2019: member

Updated 5b3a0b4fbebcb84c6ab83df40c00be075d5ef28b -> 1f54dd2184cd41268c5c6739befaec4cd60219ae (pr/keyman-cleanup2.3 -> pr/keyman-cleanup2.4, compare) replacing GetMetadata CScript argument with CTxDestination

achow101 commented at 10:45 PM on November 5, 2019: member

Code review ACK 1f54dd2184cd41268c5c6739befaec4cd60219ae

Sjors commented at 7:44 AM on November 6, 2019: member

AppVeyor failure (could be random, I restarted it):

      C:\projects\bitcoin\build_msvc\x64\Release\libleveldb\libleveldb.lib
2433         libzmq-mt-s-4_3_3.lib(zmq.cpp.obj) : MSIL .netmodule or module compiled with /GL found; restarting link with /LTCG; add /LTCG to the link command line to improve linker performance
2434    14>bech32_tests.obj : error LNK2001: unresolved external symbol "bool __cdecl CaseInsensitiveEqual(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (?CaseInsensitiveEqual@@YA_NAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@0@Z) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
2435    14>C:\projects\bitcoin\build_msvc\x64\Release\test_bitcoin.exe : fatal error LNK1120: 1 unresolved externals [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
2436    14>Done Building Project "C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj" (default targets) -- FAILED.
2437     1>Done Building Project "C:\projects\bitcoin\build_msvc\bitcoin.sln" (default targets) -- FAILED.
2438
2439Build FAILED.
2440
2441       "C:\projects\bitcoin\build_msvc\bitcoin.sln" (default target) (1) ->
2442       "C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj" (default target) (14) ->
2443       (Link target) -> 
2444         bech32_tests.obj : error LNK2001: unresolved external symbol "bool __cdecl CaseInsensitiveEqual(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (?CaseInsensitiveEqual@@YA_NAEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@0@Z) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
2445         C:\projects\bitcoin\build_msvc\x64\Release\test_bitcoin.exe : fatal error LNK1120: 1 unresolved externals [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
2446
2447    0 Warning(s)
2448    2 Error(s)

Sjors commented at 8:14 AM on November 6, 2019: member

Code review ACK 1f54dd2

ryanofsky commented at 1:40 PM on November 6, 2019: member

AppVeyor failure (could be random, I restarted it):

Appveyor error is apparently unrelated and fixed by #17384 (comment)

ryanofsky force-pushed on Nov 6, 2019

ryanofsky commented at 2:11 PM on November 6, 2019: member

Updated 1f54dd2184cd41268c5c6739befaec4cd60219ae -> 05b224a175065aee4d6d9c471722bc4503f01fdf (pr/keyman-cleanup2.4 -> pr/keyman-cleanup2.5, compare) just fixing now outdated GetMetadata comment

Sjors commented at 4:01 PM on November 6, 2019: member

re-ACK 05b224a

laanwj commented at 4:23 PM on November 6, 2019: member

Code review ACK 05b224a175065aee4d6d9c471722bc4503f01fdf

laanwj referenced this in commit 976cc766c4 on Nov 6, 2019

laanwj merged this on Nov 6, 2019

laanwj closed this on Nov 6, 2019

sidhujag referenced this in commit 19238f8e6c on Nov 7, 2019

fanquake moved this from the "PRs" to the "Done" column in a project

HashUnlimited referenced this in commit 5a7db3b40f on Apr 17, 2020

jasonbcox referenced this in commit 6c1781a66c on Sep 11, 2020

jasonbcox referenced this in commit 0b5daf4ba8 on Sep 11, 2020

jasonbcox referenced this in commit fa3481add5 on Sep 11, 2020

jasonbcox referenced this in commit 709706b9d7 on Sep 11, 2020

jasonbcox referenced this in commit 7e4e5d2a9f on Sep 11, 2020

sidhujag referenced this in commit 682a4772a0 on Nov 10, 2020

DrahtBot locked this on Dec 16, 2021