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 dummy implementation of FinalizedState::history_tree #2461

Closed
wants to merge 1 commit into from

Conversation

conradoplg
Copy link
Collaborator

Motivation

#2135 adds support for the ZIP-221 history tree in the non-finalized state. However, it needs to read the tree from the finalized state, and a todo! method was added to it. However, that makes zebrad crash.

Specifications

N/A

Designs

N/A

Solution

This adds a dummy implementation that will prevent it from crashing (but it won't give the correct result)

Review

Not sure if this is really needed (can we just wait for #2135?), so it's a draft. I also haven't tested it yet.

@teor2345 might want to take a look. This is critical only if stopping zebrad from panicking is critical right now.

Reviewer Checklist

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

Follow Up Work

#2135 will properly fix this.

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-bug Category: This is a bug P-High labels Jul 8, 2021
@teor2345
Copy link
Contributor

teor2345 commented Jul 8, 2021

Not sure if this is really needed (can we just wait for #2135?), so it's a draft. I also haven't tested it yet.

We need to calculate the note commitment trees in the non-finalized and finalized state before we do #2135. Leaving the non-finalized tests disabled until all that work is done risks introducing more bugs.

@teor2345 might want to take a look. This is critical only if stopping zebrad from panicking is critical right now.

This is critical - we don't want to merge any more code until the non-finalized state is covered by tests, and they're passing.

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.

This PR breaks the out-of-order block commit test - it never finishes:

test service::tests::state_behaves_when_blocks_are_committed_out_of_order has been running for over 60 seconds
Error: The operation was canceled.

https://github.com/ZcashFoundation/zebra/pull/2461/checks?check_run_id=3013159372#step:10:1750

.expect("block will exist if tip does");
// TODO: pass roots
// TODO: return error?
let tree = HistoryTree::from_block(self.network, tip, &Default::default(), None)
Copy link
Contributor

Choose a reason for hiding this comment

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

This history tree gets passed to check::block_commitment_is_valid_for_chain_history, which will fail because the history tree doesn't have the right blocks.

So we'll also need to disable check::block_commitment_is_valid_for_chain_history.

And at that point we might as well revert the PR. 😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep 😢 I'll close this PR since it doesn't make sense anymore.

@conradoplg
Copy link
Collaborator Author

Closing this because it seems we can't avoid doing everything bottom-up, so we need to implement everything that is required by #2135 beforing merging its PR.

@conradoplg conradoplg closed this Jul 8, 2021
@teor2345 teor2345 deleted the dummy-finalized-history-tree branch March 21, 2022 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants