wallet: Replace CDataStream& with CDataStream&& where appropriate

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

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 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: member

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 commented at 2:40 am on June 19, 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:

#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.

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: member

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: member

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

DrahtBot locked this on Feb 15, 2022

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

Conflicts