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(zebra-state): Adds a method to ChainTipChange for getting unseen blocks when the best chain changes #7935

Closed
wants to merge 13 commits into from

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Nov 10, 2023

Motivation

This PR still needs test coverage.

This can be used by a task in zebra-scan to wait for unseen blocks in the best chain tip.

Closes #7927.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Solution

  • Adds reference to tip block in ChainTipBlock
  • Adds non_finalized_state_receiver to ChainTipChange
  • Adds wait_for_blocks method on ChainTipChange for getting any unseen blocks when the best chain changes

Testing

This PR needs tests.

Review

Anyone can review.

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

@arya2 arya2 added P-Medium ⚡ C-feature Category: New features A-blockchain-scanner Area: Blockchain scanner of shielded transactions labels Nov 10, 2023
@arya2 arya2 self-assigned this Nov 10, 2023

// There's a new best chain

let non_finalized_state = self.non_finalized_state_receiver.cloned_watch_data();
Copy link
Contributor Author

@arya2 arya2 Nov 11, 2023

Choose a reason for hiding this comment

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

This may need a correctness comment about it relying on the block write task sending the updated non-finalized state before updating the chain tip sender. (Other parts of Zebra rely on this too, is it obvious enough already, or would it be better to add the comment in the block write task?)

@arya2 arya2 added the A-concurrency Area: Async code, needs extra work to make it work properly. label Nov 11, 2023
Copy link
Collaborator

@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 understand this PR is still in progress, but I wanted to suggest a significant design change: rather than keeping an implied list of the blocks seen by the receiver using a task, have the receiver supply an explicit list of seen blocks to a method on the Reset variant.

This provides the following advantages:

  • there can be multiple receivers, and they don't have to be in sync with each other
  • we only do the work to find the changed blocks when a receiver asks for them
  • the blocks seen by the receiver are an explicit list, rather than being implied by the structure of the task and its channels
  • the receiver doesn't have to have processed the entire previous chain to get the correct set of changed blocks
  • the code is much simpler

So I think we could replace the seen blocks / reset task with:

  • putting the best chain's blocks and block hashes (not just the tip) in the Reset variant
  • a "changed blocks" method called by the receiving task which takes a list of recent block hashes it has processed

For example, it could look like:

fn new_best_chain_blocks(&self, seen_blocks: &OrderedMap<block::Hash>) -> Vec<Arc<Block>> {
    // if the variant is Grow, return the tip block
    // find block hashes in the best chain that aren't in seen_blocks
    // return those blocks
}

Because forks can be different lengths, we need to keep 100 blocks (the non-finalised best chain length), plus 100 times the maximum change in difficulty per block. We have two constants for it in Zebra (increase and decrease).

So when we process a block we add it to seen_blocks, then remove any blocks over that limit.

#[allow(dead_code)]
pub fn any_block_by_hash(&self, hash: block::Hash) -> Option<Arc<Block>> {
/// Returns the `block` with the given hash in any side chain.
pub fn side_chain_block_by_hash(&self, hash: block::Hash) -> Option<Arc<Block>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't replace these unused methods, they exist to make sure the API is complete. Instead, add the new methods you need. That way, when someone looks at the API, they know they have to choose between alternative methods with different behaviour.

Also, it is important to explain what a side chain is in the method documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe these methods aren't needed if the design changes?

@@ -509,6 +524,114 @@ impl ChainTipChange {
Ok(action)
}

/// Spawns a task that listens for any new blocks on the best chain tip, sending
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why spawn a new task when we can create the list when a "changed blocks" method is called on a Reset?

if I understand this code correctly, this task holds a lot of implicit state about which blocks it has and hasn't seen. (And which blocks its receiver has seen.) But that seems like an inflexible design, because what if we want multiple receivers?

What if the receiver is slow and misses two chain fork changes? (How do we delete or ignore the irrelevant blocks in the channel from the first change?)

/// # Panics
///
/// The spawned task will panic if messages are not read from the returned channel
/// receiver and the channel buffer is full when the task tries to send a block.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: Panicking when reading stops is not a robust design. But I'm not sure if that will matter, because I don't think the task is needed.

/// # Panics
///
/// This method is meant to be used with `spawn_wait_for_blocks` and could panic when called
/// after the `ChangeTipChange` instance misses a tip block change.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: again, panicking because events don't happen in the expected order seems risky.

@arya2
Copy link
Contributor Author

arya2 commented Nov 13, 2023

This is not needed for now.

@arya2 arya2 closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-concurrency Area: Async code, needs extra work to make it work properly. C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(zebra-state): Send a reference to latest non-finalized state in TipAction::Reset
2 participants