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 execution halt when reloading blocks #924

Merged
merged 6 commits into from Jul 7, 2021
Merged

Conversation

m4ksio
Copy link
Contributor

@m4ksio m4ksio commented Jul 3, 2021

No description provided.

@m4ksio m4ksio requested a review from ramtinms as a code owner July 3, 2021 06:02
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2021

Codecov Report

Merging #924 (a407825) into master (9f526ca) will increase coverage by 0.21%.
The diff coverage is 78.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #924      +/-   ##
==========================================
+ Coverage   54.84%   55.05%   +0.21%     
==========================================
  Files         277      277              
  Lines       18322    18324       +2     
==========================================
+ Hits        10048    10089      +41     
+ Misses       6925     6879      -46     
- Partials     1349     1356       +7     
Flag Coverage Δ
unittests 55.05% <78.94%> (+0.21%) ⬆️

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

Impacted Files Coverage Δ
module/mempool/queue/queue.go 96.00% <ø> (-0.67%) ⬇️
engine/execution/ingestion/engine.go 59.62% <78.94%> (+8.67%) ⬆️
engine/common/synchronization/engine.go 65.59% <0.00%> (ø)
network/p2p/ping.go 54.54% <0.00%> (+2.27%) ⬆️
module/mempool/epochs/transactions.go 100.00% <0.00%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f526ca...a407825. Read the comment docs.

Copy link
Member

@zhangchiqing zhangchiqing left a comment

Choose a reason for hiding this comment

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

Thanks for the fix 👍

engine/execution/ingestion/engine.go Outdated Show resolved Hide resolved
module/mempool/queue/queue.go Outdated Show resolved Hide resolved
// we receive H <- G, then the queues will become:
// A <- B <- C
// ^- D <- E
// G
func enqueue(blockify queue.Blockify, queues *stdmap.QueuesBackdata) (*queue.Queue, bool) {
func enqueue(blockify queue.Blockify, queues *stdmap.QueuesBackdata) (*queue.Queue, bool, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Better to explain the two bool value. I think the first is whether the block is a new block, the second is whether the block is the head of a queue.

queue, false, false, means the block is a duplication
queue, true, true, means it's a block that is the head of a queue
queue, true, false means it's a block that is not the head of a queue, but extending a existing queue.`

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely feel like once we get to two bools, might be more clear to either change to a "queue status" enum of some sort, or maybe a struct with two bool fields, for improved clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think enum would help, as there are two decision to be made - first value (if fresh block/or not duplicate) let you bail out fast, second decides if execution can start. Representing this login with enum would seems more complicated.

engine/execution/ingestion/engine.go Outdated Show resolved Hide resolved
engine/execution/ingestion/engine.go Show resolved Hide resolved
Copy link
Member

@Kay-Zee Kay-Zee left a comment

Choose a reason for hiding this comment

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

Theres some clean up that @zhangchiqing mentioned that i would definitely like to see, but the functionality makes sense to me

// we receive H <- G, then the queues will become:
// A <- B <- C
// ^- D <- E
// G
func enqueue(blockify queue.Blockify, queues *stdmap.QueuesBackdata) (*queue.Queue, bool) {
func enqueue(blockify queue.Blockify, queues *stdmap.QueuesBackdata) (*queue.Queue, bool, bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely feel like once we get to two bools, might be more clear to either change to a "queue status" enum of some sort, or maybe a struct with two bool fields, for improved clarity.

module/mempool/queue/queue.go Outdated Show resolved Hide resolved
Add warn for impossible unexecutable highest block
Clarified enqueue return values
@m4ksio m4ksio merged commit d457915 into master Jul 7, 2021
@m4ksio m4ksio deleted the m4ksio/queueing-fix branch July 7, 2021 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants