Refactoring: Clarify code using encrypted_batch in CWallet#14144
Refactoring: Clarify code using encrypted_batch in CWallet#14144domob1812 wants to merge 2 commits into
Conversation
|
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. |
|
Still need rebase, so closing for now. Let me know when you want to work on this again, so I can reopen. |
|
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. |
domob1812
force-pushed
the
encrypted-batch
branch
from
373ab6b to
578a733
Compare
|
Thanks for reopening, I've now rebased. |
There was a problem hiding this comment.
Why is set_encrypted_batch created in the case of encrypted_batch != nullptr?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for clarifying!
domob1812
force-pushed
the
encrypted-batch
branch
from
578a733 to
77a2ff4
Compare
|
Rebased |
|
utACK 77a2ff4 |
domob1812
force-pushed
the
encrypted-batch
branch
from
77a2ff4 to
f4243e3
Compare
|
Rebased |
domob1812
force-pushed
the
encrypted-batch
branch
from
f4243e3 to
151ada5
Compare
|
Rebased |
This is a pure refactor (no functional changes) that clarifies the usage and code around CWallet::encrypted_batch. For the rationale, see bitcoin#14139 (comment).
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 bitcoin#14139.
domob1812
force-pushed
the
encrypted-batch
branch
from
151ada5 to
10d8b98
Compare
|
Rebased |
| Needs rebase |
|
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). |
|
This also overlaps with current PR #17369. I think more ideally, we could get rid of |
|
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 |
9 participants
This is a pure refactoring (no functional changes) that clarifies the use of
CWallet::encrypted_batchas 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_batchtemporarily. 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.