Refactoring: Clarify code using encrypted

domob1812 commented at 9:31 am on September 4, 2018: contributor

This is a pure refactoring (no functional changes) that clarifies the use of CWallet::encrypted_batch as well as the code using it according to my proposal in #14139. (A more detailed description and rationale can be found there.)

The change is split in two commits that correspond to logical units (matching the description in the proposal): A general cleanup of the code and the use of RAII (as also recommended in the developer guidelines) for setting/resetting encrypted_batch temporarily. In case the second commit is seen as overengineering, I’m happy to change the PR to include just the first commit.

The second commit (using RAII) obsoletes #14138.

fanquake added the label Refactoring on Sep 4, 2018

fanquake added the label Wallet on Sep 4, 2018

DrahtBot commented at 10:45 am on September 4, 2018: 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:

#17261 (Make ScriptPubKeyMan an actual interface and the wallet to have multiple by achow101)
#17260 (Split some CWallet functions into new LegacyScriptPubKeyMan by achow101)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16528 ([WIP] Native Descriptor Wallets (take 2) by achow101)
#14942 (wallet: Make scan / abort status private to CWallet by Empact)

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.

likkaait approved

DrahtBot added the label Needs rebase on Sep 10, 2018

MarcoFalke commented at 8:14 pm on September 20, 2018: member

Still need rebase, so closing for now. Let me know when you want to work on this again, so I can reopen.

MarcoFalke closed this on Sep 20, 2018

domob1812 commented at 7:19 pm on September 24, 2018: contributor

Sorry, I was on vacation without proper internet access and am still travelling now (but with perhaps better options to work on this). So please reopen, or feel free to wait until I’ve rebased it (should happen this or next week at the latest) and reopen then.

MarcoFalke reopened this on Sep 24, 2018

domob1812 force-pushed on Sep 25, 2018

domob1812 commented at 6:08 am on September 25, 2018: contributor

Thanks for reopening, I’ve now rebased.

DrahtBot removed the label Needs rebase on Sep 25, 2018

in src/wallet/wallet.cpp:393 in 578a7333c9 outdated

298-    }
299-    if (!CCryptoKeyStore::AddKeyPubKey(secret, pubkey)) {
300-        if (needsDB) encrypted_batch = nullptr;
301-        return false;
302+    {
303+        std::unique_ptr<WithScopedValue<WalletBatch*>> set_encrypted_batch;

practicalswift commented at 3:32 pm on September 30, 2018:

Why is set_encrypted_batch created in the case of encrypted_batch != nullptr?

domob1812 commented at 6:20 am on October 1, 2018:

I don’t see a way to get the required construction/destruction points with RAII without having this variable (a local variable just inside the conditional block won’t work as it will get destructed too soon). Note that the code as it is creates an empty std::unique_ptr unconditionally (here), and only creates the WithScopedValue object if encrypted_batch == nullptr. That gives us the desired RAII semantics.

practicalswift commented at 6:43 am on October 1, 2018:

Thanks for clarifying!

DrahtBot added the label Needs rebase on Oct 24, 2018

domob1812 force-pushed on Oct 24, 2018

domob1812 commented at 11:06 am on October 24, 2018: contributor

Rebased

DrahtBot removed the label Needs rebase on Oct 24, 2018

meshcollider commented at 12:45 pm on November 10, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14144/commits/77a2ff44596c64c0d4a12885e46b68a1dc1e7984

in src/wallet/wallet.cpp:267 in 77a2ff4459 outdated

178+private:
179+    T& m_reference;
180+    const T m_old_value;
181+
182+public:
183+    explicit WithScopedValue(T& reference, const T& new_value)

practicalswift commented at 9:52 pm on November 12, 2018:

~~Nit: explicit is redundant here?~~

domob1812 commented at 10:23 am on November 13, 2018:

In C++11, there are situations where this prevents a potentially unwanted implicit conversion. But I’m happy to remove the explicit in case there’s an agreement that we do not want to consider those (or actually want to allow them).

practicalswift commented at 10:27 am on November 13, 2018:

Oh, didn’t think about the initializer list case. Thanks for clarifying. Keep it there!

DrahtBot added the label Needs rebase on Dec 14, 2018

domob1812 force-pushed on Dec 14, 2018

domob1812 commented at 7:48 am on December 14, 2018: contributor

Rebased

DrahtBot removed the label Needs rebase on Dec 14, 2018

DrahtBot added the label Needs rebase on Jan 15, 2019

domob1812 force-pushed on Jan 15, 2019

domob1812 commented at 6:55 pm on January 15, 2019: contributor

Rebased

DrahtBot removed the label Needs rebase on Jan 15, 2019

DrahtBot added the label Needs rebase on Jul 12, 2019

Clarify code related to CWallet::encrypted_batch.

This is a pure refactor (no functional changes) that clarifies the usage
and code around CWallet::encrypted_batch.  For the rationale, see
https://github.com/bitcoin/bitcoin/issues/14139#issue-356554677.

610ee17dc9

Use RAII to set and reset encrypted_batch.

Instead of explicitly resetting CWallet::encrypted_batch where it was
changed locally, use an RAII helper class to do it reliably.  This
ensures that the cleanup happens on all code paths, and corresponds to
the corresponding recommendation in the developer guidelines.

This implements the second part of the proposal made in
https://github.com/bitcoin/bitcoin/issues/14139.

10d8b984d6

domob1812 force-pushed on Jul 13, 2019

domob1812 commented at 8:09 am on July 13, 2019: contributor

Rebased

DrahtBot removed the label Needs rebase on Jul 15, 2019

Sjors commented at 4:27 pm on August 15, 2019: member

@achow101 maybe rebase #16341 on this, so there’s more clarity before the major box refactor?

DrahtBot added the label Needs rebase on Oct 29, 2019

DrahtBot commented at 1:21 pm on October 29, 2019: member

domob1812 commented at 2:52 pm on November 14, 2019: contributor

Since this has not been merged in over a year, it seems there’s no interest. Thus I won’t invest any more time trying to keep it rebased, especially with the recent refactoring. I’m still happy to revive this if there is clear interest (then please let me know).

domob1812 closed this on Nov 14, 2019

ryanofsky commented at 3:38 pm on November 14, 2019: member

This also overlaps with current PR #17369. WithScopedValue could help remove some of the cleanup code that PR adds to LegacyScriptPubKeyMan::Encrypt, which I think it would an improvement. And it might help with the descriptor wallets PR #15764 if there is encryption code there (I haven’t looked).

I think more ideally, we could get rid of encrypted_batch member, and just add a generic batch or context member to SigningProvider methods, so the batch could just be passed around as a normal argument, instead of stored off to the side temporarily and checked for everywhere. But this would be a bigger change and might not be worth it.

ryanofsky commented at 3:47 pm on November 14, 2019: member

To summarize comment above, it’s a concept ACK for this PR and extending it to current and future signing providers, since I think it’s an improvement even though a more comprehensive solution may be possible.

Can’t say whether the PR should be reopened though. The biggest challenge for wallet code is always getting review, and there is a lot of related code still needing review https://github.com/bitcoin/bitcoin/projects/12

fanquake removed the label Needs rebase on Aug 20, 2020

DrahtBot locked this on Feb 15, 2022

Refactoring: Clarify code using encrypted_batch in CWallet #14144

Conflicts