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

Validators don't have owners => Update language used #1884

Closed
rigelrozanski opened this issue Jul 30, 2018 · 12 comments
Closed

Validators don't have owners => Update language used #1884

rigelrozanski opened this issue Jul 30, 2018 · 12 comments
Labels
C:CLI C:x/staking T:Docs Changes and features related to documentation.

Comments

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Jul 30, 2018

"owner" and the owner_addr are kind of misleading the because it implies that the funds of that validator are actually owned by that address whereas in reality they are more entrusted but truly held in protocol or by the delegators and the validator

stems from community conversation here: #1869 (comment)

@rigelrozanski rigelrozanski added T:Docs Changes and features related to documentation. C:CLI labels Jul 30, 2018
@alexanderbez
Copy link
Contributor

@rigelrozanski just read through that convo. Seems like we initially just want to update CLI nomenclature (flags, etc..), correct?

@cwgoes
Copy link
Contributor

cwgoes commented Jul 30, 2018

I can understand the confusion but shouldn't we focus on explaining that validators don't hold funds at all - an "entrusted" address wouldn't be "entrusted" with the funds either (that would imply custodial ownership) - they're just at stake if a protocol fault is committed.

@amrali
Copy link

amrali commented Jul 30, 2018

I think "owner" should be entirely removed and replaced with "validator". And "validator" to be replaced with "validation_key", I fleshed it out more at #1869 (comment)

@rigelrozanski
Copy link
Contributor Author

rigelrozanski commented Jul 30, 2018

I think "owner" should be entirely removed and replaced with "validator" and "validator" to be replaced with "validation_key" I fleshed it out more at

This comment doesn't make sense to me, here is the validator struct:

type Validator struct {
Owner sdk.AccAddress `json:"owner"` // sender of BondTx - UnbondTx returns here
PubKey crypto.PubKey `json:"pub_key"` // pubkey of validator
Revoked bool `json:"revoked"` // has the validator been revoked from bonded status?
Status sdk.BondStatus `json:"status"` // validator status (bonded/unbonding/unbonded)
Tokens sdk.Rat `json:"tokens"` // delegated tokens (incl. self-delegation)
DelegatorShares sdk.Rat `json:"delegator_shares"` // total shares issued to a validator's delegators
Description Description `json:"description"` // description terms for the validator
BondHeight int64 `json:"bond_height"` // earliest height as a bonded validator
BondIntraTxCounter int16 `json:"bond_intra_tx_counter"` // block-local tx index of validator change
ProposerRewardPool sdk.Coins `json:"proposer_reward_pool"` // XXX reward pool collected from being the proposer
Commission sdk.Rat `json:"commission"` // XXX the commission rate of fees charged to any delegators
CommissionMax sdk.Rat `json:"commission_max"` // XXX maximum commission rate which this validator can ever charge
CommissionChangeRate sdk.Rat `json:"commission_change_rate"` // XXX maximum daily increase of the validator commission
CommissionChangeToday sdk.Rat `json:"commission_change_today"` // XXX commission rate change today, reset each day (UTC time)
// fee related
LastBondedTokens sdk.Rat `json:"prev_bonded_tokens"` // Previous bonded tokens held
}

What do you propose we change Owner field name too? validation_key? By well thought out specifications, this index field needs to stay as an AccAddress which represents the account which the validator was created from. That's a design constraint here

@cwgoes

I can understand the confusion but shouldn't we focus on explaining that validators don't hold funds at all - an "entrusted" address wouldn't be "entrusted" with the funds either (that would imply custodial ownership) - they're just at stake if a protocol fault is committed.

totally agree, entrust doesn't make sense, any other thoughts for the best term?

@rigelrozanski rigelrozanski changed the title validator owner-address -> entrusted-address Validators don't have owners => Update language used Jul 30, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Jul 30, 2018

totally agree, entrust doesn't make sense, any other thoughts for the best term?

What about "manager"? A bit simpler, although I'm not sure whether or not that implies fund custodianship. Probably regardless of term we'll just have to explain the exact "funds-at-stake" model in several places (including the CLI) very clearly.

@rigelrozanski
Copy link
Contributor Author

I like manager - or how about operator the operator of the validator!

@amrali
Copy link

amrali commented Jul 30, 2018

What do you propose we change Owner field name too? validation_key? By well thought out specifications, this index field needs to stay as an AccAddress which represents the account which the validator was created from. That's a design constraint here

What is a validator? Right now it is an account, a keypair (either Secp256k1 or Ed25519), associated with it another keypair (Ed25519) used exclusively for signing consensus messages. So why call it an owner or a manager or a trustee when the validator's identity is literally represented by the account it was created from. The validator is literally represented by that account.

Calling the validator account a manager/operator/trustee doesn't make it clearer, it introduces another entity type to the vocabulary without a distinct role to play on its own.

Here's what I'm saying in a different form:

  • Replace "owner" with "validator".
  • Replace "validator_addr/etc" with "validation_addr/key"

The other side to this is allowing multiple "validation_addr/key" per validator (the account), if possible (I do not know if it is) it would solve the validation node availability problem by allowing redundancy.

@cwgoes
Copy link
Contributor

cwgoes commented Jul 30, 2018

I like manager - or how about operator the operator of the validator!

"Operator" is definitely better than "manager".

Calling the validator account a manager/operator/trustee doesn't make it clearer, it introduces another entity type to the vocabulary without a distinct role to play on its own.

The validator account is in all other respects a standard account - it can have a balance, send/receive transactions, vote in governance, etc. - it just happens to be { operating } (insert verb of choice) a validator. I think we need a distinct word for that relationship between an account and a validator struct (including Tendermint signing key) - but definitely open to suggestions!

The other side to this is allowing multiple "validation_addr/key" per validator (the account), if possible (I do not know if it is) it would solve the validation node availability problem by allowing redundancy.

I think you're talking about Tendermint signing keys - the primary reason we can't implement that (in the simple way) is that the light clients would need to know about the other signing keys too.

@amrali
Copy link

amrali commented Jul 30, 2018

The validator account is in all other respects a standard account - it can have a balance, send/receive transactions, vote in governance, etc. - it just happens to be { operating } (insert verb of choice) a validator. I think we need a distinct word for that relationship between an account and a validator struct (including Tendermint signing key) - but definitely open to suggestions!

I think there's no relationship, an account through a transaction or by including it in some state structure (e.g., genesis.json) becomes a validator, the account assumes the role of a validator. Think of it as a flag you flip on an account, it's not a distinct addressable entity, in fact you cannot address a validator through any other means but its account.

So a validator addressable via its account, an account that happens to be a validator, can be associated with at least a single Tendermint signing key (through its address or public key).

I think the ideal relationship is Account (of type validator) 1-to-M signing keys.

@rigelrozanski
Copy link
Contributor Author

rigelrozanski commented Jul 31, 2018

the account assumes the role of a validator

the account doesn't assume that role, the owner of the account is presumably the "validator" by that definition.

it's not a distinct addressable entity,

it is. it's literally an address.

in fact you cannot address a validator through any other means but it's account.

You can and we do throughout the codebase. The operator address is however the only static addressable index, The Tendermint Pubkey however holds an index to the operator-address and thus you can also address a the validator through the Tendermint-Pubkey without the operator-address - This functionality has not yet been exposed in the CLI yet.

at least a single Tendermint signing key

For the current design of staking, the constraint is that there must only be one Tendermint signing key associated with a operator-key (aka you cannot re-use an operator key across multiple validators)


Calling the operator-account validator doesn't make any sense, It's overloading the term validator, which is already the whole object. That account is by definition, DOES have a relationship to the validator object, it is the account which is permitted to modify the operation of the validator, and presumable the owner of that account is also the owner of the Tendermint signing key which is what is actually doing the validation (aka the Tendermint key is the actual validation_key), thus renaming the validator_key to validation_key ALSO doesn't make any sense because it's not the validation key.

@joe-bowman
Copy link
Contributor

joe-bowman commented Aug 1, 2018

I see an account as an individual/organisation.

The owner of that account may choose to create a validator, but they are still free to hold funds in that account that are totally unrelated to the running of the validator, and indeed may be delegated elsewhere - that is entirely their choice! There is not an intrinsic link between an account (and any funds it may hold) and a validation signing keypair.

The main issue here for me is consistency.

In different parts of the system, we are addressing a validator by it's cosmosValPubKey, rarely by it's cosmosValAddr, often by it's cosmosAccAddr, and it is confusing, unituitive and unclear. The number of comments/questions in riot about 'what key shall I use where' is ridiculous. More documentation isn't the answer here, we have to give consideration to the user experience.

If we are refering to a validator (delegation, unbonding, checking signing-info, etc.) then we should be refering to the cosmosVal bech32 value (whether this is the public key or address is largely irrelevant, but let's choose one) and if we are refering to an individual's account (where am I sending funds from/to) then use cosmosAccAddr and let the application determine what it needs to use internally - the user shouldn't care what the application internals actually need to use underneath.

  • Sending funds to @amrali, I should be sending to his cosmosAccAddr
  • Delegating to the validator that @cwgoes operates, should reference his cosmosValPubKey

and so on....

This way we can avoid having to define this relationship that may or may not exist ;)

It also allows at a later date, if it determined to be useful, to abstract away validation keypairs / accounts such that one account can control multiple validators, without having to make breaking changes to the API (because all validator interactions are solely concerned with a validator address and not an account).

@rigelrozanski
Copy link
Contributor Author

In different parts of the system, we are addressing a validator by it's cosmosValPubKey, rarely by it's cosmosValAddr, often by it's cosmosAccAddr, and it is confusing, unituitive and unclear. The number of comments/questions in riot about 'what key shall I use where' is ridiculous. More documentation isn't the answer here, we have to give consideration to the user experience.

agreed - we should probably switch as much literally everything user facing on gaia to use the operator address all references which require cosmosValPubKey should be removed from gaia (besides advanced tooling for tendermint for the advanced user). Realistically - the signing address should only really be used in the backend - I think making this total switch could alleviate a lot of the headache and should make everything more clear.

Generally speaking I think we want to reference everything by address not PubKey within the CLI.

As per the distinction between using the bech cosmosAccAddr vs cosmosValAddr I tend to think that it might make things slightly more clear - I think this can probably be rectified in the back end.

Within the CLI I can see calling it the flag where you feed in the cosmosValAddr being renamed to --validator as being intuitive and making sense - HOWEVER within the code this should absolutely not be called validator the overloading is much too much - this also carries through to and json usage (aka genesis) - for this I persist that operator should be used.

I'm going to close this issue for now, this has been a good/endured conversation, opening some new issues to reflect this conversation:


It also allows at a later date, if it determined to be useful, to abstract away validation keypairs / accounts such that one account can control multiple validators, without having to make breaking changes to the API (because all validator interactions are solely concerned with a validator address and not an account).

Yeah this is definitely on the table for post launch - there has been discussion of good abstractions here - for now - this is an extra feature so we're not focusing on it - much cleaner to just stick to 1-to-1 for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:x/staking T:Docs Changes and features related to documentation.
Projects
None yet
Development

No branches or pull requests

5 participants