walletdb: Add PrefixCursor #27790

achow101 · 2023-05-31T02:56:13Z

Split from #24914 as suggested in #24914 (review)

This PR adds a wallet database cursor that gives a view over all of the records beginning with the same prefix.

DrahtBot · 2023-05-31T02:56:15Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#24914 (wallet: Load database records in a particular order 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.

src/wallet/sqlite.cpp

src/wallet/test/db_tests.cpp

Before writing data to the output key and value streams, make sure they are cleared.

ryanofsky

Code review ACK 13476fe

src/wallet/test/db_tests.cpp

DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes. Having this inconsistency is not necessary and could be confusing (see bitcoin#27790 (comment)) so this PR just drops the DataStream::Serialize method.

I found sqlite tracing was useful for debugging a test in bitcoin#27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.

In order to get records beginning with a prefix, we will need a cursor specifically for that prefix. So add a GetPrefixCursor function and DatabaseCursor classes for dealing with those prefixes. Tested on each supported db engine. 1) Write two different key->value elements to db. 2) Create a new prefix cursor and walk-through every returned element, verifying that it gets parsed properly. 3) Try to move the cursor outside the filtered range: expect failure and flag complete=true. Co-Authored-By: Ryan Ofsky <ryan@ofsky.org> Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>

ryanofsky

Code review ACK ba616b9. Just suggested changes since last review

furszy · 2023-06-01T21:31:17Z

src/wallet/test/db_tests.cpp

+#ifdef USE_BDB
+    dbs.emplace_back(MakeBerkeleyDatabase(path_root / "bdb", options, status, error));
+#endif
+#ifdef USE_SQLITE
+    dbs.emplace_back(MakeSQLiteDatabase(path_root / "sqlite", options, status, error));
+#endif


Could:

for (const DatabaseFormat& db_format : DATABASE_FORMATS) { dbs.emplace_back(MakeDatabase(path_root / strprintf("%d", db_format).c_str(), options, status, error)); BOOST_ASSERT(status == DatabaseStatus::SUCCESS); }

src/wallet/sqlite.cpp

…alize method and << operator 5cd0717 streams: Drop confusing DataStream::Serialize method and << operator (Ryan Ofsky) Pull request description: DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes. Having this inconsistency is not necessary and could be confusing (see bitcoin/bitcoin#27790 (comment)) so this PR just drops the DataStream::Serialize method. ACKs for top commit: furszy: lgtm ACK 5cd0717 MarcoFalke: lgtm ACK 5cd0717 🌙 Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2

furszy

ACK ba616b9

Nice db_cursor_prefix_byte_test test case 👌🏼.

…thod and << operator 5cd0717 streams: Drop confusing DataStream::Serialize method and << operator (Ryan Ofsky) Pull request description: DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes. Having this inconsistency is not necessary and could be confusing (see bitcoin#27790 (comment)) so this PR just drops the DataStream::Serialize method. ACKs for top commit: furszy: lgtm ACK 5cd0717 MarcoFalke: lgtm ACK 5cd0717 🌙 Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2

ba616b9 wallet: Add GetPrefixCursor to DatabaseBatch (Andrew Chow) 1d858b0 walletdb: Handle when database keys are empty (Ryan Ofsky) 84b2f35 walletdb: Consistently clear key and value streams before writing (Andrew Chow) Pull request description: Split from bitcoin#24914 as suggested in bitcoin#24914 (review) This PR adds a wallet database cursor that gives a view over all of the records beginning with the same prefix. ACKs for top commit: ryanofsky: Code review ACK ba616b9. Just suggested changes since last review furszy: ACK ba616b9 Tree-SHA512: 38a61849f108d8003d28c599b1ad0421ac9beb3afe14c02f1253e7b4efc3d4eef483e32647a820fc6636bca3f9efeff9fe062b6b602e0cded69f21f8b26af544

I found sqlite tracing was useful for debugging a test in bitcoin#27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.

ff9d961 wallet: Add tracing for sqlite statements (Ryan Ofsky) Pull request description: I found sqlite tracing was useful for debugging a test in #27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options. ACKs for top commit: achow101: ACK ff9d961 kevkevinpal: ACK ff9d961 theStack: ACK ff9d961 Tree-SHA512: 592fabfab3218cec36c2d00a21cd535fa840daa126ee8440c384952fbb3913180aa3796066c630087e933d6517f19089b867f158e0b737f25283a14799eefb05

ff9d961 wallet: Add tracing for sqlite statements (Ryan Ofsky) Pull request description: I found sqlite tracing was useful for debugging a test in bitcoin#27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options. ACKs for top commit: achow101: ACK ff9d961 kevkevinpal: ACK bitcoin@ff9d961 theStack: ACK ff9d961 Tree-SHA512: 592fabfab3218cec36c2d00a21cd535fa840daa126ee8440c384952fbb3913180aa3796066c630087e933d6517f19089b867f158e0b737f25283a14799eefb05

… linter 28fff06 test: Make linter to look for `BOOST_ASSERT` macros (Hennadii Stepanov) 47fe551 test: Kill `BOOST_ASSERT` (Hennadii Stepanov) Pull request description: One of the goals of bitcoin/bitcoin#27783 was to get rid of the `BOOST_ASSERT` macros instead of including the `boost/assert.hpp` headers. See bitcoin/bitcoin#27783 (comment). It turns out that a couple of those macros sneaked into the codebase in bitcoin/bitcoin#27790. This PR makes the linter guard against new instances of the `BOOST_ASSERT` macros and replaces the current ones. ACKs for top commit: kevkevinpal: ACK [28fff06](bitcoin/bitcoin@28fff06) stickies-v: ACK 28fff06 TheCharlatan: ACK 28fff06 Tree-SHA512: 371f613592cf677afe0196d18c83943c6c8f1e998f57b4ff3ee58bfeff8636e4dac1357840d8611b4f7b197def94df10fe1a8ca3282b00b7b4eff4624552dda8

28fff06 test: Make linter to look for `BOOST_ASSERT` macros (Hennadii Stepanov) 47fe551 test: Kill `BOOST_ASSERT` (Hennadii Stepanov) Pull request description: One of the goals of bitcoin#27783 was to get rid of the `BOOST_ASSERT` macros instead of including the `boost/assert.hpp` headers. See bitcoin#27783 (comment). It turns out that a couple of those macros sneaked into the codebase in bitcoin#27790. This PR makes the linter guard against new instances of the `BOOST_ASSERT` macros and replaces the current ones. ACKs for top commit: kevkevinpal: ACK [28fff06](bitcoin@28fff06) stickies-v: ACK 28fff06 TheCharlatan: ACK 28fff06 Tree-SHA512: 371f613592cf677afe0196d18c83943c6c8f1e998f57b4ff3ee58bfeff8636e4dac1357840d8611b4f7b197def94df10fe1a8ca3282b00b7b4eff4624552dda8

I found sqlite tracing was useful for debugging a test in bitcoin#27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.

DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes. Having this inconsistency is not necessary and could be confusing (see bitcoin/bitcoin#27790 (comment)) so this PR just drops the DataStream::Serialize method.

achow101 mentioned this pull request May 31, 2023

wallet: Load database records in a particular order #24914

Merged

bitcoin deleted a comment from joeyvee1986 May 31, 2023

DrahtBot added the CI failed label May 31, 2023

DrahtBot mentioned this pull request May 31, 2023

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Merged

DrahtBot removed the CI failed label May 31, 2023

ryanofsky reviewed May 31, 2023

View reviewed changes

src/wallet/sqlite.cpp Show resolved Hide resolved

src/wallet/test/db_tests.cpp Outdated Show resolved Hide resolved

src/wallet/test/db_tests.cpp Outdated Show resolved Hide resolved

achow101 and others added 2 commits May 31, 2023 15:17

walletdb: Consistently clear key and value streams before writing

84b2f35

Before writing data to the output key and value streams, make sure they are cleared.

walletdb: Handle when database keys are empty

1d858b0

achow101 force-pushed the walletdb-prefix-cursor branch from 0110871 to 13476fe Compare May 31, 2023 19:34

ryanofsky approved these changes May 31, 2023

View reviewed changes

src/wallet/test/db_tests.cpp Outdated Show resolved Hide resolved

maflcko reviewed Jun 1, 2023

View reviewed changes

src/wallet/test/db_tests.cpp Outdated Show resolved Hide resolved

ryanofsky mentioned this pull request Jun 1, 2023

streams: Drop confusing DataStream::Serialize method and << operator #27800

Merged

achow101 force-pushed the walletdb-prefix-cursor branch from 13476fe to 61e118e Compare June 1, 2023 14:46

ryanofsky mentioned this pull request Jun 1, 2023

wallet: Add tracing for sqlite statements #27801

Merged

achow101 force-pushed the walletdb-prefix-cursor branch from 61e118e to ba616b9 Compare June 1, 2023 17:09

ryanofsky approved these changes Jun 1, 2023

View reviewed changes

furszy reviewed Jun 2, 2023

View reviewed changes

glozow added the Wallet label Jun 2, 2023

furszy approved these changes Jun 2, 2023

View reviewed changes

fanquake merged commit 7f20197 into bitcoin:master Jun 2, 2023

hebasto mentioned this pull request Jun 14, 2023

test: Kill BOOST_ASSERT and update the linter #27889

Merged

maflcko mentioned this pull request Jun 21, 2023

util: Require explicit Span in serialize #27925

Closed

ryanofsky mentioned this pull request Jun 21, 2023

util: Allow std::byte and char Span serialization #27927

Merged

bitcoin locked and limited conversation to collaborators Jun 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

walletdb: Add PrefixCursor #27790

walletdb: Add PrefixCursor #27790

Uh oh!

achow101 commented May 31, 2023 •

edited

Loading

Uh oh!

DrahtBot commented May 31, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Uh oh!

furszy Jun 1, 2023

Uh oh!

Uh oh!

furszy left a comment

Uh oh!

Uh oh!

walletdb: Add PrefixCursor #27790

walletdb: Add PrefixCursor #27790

Uh oh!

Conversation

achow101 commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Jun 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented May 31, 2023 •

edited

Loading

DrahtBot commented May 31, 2023 •

edited

Loading