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

Refactor BestTipHeight into a generic ChainTip sender and receiver #2676

Merged
merged 7 commits into from
Aug 27, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Aug 26, 2021

Motivation

In ticket #2639, we want to get block hashes, transaction information, and rollback information out of the state.

The BestTipHeight struct already gets block heights out of the state. We can add this other information to it as well.

Solution

  • Rename BestTipHeight to ChainTipSender
  • Rename the best_tip_height module to chain_tip
  • Wrap the chain tip watch channel in a ChainTipReceiver struct
  • Create a ChainTip trait to avoid tricky crate dependencies
  • Use the ChainTip trait in zebra-network

This refactor does not add any new information, or close any tickets.

Review

@jvff worked on this a few weeks ago.

The code might make more sense once you've had a look at PR #2677, which adds block hashes to the chain tip recevier.

Reviewer Checklist

  • Code makes sense
  • Existing tests pass

Follow Up Work

Actually provide all the extra information.

`fastmod BestTipHeight ChainTipSender zebra*`

For senders:
`fastmod best_tip_height chain_tip_sender zebra*`

For receivers:
`fastmod best_tip_height chain_tip_receiver zebra*`
And add convenience impls for optional and empty chain tips.
@teor2345 teor2345 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium labels Aug 26, 2021
@teor2345 teor2345 requested a review from jvff August 26, 2021 04:09
@teor2345 teor2345 self-assigned this Aug 26, 2021
@mpguerra

This comment has been minimized.

jvff
jvff previously approved these changes Aug 26, 2021
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks good, I only added one (small but non-trivial) improvement suggestion.

zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
zebra-network/src/peer/handshake.rs Outdated Show resolved Hide resolved
zebra-chain/src/chain_tip.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

Need to take this PR into account when we get around to doing #2573

I've updated the type names in that ticket, and added another impacted type.

Co-authored-by: Janito Vaqueiro Ferreira Filho <[email protected]>
@teor2345 teor2345 enabled auto-merge (squash) August 26, 2021 23:58
@teor2345 teor2345 requested a review from jvff August 26, 2021 23:59
@teor2345 teor2345 disabled auto-merge August 27, 2021 01:34
@teor2345 teor2345 merged commit d2e14b2 into main Aug 27, 2021
@teor2345 teor2345 deleted the refactor-best-tip-height branch August 27, 2021 01:34
@teor2345
Copy link
Contributor Author

I applied the approved updates, and we're a few PRs behind, so I admin-merged this PR.

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

👍

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-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants