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

Add value pools to FinalizedState #2599

Merged
merged 24 commits into from
Aug 19, 2021

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Aug 10, 2021

Motivation

We need to save value balances into the database so when a new block gets in we can make sure the overall sum does not became negative.

Specifications

https://zebra.zfnd.org/dev/rfcs/0012-value-pools.html#consensus-rules

Designs

https://zebra.zfnd.org/dev/rfcs/0012-value-pools.html#pass-the-value-balance-for-this-block-from-the-consensus-into-the-state and below.

Solution

Implement as described in https://zebra.zfnd.org/dev/rfcs/0012-value-pools.html#pass-the-value-balance-for-this-block-from-the-consensus-into-the-state

Closes #2518, because we look up all spent UTXOs in the finalized state to get their values.

Review

Currently a bunch of tests fails by missing utxos:

service::check::tests::utxo::accept_arbitrary_transparent_spend_from_previous_block
service::check::tests::utxo::reject_duplicate_transparent_spend_in_same_block_from_previous_block
service::check::tests::utxo::reject_duplicate_transparent_spend_in_same_chain_from_previous_block
service::check::tests::utxo::reject_duplicate_transparent_spend_in_same_transaction_from_previous_block
service::check::tests::utxo::reject_missing_transparent_spend
service::finalized_state::tests::prop::blocks_with_v5_transactions
service::non_finalized_state::queued_blocks::tests::dequeue_gives_right_children
service::non_finalized_state::queued_blocks::tests::prune_removes_right_children
service::non_finalized_state::tests::prop::finalized_equals_pushed
service::non_finalized_state::tests::prop::forked_equals_pushed
service::non_finalized_state::tests::prop::rejection_restores_internal_state
service::non_finalized_state::tests::vectors::construct_many
service::non_finalized_state::tests::vectors::construct_single
service::non_finalized_state::tests::vectors::ord_matches_work

This is work in progress but @teor2345 will probably know what is going on here.

Reviewer Checklist

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

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I have some questions about the design here - I wonder if we can avoid changing zebra-consensus?

  • Can you change the title of this PR so it's clear that it only covers the finalized state?
  • What's your plan for verifying the value pools in the non-finalized state?

It's a good idea to do the finalized and non-finalized state in separate PRs. But it might help to have a rough design, so that you know what data you'll have by the time you get to the finalized state.

@oxarbitrage oxarbitrage marked this pull request as ready for review August 11, 2021 16:47
@oxarbitrage oxarbitrage changed the title Add value pools to the database Add value pools to FinalizedState Aug 11, 2021
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

In this PR, we only need to calculate the correct chain value balances for the finalized tip, and write them to the state.

(We don't need to do any verification, because all the blocks are verified by the non-finalized state, or the checkpoint verifier.)

Here's how you could do that:

Merge test fixes

Calculate the new chain value balance

  • create a list of UTXOs spent by the new block
  • use the list of spent UTXOs to update the chain value balance
  • remove all the other changes from this PR - we don't need to do verification

I've written more details near the code that needs to change.
Let me know if you need any help here.

Update the state version

  • increment the state version (in this PR)
  • rebuild the cached state (using Google Cloud)
  • update the cache disk in the cached state test workflow (in this PR)
  • merge this PR

Deirdre or Conrado can help you with the cached state update.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

The code looks good!

Do you want to add a test that makes sure we're storing the correct values in the finalized state?

Here's a similar test that adds some test vector blocks to the finalized state:

fn best_tip_height_is_updated(

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I'm not sure how to rebuild the cached state, so I'm going to get @dconnolly or @conradoplg to help out with that part.

@teor2345 teor2345 dismissed their stale review August 18, 2021 06:50

Code and tests look good now

@conradoplg
Copy link
Collaborator

I'm not sure how to rebuild the cached state, so I'm going to get @dconnolly or @conradoplg to help out with that part.

I've started a run to regenerate the state in https://github.com/ZcashFoundation/zebra/actions/runs/1143386535 (after it finishes, we have to manually change test.yml)

It seems GitHub doesn't support running from branches in forks, so I also pushed this PR to a value-pools-database branch in the Zebra repo just to run the action, and we can delete it later.

@conradoplg
Copy link
Collaborator

I'm not sure how to rebuild the cached state, so I'm going to get @dconnolly or @conradoplg to help out with that part.

I've started a run to regenerate the state in https://github.com/ZcashFoundation/zebra/actions/runs/1143386535 (after it finishes, we have to manually change test.yml)

It seems GitHub doesn't support running from branches in forks, so I also pushed this PR to a value-pools-database branch in the Zebra repo just to run the action, and we can delete it later.

The test timed out :( It seems it was close-ish to finish. I'll run it manually in GCloud (where there is no timeout) in a bit.

@oxarbitrage
Copy link
Contributor Author

The test timed out :( It seems it was close-ish to finish. I'll run it manually in GCloud (where there is no timeout) in a bit.

Thanks for your work on this.

@conradoplg
Copy link
Collaborator

I've updated the cache in #2644

We can try to merge both approximately at the same time (that first and then this one).

I've started a run with the value-pools-database that I created before with the #2644 change cherry-picked, to see if it works beforehand https://github.com/ZcashFoundation/zebra/actions/runs/1147067869

@conradoplg
Copy link
Collaborator

The test passed, but it's stuck in GitHub Actions because this branch is not up-to-date with the recent test.yml fixes in main. @oxarbitrage can you update it?

image

For me this is good to go, I'll leave merging to whoever reviews #2644

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.

Re-approving after running the cached state test and syncing with main

@conradoplg conradoplg merged commit d2e417c into ZcashFoundation:main Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check that finalized state UTXOs are present before deleting them
3 participants