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

go/executor/node: check if we are in correct state before publishing a message #3342

Merged
merged 1 commit into from
Sep 30, 2020

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Sep 30, 2020

Currently (since #3320) this check is made after a node publishes a message, instead this check should be made before node publishes a message to prevent proposing another batch after node starts processing a batch.
Note that p2p.Publish is async so this shouldn't increase the time the CrossNode.Lock is held by much.

@ptrus ptrus added the c:bug Category: bug label Sep 30, 2020
@ptrus ptrus force-pushed the ptrus/fix/executor-unnecessary-publish branch from 0926df1 to a780b94 Compare September 30, 2020 08:58
.changelog/3342.bugfix.md Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/fix/executor-unnecessary-publish branch from a780b94 to 1c8e69a Compare September 30, 2020 09:02
@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #3342 into master will increase coverage by 0.08%.
The diff coverage is 53.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3342      +/-   ##
==========================================
+ Coverage   65.34%   65.43%   +0.08%     
==========================================
  Files         371      371              
  Lines       33151    33151              
==========================================
+ Hits        21664    21693      +29     
+ Misses       8281     8253      -28     
+ Partials     3206     3205       -1     
Impacted Files Coverage Δ
go/worker/compute/executor/committee/node.go 65.51% <53.84%> (+2.95%) ⬆️
go/common/grpc/errors.go 72.34% <0.00%> (-12.77%) ⬇️
go/consensus/tendermint/api/errors.go 90.00% <0.00%> (-10.00%) ⬇️
go/common/grpc/policy/policy.go 62.12% <0.00%> (-7.58%) ⬇️
go/keymanager/client/client.go 81.41% <0.00%> (-6.20%) ⬇️
...nsensus/tendermint/apps/staking/signing_rewards.go 20.00% <0.00%> (-5.72%) ⬇️
go/worker/storage/committee/checkpoint_sync.go 74.90% <0.00%> (-5.17%) ⬇️
go/worker/common/committee/runtime_host.go 62.88% <0.00%> (-5.16%) ⬇️
go/consensus/tendermint/full/services.go 81.35% <0.00%> (-4.24%) ⬇️
go/consensus/tendermint/apps/staking/state/gas.go 81.13% <0.00%> (-3.78%) ⬇️
... and 27 more

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 007ccf6...1c8e69a. Read the comment docs.

@ptrus ptrus merged commit 0755b75 into master Sep 30, 2020
@ptrus ptrus deleted the ptrus/fix/executor-unnecessary-publish branch September 30, 2020 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:bug Category: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants