LegacyScriptPubKeyMan code cleanups #17381

ryanofsky · 2019-11-05T16:04:17Z

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

This also fixes unused variable warnings in rpcdump.cpp

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 a18edd7 from bitcoin#17304 Change suggested by Andrew Chow <achow101-github@achow101.com> in bitcoin#17304 (comment) and bitcoin#17381 (comment)

Previous discussion bitcoin#17304 (comment)

Suggested bitcoin#17304 (comment) 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.

Suggested bitcoin#17304 (comment) by me

Sjors

Code review ACK ~~5ed6daf~~ 5b3a0b4

src/wallet/rpcwallet.cpp

ryanofsky · 2019-11-05T18:32:14Z

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

achow101 · 2019-11-05T20:01:35Z

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 · 2019-11-05T20:21:10Z

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.

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 a18edd7 from bitcoin#17304 Change suggested by Andrew Chow <achow101-github@achow101.com> in bitcoin#17304 (comment) and bitcoin#17381 (comment)

ryanofsky · 2019-11-05T21:15:02Z

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

achow101 · 2019-11-05T22:45:47Z

Code review ACK 1f54dd2

Sjors · 2019-11-06T07:44:27Z

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 · 2019-11-06T08:14:34Z

Code review ACK 1f54dd2

ryanofsky · 2019-11-06T13:40:36Z

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

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

ryanofsky · 2019-11-06T14:11:38Z

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

Sjors · 2019-11-06T16:01:12Z

re-ACK 05b224a

laanwj · 2019-11-06T16:23:09Z

Code review ACK 05b224a

05b224a Add missing SetupGeneration error handling in EncryptWallet (Russell Yanofsky) bfd826a Clean up nested scope in GetReservedDestination (Russell Yanofsky) 491a599 Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method (Russell Yanofsky) 4a0abf6 Pass CTxDestination to ScriptPubKeyMan::GetMetadata (Russell Yanofsky) b07b07c Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from #17300 and #17304 review comments ACKs for top commit: Sjors: re-ACK 05b224a laanwj: Code review ACK 05b224a Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45

05b224a Add missing SetupGeneration error handling in EncryptWallet (Russell Yanofsky) bfd826a Clean up nested scope in GetReservedDestination (Russell Yanofsky) 491a599 Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method (Russell Yanofsky) 4a0abf6 Pass CTxDestination to ScriptPubKeyMan::GetMetadata (Russell Yanofsky) b07b07c Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from bitcoin#17300 and bitcoin#17304 review comments ACKs for top commit: Sjors: re-ACK 05b224a laanwj: Code review ACK 05b224a Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45

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 a18edd7 from bitcoin#17304 Change suggested by Andrew Chow <achow101-github@achow101.com> in bitcoin#17304 (comment) and bitcoin#17381 (comment)

…llet.cpp Summary: This also fixes unused variable warnings in rpcdump.cpp --- bitcoin/bitcoin@b07b07c Partial backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7416

Summary: 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 bitcoin/bitcoin@a18edd7 Change suggested by Andrew Chow <achow101-github@achow101.com> in bitcoin/bitcoin#17304 (comment) and bitcoin/bitcoin#17381 (comment) --- bitcoin/bitcoin@4a0abf6 Depends on D7416 Partial backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7417

…pKeyPool method Summary: Previous discussion bitcoin/bitcoin#17304 (comment) --- bitcoin/bitcoin@491a599 Depends on D7417 Partial backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7418

Summary: Suggested bitcoin/bitcoin#17304 (comment) 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. --- bitcoin/bitcoin@bfd826a Depends on D7418 Partial backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7419

…cryptWallet Summary: Suggested bitcoin/bitcoin#17304 (comment) by me --- bitcoin/bitcoin@05b224a Depends on D7419 Concludes backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7420

05b224a Add missing SetupGeneration error handling in EncryptWallet (Russell Yanofsky) bfd826a Clean up nested scope in GetReservedDestination (Russell Yanofsky) 491a599 Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method (Russell Yanofsky) 4a0abf6 Pass CTxDestination to ScriptPubKeyMan::GetMetadata (Russell Yanofsky) b07b07c Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from bitcoin#17300 and bitcoin#17304 review comments ACKs for top commit: Sjors: re-ACK 05b224a laanwj: Code review ACK 05b224a Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45

ryanofsky added 5 commits November 5, 2019 10:13

Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp

b07b07c

This also fixes unused variable warnings in rpcdump.cpp

Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method

491a599

Previous discussion bitcoin#17304 (comment)

Clean up nested scope in GetReservedDestination

bfd826a

Suggested bitcoin#17304 (comment) 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.

Add missing SetupGeneration error handling in EncryptWallet

05b224a

Suggested bitcoin#17304 (comment) by me

ryanofsky mentioned this pull request Nov 5, 2019

wallet: remove unused variable spk_man in import* RPCs #17377

Closed

ryanofsky force-pushed the pr/keyman-cleanup2 branch from b89bb40 to 5ed6daf Compare November 5, 2019 16:27

fanquake added the Wallet label Nov 5, 2019

Sjors approved these changes Nov 5, 2019

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/keyman-cleanup2 branch from 5ed6daf to 5b3a0b4 Compare November 5, 2019 18:31

ryanofsky force-pushed the pr/keyman-cleanup2 branch from 5b3a0b4 to fed8b28 Compare November 5, 2019 21:05

ryanofsky force-pushed the pr/keyman-cleanup2 branch from fed8b28 to 1f54dd2 Compare November 5, 2019 21:10

ryanofsky force-pushed the pr/keyman-cleanup2 branch from 1f54dd2 to 05b224a Compare November 6, 2019 14:10

laanwj merged commit 05b224a into bitcoin:master Nov 6, 2019

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

LegacyScriptPubKeyMan code cleanups #17381

LegacyScriptPubKeyMan code cleanups #17381

Uh oh!

ryanofsky commented Nov 5, 2019

Uh oh!

Sjors left a comment •

edited

Loading

Uh oh!

Uh oh!

ryanofsky commented Nov 5, 2019

Uh oh!

achow101 commented Nov 5, 2019 •

edited

Loading

Uh oh!

DrahtBot commented Nov 5, 2019

Uh oh!

ryanofsky commented Nov 5, 2019

Uh oh!

achow101 commented Nov 5, 2019

Uh oh!

Sjors commented Nov 6, 2019

Uh oh!

Sjors commented Nov 6, 2019

Uh oh!

ryanofsky commented Nov 6, 2019

Uh oh!

ryanofsky commented Nov 6, 2019

Uh oh!

Sjors commented Nov 6, 2019

Uh oh!

laanwj commented Nov 6, 2019

Uh oh!

Uh oh!

LegacyScriptPubKeyMan code cleanups #17381

LegacyScriptPubKeyMan code cleanups #17381

Uh oh!

Conversation

ryanofsky commented Nov 5, 2019

Uh oh!

Sjors left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented Nov 5, 2019

Uh oh!

achow101 commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 5, 2019

Conflicts

Uh oh!

ryanofsky commented Nov 5, 2019

Uh oh!

achow101 commented Nov 5, 2019

Uh oh!

Sjors commented Nov 6, 2019

Uh oh!

Sjors commented Nov 6, 2019

Uh oh!

ryanofsky commented Nov 6, 2019

Uh oh!

ryanofsky commented Nov 6, 2019

Uh oh!

Sjors commented Nov 6, 2019

Uh oh!

laanwj commented Nov 6, 2019

Uh oh!

Uh oh!

Sjors left a comment •

edited

Loading

achow101 commented Nov 5, 2019 •

edited

Loading