wallet: Replace CDataStream& with CDataStream&& where appropriate #19320
Conversation
src/wallet/bdb.h
Outdated
| bool WriteKey(CDataStream& key, CDataStream& value, bool overwrite=true); | ||
| bool EraseKey(CDataStream& key); | ||
| bool HasKey(CDataStream& key); | ||
| bool ReadKey(Span<char> key, CDataStream& value); |
There was a problem hiding this comment.
These Spans can be const char ones. That also has the advantage of supporting automatic conversion from temporaries.
There was a problem hiding this comment.
SafeDbt is used to assign 0 to the chars, which wouldn't work with const char
There was a problem hiding this comment.
Code review ACK fafeffc
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
src/wallet/bdb.h
Outdated
| bool WriteKey(CDataStream& key, CDataStream& value, bool overwrite=true); | ||
| bool EraseKey(CDataStream& key); | ||
| bool HasKey(CDataStream& key); | ||
| bool ReadKey(Span<char> key, CDataStream& value); |
There was a problem hiding this comment.
In commit "wallet: Replace CDataStream& with Span where possible" (fafeffc)
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 (comment). 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
re: #19320 (comment)
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
|
ACK fafeffc |
|
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. |
maflcko
changed the title
Also, remove redundant comments
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.
Fixed by using the double ampersand trick |
There was a problem hiding this comment.
Code review ACK fa8a341. 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.
|
utACK fa8a341 |
…where appropriate fa8a341 wallet: Replace CDataStream& with CDataStream&& where appropriate (MarcoFalke) fa021e9 wallet: Remove confusing double return value ret+success (MarcoFalke) Pull request description: 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. ACKs for top commit: sipa: utACK fa8a341 ryanofsky: Code review ACK fa8a341. Nice changes. Tree-SHA512: 5c0218bae0f3cd2a07346f1bbf4ad232e5dde7ef2f807d82cc6cfd208d11fe60c8b0f37e7986087b52fbfc79cdfd33c3c8a5822b3d4d9a44d1c6b09e354fc424
Summary: Also, remove redundant comments This is a backport of [[bitcoin/bitcoin#19320 | core#19320]] [1/2] bitcoin/bitcoin@fa021e9 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9982
Summary: 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. This is a backport of [[bitcoin/bitcoin#19320 | core#19320]] [2/2] bitcoin/bitcoin@fa8a341 Depends on D9982 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9983
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.