Skip to content

Update ADR-025 and mark it as Accepted#3958

Merged
tac0turtle merged 6 commits intomasterfrom
bucky/adr-025-update
Sep 11, 2019
Merged

Update ADR-025 and mark it as Accepted#3958
tac0turtle merged 6 commits intomasterfrom
bucky/adr-025-update

Conversation

@ebuchman
Copy link
Contributor

@ebuchman ebuchman commented Sep 7, 2019

Added some more context and reference to issues, and marked it as accepted. We should start moving forward on this and roll out a plan for getting it tested on gaia testnets and ultimately proposed to the cosmos hub for a breaking upgrade (ie. the hub's first breaking upgrade of tendermint itself). cc @zmanian @marbar3778 @milosevic. We can probably include a couple other small block protocol breaking changes, but we shouldn't really block on it.

Summary of the result here:

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@codecov-io
Copy link

codecov-io commented Sep 7, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3958      +/-   ##
==========================================
- Coverage      67%   66.95%   -0.05%     
==========================================
  Files         219      219              
  Lines       18486    18486              
==========================================
- Hits        12386    12377       -9     
- Misses       5178     5189      +11     
+ Partials      922      920       -2
Impacted Files Coverage Δ
privval/signer_server.go 95.65% <0%> (-4.35%) ⬇️
privval/signer_endpoint.go 81.33% <0%> (-2.67%) ⬇️
p2p/pex/pex_reactor.go 82.02% <0%> (-2.32%) ⬇️
libs/clist/clist.go 66.66% <0%> (-1.52%) ⬇️
consensus/state.go 79.2% <0%> (-0.36%) ⬇️
consensus/reactor.go 79.01% <0%> (-0.24%) ⬇️
blockchain/v0/pool.go 80.65% <0%> (+0.65%) ⬆️
blockchain/v0/reactor.go 79.71% <0%> (+0.94%) ⬆️
consensus/replay.go 72.98% <0%> (+2.01%) ⬆️
consensus/ticker.go 95.83% <0%> (+4.16%) ⬆️

@tac0turtle tac0turtle mentioned this pull request Sep 9, 2019
5 tasks
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

created. This was the consensus from
[#3485](https://github.com/tendermint/tendermint/issues/3485)

We may want to consider supporting other blockIDs later, as a way to capture
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't it be possible to capture evidence without including votes for other blockIDs into CommitSig? The same question for nil. If it's possible, then we won't have to include votes for nil / other block IDs.

Copy link
Contributor Author

@ebuchman ebuchman Sep 10, 2019

Choose a reason for hiding this comment

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

We want to include nil to signal about validator liveness since its not necessarily a validators fault if they voted nil. But it is their fault for voting a different block ID (unless +1/3 are malicious). I'm not sure exactly the case here where it would help to capture this in the commit but I figured I'd just leave the note so that we think about it later. Meanwhile, I think we move forward with including votes for nil but not votes for other blockids.

Copy link
Contributor

@milosevic milosevic left a comment

Choose a reason for hiding this comment

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

Very nice and complete.

@tac0turtle tac0turtle merged commit fc1c37c into master Sep 11, 2019
@tac0turtle tac0turtle deleted the bucky/adr-025-update branch September 11, 2019 14:10
@zmanian
Copy link
Contributor

zmanian commented Sep 13, 2019

This seems acceptable but we are most likely going to want to make future breaking changes.

This block header structure is seems nearly a perfect fit for "enterprise" Tendermint use case but will probably need to change if we see increased demand for many validators public chains.

@ebuchman
Copy link
Contributor Author

ebuchman commented Sep 13, 2019

This block header structure is seems nearly a perfect fit for "enterprise" Tendermint use case but will probably need to change if we see increased demand for many validators public chains.

Can you clarify? You mean aggregation and/or randomness or something else ?

I think basically we're looking for a structure that will tide us over sufficiently until aggregation.

Also note Dev's proposal here if you haven't seen - would appreciate more review of it: #7892

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

6 participants