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/roothash: Finish commitment processing early when possible #3790

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

kostko
Copy link
Member

@kostko kostko commented Mar 17, 2021

When the set of current commitments already determines the fate of the process
(e.g., when there is a discrepancy or a majority of votes indicate success),
proceed with the process instead of waiting for the remaining commitments.

@kostko kostko added c:roothash Category: root hash service c:breaking/consensus Category: breaking consensus changes labels Mar 17, 2021
@kostko kostko force-pushed the kostko/feature/roothash-pool-enough-early branch 2 times, most recently from f6c10a6 to b5d2085 Compare March 17, 2021 11:18
Copy link
Member

@ptrus ptrus left a comment

Choose a reason for hiding this comment

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

Nice! The ProcessCommitments code looks correct. Be sure to add tests to pool_test for the new early processing cases (i know you already plan to do it 😬 )

@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #3790 (b5d2085) into master (5bb676e) will increase coverage by 0.11%.
The diff coverage is 92.42%.

❗ Current head b5d2085 differs from pull request most recent head d604107. Consider uploading reports for the commit d604107 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3790      +/-   ##
==========================================
+ Coverage   67.10%   67.22%   +0.11%     
==========================================
  Files         403      403              
  Lines       40912    40874      -38     
==========================================
+ Hits        27454    27477      +23     
+ Misses       9567     9506      -61     
  Partials     3891     3891              
Impacted Files Coverage Δ
go/roothash/api/commitment/pool.go 69.00% <92.42%> (-1.00%) ⬇️
go/oasis-node/cmd/common/metrics/disk.go 65.38% <0.00%> (-19.24%) ⬇️
go/consensus/tendermint/apps/governance/query.go 83.33% <0.00%> (-16.67%) ⬇️
go/oasis-node/cmd/common/metrics/resource.go 78.94% <0.00%> (-10.53%) ⬇️
go/worker/upgrade/worker.go 64.35% <0.00%> (-4.96%) ⬇️
go/runtime/tagindexer/tagindexer.go 66.66% <0.00%> (-4.60%) ⬇️
...consensus/tendermint/apps/roothash/transactions.go 66.19% <0.00%> (-4.29%) ⬇️
go/consensus/tendermint/full/services.go 75.83% <0.00%> (-4.17%) ⬇️
go/consensus/tendermint/apps/beacon/state/state.go 71.42% <0.00%> (-3.18%) ⬇️
go/storage/mkvs/iterator.go 80.29% <0.00%> (-2.92%) ⬇️
... and 30 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 9df8f95...d604107. Read the comment docs.

@kostko kostko force-pushed the kostko/feature/roothash-pool-enough-early branch from b5d2085 to a5631b0 Compare March 17, 2021 12:33
@kostko kostko marked this pull request as ready for review March 17, 2021 12:33
@kostko
Copy link
Member Author

kostko commented Mar 17, 2021

Be sure to add tests to pool_test for the new early processing cases (i know you already plan to do it 😬 )

Yeah, test case is in now. Please take one more look.

Copy link
Member

@ptrus ptrus 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 👍

@kostko kostko enabled auto-merge March 17, 2021 13:55
When the set of current commitments already determines the fate of the process
(e.g., when there is a discrepancy or a majority of votes indicate success),
proceed with the process instead of waiting for the remaining commitments.
@kostko kostko force-pushed the kostko/feature/roothash-pool-enough-early branch from a5631b0 to d604107 Compare March 17, 2021 14:07
@kostko kostko merged commit ce69407 into master Mar 17, 2021
@kostko kostko deleted the kostko/feature/roothash-pool-enough-early branch March 17, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:breaking/consensus Category: breaking consensus changes c:roothash Category: root hash service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants