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

fix: poll logic when pipeline active #3761

Merged
merged 1 commit into from
Jul 13, 2023
Merged

fix: poll logic when pipeline active #3761

merged 1 commit into from
Jul 13, 2023

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Jul 13, 2023

Closes #3754

the prune changes broke the poll logic when the pipeline is active

#3566

in which case we were returning pending even if there still is work to do (available engine messages for example)

@mattsse mattsse requested a review from shekhirin July 13, 2023 15:58
@mattsse mattsse added C-bug An unexpected or incorrect behavior A-consensus Related to the consensus engine labels Jul 13, 2023
Copy link
Collaborator

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

LGTM

@mattsse mattsse enabled auto-merge July 13, 2023 16:07
Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Would tokio console or something help us detect this somehow if we were running it live and observed the insights from it?

@mattsse
Copy link
Collaborator Author

mattsse commented Jul 13, 2023

need to familiarize myself with tokio console features first, if it's possible to monitor certain tasks and their state

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #3761 (f7fd093) into main (f3e13db) will increase coverage by 7.29%.
The diff coverage is 100.00%.

Impacted file tree graph

Impacted Files Coverage Δ
crates/consensus/beacon/src/engine/mod.rs 76.78% <100.00%> (+11.34%) ⬆️

... and 183 files with indirect coverage changes

Flag Coverage Δ
integration-tests 12.72% <0.00%> (-3.08%) ⬇️
unit-tests 64.33% <100.00%> (+9.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 26.15% <ø> (+6.78%) ⬆️
blockchain tree 83.02% <ø> (+6.65%) ⬆️
pipeline 87.13% <ø> (+10.81%) ⬆️
storage (db) 73.24% <ø> (+5.10%) ⬆️
trie 94.65% <ø> (+11.53%) ⬆️
txpool 49.37% <ø> (+7.55%) ⬆️
networking 77.31% <ø> (+10.63%) ⬆️
rpc 49.91% <ø> (-1.85%) ⬇️
consensus 64.90% <100.00%> (+7.71%) ⬆️
revm 32.98% <ø> (+6.26%) ⬆️
payload builder 4.13% <ø> (-2.49%) ⬇️
primitives 87.99% <ø> (+11.68%) ⬆️

@gakonst
Copy link
Member

gakonst commented Jul 13, 2023

cc @jonasbostoen as i saw you using tokio-console before, in case any input

Copy link
Member

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

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

good find!

@mattsse mattsse added this pull request to the merge queue Jul 13, 2023
Merged via the queue into main with commit 7edab97 Jul 13, 2023
@mattsse mattsse deleted the matt/fix-poll-logic branch July 13, 2023 17:03
merklefruit pushed a commit to anton-rs/op-reth that referenced this pull request Jul 18, 2023
merklefruit pushed a commit to merklefruit/op-reth-old that referenced this pull request Jul 26, 2023
merklefruit pushed a commit to anton-rs/op-reth that referenced this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Related to the consensus engine C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continous timeout error between CL<>EL communication
4 participants