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

achow101 · 2020-06-16T00:57:22Z

In order to override these later, the specific details of how the Read, Write, Erase, and Exists functions interact with the actual database file need to go into functions that are not templated.

The functions ReadKey, WriteKey, EraseKey, and HasKey are introduced to handle the actual interaction with the database.

This is mostly a moveonly.

Based on #19290

DrahtBot · 2020-06-16T09:46:12Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
wallet: Refactor the classes in wallet/db.{cpp/h} #18971 (wallet: Refactor the classes in wallet/db.{cpp/h} 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.

maflcko · 2020-06-17T13:04:16Z

Needs some kind of force push to prod github

maflcko · 2020-06-17T13:06:53Z

Have you considered making a commit for each function? Would help with scrolling during review, but no big deal if it is too cumbersome to split up.

ryanofsky

Code review ACK 69e56486186bbcae7c8e68d891493949ea0f2af8

In order to override these later, the specific details of how the Read, Write, Erase, and Exists functions interact with the actual database file need to go into functions that are not templated.

achow101 · 2020-06-17T14:29:41Z

Rebased

ryanofsky

Code review ACK a389ed5. No changes since last review, just non-conflicting rebase

Sjors

utACK a389ed5

Sjors · 2020-06-17T16:33:06Z

src/wallet/bdb.h

    };

+private:
+    bool ReadKey(CDataStream& key, CDataStream& value);


This could use documentation. In particular the behavior when pdb is absent is not super intuitive, e.g. it returns false for ReadKey and true for WriteKey.

WriteKey returns true so that it works with dummy database. After #19102, this can (and probably should) be changed to false.

Sjors · 2020-06-17T16:40:17Z

src/wallet/bdb.cpp

+
+    // Read
+    SafeDbt datValue;
+    int ret = pdb->get(activeTxn, datKey, datValue, 0);


Maybe just early return false here if ret != 0 || datValue.get_data() == nullptr?

Meh. Will change if another push is needed.

Sjors · 2020-06-17T16:43:11Z

src/wallet/bdb.cpp

+    SafeDbt datValue;
+    int ret = pdb->get(activeTxn, datKey, datValue, 0);
+    if (ret == 0 && datValue.get_data() != nullptr) {
+        value.write((char*)datValue.get_data(), datValue.get_size());


This is no longer inside a try-catch block, is that safe?

The try-catch block is for the deserialization operation (the >> operator), not this write.

Sjors · 2020-06-17T16:46:09Z

src/wallet/bdb.cpp

+
+bool BerkeleyBatch::WriteKey(CDataStream& key, CDataStream& value, bool overwrite)
+{
+    if (!pdb)


It's a non-trivial move, so might as well add brackets or make it inline.

Will change if another push is needed.

maflcko

ACK a389ed5 🔳

Show signature and timestamp

Signature:

Timestamp of file with hash 6bf76cee68718cc9e56aef4002215a6e74aed5587fdc5d0275fb0e5769554ec1 -

maflcko · 2020-06-18T13:19:40Z

src/wallet/bdb.h

+    bool ReadKey(CDataStream& key, CDataStream& value);
+    bool WriteKey(CDataStream& key, CDataStream& value, bool overwrite=true);
+    bool EraseKey(CDataStream& key);
+    bool HasKey(CDataStream& key);


I found it really confusing that all the const& are replaced by &. Is there any reason for that? If not, it could make sense to fix this up in a follow-up commit.

I agree. If nothing else, const is good documentation about what can potentially be modified and what not.

re: #19292 (comment)

I found it really confusing that all the const& are replaced by &. Is there any reason for that? If not, it could make sense to fix this up in a follow-up commit.

I assumed it was necessary to remove const from all the arguments because reading from CDataStream objects changes their internal real read position. This is a reason I suggested replacing all the CDataStream arguments with Spans #18971 (comment). This could be done as a later followup.

Both data and size are marked const in the CDataStream class. How would this affect the read position anyway?

I think the reason is that the in-memory key data needs to be purged (in case it is a private key)?

Both data and size are marked const in the CDataStream class. How would this affect the read position anyway?

Guess it wouldn't. I didn't check if these were const because I just remembered CDataStream being goofy about reading. Maybe it's less goofy than I thought but I still think Spans are better.

@achow101 Let us know if you want to change this to span or rather have the current version of the pr merged

It looks like my patch only compiles after commit b8740d6 (Merge #18468: Span improvements)

I'll leave it for a followup.

+1 on using spans
I also thought about making BerkeleyBatch::ReadKey return an Option<CDataStream> instead of passing it in, but dunno.

Done in #19320

Summary: In order to override these later, the specific details of how the Read, Write, Erase, and Exists functions interact with the actual database file need to go into functions that are not templated. This is a backport of [[bitcoin/bitcoin#19292 | core#19292]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9618

fanquake added the Wallet label Jun 16, 2020

achow101 mentioned this pull request Jun 16, 2020

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

8 tasks

ryanofsky approved these changes Jun 17, 2020

View reviewed changes

walletdb: refactor Read, Write, Erase, and Exists into non-template func

a389ed5

In order to override these later, the specific details of how the Read, Write, Erase, and Exists functions interact with the actual database file need to go into functions that are not templated.

achow101 force-pushed the refactor-bdb-read branch from 69e5648 to a389ed5 Compare June 17, 2020 14:29

ryanofsky approved these changes Jun 17, 2020

View reviewed changes

Sjors reviewed Jun 17, 2020

View reviewed changes

DrahtBot mentioned this pull request Jun 18, 2020

wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets #19077

Merged

maflcko approved these changes Jun 18, 2020

View reviewed changes

maflcko merged commit c7ebab1 into bitcoin:master Jun 18, 2020

ryanofsky mentioned this pull request Jun 18, 2020

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

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

achow101 commented Jun 16, 2020

Uh oh!

DrahtBot commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jun 17, 2020

Uh oh!

maflcko commented Jun 17, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 17, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Jun 16, 2020 •

edited

Loading

laanwj Jun 18, 2020 •

edited

Loading