Replace CWallet::Set* functions that use memonly with Add/Load variants

achow101 commented at 3:33 am on May 22, 2020: member

SetHDChaiin, SetActiveScriptPubKeyMan, and SetWalletFlags have a memonly argument which is kind of confusing, as noted in #17681 (review). 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.

Split SetHDChain into AddHDChain and LoadHDChain

Remove the memonly bool and follow our typical Add and Load pattern.

0122fbab4c

Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan

Remove the memonly bool and follow the Add and Load pattern we use
everywhere else.

d9cd095b59

fanquake added the label Wallet on May 22, 2020

laanwj commented at 12:50 pm on May 26, 2020: member

I agree that having separate Add/Load functions instead of a confusing boolean argument improves understandability of the code quite a bit. Code review ACK f123374a661d54e091108e490f8c4cc9ebc0d251

Empact commented at 9:08 pm on May 26, 2020: member

Concept ACK

in src/wallet/wallet.cpp:1405 in f123374a66 outdated

1402+}
1403+
1404+bool CWallet::AddWalletFlags(uint64_t flags)
1405+{
1406+    LOCK(cs_wallet);
1407+    if (!WalletBatch(*database).WriteWalletFlags(flags)) {

Empact commented at 9:11 pm on May 26, 2020:

In the existing code, on Add, the contains unknown non-tolerable wallet flags test and bail will occur first and prevent writing the database. With this change, the values are written, and the failure is only identified after, which seems improper.

Empact commented at 9:12 pm on May 26, 2020:

Perhaps add a test for this case?

achow101 commented at 3:49 pm on June 8, 2020:

That failure is only relevant when loading a wallet from disk. AddWalletFlags is only used internally in wallet creation; we would never add flags that we don’t know.

jnewbery commented at 2:43 pm on June 22, 2020:

I agree that this seems less safe than the existing code. Is it possible to maintain the current order of checking first and then writing to disk?

ryanofsky commented at 3:01 pm on June 22, 2020:

I agree that this seems less safe than the existing code.

In current code m_wallet_flags can be updated even if database write fails leading to inconsistency between disk and memory state and possibility for data inconsistent with flags to subseqently be written to wallet. New code writing to disk first, memory second, prevents this type of issue and would seem to be more safe. What’s the scenario where it’s less safe?

jnewbery commented at 3:25 pm on June 22, 2020:

If this function is called with an unknown wallet flag, it’ll be written to disk and then won’t be applied to the in-memory wallet (and won’t be able to be loaded later), which is also a data inconsistency.

The unknown flag check was added to protect when loading a wallet from a newer version on an older version, so I think your concern is more valid. I don’t see any harm in a sanity assert that we don’t try to add an unknown flag to the wallet before writing to disk.

achow101 commented at 6:59 pm on June 22, 2020:

I’ve added an assert in AddWalletFlags to handle that case.

DrahtBot commented at 8:10 pm on May 27, 2020: 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:

#18354 (Use shared pointers only in validation interface by bvbfan)
#16546 (External signer support - Wallet Box edition by Sjors)

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 approved

ryanofsky commented at 10:46 pm on June 18, 2020: member

Code review ACK f123374a661d54e091108e490f8c4cc9ebc0d251. Contra #19046 (review) 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

Split SetWalletFlags into Add/LoadWalletFlags

Remove memonly bool and follow typical Add and Load pattern used
everywhere else.

3a9aba21a4

achow101 force-pushed on Jun 22, 2020

in src/wallet/wallet.cpp:1403 in 3a9aba21a4

1402+}
1403+
1404+bool CWallet::AddWalletFlags(uint64_t flags)
1405+{
1406+    LOCK(cs_wallet);
1407+    // We should never be writing unknown onon-tolerable wallet flags

jnewbery commented at 7:39 pm on June 22, 2020:

s/onon/non/

achow101 commented at 3:52 pm on June 25, 2020:

Oops. I’ll fix that if I have to push again.

jnewbery commented at 7:39 pm on June 22, 2020: member

Code review ACK 3a9aba21a49a6d80bd187940d5e26893937b6832

One small typo in a comment if you feel like retouching.

in src/wallet/wallet.cpp:1404 in 3a9aba21a4

1403+
1404+bool CWallet::AddWalletFlags(uint64_t flags)
1405+{
1406+    LOCK(cs_wallet);
1407+    // We should never be writing unknown onon-tolerable wallet flags
1408+    assert(!(((flags & KNOWN_WALLET_FLAGS) >> 32) ^ (flags >> 32)));

ryanofsky commented at 8:01 pm on June 22, 2020:

In commit “Split SetWalletFlags into Add/LoadWalletFlags” (3a9aba21a49a6d80bd187940d5e26893937b6832)

If you wind up updating check may be more readable as assert(((flags & KNOWN_WALLET_FLAGS) >> 32) == (flags >> 32))

ryanofsky approved

ryanofsky commented at 8:04 pm on June 22, 2020: member

Code review ACK 3a9aba21a49a6d80bd187940d5e26893937b6832. Only changes since last review tweaks making m_wallet_flags updates more safe

meshcollider commented at 10:57 am on July 11, 2020: contributor

utACK 3a9aba21a49a6d80bd187940d5e26893937b6832

meshcollider merged this on Jul 11, 2020

meshcollider closed this on Jul 11, 2020

sidhujag referenced this in commit 51b42c40d7 on Jul 11, 2020

meshcollider referenced this in commit 32302e5c88 on Jul 12, 2020

sidhujag referenced this in commit 8263f27d4e on Jul 12, 2020

Fabcien referenced this in commit 1147f7663b on Jun 7, 2021

Fabcien referenced this in commit fee8cf69cb on Jun 7, 2021

Fabcien referenced this in commit 4b6a468d3e on Jun 7, 2021

DrahtBot locked this on Feb 15, 2022

Replace CWallet::Set* functions that use memonly with Add/Load variants #19046

Conflicts