R4R: Emit Warning Events when Validator Misses Blocks

[AdityaSripal]

Emits warning events if a validator misses a certain number of blocks DowntimeWarning. Continues to emit warning events for any additional DowntimeWarning blocks that validator misses during SigningBlockWindow

• Targeted PR against correct branch (see CONTRIBUTING.md)

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

• Wrote tests

• Updated relevant documentation (docs/)

• Added a relevant changelog entry: clog add [section] [stanza] [message]

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #4629 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4629      +/-   ##
==========================================
+ Coverage   54.11%   54.14%   +0.03%     
==========================================
  Files         260      260              
  Lines       16371    16379       +8     
==========================================
+ Hits         8859     8869      +10     
+ Misses       6866     6864       -2     
  Partials      646      646

[fedekunze]

I really like the goal of this PR, although I'm not convinced a parameter is the right way to go here. Could it be set on the configuration instead ? Maybe some validators will want to have their custom block period for the warning.

Thoughts ? @alessio @sabau

[fedekunze]

Suggested change

	Added warning event that gets emitted if validator misses certain number of blocks, and re-emits periodically.
	#4629 Added warning event that gets emitted if validator misses certain number of blocks, and re-emits periodically.

[alexanderbez]

mhmmm, so this is slightly different than what we talked about out of band. We're adding a new parameter here which is state machine breaking.

I was under the impression we'd always emit an event whenever a validator misses a block (which is pretty low from my inspection) which makes the changes really simple and has no need for a new param:

ctx.EventManager().EmitEvent(
    sdk.NewEvent(
	types.EventTypeLiveness,
	sdk.NewAttribute(types.AttributeKeyAddress, consAddr.String()),
	sdk.NewAttribute(types.AttributeKeyPower, fmt.Sprintf("%d", power)),
	sdk.NewAttribute(types.AttributeKeyHeight, fmt.Sprintf("%d", height)),
    ),
)

[alessio]

The implementation per se looks fine - I question the rationale behind the decision to make this a parameter. In my view, this should be a configuration option

[alexanderbez]

It shouldn't be any option at all...just simply emitted.

[alessio]

Yes, agree to that. Most definitely not a parameter

…

On Thu, 27 Jun 2019, 14:57 Alexander Bezobchuk, ***@***.***> wrote: It shouldn't be any option at all...just simply emitted. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4629?email_source=notifications&email_token=AABX73FM72WWXTC66ZSBU7TP4TBMFA5CNFSM4H3WXIW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYXGMIQ#issuecomment-506357282>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABX73EUSGXTNHQP6NOPKYLP4TBMFANCNFSM4H3WXIWQ> .

[AdityaSripal]

@alexanderbez, yea I thought it might make sense to have the warning events be tunable. Even if Gaia currently doesn't have an issue with missed blocks; other chains or Gaia in the future might have too many missed blocks for an event for every missed_block for each validator to be feasible.

I agree that it shouldn't be an application-level parameter tho. @fedekunze's suggestion to make this a config option seems right to me. The only potential weirdness i see is that different validators would fire events at different times because they are unilaterally setting this parameter in config. I don't think this is a huge deal tho.

[alessio]

ACKed

[alexanderbez]

ACK