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

blockchain/v2: fix panic: processed height X+1 but expected height X #5530

Merged
merged 3 commits into from
Oct 20, 2020
Merged

blockchain/v2: fix panic: processed height X+1 but expected height X #5530

merged 3 commits into from
Oct 20, 2020
+21 −22

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Oct 20, 2020 •
edited
Loading

Before: scheduler receives psBlockProcessed event, but does not mark block as processed because peer timed out (or was removed for other reasons) and all associated blocks were rescheduled.

After: scheduler receives psBlockProcessed event and marks block as processed in any case (even if peer who provided this block errors).

Closes #5387

@melekes melekes requested a review from tac0turtle as a code owner October 20, 2020 07:41
@melekes melekes added the S:automerge Automatically merge PR when requirements pass label Oct 20, 2020
@melekes melekes self-assigned this Oct 20, 2020
@melekes melekes changed the title blockchain/v2: fix panic: processed height 4541 but expected height 4540 blockchain/v2: fix panic: processed height X+1 but expected height X Oct 20, 2020
@codecov
Copy link

codecov bot commented Oct 20, 2020 •
edited
Loading

Codecov Report

Merging #5530 into master will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5530      +/-   ##
==========================================
- Coverage   61.15%   61.09%   -0.06%     
==========================================
  Files         263      263              
  Lines       23730    23729       -1     
==========================================
- Hits        14511    14497      -14     
- Misses       7744     7753       +9     
- Partials     1475     1479       +4
Impacted Files Coverage Δ
blockchain/v2/scheduler.go 83.49% <100.00%> (-0.70%) ⬇️
crypto/sr25519/pubkey.go 43.47% <0.00%> (-8.70%) ⬇️
consensus/reactor.go 74.33% <0.00%> (-3.40%) ⬇️
privval/signer_endpoint.go 78.78% <0.00%> (-3.04%) ⬇️
consensus/state.go 68.44% <0.00%> (ø)
proxy/multi_app_conn.go 48.64% <0.00%> (ø)
blockchain/v0/pool.go 79.70% <0.00%> (+0.36%) ⬆️
p2p/pex/pex_reactor.go 78.86% <0.00%> (+0.59%) ⬆️
statesync/syncer.go 79.32% <0.00%> (+0.84%) ⬆️
p2p/switch.go 68.32% <0.00%> (+4.65%) ⬆️

@melekes melekes removed the S:automerge Automatically merge PR when requirements pass label Oct 20, 2020
erikgrinaker
erikgrinaker approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM, nice find

@erikgrinaker erikgrinaker mentioned this pull request Oct 20, 2020
Merged
@melekes melekes merged commit b4adeab into master Oct 20, 2020
@melekes melekes deleted the anton/5387-blockchain-v2-2 branch October 20, 2020 10:29
mergify bot pushed a commit that referenced this pull request Oct 20, 2020
@erikgrinaker
test: enable blockchain v2 in E2E testnet generator (#5533)
9e6248c 
When #5499 and #5530 land, we can re-enable v2 in the E2E testnet generator (and thus the nightly E2E tests).
erikgrinaker added a commit that referenced this pull request Oct 20, 2020
@erikgrinaker
test: enable blockchain v2 in E2E testnet generator (#5533)
306bbad 
When #5499 and #5530 land, we can re-enable v2 in the E2E testnet generator (and thus the nightly E2E tests).
melekes added a commit that referenced this pull request Oct 21, 2020
@melekes
blockchain/v2: fix panic: processed height X+1 but expected height X (#…
020edbc 
…5530)

Before: scheduler receives psBlockProcessed event, but does not mark block as processed because peer timed out (or was removed for other reasons) and all associated blocks were rescheduled.

After: scheduler receives psBlockProcessed event and marks block as processed in any case (even if peer who provided this block errors).

Closes #5387
erikgrinaker added a commit that referenced this pull request Oct 22, 2020
@erikgrinaker
test: enable blockchain v2 in E2E testnet generator (#5533)
44f588b 
When #5499 and #5530 land, we can re-enable v2 in the E2E testnet generator (and thus the nightly E2E tests).
erikgrinaker added a commit that referenced this pull request Oct 22, 2020
@erikgrinaker
test: enable blockchain v2 in E2E testnet generator (#5533)
1cb6c17 
When #5499 and #5530 land, we can re-enable v2 in the E2E testnet generator (and thus the nightly E2E tests).
erikgrinaker added a commit that referenced this pull request Oct 22, 2020
@erikgrinaker
test: enable blockchain v2 in E2E testnet generator (#5533)
d113da0 
When #5499 and #5530 land, we can re-enable v2 in the E2E testnet generator (and thus the nightly E2E tests).
@cmwaters cmwaters mentioned this pull request Feb 22, 2022
Merged
This was referenced Apr 22, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikgrinaker erikgrinaker

@ebuchman ebuchman

@tessr tessr

@tac0turtle tac0turtle

Assignees

@melekes melekes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

blockchain/v2: nondeterministic panic
2 participants
@melekes @erikgrinaker