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/consensus/tendermint: Store consensus parameters in ABCI state #2715

Closed
wants to merge 1 commit into from

Conversation

kostko
Copy link
Member

@kostko kostko commented Feb 24, 2020

Fixes #2710.

This allows for updating the consensus parameters later.

@kostko kostko added the c:breaking/consensus Category: breaking consensus changes label Feb 24, 2020
This allows for updating the consensus parameters later.
@kostko kostko force-pushed the kostko/feature/consensus-selfparams-state branch from 38027bf to 67e27bc Compare February 24, 2020 13:04
@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

Merging #2715 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2715      +/-   ##
==========================================
- Coverage   63.10%   62.91%   -0.19%     
==========================================
  Files         378      378              
  Lines       35515    35531      +16     
==========================================
- Hits        22413    22356      -57     
- Misses      10280    10335      +55     
- Partials     2822     2840      +18     
Impacted Files Coverage Δ
go/storage/mkvs/urkel/writelog/iterator.go 76.36% <0.00%> (-16.37%) ⬇️
go/worker/common/host/interface.go 38.46% <0.00%> (-15.39%) ⬇️
go/common/grpc/policy/policy.go 65.71% <0.00%> (-7.15%) ⬇️
go/worker/common/p2p/p2p.go 65.31% <0.00%> (-5.86%) ⬇️
.../consensus/tendermint/apps/epochtime_mock/state.go 84.61% <0.00%> (-5.13%) ⬇️
go/worker/storage/service_external.go 43.01% <0.00%> (-4.31%) ⬇️
go/worker/storage/committee/node.go 77.24% <0.00%> (-3.71%) ⬇️
go/storage/mkvs/urkel/prefetch.go 59.32% <0.00%> (-3.39%) ⬇️
go/storage/api/grpc.go 66.88% <0.00%> (-3.02%) ⬇️
go/worker/common/host/protocol/protocol.go 65.67% <0.00%> (-2.99%) ⬇️
... and 15 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 177e474...67e27bc. Read the comment docs.

@Yawning
Copy link
Contributor

Yawning commented Feb 24, 2020

I'm not convinced this is correct. This behaves quite differently from everything else (that stages alterations to state in the checktx tree, then applies them to the delivertx tree).

Additionally, while I am in favor of "performance", I'm not convinced that caching this is worth the hassle from a safety point of view.

@kostko
Copy link
Member Author

kostko commented Feb 24, 2020

Right, so "set" must not be used outside InitChain/BeginBlock/EndBlock. We can enforce this nicely (also for all other apps where we support setting consensus parameters) in #2674 so let's move it there.

@kostko kostko closed this Feb 24, 2020
@kostko kostko deleted the kostko/feature/consensus-selfparams-state branch February 24, 2020 14:16
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store consensus parameters for consensus backend itself
2 participants