[R4R] Add proposer to NewRound event and proposal info to CompleteProposal event #2767

kevlubkcm · 2018-11-06T22:14:48Z

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

… set yet

codecov-io · 2018-11-06T22:22:09Z

Codecov Report

Merging #2767 into develop will decrease coverage by 0.06%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2767      +/-   ##
===========================================
- Coverage    62.21%   62.15%   -0.07%     
===========================================
  Files          212      212              
  Lines        17269    17212      -57     
===========================================
- Hits         10744    10698      -46     
+ Misses        5621     5608      -13     
- Partials       904      906       +2

Impacted Files	Coverage Δ
consensus/state.go	`79.78% <100%> (ø)`	⬆️
privval/tcp_server.go	`78.57% <0%> (-2.86%)`	⬇️
consensus/reactor.go	`66.39% <0%> (-1.74%)`	⬇️
libs/db/prefix_db.go	`58.55% <0%> (+0.82%)`	⬆️
privval/ipc_server.go	`69.81% <0%> (+5.66%)`	⬆️

…to proposerevent

ebuchman

Nice - just add the Step, and a CHANGELOG_PENDING#Improvements. Anything else ?

types/events.go

kevlubkcm · 2018-11-12T13:47:59Z

I was thinking that maybe we should also add the ProposerAddress and ProposerIndex to the NewRound event as well, since we may want to know who the proposer should be before actually receiving the proposal. wdyt? should I include that in this pull request or open a separate one?

ebuchman · 2018-11-12T14:07:05Z

Hmm - can you briefly describe the use case here? We should try to think about the event msgs a bit more holistically before we start adding fields here and there.

kevlubkcm · 2018-11-12T14:28:50Z

Sure. I am trying to build a live tendermint consensus visualizer with all of the critical steps for consensus

essentially a live version of this flow chart https://tendermint.com/docs/assets/img/consensus_logic.e9f4ca6f.png
(with a visual for each validator and each message being passed around)

Really the only thing I am missing is being able to highlight the proposer. Originally I was thinking it makes sense to include that in the CompleteProposal event, but that is at the wrong point. Makes more sense to do it at New Round and/or New Height. (though also it is nice to get the BlockID in the CompleteProposal event)

ebuchman · 2018-11-12T15:01:01Z

Right of course. I think a NewRound msg makes sense. Currently we just use EventDataRoundState but I don't think we should add the proposer stuff to that. Instead we should probably have a new type, EventDataNewRound, that has the basic H/R/S info as well as a ProposerInfo field that contains the proposer's address and index. How's that sound?

…te changelog

kevlubkcm · 2018-11-12T15:07:32Z

sounds good to me. will update

types/events.go

ebuchman

Thanks! I'll address most of my comments and merge this :)

ebuchman · 2018-11-15T23:27:04Z

types/events.go

+	Round  int    `json:"round"`
+	Step   string `json:"step"`
+
+	BlockID         BlockID `json:"block_id"`


Do we want to add the ValidatorInfo to this event as well? Seems it would make sense

I had it there at first, but EventDataNewRound seems like the correct place to put it. Can add it back if you like.

No worries. Let's leave it as is for now - can always add it later.

consensus/types/round_state.go

ebuchman · 2018-11-15T23:30:03Z

consensus/common_test.go

@@ -492,6 +524,31 @@ func ensureVote(voteCh <-chan interface{}, height int64, round int,
 	}
 }

+func ensureProposal(proposalCh <-chan interface{}, height int64, round int, propId types.BlockID) {


What prompted the need for this if we already have ensureNewProposal?

allows us to also assert details about the proposal (eg we want to assert this specific block is proposed vs a block is proposed). I took the pattern from ensureNewVote (though it isnt actually used at the moment) vs ensureVote

Yep - we probably want to replace ensureNewProposal with this eventually so we're always asserting the correct value. Fine for now.

consensus/common_test.go

CHANGELOG_PENDING.md

kevlubkcm · 2018-11-16T16:00:55Z

thanks! responded to leftover comments

kevlubkcm added 3 commits November 6, 2018 18:07

add proposer info to EventCompleteProposal

d843c94

create separate EventData structure for CompleteProposal

f4c4718

cant us rs.Proposal to get BlockID because it is not guaranteed to be…

390f955

… set yet

kevlubkcm requested review from ebuchman, melekes and xla as code owners November 6, 2018 22:14

kevlubkcm added 2 commits November 6, 2018 22:31

copying RoundState isnt helping us here

3a040d5

Merge branch 'develop' of https://github.com/tendermint/tendermint in…

2d46f0e

…to proposerevent

ebuchman reviewed Nov 11, 2018

View reviewed changes

types/events.go Show resolved Hide resolved

add Step back to make compatible with original RoundState event. upda…

b08349b

…te changelog

kevlubkcm added 2 commits November 12, 2018 15:33

add NewRound event

373ca5a

fix test

49e8ade

kevlubkcm changed the title ~~[WIP] Add proposer and proposal info to CompleteProposal event~~ [R4R] Add proposer to NewRound event and proposal info to CompleteProposal event Nov 12, 2018

kevlubkcm added 2 commits November 12, 2018 15:55

remove unneeded RoundState

53d3e32

put height round step into a struct

3a6e219

ebuchman reviewed Nov 12, 2018

View reviewed changes

types/events.go Outdated Show resolved Hide resolved

kevlubkcm added 2 commits November 12, 2018 16:48

pull out ValidatorInfo struct. add ensureProposal assert

8c995ab

remove height-round-state sub-struct refactor

2c14efe

kevlubkcm commented Nov 12, 2018

View reviewed changes

types/events.go Outdated Show resolved Hide resolved

ebuchman approved these changes Nov 15, 2018

View reviewed changes

ebuchman merged commit a676c71 into tendermint:develop Nov 15, 2018

ebuchman added 2 commits November 15, 2018 18:42

Merge branch 'develop' into HEAD

9ceb9da

minor fixes from review

efaae61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R4R] Add proposer to NewRound event and proposal info to CompleteProposal event #2767

[R4R] Add proposer to NewRound event and proposal info to CompleteProposal event #2767

kevlubkcm commented Nov 6, 2018 •

edited

codecov-io commented Nov 6, 2018 •

edited

ebuchman left a comment •

edited

kevlubkcm commented Nov 12, 2018

ebuchman commented Nov 12, 2018

kevlubkcm commented Nov 12, 2018

ebuchman commented Nov 12, 2018

kevlubkcm commented Nov 12, 2018

ebuchman left a comment

ebuchman Nov 15, 2018

kevlubkcm Nov 16, 2018

ebuchman Nov 16, 2018

ebuchman Nov 15, 2018

kevlubkcm Nov 16, 2018

ebuchman Nov 16, 2018

kevlubkcm commented Nov 16, 2018

[R4R] Add proposer to NewRound event and proposal info to CompleteProposal event #2767

[R4R] Add proposer to NewRound event and proposal info to CompleteProposal event #2767

Conversation

kevlubkcm commented Nov 6, 2018 • edited

codecov-io commented Nov 6, 2018 • edited

Codecov Report

ebuchman left a comment • edited

Choose a reason for hiding this comment

kevlubkcm commented Nov 12, 2018

ebuchman commented Nov 12, 2018

kevlubkcm commented Nov 12, 2018

ebuchman commented Nov 12, 2018

kevlubkcm commented Nov 12, 2018

ebuchman left a comment

Choose a reason for hiding this comment

ebuchman Nov 15, 2018

Choose a reason for hiding this comment

kevlubkcm Nov 16, 2018

Choose a reason for hiding this comment

ebuchman Nov 16, 2018

Choose a reason for hiding this comment

ebuchman Nov 15, 2018

Choose a reason for hiding this comment

kevlubkcm Nov 16, 2018

Choose a reason for hiding this comment

ebuchman Nov 16, 2018

Choose a reason for hiding this comment

kevlubkcm commented Nov 16, 2018

kevlubkcm commented Nov 6, 2018 •

edited

codecov-io commented Nov 6, 2018 •

edited

ebuchman left a comment •

edited