Tracking: Finalized State Implementation

[yaahc]

Finalized State Implementation

Bugs

• Assert that the chain hash and height are valid in commit_finalized_direct - Add check to ensure heights in state service are sequential #1290
  • previous hash must match the finalized tip hash
  • height must be one more than the finalized tip height
  • The genesis block does not have a parent block. For genesis blocks, check that block's parent hash is null (all zeroes) and its height is 0.
  • Note: the TODO for this change is in the wrong location - the check should be before the sled transaction
• Make sure that our existing tests pass the genesis and non-genesis assertion success cases
  • there's no need to test assertion failures
  • Test a block with a matching previous hash, but an incorrect height (cases: parent height, parent height + 2)
• If the block is a genesis block, skip any transaction updates. (Due to a bug in zcashd, genesis block transactions are ignored during validation.)

Incomplete

These changes all require a state version increment, so they should be done in PR #1172.

• For each TransparentInput::PrevOut { outpoint, .. } in the transaction's inputs(), remove outpoint from utxo_by_output
• Insert (transaction_hash, block_height || BE32(tx_index)) to tx_by_hash
• For each JoinSplit description in the transaction, insert (nullifiers[0],()) and (nullifiers[1],()) into sprout_nullifiers
• For each Spend description in the transaction, insert (nullifier,()) into sapling_nullifiers

Cleanup

• remove TODO comment about deduplicating serialization calls in sled_state when committing new blocks to sled

Tests

• Assert that hash_by_height, height_by_hash, and block_by_height are consistent, and are all bijections (1:1 mappings) by asserting insert doesn't overwrite an existing value
• write prop tests for sled formats (Add prop tests for each type that implements both IntoSled and FromSled #1214)

[teor2345]

@yaahc I've added the height and hash assertions to the checklist in this ticket:

Assert that the chain hash and height are valid in commit_finalized_direct...

These assertions were in the Request::CommitFinalizedBlock of the State RFC and the State RFC tracking issue #1049, but they actually get implemented in the FinalizedState. (They didn't make it into this tracking issue or the state service tracking issue #1251 during the split of #1049.)

[yaahc]

So it turns out the previous hash check won't be necessary. We commit blocks by looking them up based on their previous hash, so that bug is impossible by construction.

Also, I don't think it's worthwhile to add tests to ensure this assert works. I feel the logic is short enough to reason about and I'm confident the check will work as desired.

[teor2345]

So it turns out the previous hash check won't be necessary. We commit blocks by looking them up based on their previous hash, so that bug is impossible by construction.

See my response in the PR.

Also, I don't think it's worthwhile to add tests to ensure this assert works. I feel the logic is short enough to reason about and I'm confident the check will work as desired.

Also, I don't think it's worthwhile to add tests to ensure this assert works. I feel the logic is short enough to reason about and I'm confident the check will work as desired.

One of the checks was incorrect in the latest version of the PR. Some of our existing tests already commit a genesis block to the state via that code path, so CI failed.

I'm ok with skipping tests for assertion failures. But let's make sure that the existing tests cover genesis and non-genesis assertion successes. (If we move the checks to commit_finalized_direct, then they'll definitely be tested by our existing CI.)