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

Zebra pools RFC #1928

Closed
wants to merge 1 commit into from
Closed

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Mar 21, 2021

TODO

  • design should support historical statistics for ancestors of the finalized tip, but we don't want to store them by default

Summary

Please copy the RFC summary over here.

Zebra needs to calculate the different value pools to apply ZIP-209. #1895

Design the value pool implementation in the form of an RFC.

More information

Feature Name: my_feature

Start Date: YYYY-MM-DD

Design PR: ZcashFoundation/zebra#0000

Zebra Issue: ZcashFoundation/zebra#0000

Document

Rendered.

Review

This is very draft work i made over the weekend just to share the overall idea i have for this implementation. If there are other plans then this can just be dropped.

Zebra Team Approval

Everyone on the Zebra team should review design RFCs:


Each of the 4 value pools change its value with every upcoming block. This is a state feature and Zebra handle this in the `zebra-state` crate. We propose to store the pool values among a reference block height into the disk.

The proposed design goes from calculating transaction pools, summing them all to form block pools and finally summing all block pools to form the final pool value.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can add mathematical formulas for each pool.


### `transparent_value_pool()`

TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we need to call the state service. Before making some prototype i want to make sure if it is ok for us to call state here.

Basically we will loop over the inputs getting Outpoint and calling zebra_state::Request::AwaitUtxo(outpoint)

Copy link
Contributor

Choose a reason for hiding this comment

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

The transaction verifier already fetches all these inputs, why do it again in the state?

for input_index in 0..inputs.len() {
let rsp = script_verifier.ready_and().await?.call(script::Request {
upgrade,
known_utxos: known_utxos.clone(),
cached_ffi_transaction: cached_ffi_transaction.clone(),
input_index,
});

Here's one possible design:

Transaction and Block Verifiers (zebra-consensus):

  1. Create a ValueBalance<C> struct containing an Amount<C> for each value pool
  2. Make the transaction verifier service return a ValueBalance<NegativeAllowed> for each block
  3. Put those ValueBalances in the existing PreparedBlock struct

Non-Finalized State (zebra-state):
4. Put a ValueBalance<NonNegative> in the existing non_finalized_state::Chain struct
5. Do the pool value balance contextual checks in check::block_is_contextually_valid, based on the value balance for the relevant chain
7. When committing a PreparedBlock (non-finalized block), pass the Chain's pool value balance

Checkpoint Verified FinalizedBlocks (zebra-state):
6. When committing a FinalizedBlock, calculate the value balance for that block
7. Look up the outpoints from the finalized state, asserting that they are present using future::now_or_never
8. calculate the block value balances
9. calculate the pool value balances

commit_finalized_direct (zebra-state):
10. Update the pool value balances in the state as part of each block commit batch

value_balance = (value_balance + (js.vpub_old - js.vpub_new).unwrap().constrain()).unwrap();
}
value_balance
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code inside is the exact same however as V2 and V3 sprout data is Bctv14Proof and V4 is Groth16Proof. This produce a build error when joined, i didn't researched much yet but it is probably an easy fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you move these value balance calculations into methods on Transaction.

Since they'll all return an Amount<NegativeAllowed>, any type issues will be dealt with in the Transaction methods.

Transaction::V1 { .. } => Amount::try_from(0).unwrap(),
Transaction::V2 { .. } => Amount::try_from(0).unwrap(),
Transaction::V3 { .. } => Amount::try_from(0).unwrap(),
Transaction::V4 { value_balance, .. } => *value_balance,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can change if #1886 is merged as it is because proposes to move value_balance into ShieldedData::value_balance

Copy link
Contributor

Choose a reason for hiding this comment

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

It is ok to assume that #1886 or something like it will be merged.

If we make any changes like this to #1886, they'll be picked up by the compiler.

Transaction::V3 { .. } => Amount::try_from(0).unwrap(),
Transaction::V4 { .. } => Amount::try_from(0).unwrap(),
Transaction::V5 { .. } => Amount::try_from(0).unwrap(),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Implement after #1886

Copy link
Contributor

Choose a reason for hiding this comment

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

V5 transactions have Orchard, so this code silently breaks the consensus rules.

Instead, assume that #1886 will be merged, and use it to complete this design.

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 suggested some major redesigns.

ZIP-209 isn't on the critical path for Orchard validation. And its design isn't particularly complex or surprising. So we should probably make it a low priority.

Comment on lines +1 to +4
- Feature Name: 2021-03-21
- Start Date: (fill me in with today's date, YYYY-MM-DD)
- Design PR: [ZcashFoundation/zebra#0000](https://github.com/ZcashFoundation/zebra/pull/0000)
- Zebra Issue: [ZcashFoundation/zebra#0000](https://github.com/ZcashFoundation/zebra/issues/0000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fill out these fields. The date is also in the wrong field.

Comment on lines +17 to +19
- [Miner fees](https://github.com/ZcashFoundation/zebra/blob/62b3344f4d8605aad03133b8c0fa94cfb622d4a7/book/src/dev/rfcs/xxxx-block-subsidy.md#transaction-fees-calculation) - Fees collected for a block is the value pool for that block and they belong to the miner reward.

In addition, Zcashd has RPC calls that allow to get each pool on each block with `getblock()` and the value pools at current tip with `getblockchaininfo()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the RFC doesn't mention miner fees and RPCs, so mentioning them here is a bit confusing.


Value pools are used in consensus rules in at least 2 places:

- [ZIP-209](https://zips.z.cash/zip-0209) - Consensus rules to reject any block that results in negative pools.
Copy link
Contributor

@teor2345 teor2345 Mar 21, 2021

Choose a reason for hiding this comment

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

Suggested change
- [ZIP-209](https://zips.z.cash/zip-0209) - Consensus rules to reject any block that results in negative pools.
- [ZIP-209](https://zips.z.cash/zip-0209) - Consensus rules to reject any block that results in a chain with negative pool values.

Transaction::V2 { .. } => Amount::try_from(0).unwrap(),
Transaction::V3 { .. } => Amount::try_from(0).unwrap(),
Transaction::V4 { value_balance, .. } => *value_balance,
Transaction::V5 { .. } => Amount::try_from(0).unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

V5 transactions have Sapling, so this code breaks the Zcash consensus rules.

Since the code looks valid, this bug won't be detected at compile-time or runtime. So you're relying on a reviewer (or tests) to notice the consensus rule break.

Please don't write incorrect code - even in designs or examples. Instead, use unimplemented!("v5 transactions") or something similar.


### `transparent_value_pool()`

TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

The transaction verifier already fetches all these inputs, why do it again in the state?

for input_index in 0..inputs.len() {
let rsp = script_verifier.ready_and().await?.call(script::Request {
upgrade,
known_utxos: known_utxos.clone(),
cached_ffi_transaction: cached_ffi_transaction.clone(),
input_index,
});

Here's one possible design:

Transaction and Block Verifiers (zebra-consensus):

  1. Create a ValueBalance<C> struct containing an Amount<C> for each value pool
  2. Make the transaction verifier service return a ValueBalance<NegativeAllowed> for each block
  3. Put those ValueBalances in the existing PreparedBlock struct

Non-Finalized State (zebra-state):
4. Put a ValueBalance<NonNegative> in the existing non_finalized_state::Chain struct
5. Do the pool value balance contextual checks in check::block_is_contextually_valid, based on the value balance for the relevant chain
7. When committing a PreparedBlock (non-finalized block), pass the Chain's pool value balance

Checkpoint Verified FinalizedBlocks (zebra-state):
6. When committing a FinalizedBlock, calculate the value balance for that block
7. Look up the outpoints from the finalized state, asserting that they are present using future::now_or_never
8. calculate the block value balances
9. calculate the pool value balances

commit_finalized_direct (zebra-state):
10. Update the pool value balances in the state as part of each block commit batch

i64::from_be_bytes(array).try_into().unwrap()
}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

If we add a ValueBalance type, you'll also want to serialize and deserialize it here.

Comment on lines +24 to +29
- `transparent pool` - Sum of all `tx_in` fields for transactions in the block chain, minus the sum of all `tx_out` fields for transactions in the block chain.
- `sprout pool` - Sum of all `vpub_old` fields for transactions in the block chain, minus the sum of all `vpub_new` fields for transactions in the block chain [ZIP-209](https://zips.z.cash/zip-0209#terminology).
- `sapling pool` - Negation of the sum of all `valueBalanceSapling` fields for transactions in the block chain [ZIP-209](https://zips.z.cash/zip-0209#terminology).
- `orchard pool` - Negation of the sum of all `valueBalanceOrchard` fields for transactions in the block chain.
- `transaction pool`
- `block pool`
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's distinguish "value balances" from "chain pool value balances":
https://zips.z.cash/zip-0209#terminology

Suggested change
- `transparent pool` - Sum of all `tx_in` fields for transactions in the block chain, minus the sum of all `tx_out` fields for transactions in the block chain.
- `sprout pool` - Sum of all `vpub_old` fields for transactions in the block chain, minus the sum of all `vpub_new` fields for transactions in the block chain [ZIP-209](https://zips.z.cash/zip-0209#terminology).
- `sapling pool` - Negation of the sum of all `valueBalanceSapling` fields for transactions in the block chain [ZIP-209](https://zips.z.cash/zip-0209#terminology).
- `orchard pool` - Negation of the sum of all `valueBalanceOrchard` fields for transactions in the block chain.
- `transaction pool`
- `block pool`
- `value balance` - the total value represented by a subset of the blockchain.
- `transparent value balance` - the sum of all transparent `tx_in` fields, minus the sum of all `tx_out` fields.
- `sprout value balance` - the sum of all sprout `vpub_old` fields, minus the sum of all `vpub_new` fields.
- `sapling value balance` - the negation of the sum of all `valueBalanceSapling` fields.
- `orchard value balance` - the negation of the sum of all `valueBalanceOrchard` fields.
- `transaction value pool balance` - the sum of all the value balances in each transaction.
- `block value pool balance` - the sum of all the value balances in each block.
- `chain value pool balance` - the sum of all the value balances in a valid blockchain. Each of the transparent, sprout, sapling, and orchard chain value pool balances must be non-negative.

# Drawbacks
[drawbacks]: #drawbacks

- Increase the HD requirements as new data will be saved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Increase the HD requirements as new data will be saved.
- Increase the disk space requirements as new data will be saved.

})
}
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +156 to +220
## Changes to `zebra-state/src/service/finalized_state.rs`

First we add columns for each pool:

```rust
rocksdb::ColumnFamilyDescriptor::new("transparent_pool", db_options.clone()),
rocksdb::ColumnFamilyDescriptor::new("sprout_pool", db_options.clone()),
rocksdb::ColumnFamilyDescriptor::new("sapling_pool", db_options.clone()),
rocksdb::ColumnFamilyDescriptor::new("orchard_pool", db_options.clone()),
```

At block commit(`commit_finalized_direct()`) we crete the handles for the new columns:

```rust
let transparent_pool = self.db.cf_handle("transparent_pool").unwrap();
let sprout_pool = self.db.cf_handle("sprout_pool").unwrap();
let sapling_pool = self.db.cf_handle("sapling_pool").unwrap();
let orchard_pool = self.db.cf_handle("orchard_pool").unwrap();
```

Next we save each value pool for each upcoming block:

```rust
let mut current_transparent_pool = Amount::try_from(0).unwrap();
let mut current_sprout_pool = Amount::try_from(0).unwrap();
let mut current_sapling_pool = Amount::try_from(0).unwrap();
let mut current_orchard_pool = Amount::try_from(0).unwrap();

if height > block::Height(1) {
current_transparent_pool = self.pool(Pool::Transparent).unwrap().1;
current_sprout_pool = self.pool(Pool::Sprout).unwrap().1;
current_sapling_pool = self.pool(Pool::Sapling).unwrap().1;
current_orchard_pool = self.pool(Pool::Orchard).unwrap().1;
}

batch.zs_insert(transparent_pool, height, (current_transparent_pool + block.value_pool(Pool::Transparent)).unwrap());
batch.zs_insert(sprout_pool, height, (current_sprout_pool + block.value_pool(Pool::Sprout)).unwrap());
batch.zs_insert(sapling_pool, height, (current_sapling_pool + block.value_pool(Pool::Sapling)).unwrap());
batch.zs_insert(orchard_pool, height, (current_orchard_pool + block.value_pool(Pool::Orchard)).unwrap());
```

Note that each pool is a sum of the amount in the last block plus the amount in the upcoming current block. Each block must have a value pool amount in the previous block (can be 0) except for the genesis block.

The `pool()` function will get the value of the pool at the tip as follows:

```rust
/// Returns the requested pool at tip.
pub fn pool(&self, pool_name: Pool) -> Option<(block::Height, Amount)> {
let pool = match pool_name {
Pool::Transparent => self.db.cf_handle("transparent_pool").unwrap(),
Pool::Sprout => self.db.cf_handle("sprout_pool").unwrap(),
Pool::Sapling => self.db.cf_handle("sapling_pool").unwrap(),
Pool::Orchard => self.db.cf_handle("orchard_pool").unwrap(),
};
self.db
.iterator_cf(pool, rocksdb::IteratorMode::End)
.next()
.map(|(height_bytes, amount_bytes)| {
let height = block::Height::from_bytes(height_bytes);
let amount = Amount::from_bytes(amount_bytes);

(height, amount)
})
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to store the balance for every block?

The consensus rule is that:

blocks are only valid if the chain value pool balance at that block is non-negative

Here is one way to ensure that rule:

  1. Store zero chain value pool balances for the genesis block, regardless of its contents. (This is an explicit consensus rule.)
  2. For each new block, calculate the chain value pool balances for that block, based on the chain value pool balances for the relevant tip. (The relevant tip is the chain, including all the previous blocks back to genesis.)
  3. Reject the block if any of the chain value pool balances is negative.
  4. Otherwise, commit the block, and update the chain value pool balances for the tip.

This algorithm only uses the value balance at the tip. (And there are no other consensus rules that use value balances behind the tip.)

So we can just store 4 value balances, rather than 4 million. That saves us some complexity, and a lot of disk space.

Copy link
Contributor

Choose a reason for hiding this comment

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

We want our design to support historical statistics, even if we don't store them by default.

So let's:

  • index the pool values by block height
  • store the new pool values for the finalized tip
  • delete the old pool values behind the finalized tip

That way, if we want to store historical pool statistics in a future Zebra version, our existing code will still work.

@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-docs Area: Documentation C-design Category: Software design work NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) NU-1 Sapling Network Upgrade: Sapling specific tasks NU-5 Network Upgrade: NU5 specific tasks P-Medium S-needs-triage Status: A bug report needs triage labels Mar 22, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Mar 23, 2021
@teor2345 teor2345 added this to the 2021 Sprint 8 milestone Mar 31, 2021
@mpguerra mpguerra linked an issue May 13, 2021 that may be closed by this pull request
@mpguerra mpguerra removed this from the 2021 Sprint 12 milestone May 13, 2021
# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

Each of the 4 value pools change its value with every upcoming block. This is a state feature and Zebra handle this in the `zebra-state` crate. We propose to store the pool values among a reference block height into the disk.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Each of the 4 value pools change its value with every upcoming block. This is a state feature and Zebra handle this in the `zebra-state` crate. We propose to store the pool values among a reference block height into the disk.
Each of the 4 value pools can change its value with every block added to the chain. This is a state feature and Zebra handle this in the `zebra-state` crate. We propose to store the pool values for the finalized tip height on disk.

@teor2345
Copy link
Contributor

We made a ticket for further work on this PR, see #2152.

@teor2345 teor2345 closed this May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-docs Area: Documentation C-design Category: Software design work NU Sprout Network Upgrade: Sprout specific tasks (before Overwinter) NU-1 Sapling Network Upgrade: Sapling specific tasks NU-5 Network Upgrade: NU5 specific tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZIP-209 RFC: Chain and Transaction Value Pools Design
3 participants