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

Validator metadata #2045

Merged
merged 9 commits into from
Nov 11, 2023
Merged

Validator metadata #2045

merged 9 commits into from
Nov 11, 2023

Conversation

brentstone
Copy link
Collaborator

@brentstone brentstone commented Oct 24, 2023

Describe your changes

Closes #1609.

This PR implements the storage of validator metadata, such as an email, description, website, and discord_handle. An email is the only required piece of this data. A TX is implemented to change this metadata that also allows one to change a validator's commission rate at the same time.

Indicate on which release or other PRs this topic is based on

v0.25.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@brentstone brentstone added the PoS label Oct 24, 2023
@brentstone brentstone force-pushed the brent/validator-metadata branch from 05f3410 to daa1a2f Compare October 24, 2023 21:11
@s0uk4
Copy link

s0uk4 commented Oct 24, 2023

Adding an email should be required, with a tx implemented to change this field if need be (it should not be immutable)

Ideally, this should allow us to select for everyone who is either active and without VP and those who are active and have VP if there is ever a need to coordinate.

sdk/src/tx.rs Outdated Show resolved Hide resolved
tzemanovic
tzemanovic previously approved these changes Oct 26, 2023
@@ -200,6 +201,16 @@ pub mod genesis_config {
/// Tendermint node key is used to derive Tendermint node ID for node
/// authentication
pub tendermint_node_key: Option<HexString>,
/// Validator email (required)
pub email: String,
Copy link
Member

Choose a reason for hiding this comment

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

@s0uk4 @Fraccaman because this is required, this will be a breaking change for testnet setup - you'll need to ask all validators to add this

Copy link
Member

Choose a reason for hiding this comment

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

It can just be an empty string. We can't validate the email anyway during the submission.

Copy link
Member

Choose a reason for hiding this comment

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

So based on that I think we can also make this an option too. Ie, someone can make their email potatoe@[email protected], which is equivalent to None really.

Copy link

Choose a reason for hiding this comment

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

even if this is true, that's the validator's problem, not ours. they need to put an email in for the case where the network needs them to respond to potential security issues. asking for an email field helps us, but it's also a favor to the valds themselves.

if they put in an empty string rather than the proper email and there is a breaking change or risk requiring validators to respond, and they don't see discord /or/ the email, they will go down/put the network at risk and be penalized, which means they will lose money. there is incentive for them not to defect here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's unclear to me if @s0uk4 is convincing others here, so can we get some clear consensus on how the email should be done? Atm, it is required, and providing an empty string for this arg will return an error, since empty strings are used to indicate that one wants to remove metadata from storage (i.e. if I want to delete my validator website, I pass --website "" at the CLI).

@tzemanovic
Copy link
Member

I'll squash the fixup commits to get this rdy

@tzemanovic tzemanovic force-pushed the brent/validator-metadata branch from 7b9e92f to 8f941d2 Compare October 27, 2023 07:43
tzemanovic
tzemanovic previously approved these changes Oct 27, 2023
@Fraccaman Fraccaman mentioned this pull request Oct 27, 2023
@tzemanovic tzemanovic force-pushed the brent/validator-metadata branch from 8f941d2 to 8476698 Compare October 27, 2023 15:18
@brentstone brentstone force-pushed the brent/validator-metadata branch 5 times, most recently from 2e38800 to ff0a027 Compare November 6, 2023 09:46
@brentstone brentstone force-pushed the brent/validator-metadata branch 3 times, most recently from c881f1a to 0bec3ef Compare November 7, 2023 10:48
@brentstone brentstone requested a review from tzemanovic November 7, 2023 13:24
@brentstone brentstone mentioned this pull request Nov 7, 2023
@grarco grarco mentioned this pull request Nov 7, 2023
@brentstone brentstone force-pushed the brent/validator-metadata branch from 0bec3ef to 618c927 Compare November 7, 2023 15:49
@grarco grarco mentioned this pull request Nov 7, 2023
@brentstone brentstone force-pushed the brent/validator-metadata branch from 618c927 to 3268996 Compare November 7, 2023 20:44
brentstone added a commit that referenced this pull request Nov 8, 2023
* brent/validator-metadata:
  new comments and logging for keys
  regenerate masp proofs
  remove hard-coded keys, get from pre-genesis wallet
  get validator consensus key from wallet in tests rather than hard-code
  fix e2e tests
  add email to transactions.toml and update sigs
  update with new genesis + remove alias
  changelog: add #1945
  validator metadata with a tx to change it on-chain
@adrianbrink adrianbrink merged commit c0b4057 into main Nov 11, 2023
13 of 14 checks passed
@adrianbrink adrianbrink deleted the brent/validator-metadata branch November 11, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PoS validator metadata
4 participants