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

send ValidatorSetUpdates event when validator set changes #2161

Merged
merged 1 commit into from
Aug 14, 2018

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Aug 6, 2018

Refs #1916

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

REBASE AGAINST DEVELOP ONCE #2159 MERGED

"data": {
"type": "tendermint/event/ValidatorSetUpdates",
"value": {
"validator_updates": [
Copy link
Contributor

Choose a reason for hiding this comment

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

@melekes does it contains only the new validators and validators that changed their power or the updated list with the new validator set ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither. It's the list of updates. So if the current validator set is A, B, C and application includes D, then ValidatorSetUpdates will contain only D.

Is that not enough? Given you track validators from the beginning (or get them using RPC /validators), you should be able to construct the updated set.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if the current validator set is A, B, C and and application includes D and drops A ? will ValidatorSetUpdates only include D as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It will be A (pubkeyA, power: 0), D (pubkeyD, power: X)

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

:octocat: 🎆 💯 ✔️ LGTM

@fedekunze fedekunze mentioned this pull request Aug 8, 2018
6 tasks
@ebuchman
Copy link
Contributor

+ make metalinter
--> Running linter
abci/types/types.pb.go:6768:1:warning: randUnrecognizedTypes is unused (deadcode)
abci/types/types.pb.go:7637:1:warning: sozTypes is unused (deadcode)
Makefile:221: recipe for target 'metalinter' failed
make: *** [metalinter] Error 1
Exited with code 2

Copy link
Contributor

@ebuchman ebuchman left a comment

Choose a reason for hiding this comment

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

Awesome thanks Anton.

Only thing is we should consider how we feel about passing ABCI pubkey over RPC here inconsistent with other instances of pubkey over RPC.

Also, should we send this to master so we can get a hot fix release with this on testnets instead of waiting for the next breaking tendermint release?

EventTx = "Tx"
EventUnlock = "Unlock"
EventValidatorSetUpdates = "ValidatorSetUpdates"
EventVote = "Vote"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should note in the comments that these are alphabetical for the poor soul that adds an event to this list without realizing :P

types/events.go Outdated
@@ -92,6 +86,10 @@ type EventDataVote struct {

type EventDataString string

type EventDataValidatorSetUpdates struct {
ValidatorUpdates []abci.ValidatorUpdate `json:"validator_updates"`
Copy link
Contributor

Choose a reason for hiding this comment

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

is it weird that we're going to be passing an ABCI validator over the RPC here ? ie. it will be different than the usual representation of pubkeys using AminoJSON

@melekes melekes force-pushed the 1916-ws-event-for-validator-set-changes branch from 14f66f2 to 2d7d367 Compare August 13, 2018 07:41
@melekes melekes changed the base branch from bucky/abci-validators to develop August 13, 2018 07:41
@codecov-io
Copy link

codecov-io commented Aug 13, 2018

Codecov Report

Merging #2161 into develop will increase coverage by 0.08%.
The diff coverage is 68.51%.

@@             Coverage Diff             @@
##           develop    #2161      +/-   ##
===========================================
+ Coverage    62.28%   62.37%   +0.08%     
===========================================
  Files          212      212              
  Lines        17565    17562       -3     
===========================================
+ Hits         10941    10954      +13     
+ Misses        5718     5701      -17     
- Partials       906      907       +1
Impacted Files Coverage Δ
types/events.go 28.57% <0%> (+8.57%) ⬆️
types/nop_event_bus.go 0% <0%> (ø) ⬆️
state/execution.go 75.38% <100%> (+2.6%) ⬆️
types/event_bus.go 83.09% <97.05%> (+0.48%) ⬆️
consensus/state.go 76.58% <0%> (-0.61%) ⬇️
consensus/reactor.go 73.02% <0%> (-0.27%) ⬇️
libs/db/remotedb/grpcdb/client.go 0% <0%> (ø) ⬆️
proxy/app_conn.go 0% <0%> (ø) ⬆️
p2p/peer.go 59.09% <0%> (+1.13%) ⬆️
... and 1 more

@melekes melekes force-pushed the 1916-ws-event-for-validator-set-changes branch from 2d7d367 to 420d8cb Compare August 13, 2018 07:51
@melekes melekes merged commit 80e49ab into develop Aug 14, 2018
@melekes melekes deleted the 1916-ws-event-for-validator-set-changes branch August 14, 2018 15:16
firelizzard18 pushed a commit to AccumulateNetwork/tendermint that referenced this pull request Feb 1, 2024
…ndermint#2161)

Bumps
[bufbuild/buf-setup-action](https://github.com/bufbuild/buf-setup-action)
from 1.28.1 to 1.29.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/bufbuild/buf-setup-action/releases">bufbuild/buf-setup-action's
releases</a>.</em></p>
<blockquote>
<h2>v1.29.0</h2>
<p>Release v1.29.0</p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/bufbuild/buf-setup-action/commit/88db93f5d74ffa329bb43e42aa95cd822697d214"><code>88db93f</code></a>
Release v1.29.0 (<a
href="https://github.com/bufbuild/buf-setup-action/issues/181">#181</a>)</li>
<li>See full diff in <a
href="https://github.com/bufbuild/buf-setup-action/compare/v1.28.1...v1.29.0">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=bufbuild/buf-setup-action&package-manager=github_actions&previous-version=1.28.1&new-version=1.29.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants