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.

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

feat(state): add transparent address indexes to the non-finalized state #4022

Merged
merged 28 commits into from
Apr 12, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 1, 2022

Motivation

As part of the address index RPCs, we need to track address balances, UTXOs, and transactions in the non-finalized state.

This PR creates the address indexes in the non-finalized state, but it doesn't connect them to state requests or RPCs yet.

Specifications

See tickets:

Designs

The TransparentTransfers struct holds the address indexes for a single address.

Index updates use UpdateWith<T> for TransparentTransfers, where T is a tuple of arguments for:

  • created UTXOs, or
  • spent UTXOs

Spent UTXOs are a bit tricky, because we need to add their values to the ContextuallyValidBlock.

Solution

  • Create a TransparentTransfers address index type
  • Implement balance, UTXO, and transaction ID updates for created UTXOs
  • Implement balance, UTXO, and transaction ID updates for spent UTXOs
  • Remove empty TransparentTransfers after a spend revert
  • Add address transfers index accessor methods, which return UTXOs and transactions in chain order
  • Make accessor methods provide all the information required for each query

Related changes:

Related bug fixes:

  • Stop indexing each transparent output multiple times

Previously, the chain code was indexing each transparent output in the block multiple times (once per transaction). Because we were using HashMaps for chain indexes, this bug did not cause any problems. But in the address indexes, it made balances overflow.

(This should also improve non-finalized state performance for large blocks.)

Review

@conradoplg reviewed a draft of this PR.

We still need to connect these indexes to state requests, but the changes in this PR are enough to get us started.

Reviewer Checklist

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

Follow Up Work

In other PRs:

  • Implement the address RPCs and state requests
    • Add unit tests
    • Add RPC snapshot tests

@teor2345 teor2345 added C-enhancement Category: This is an improvement P-Medium ⚡ A-state Area: State / database changes lightwalletd any work associated with lightwalletd labels Apr 1, 2022
@teor2345 teor2345 self-assigned this Apr 1, 2022
@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 force-pushed the non-finalized-addr-index branch from 31abad2 to 62e1fc1 Compare April 6, 2022 06:34
@teor2345 teor2345 changed the title WIP: add transparent address indexes to the non-finalized state feat(state): add transparent address indexes to the non-finalized state Apr 6, 2022
@teor2345 teor2345 marked this pull request as ready for review April 6, 2022 06:40
@teor2345 teor2345 requested review from a team as code owners April 6, 2022 06:40
@teor2345 teor2345 requested review from upbqdn and conradoplg and removed request for a team and upbqdn April 6, 2022 06:40
@teor2345 teor2345 marked this pull request as draft April 7, 2022 02:53
@teor2345
Copy link
Contributor Author

teor2345 commented Apr 7, 2022

There's something wrong with the balance calculations in this PR:

The application panicked (crashed).
Message: called Result::unwrap() on an Err value: Constraint { value: 2100025173264129, range: -2100000000000000..=2100000000000000 }
Location: zebra-state/src/service/non_finalized_state/chain/index.rs:85

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.

I keep reviewing this PR but I can't seem to find the bug 😞 (or any other issue). I'll leave it for someone else to review.

@teor2345 teor2345 force-pushed the non-finalized-addr-index branch from 3c48d5f to 7cb552f Compare April 11, 2022 01:15
@teor2345
Copy link
Contributor Author

I keep reviewing this PR but I can't seem to find the bug 😞 (or any other issue). I'll leave it for someone else to review.

I think it's probably a mismatch between the created and spent UTXOs. Probably some missing spent UTXOs, because it's an overflow.

I'll make this part of the code easier to get right.

@teor2345 teor2345 force-pushed the non-finalized-addr-index branch from 7cb552f to 207d3fd Compare April 11, 2022 04:09
@teor2345 teor2345 marked this pull request as ready for review April 11, 2022 04:09
@teor2345 teor2345 added C-bug Category: This is a bug P-High 🔥 and removed P-Medium ⚡ labels Apr 11, 2022
@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2022

update

✅ Branch has been successfully updated

@teor2345
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Apr 12, 2022

update

✅ Branch has been successfully updated

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Re-approving.

mergify bot added a commit that referenced this pull request Apr 12, 2022
mergify bot added a commit that referenced this pull request Apr 12, 2022
@mergify mergify bot merged commit e49c1d7 into main Apr 12, 2022
@mergify mergify bot deleted the non-finalized-addr-index branch April 12, 2022 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-state Area: State / database changes C-bug Category: This is a bug C-enhancement Category: This is an improvement lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants