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.

Sign up for GitHub

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

Jump to bottom

Stop panicking when using sync and async methods on the same ChainTipChange #2800

Merged
merged 12 commits into from
Sep 28, 2021
Merged

Stop panicking when using sync and async methods on the same ChainTipChange #2800

merged 12 commits into from
Sep 28, 2021

Conversation

teor2345
Copy link
Contributor

Motivation

In PR #2729, I used wait_for_tip_change and last_tip_change on the same ChainTipChange.
But that causes an assertion failure in some cases.

Solution

  • Ignore changes if the tip hash is the same
  • Expand the tests to cover these cases

Review

@jvff knows all the details here.

Reviewer Checklist

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

@teor2345 teor2345 added C-bug Category: This is a bug A-rust Area: Updates to Rust code P-Medium I-panic Zebra panics with an internal error message labels Sep 24, 2021
@teor2345 teor2345 added this to the 2021 Sprint 19 milestone Sep 24, 2021
@teor2345 teor2345 requested a review from jvff September 24, 2021 05:23
@teor2345 teor2345 self-assigned this Sep 24, 2021
@teor2345 teor2345 changed the title Stop panicking when using sync and async methods on ChainTipChange Stop panicking when using sync and async methods on the same ChainTipChange Sep 24, 2021
@teor2345 teor2345 mentioned this pull request Sep 24, 2021
Merged
3 tasks
jvff
jvff previously approved these changes Sep 26, 2021
View reviewed changes
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 just added some optional suggestions and some minor questions.

zebra-state/src/service/chain_tip.rs Outdated Show resolved Hide resolved
zebra-state/src/service/chain_tip.rs Outdated Show resolved Hide resolved
zebra-state/src/service/chain_tip/tests/vectors.rs Outdated Show resolved Hide resolved
zebra-state/src/service/chain_tip/tests/prop.rs Outdated Show resolved Hide resolved
@teor2345 teor2345 requested a review from jvff September 27, 2021 22:13
@teor2345 teor2345 enabled auto-merge (squash) September 28, 2021 00:32
@teor2345 teor2345 merged commit 1601c9f into main Sep 28, 2021
@teor2345 teor2345 deleted the chain-tip-change-panic branch September 28, 2021 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvff jvff jvff approved these changes

Assignees

@teor2345 teor2345

Labels
A-rust Area: Updates to Rust code C-bug Category: This is a bug I-panic Zebra panics with an internal error message
Projects
None yet
Milestone
2021 Sprint 19
Development

Successfully merging this pull request may close these issues.
2 participants
@teor2345 @jvff