Replace CWallet::Set* functions that use memonly with Add/Load variants #19046
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove the memonly bool and follow our typical Add and Load pattern.
Remove the memonly bool and follow the Add and Load pattern we use everywhere else.
|
I agree that having separate Add/Load functions instead of a confusing boolean argument improves understandability of the code quite a bit. |
|
Concept ACK |
Empact
reviewed
May 26, 2020
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
This was referenced Jun 11, 2020
ryanofsky
approved these changes
Jun 18, 2020
There was a problem hiding this comment.
Code review ACK f123374. Contra #19046 (comment) it seems good to me that these methods now try to update the wallet database first and return early on failure before updating the state in memory. Haven't thought about it too much but it seems better if that there's an error writing to disk for some reason, operation just fails and wallet stays in the current state.
@jnewbery, you may be interested in this PR since it was inspired by your review comment
Remove memonly bool and follow typical Add and Load pattern used everywhere else.
f123374 to
3a9aba2
Compare
jnewbery
reviewed
Jun 22, 2020
| bool CWallet::AddWalletFlags(uint64_t flags) | ||
| { | ||
| LOCK(cs_wallet); | ||
| // We should never be writing unknown onon-tolerable wallet flags |
There was a problem hiding this comment.
Oops. I'll fix that if I have to push again.
ryanofsky
approved these changes
Jun 22, 2020
| { | ||
| LOCK(cs_wallet); | ||
| // We should never be writing unknown onon-tolerable wallet flags | ||
| assert(!(((flags & KNOWN_WALLET_FLAGS) >> 32) ^ (flags >> 32))); |
There was a problem hiding this comment.
In commit "Split SetWalletFlags into Add/LoadWalletFlags" (3a9aba2)
If you wind up updating check may be more readable as assert(((flags & KNOWN_WALLET_FLAGS) >> 32) == (flags >> 32))
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 11, 2020
… with Add/Load variants 3a9aba2 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow) d9cd095 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow) 0122fba Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow) Pull request description: `SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in bitcoin#17681 (comment). This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet. `AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet. `LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk. ACKs for top commit: jnewbery: Code review ACK 3a9aba2 ryanofsky: Code review ACK 3a9aba2. Only changes since last review tweaks making m_wallet_flags updates more safe meshcollider: utACK 3a9aba2 Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f
meshcollider
added a commit
that referenced
this pull request
Jul 12, 2020
facd7dd wallet: Fix typo in comments; Simplify assert (MarcoFalke) Pull request description: Follow up to #19046 (comment) and #19046 (comment) ACKs for top commit: practicalswift: ACK facd7dd jonatack: ACK facd7dd hebasto: ACK facd7dd, spelling verified with `test/lint/lint-spelling.sh`: all remaining warnings are false positive. Tree-SHA512: 2b185d138058840db56726bb6bcc42e5288a954e2a410c49e04806a047fbbdaf0bb2decc70ecf7613c69caa766655705ca44151613e7ea5015b386d1e726d870
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jul 12, 2020
facd7dd wallet: Fix typo in comments; Simplify assert (MarcoFalke) Pull request description: Follow up to bitcoin#19046 (comment) and bitcoin#19046 (comment) ACKs for top commit: practicalswift: ACK facd7dd jonatack: ACK facd7dd hebasto: ACK facd7dd, spelling verified with `test/lint/lint-spelling.sh`: all remaining warnings are false positive. Tree-SHA512: 2b185d138058840db56726bb6bcc42e5288a954e2a410c49e04806a047fbbdaf0bb2decc70ecf7613c69caa766655705ca44151613e7ea5015b386d1e726d870
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 7, 2021
Summary: Remove the memonly bool and follow our typical Add and Load pattern. This is a backport of [[bitcoin/bitcoin#19046 | core#19046]] [1/3] bitcoin/bitcoin@0122fba Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9630
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 7, 2021
Summary: Remove the memonly bool and follow the Add and Load pattern we use everywhere else. This is a backport of [[bitcoin/bitcoin#19046 | core#19046]] [2/3] bitcoin/bitcoin@d9cd095 Depends on D9630 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9631
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 7, 2021
Summary: Remove memonly bool and follow typical Add and Load pattern used everywhere else. This is a backport of [[bitcoin/bitcoin#19046 | core#19046]] [3/3] bitcoin/bitcoin@3a9aba2 Depends on D9631 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9632
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SetHDChaiin,SetActiveScriptPubKeyMan, andSetWalletFlagshave amemonlyargument which is kind of confusing, as noted in #17681 (comment). This PR replaces those functions withAdd*andLoad*variants so that they follow the pattern used elsewhere in the wallet.AddHDChain,AddActiveScriptPubKeyMan, andAddWalletFlagsboth set their respective variables inCWalletand writes them to disk. These functions are used by the actions which modify the wallet such assethdseed,importdescriptors, and creating a new wallet.LoadHDChain,LoadActiveScriptPubKeyMan, andLoadWalletFlagsjust set theCWalletvariables. These functions are used byLoadWalletwhen loading the wallet from disk.