Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 13 commits into from
Apr 7, 2022
Merged

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

merged 13 commits into from
Apr 7, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Mar 25, 2022 •
edited
Loading

Motivation

  1. Zebra needs to return transparent address balances efficiently for getaddressbalance JSON-RPC method #3157
  2. Zebra needs a compact way to refer to addresses in the database (AddressLocation) for the rest of the address indexes

Designs

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

Solution

Database Changes:

  • Add an AddressBalanceLocation type
  • Add a balance_by_transparent_addr column family
  • Add serialization for balance_by_transparent_addr types
  • Read and update address balances in the finalized state
  • Increment the database format to version 16

Related Refactors:

  • Add a network field to DiskWriteBatch
  • Refactor confusing all_utxos_spent_by_block argument

Related Tests:

  • Add test-only deserialization for transparent addresses
  • Add round-trip tests for the new serialized types
  • Add missing round-trip and serialized equality tests
  • Update raw data snapshots for transparent address balances
  • Add high-level snapshot tests for address balances

(My local full syncs worked successfully, the database now takes ~35 GB.)

I ran a manual full sync test here:

It got to 98.925% after 6 hours, which is acceptable performance.

Closes #3950.

Review

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

This is unrelated to previous PRs, so I'll pick a random reviewer.

This PR is based on PR #3934.

Reviewer Checklist

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

Follow Up Work

The rest of the database changes.

@teor2345 teor2345 added A-dependencies Area: Dependency file updates C-enhancement Category: This is an improvement P-High 🔥 A-state Area: State / database changes do-not-merge Tells Mergify not to merge this PR lightwalletd any work associated with lightwalletd labels Mar 25, 2022
@teor2345 teor2345 requested a review from a team as a code owner March 25, 2022 07:14
@teor2345 teor2345 self-assigned this Mar 25, 2022
@teor2345 teor2345 requested a review from a team as a code owner March 25, 2022 07:14
@teor2345 teor2345 requested review from upbqdn and removed request for a team March 25, 2022 07:14
@teor2345 teor2345 force-pushed the db-transaction-split branch from cb0ee0c to adcff1e Compare March 27, 2022 23:59
@teor2345 teor2345 removed request for a team and upbqdn March 28, 2022 00:00
@teor2345 teor2345 mentioned this pull request Mar 28, 2022
Merged
3 tasks
@teor2345 teor2345 force-pushed the db-transaction-split branch from adcff1e to 5d3181c Compare March 29, 2022 00:01
@teor2345 teor2345 requested review from upbqdn and oxarbitrage and removed request for upbqdn March 29, 2022 20:49
@teor2345 teor2345 force-pushed the db-transaction-split branch from 5d3181c to 6f28aa8 Compare March 29, 2022 23:06
@teor2345 teor2345 force-pushed the db-transaction-split branch from 6f28aa8 to 26a4de5 Compare March 31, 2022 01:20
@teor2345 teor2345 force-pushed the db-transaction-split branch from 26a4de5 to 4a047a6 Compare April 4, 2022 00:27
@teor2345 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 4, 2022
@teor2345 teor2345 mentioned this pull request Apr 5, 2022
Closed
4 tasks
@teor2345 teor2345 removed the do-not-merge Tells Mergify not to merge this PR label Apr 5, 2022
Base automatically changed from db-transaction-split to main April 7, 2022 08:30
teor2345 added 13 commits April 7, 2022 11:51
@teor2345 @conradoplg
Add an empty balance_by_transparent_addr column family
9d4deb9
@teor2345 @conradoplg
Add an AddressBalanceLocation type for balance_by_transparent_addr
50a7cfd
@teor2345 @conradoplg
Add serialization for balance_by_transparent_addr types
4af8cd7
@teor2345 @conradoplg
Add round-trip tests for the new serialized types
3e55258
@teor2345 @conradoplg
Add missing round-trip and serialized equality tests
b4c159b
@teor2345 @conradoplg
Add a network field to DiskWriteBatch
3ee0c24
@teor2345 @conradoplg
Refactor confusing all_utxos_spent_by_block argument
99326d5 
It was actually just the UTXOs from the state spent by the block,
excluding the UTXOs created and spent within the block.

But now we need it to contain all the spent outputs,
including the ones created by the block.
@teor2345 @conradoplg
Read and update address balances in the finalized state
7d86b28
@teor2345 @conradoplg
Update raw data snapshots for transparent address balances
d1f4265
@teor2345 @conradoplg
Add test-only deserialization for transparent addresses
befcc24
@teor2345 @conradoplg
Add high-level snapshot test code for address balances
fff3787
@teor2345 @conradoplg
Add high-level snapshots for address balances
926dc96
@teor2345 @conradoplg
Increment the state version after NU5 testnet 2 rollback
a602758
@conradoplg
Copy link
Collaborator

Rebased after base PR was merged

conradoplg
conradoplg approved these changes Apr 7, 2022
View reviewed changes
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

mergify bot added a commit that referenced this pull request Apr 7, 2022
@mergify
Merge of #3963
27f008c
@mergify mergify bot mentioned this pull request Apr 7, 2022
Closed
19 tasks
@mergify mergify bot merged commit 7faa6a2 into main Apr 7, 2022
@mergify mergify bot deleted the db-trans-balance branch April 7, 2022 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@conradoplg conradoplg conradoplg approved these changes

@oxarbitrage oxarbitrage Awaiting requested review from oxarbitrage

Assignees

@teor2345 teor2345

Labels
A-dependencies Area: Dependency file updates A-state Area: State / database changes C-enhancement Category: This is an improvement lightwalletd any work associated with lightwalletd
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add balance_by_transparent_addr index to the finalized state
2 participants
@teor2345 @conradoplg