refactor: use CWallet const shared pointers in dump{privkey,wallet} #22805

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202108-refactor-const_correctness_for_further_dump_methods changing 3 files +21 −8

theStack commented at 1:33 PM on August 26, 2021: member

~~This PR is based on #22787 ("refactor: actual immutable pointing"), which should be reviewed first.~~ (merged by now)

It aims to make the CWallet shared pointers actually immutable also for the dumpprivkey and dumpwallet RPC methods. For doing that, some more preparations are needed; we need a const-counterpart to the helper EnsureLegacyScriptPubKeyMan that accepts a const CWallet pointer and accordingly also returns a const LegacyScriptPubKeyMan instance. The metadata lookup in dumpwallet is changed to not need a mutable ScriptPubKeyMan instance by avoiding using the operator[] in its mapKeyMetadata map, which also avoids repeated lookups.
fanquake added the label Wallet on Aug 26, 2021
fanquake added the label Refactoring on Aug 26, 2021
DrahtBot commented at 8:50 PM on August 26, 2021: 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:
- #11803 (Bugfix: RPC/Wallet: Include HD key metadata in dumpwallet by luke-jr)
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 Needs rebase on Sep 3, 2021
theStack force-pushed on Sep 5, 2021
DrahtBot removed the label Needs rebase on Sep 5, 2021
DrahtBot added the label Needs rebase on Sep 30, 2021
theStack force-pushed on Oct 2, 2021
theStack commented at 9:14 PM on October 2, 2021: member

Rebased on #22787 again (which in turn rebased on master recently).
DrahtBot removed the label Needs rebase on Oct 2, 2021
DrahtBot added the label Needs rebase on Oct 22, 2021
theStack force-pushed on Oct 28, 2021
theStack commented at 11:44 PM on October 28, 2021: member

Rebased on #22787 again (which in turn rebased on master recently).
DrahtBot removed the label Needs rebase on Oct 29, 2021
laanwj commented at 4:08 PM on November 10, 2021: member

Concept ACK, making const-wallet operations take const pointers is a good precaution.

Code review ACK d150fe3ad5ce181511f2d2fd035a10530eaa4203

refactor: avoid multiple key->metadata lookups in dumpwallet RPC

This also enables working with a const ScriptPubKeyMan which was
previously not possible due to std::map::operator[] not being const.

29905c092f

refactor: use const `LegacyScriptPubKeyMan` references in dump{privkey,wallet} RPCs ec2792d1dc
refactor: use `CWallet` const shared pointers in dump{privkey,wallet} RPCs d150fe3ad5
MarcoFalke commented at 4:15 PM on November 10, 2021: member

Maybe rebase once more for fun?
theStack force-pushed on Nov 10, 2021
theStack commented at 4:25 PM on November 10, 2021: member

Maybe rebase once more for fun?

Done :upside_down_face:
laanwj merged this on Nov 10, 2021
laanwj closed this on Nov 10, 2021
sidhujag referenced this in commit a5de714ef0 on Nov 10, 2021
DrahtBot locked this on Nov 10, 2022

Contributors

Labels

Linked (view graph)

#22787 refactor: actual immutable pointing

refactor: use CWallet const shared pointers in dump{privkey,wallet} #22805

Conflicts