1. feat(db): Store transactions in a separate database index, to improve query speed #3934

teor2345 · 2022-03-22T04:10:50Z

Motivation

Currently, Zebra reads the entire block each time we lookup a transaction. But we want to serve transactions to lightwalletd quickly and efficiently.

Consensus Rules

This PR adds the genesis transaction hash to the transaction indexes, so Zebra can re-create the genesis block from its header and transaction.

There is a consensus rule that prevents spends of the genesis transparent UTXO. This PR doesn't change the UTXO index, so Zebra's consensus behaviour remains the same.

Designs

The Blocks and Transactions parts of:
https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0005-state-updates.md#rocksdb-data-structures

Solution

Database Changes:

Implement disk serialization for block headers and transactions
Add new transaction column families
Split block header and transaction data
Increment the database format to version 15

Related Database Changes:

Re-write disk read API to avoid iterator-related hangs
Move disk read API to ReadDisk
Make the database generic over the thread mode, but don't actually change the thread mode in this PR

(In future PRs, we might need to change the thread mode for hang or performance testing. So I left the generic thread mode code in this PR.)

Testing:

Update snapshot test data for transaction split
Test that the new transaction indexes are consistent

To make sure there are no more hang bugs, I've started a full sync test on commit fce2ffe:
https://github.com/ZcashFoundation/zebra/actions/runs/2026544541

Closes #3151.

Review

This PR is on the critical path for the lightwalletd work, so I've tagged it as a high priority.

I tagged Conrado as the reviewer, because this PR will conflict with the ZebraDb::transaction changes in PR #3908. (It's only a minor conflict.)

Reviewer Checklist

Code implements Specs and Designs
Tests for Expected Behaviour
Tests for Errors

conradoplg

Looks good, but I have a question about something that might need changing

zebra-state/src/service/finalized_state/disk_db.rs

teor2345 · 2022-03-23T20:35:54Z

To make sure there are no more hang bugs, I've started a full sync test on commit fce2ffe:
https://github.com/ZcashFoundation/zebra/actions/runs/2026544541

Turns out that splitting transactions out might actually be faster than writing whole blocks.

(The full sync test passed in 5h46m, previous times were around 5h50m - 6h.)

conradoplg

Looks good, I'll leave the unresolved conversation to be sure it's not merged yet

teor2345 · 2022-03-27T23:26:37Z

Looks good, I'll leave the unresolved conversation to be sure it's not merged yet

The do-not-merge label also stops merges, but only in mergify.

(I've added a conversation that explains why we're not merging yet.)

zebra-state/src/constants.rs

Prefix iteration caused database hangs.

Also: - re-write disk read API to avoid iterator hangs - move disk read API to ReadDisk - re-write impl rocksdb::AsColumnFamilyRef to a where clause, for consistency

…sion

zebra-state/src/constants.rs

conradoplg

Re-approving

* ZIPs were updated to remove ambiguity, this was tracked in #1267. * #2105 was fixed by #3039 and #2379 was closed by #3069 * #2230 was a duplicate of #2231 which was closed by #2511 * #3235 was obsoleted by #2156 which was fixed by #3505 * #1850 was fixed by #2944, #1851 was fixed by #2961 and #2902 was fixed by #2969 * We migrated to Rust 2021 edition in Jan 2022 with #3332 * #1631 was closed as not needed * #338 was fixed by #3040 and #1162 was fixed by #3067 * #2079 was fixed by #2445 * #4794 was fixed by #6122 * #1678 stopped being an issue * #3151 was fixed by #3934 * #3204 was closed as not needed * #1213 was fixed by #4586 * #1774 was closed as not needed * #4633 was closed as not needed * Clarify behaviour of difficulty spacing Co-authored-by: teor <teor@riseup.net> * Update comment to reflect implemented behaviour Co-authored-by: teor <teor@riseup.net> * Update comment to reflect implemented behaviour when retrying block downloads Co-authored-by: teor <teor@riseup.net> * Update `TODO` to remove closed issue and clarify when we might want to fix Co-authored-by: teor <teor@riseup.net> * Update `TODO` to remove closed issue and clarify what we might want to change in future Co-authored-by: teor <teor@riseup.net> * Clarify benefits of how we do block verification Co-authored-by: teor <teor@riseup.net> * Fix rustfmt errors --------- Co-authored-by: teor <teor@riseup.net>

teor2345 added C-enhancement Category: This is an improvement P-High 🔥 I-slow Problems with performance or responsiveness A-state Area: State / database changes lightwalletd any work associated with lightwalletd labels Mar 22, 2022

teor2345 requested a review from conradoplg March 22, 2022 04:10

teor2345 self-assigned this Mar 22, 2022

teor2345 requested a review from a team as a code owner March 22, 2022 04:10

teor2345 changed the base branch from main to small-db-keys-height March 22, 2022 04:11

Base automatically changed from small-db-keys-height to main March 22, 2022 06:19

conradoplg force-pushed the db-transaction-split branch from dff81df to 72f0656 Compare March 22, 2022 16:54

This comment was marked as resolved.

Sign in to view

conradoplg requested changes Mar 22, 2022

View reviewed changes

zebra-state/src/service/finalized_state/disk_db.rs Outdated Show resolved Hide resolved

teor2345 force-pushed the db-transaction-split branch from 72f0656 to feaf30a Compare March 22, 2022 19:35

teor2345 requested a review from conradoplg March 22, 2022 19:42

teor2345 changed the title ~~8. feat(db): Store transactions in a separate database index, to improve query speed~~ 1. feat(db): Store transactions in a separate database index, to improve query speed Mar 22, 2022

This comment was marked as outdated.

Sign in to view

teor2345 marked this pull request as draft March 22, 2022 19:46

This comment was marked as resolved.

Sign in to view

teor2345 force-pushed the db-transaction-split branch from 04209f0 to fce2ffe Compare March 23, 2022 05:21

This comment was marked as resolved.

Sign in to view

teor2345 marked this pull request as ready for review March 23, 2022 05:27

teor2345 added the do-not-merge Tells Mergify not to merge this PR label Mar 23, 2022

teor2345 force-pushed the db-transaction-split branch 2 times, most recently from 4a9a223 to cb0ee0c Compare March 25, 2022 00:25

teor2345 mentioned this pull request Mar 25, 2022

2. feat(db): Add address balance indexes to the finalized state #3963

Merged

3 tasks

conradoplg previously approved these changes Mar 25, 2022

View reviewed changes

teor2345 commented Mar 27, 2022

View reviewed changes

zebra-state/src/constants.rs Outdated Show resolved Hide resolved

teor2345 added 17 commits April 4, 2022 08:45

Re-order column family initialization to match the design

d6628ea

Add new empty transaction column families

97daac4

Split writing block header and transaction data

e7aff7f

Re-order column families for consistency

236bbdd

Update write snapshots for transaction split

f00c9b1

Use split block and transaction data when reading

1a3eb13

Update snapshots to include genesis transaction hash location

f96b7f0

Filter all prefix iterators to make sure they return the correct values

c2b49eb

Test that the new transaction indexes are consistent

1c673bf

Add some cleanup TODOs

9b87bb0

Increment the database format to version 15

83b41f6

Remove unused fisk format impls for Block

0c7408c

Add a missing prefix extractor for transaction locations

29f64ea

Make the database generic over the thread mode

ce8cfe9

Replace prefix iteration with iteration from a key, and a filter

a8bc329

Prefix iteration caused database hangs.

Manually iterate through transaction locations to re-create blocks

c2da49a

Also: - re-write disk read API to avoid iterator hangs - move disk read API to ReadDisk - re-write impl rocksdb::AsColumnFamilyRef to a where clause, for consistency

Update the database version so it's larger than the NU5 testnet 2 ver…

4a047a6

…sion

teor2345 force-pushed the db-transaction-split branch from 26a4de5 to 4a047a6 Compare April 4, 2022 00:27

oxarbitrage reviewed Apr 4, 2022

View reviewed changes

zebra-state/src/constants.rs Show resolved Hide resolved

teor2345 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Apr 5, 2022

teor2345 mentioned this pull request Apr 5, 2022

feat(ci): create disk image after a successful full sync test #3986

Merged

1 task

teor2345 removed the do-not-merge Tells Mergify not to merge this PR label Apr 7, 2022

conradoplg approved these changes Apr 7, 2022

View reviewed changes

mergify bot added a commit that referenced this pull request Apr 7, 2022

Merge of #3934

2eac72c

mergify bot mentioned this pull request Apr 7, 2022

merge-queue: embarking main (7f351ab) and #3934 together #4057

Closed

19 tasks

mergify bot merged commit 6aba60d into main Apr 7, 2022

mergify bot deleted the db-transaction-split branch April 7, 2022 08:30

mpguerra added a commit that referenced this pull request May 19, 2023

#3151 was fixed by #3934

3bdf2cf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1. feat(db): Store transactions in a separate database index, to improve query speed #3934

1. feat(db): Store transactions in a separate database index, to improve query speed #3934

teor2345 commented Mar 22, 2022 •

edited

Loading

This comment was marked as resolved.

conradoplg left a comment

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

teor2345 commented Mar 23, 2022

conradoplg left a comment

teor2345 commented Mar 27, 2022 •

edited

Loading

conradoplg left a comment

1. feat(db): Store transactions in a separate database index, to improve query speed #3934

1. feat(db): Store transactions in a separate database index, to improve query speed #3934

Conversation

teor2345 commented Mar 22, 2022 • edited Loading

Motivation

Consensus Rules

Designs

Solution

Review

Reviewer Checklist

This comment was marked as resolved.

conradoplg left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

teor2345 commented Mar 23, 2022

conradoplg left a comment

Choose a reason for hiding this comment

teor2345 commented Mar 27, 2022 • edited Loading

conradoplg left a comment

Choose a reason for hiding this comment

teor2345 commented Mar 22, 2022 •

edited

Loading

teor2345 commented Mar 27, 2022 •

edited

Loading