wallet: Replace CDataStream& with CDataStream&& where appropriate #19320

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2006-walletSpan changing 2 files +17 −39

MarcoFalke commented at 2:47 PM on June 18, 2020: member

The keys and values are only to be used once because their memory is set to zero. Make that explicit by moving the bytes into the lower level methods.
MarcoFalke added the label Refactoring on Jun 18, 2020
MarcoFalke added the label Wallet on Jun 18, 2020
MarcoFalke cross-referenced this on Jun 18, 2020 from issue wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions by achow101
in src/wallet/bdb.h:211 in fafeffc6b2 outdated
```
 207 | @@ -208,10 +208,10 @@ class BerkeleyBatch
 208 |      };
 209 |  
 210 |  private:
 211 | -    bool ReadKey(CDataStream& key, CDataStream& value);
 212 | -    bool WriteKey(CDataStream& key, CDataStream& value, bool overwrite=true);
 213 | -    bool EraseKey(CDataStream& key);
 214 | -    bool HasKey(CDataStream& key);
 215 | +    bool ReadKey(Span<char> key, CDataStream& value);
```
sipa commented at 3:59 PM on June 18, 2020:

These Spans can be const char ones. That also has the advantage of supporting automatic conversion from temporaries.

MarcoFalke commented at 4:38 PM on June 18, 2020:

SafeDbt is used to assign 0 to the chars, which wouldn't work with const char

ryanofsky commented at 6:06 PM on June 18, 2020:

In commit "wallet: Replace CDataStream& with Span<char> where possible" (fafeffc6b23bea9addc091f69fe72440af185db8)

Might be worth mentioning in a comment here that memory_cleanse will be called on all these spans. I didn't realize this was happening, even after your previous comment in #19292 (review). You could imagine this leading to bugs if for example an EraseKey call was made after a ReadKey call with the same span, nothing would be erased because the key would be wiped between calls.

MarcoFalke commented at 8:41 PM on June 18, 2020:

Does it even make sense to wipe the memory of bdb keys (not values)? I'd be surprised if the db-keys itself are encrypted, so I'd rather not add this comment.

ryanofsky commented at 9:16 PM on June 18, 2020:

re: #19320 (review)

Does it even make sense to wipe the memory of bdb keys (not values)? I'd be surprised if the db-keys itself are encrypted, so I'd rather not add this comment.

No I think it's bad behavior that could cause bugs like reading and writing and reading then erasing, or calling haskey before readkey all not to work. The point of a comment mentioning keys are wiped would be to document this surprising behavior and prevent bugs. But the behavior precedes this PR so definitely feel to ignore it
ryanofsky approved
ryanofsky commented at 6:47 PM on June 18, 2020: contributor

Code review ACK fafeffc6b23bea9addc091f69fe72440af185db8

If anyone wants to do more cleanup like this I had a note to change the walletdb ReadKeyValue function to accept Span arguments as well. That'd require an additional change to support deserializing from Spans, such as tweaking VectorReader to accept a Span instead of a vector
achow101 commented at 9:21 PM on June 18, 2020: member

ACK fafeffc6b23bea9addc091f69fe72440af185db8
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class by achow101
DrahtBot commented at 2:40 AM on June 19, 2020: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #19335 (wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101)
- #19334 (wallet: Introduce WalletDatabase abstract class by achow101)
- #19325 (wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class by achow101)
- #19137 (wallettool: Add dump and createfromdump commands by achow101)
- #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
- #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
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 cross-referenced this on Jun 19, 2020 from issue wallettool: Add dump and createfromdump commands by achow101
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101
DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Refactor the classes in wallet/db.{cpp/h} by achow101
DrahtBot cross-referenced this on Jun 20, 2020 from issue wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101
DrahtBot cross-referenced this on Jun 20, 2020 from issue wallet: Introduce WalletDatabase abstract class by achow101
MarcoFalke renamed this:
~~wallet: Replace CDataStream& with Span<char> where possible~~
wallet: Replace CDataStream& with CDataStream&& where appropriate
on Jun 20, 2020
MarcoFalke force-pushed on Jun 20, 2020
wallet: Remove confusing double return value ret+success
Also, remove redundant comments
fa021e9a5b

wallet: Replace CDataStream& with CDataStream&& where appropriate

The keys and values are only to be used once because their memory is set
to zero. Make that explicit by moving the bytes into the lower level
methods.

fa8a341b88

MarcoFalke force-pushed on Jun 20, 2020
MarcoFalke commented at 1:38 PM on June 20, 2020: member

leading to bugs if for example an EraseKey call was made after a ReadKey call with the same span, nothing would be erased because the key would be wiped between calls.

Fixed by using the double ampersand trick
ryanofsky approved
ryanofsky commented at 8:34 PM on July 8, 2020: contributor

Code review ACK fa8a341b88cabfd7f8d702db7cb9972b0804bf2a. Nice changes.

PR is rewritten since last review, so warning to others some previous discussion isn't relevant anymore (might have been a clearer to close this PR and open a new one). I still think it'd be good to replace all the cdatastreams with spans, but this would require externalizing memory_cleanse to be safe, so would a bigger change. More could be done here but this PR is a nice improvement.
sipa commented at 8:54 PM on July 8, 2020: member

utACK fa8a341b88cabfd7f8d702db7cb9972b0804bf2a
MarcoFalke merged this on Jul 8, 2020
MarcoFalke closed this on Jul 8, 2020
MarcoFalke deleted the branch on Jul 8, 2020
jonatack commented at 11:06 AM on July 9, 2020: contributor

Nice refactoring.

post-merge ACK fa8a341b88cabfd7f8d702db7cb9972b0804bf2a
sidhujag referenced this in commit aaba3f4500 on Jul 9, 2020
Fabcien referenced this in commit 584ae6ca3b on Aug 31, 2021
Fabcien referenced this in commit 9be27c4a93 on Aug 31, 2021
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19292 wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions

wallet: Replace CDataStream& with CDataStream&& where appropriate #19320

Conflicts