-
Notifications
You must be signed in to change notification settings - Fork 119
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
Key assignment #515
Merged
Merged
Key assignment #515
Changes from all commits
Commits
Show all changes
64 commits
Select commit
Hold shift + click to select a range
f83c731
add MsgAssignConsumerKey
mpoke 084ef62
add MsgAssignConsumerKey
mpoke ebd0398
fix package name
mpoke 020f285
add keys
mpoke 75bf2a1
add keeper methods for key assignment
mpoke ac30f74
handle MsgAssignConsumerKey
mpoke 6707ac9
map addresses in slash requests
mpoke 26ff35a
prune old consumer addresses
mpoke 6974766
move AssignConsumerKey logic to keeper
mpoke 35bb100
update consumer initial valset
mpoke 09fd054
add ApplyKeyAssignmentToValUpdates
mpoke 098a4e6
fix client creation
mpoke f8cd226
do not check init valset on consumer
mpoke 533c6cf
clean state on val removal
mpoke 3091bd1
fix TestAssignConsensusKeyForConsumerChain
mpoke b85e87c
delete on val removal
mpoke 595c3eb
remove reverse mapping on val removal
mpoke 1c2e696
remove pending key assignment in EndBlock
mpoke f5be337
add query endpoints
jtremback 40b3bc9
Refactor AssignConsumerKey for clarity (IMO)
jtremback 19cc625
finish key assignment genesis code- untested
jtremback 3270302
FIxed mocks compile issue - not sure if it works right though.
jtremback 87fff1b
add test for init and export genesis
jtremback d7f3be4
set after get in AssignConsumerKey
mpoke 4a4dce2
enable AssignConsumerKey to be called twice
mpoke a621b9b
remove key assignment on chain removal
mpoke cf1bc61
apply some review comments
mpoke 86c5979
fix bug: two validator with same consumer key
mpoke c050cc8
rename key: ConsumerValidatorsByVscIDBytePrefix -> ConsumerAddrsToPru…
mpoke 605d562
PendingKeyAssignment -> KeyAssignmentReplacements
mpoke 0c9f620
msg.ProviderAddr is a validator addr
mpoke f0ae261
fix: key assignment genesis tests (#517)
sainoe d64f7a6
add key assignment CRUD operations unit tests (#516)
MSalopek 9e84264
improve KeyAssignmentReplacement set and get
mpoke 5872f93
remove ApplyKeyAssignmentToInitialValset (redundant)
mpoke c396b3f
add invariant to docstring of AppendConsumerAddrsToPrune
mpoke e772dfb
fix address conversion
mpoke 7fbbc76
Merge branch 'main' into marius/key-assignment
mpoke 6d8cd10
adding e2e tests
mpoke aca9578
fix linter
mpoke 8641273
add queries; setup integration tests (#519)
MSalopek eb7ff69
Adds some very basic random testing and unit tests (#522)
danwt 5820c66
Enable key assignment testing for all e2e tests (#524)
mpoke 7616700
Merge branch 'main' into marius/key-assignment
jtremback a1f592a
Merge branch 'main' into marius/key-assignment
danwt 0568ce8
adding ADR
mpoke 561c567
Merge branch 'marius/key-assignment' of github.com:cosmos/interchain-…
mpoke 0d3123f
move handler.go outside client/
mpoke 1bec53f
replace [][]byte with AddressList
mpoke 23d4b54
remove IterateAllConsumerAddrsToPrune; not needed
mpoke 0c29fb4
apply review suggestions
mpoke bb4e1ea
fix merge conflicts
mpoke 637fc55
fix linter
mpoke ca05578
Danwt/key assignment slash test (#545)
danwt f1f5ed4
Merge branch 'main' into marius/key-assignment
danwt 0fe2305
Fixes #503 prevents two key assignment key overlap security issues (#…
danwt a519d12
Bump AssignConsumerKey comment
a833334
Merge branch 'main' into marius/key-assignment
danwt da08407
improve comments for iterators
mpoke ab43903
Masa/key assignment integration tests amend (#548)
MSalopek f87973d
remove node_modules
mpoke 26ddbf6
fix comment
mpoke 6e0ca7f
fix merge conflicts
mpoke 530cdcb
Merge branch 'main' into marius/key-assignment
mpoke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
.DS_STORE | ||
vue/node_modules | ||
*/node_modules | ||
vue/dist | ||
release/ | ||
docs/tla/states/ | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# Architecture Decision Records (ADR) | ||
|
||
This is a location to record all high-level architecture decisions in the Interchain Security project. | ||
|
||
You can read more about the ADR concept in this [blog post](https://product.reverb.com/documenting-architecture-decisions-the-reverb-way-a3563bb24bd0#.78xhdix6t). | ||
|
||
An ADR should provide: | ||
|
||
- Context on the relevant goals and the current state | ||
- Proposed changes to achieve the goals | ||
- Summary of pros and cons | ||
- References | ||
- Changelog | ||
|
||
Note the distinction between an ADR and a spec. The ADR provides the context, intuition, reasoning, and | ||
justification for a change in architecture, or for the architecture of something | ||
new. The spec is much more compressed and streamlined summary of everything as | ||
it is or should be. | ||
|
||
If recorded decisions turned out to be lacking, convene a discussion, record the new decisions here, and then modify the code to match. | ||
|
||
Note the context/background should be written in the present tense. | ||
|
||
To suggest an ADR, please make use of the [ADR template](./adr-template.md) provided. | ||
|
||
## Table of Contents | ||
|
||
| ADR \# | Description | Status | | ||
| ------ | ----------- | ------ | | ||
| [001](./adr-001-key-assignment.md) | Consumer chain key assignment | Accepted, Implemented | |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,267 @@ | ||
# ADR 001: Key Assignment | ||
|
||
## Changelog | ||
* 2022-12-01: Initial Draft | ||
|
||
## Status | ||
|
||
Accepted | ||
|
||
## Context | ||
|
||
KeyAssignment is the name of the feature that allows validator operators to use different consensus keys for each consumer chain validator node that they operate. | ||
|
||
## Decision | ||
|
||
It is possible to change the keys at any time by submitting a transaction (i.e., `MsgAssignConsumerKey`). | ||
|
||
### State required | ||
|
||
- `ValidatorConsumerPubKey` - Stores the validator assigned keys for every consumer chain. | ||
```golang | ||
ConsumerValidatorsBytePrefix | len(chainID) | chainID | providerConsAddress -> consumerKey | ||
``` | ||
- `ValidatorByConsumerAddr` - Stores the mapping from validator addresses on consumer chains to validator addresses on the provider chain. Needed for the consumer initiated slashing sub-protocol. | ||
```golang | ||
ValidatorsByConsumerAddrBytePrefix | len(chainID) | chainID | consumerConsAddress -> providerConsAddress | ||
``` | ||
- `KeyAssignmentReplacements` - Stores the key assignments that need to be replaced in the current block. Needed to apply the key assignments received in a block to the validator updates sent to the consumer chains. | ||
```golang | ||
KeyAssignmentReplacementsBytePrefix | len(chainID) | chainID | providerConsAddress -> abci.ValidatorUpdate{PubKey: oldConsumerKey, Power: currentPower}, | ||
``` | ||
- `ConsumerAddrsToPrune` - Stores the mapping from VSC ids to consumer validators addresses. Needed for pruning `ValidatorByConsumerAddr`. | ||
```golang | ||
ConsumerAddrsToPruneBytePrefix | len(chainID) | chainID | vscID -> []consumerConsAddresses | ||
``` | ||
|
||
### Protocol overview | ||
|
||
On receiving a `MsgAssignConsumerKey(chainID, providerAddr, consumerKey)` message: | ||
```golang | ||
// get validator from staking module | ||
validator, found := stakingKeeper.GetValidator(providerAddr) | ||
if !found { | ||
return ErrNoValidatorFound | ||
} | ||
providerConsAddr := validator.GetConsAddr() | ||
|
||
// make sure consumer key is not in use | ||
consumerAddr := utils.TMCryptoPublicKeyToConsAddr(consumerKey) | ||
if _, found := GetValidatorByConsumerAddr(ChainID, consumerAddr); found { | ||
return ErrInvalidConsumerConsensusPubKey | ||
} | ||
|
||
// check whether the consumer chain is already registered | ||
// i.e., a client to the consumer was already created | ||
if _, consumerRegistered := GetConsumerClientId(chainID); consumerRegistered { | ||
// get the previous key assigned for this validator on this consumer chain | ||
oldConsumerKey, found := GetValidatorConsumerPubKey(chainID, providerConsAddr) | ||
if found { | ||
// mark this old consumer key as prunable once the VSCMaturedPacket | ||
// for the current VSC ID is received | ||
oldConsumerAddr := utils.TMCryptoPublicKeyToConsAddr(oldConsumerKey) | ||
vscID := GetValidatorSetUpdateId() | ||
AppendConsumerAddrsToPrune(chainID, vscID, oldConsumerAddr) | ||
} else { | ||
// the validator had no key assigned on this consumer chain | ||
oldConsumerKey := validator.TmConsPublicKey() | ||
} | ||
|
||
// check whether the validator is valid, i.e., its power is positive | ||
if currentPower := stakingKeeper.GetLastValidatorPower(providerAddr); currentPower > 0 { | ||
// to enable multiple calls of AssignConsumerKey in the same block by the same validator | ||
// the key assignment replacement should not be overwritten | ||
if _, found := GetKeyAssignmentReplacement(chainID, providerConsAddr); !found { | ||
// store old key and power for modifying the valset update in EndBlock | ||
oldKeyAssignment := abci.ValidatorUpdate{PubKey: oldConsumerKey, Power: currentPower} | ||
SetKeyAssignmentReplacement(chainID, providerConsAddr, oldKeyAssignment) | ||
} | ||
} | ||
} else { | ||
// if the consumer chain is not registered, then remove the previous reverse mapping | ||
if oldConsumerKey, found := GetValidatorConsumerPubKey(chainID, providerConsAddr); found { | ||
oldConsumerAddr := utils.TMCryptoPublicKeyToConsAddr(oldConsumerKey) | ||
DeleteValidatorByConsumerAddr(chainID, oldConsumerAddr) | ||
} | ||
} | ||
|
||
|
||
// set the mapping from this validator's provider address to the new consumer key | ||
SetValidatorConsumerPubKey(chainID, providerConsAddr, consumerKey) | ||
|
||
// set the reverse mapping: from this validator's new consensus address | ||
// on the consumer to its consensus address on the provider | ||
SetValidatorByConsumerAddr(chainID, consumerAddr, providerConsAddr) | ||
``` | ||
|
||
When a new consumer chain is registered, i.e., a client to the consumer chain is created, the provider constructs the consumer CCV module part of the genesis state (see `MakeConsumerGenesis`). | ||
```golang | ||
func (k Keeper) MakeConsumerGenesis(chainID string) (gen consumertypes.GenesisState, nextValidatorsHash []byte, err error) { | ||
// ... | ||
// get initial valset from the staking module | ||
var updates []abci.ValidatorUpdate{} | ||
stakingKeeper.IterateLastValidatorPowers(func(providerAddr sdk.ValAddress, power int64) (stop bool) { | ||
validator := stakingKeeper.GetValidator(providerAddr) | ||
providerKey := validator.TmConsPublicKey() | ||
updates = append(updates, abci.ValidatorUpdate{PubKey: providerKey, Power: power}) | ||
return false | ||
}) | ||
|
||
// applies the key assignment to the initial validator | ||
for i, update := range updates { | ||
providerAddr := utils.TMCryptoPublicKeyToConsAddr(update.PubKey) | ||
if consumerKey, found := GetValidatorConsumerPubKey(chainID, providerAddr); found { | ||
updates[i].PubKey = consumerKey | ||
} | ||
} | ||
gen.InitialValSet = updates | ||
|
||
// get a hash of the consumer validator set from the update | ||
updatesAsValSet := tendermint.PB2TM.ValidatorUpdates(updates) | ||
hash := tendermint.NewValidatorSet(updatesAsValSet).Hash() | ||
|
||
return gen, hash, nil | ||
} | ||
``` | ||
|
||
On `EndBlock` while queueing `VSCPacket`s to send to registered consumer chains: | ||
```golang | ||
func QueueVSCPackets() { | ||
valUpdateID := GetValidatorSetUpdateId() | ||
// get the validator updates from the staking module | ||
valUpdates := stakingKeeper.GetValidatorUpdates() | ||
|
||
IterateConsumerChains(func(chainID, clientID string) (stop bool) { | ||
// apply the key assignment to the validator updates | ||
valUpdates := ApplyKeyAssignmentToValUpdates(chainID, valUpdates) | ||
// .. | ||
}) | ||
// ... | ||
} | ||
|
||
func ApplyKeyAssignmentToValUpdates( | ||
chainID string, | ||
valUpdates []abci.ValidatorUpdate, | ||
) (newUpdates []abci.ValidatorUpdate) { | ||
for _, valUpdate := range valUpdates { | ||
providerAddr := utils.TMCryptoPublicKeyToConsAddr(valUpdate.PubKey) | ||
|
||
// if a key assignment replacement is found, then | ||
// remove the valupdate with the old consumer key | ||
// and create two new valupdates | ||
prevConsumerKey, _, found := GetKeyAssignmentReplacement(chainID, providerAddr) | ||
if found { | ||
// set the old consumer key's power to 0 | ||
newUpdates = append(newUpdates, abci.ValidatorUpdate{ | ||
PubKey: prevConsumerKey, | ||
Power: 0, | ||
}) | ||
// set the new consumer key's power to the power in the update | ||
newConsumerKey := GetValidatorConsumerPubKey(chainID, providerAddr) | ||
newUpdates = append(newUpdates, abci.ValidatorUpdate{ | ||
PubKey: newConsumerKey, | ||
Power: valUpdate.Power, | ||
}) | ||
// delete key assignment replacement | ||
DeleteKeyAssignmentReplacement(chainID, providerAddr) | ||
} else { | ||
// there is no key assignment replacement; | ||
// check if the validator's key is assigned | ||
consumerKey, found := k.GetValidatorConsumerPubKey(ctx, chainID, providerAddr) | ||
if found { | ||
// replace the update containing the provider key | ||
// with an update containing the consumer key | ||
newUpdates = append(newUpdates, abci.ValidatorUpdate{ | ||
PubKey: consumerKey, | ||
Power: valUpdate.Power, | ||
}) | ||
} else { | ||
// keep the same update | ||
newUpdates = append(newUpdates, valUpdate) | ||
} | ||
} | ||
} | ||
|
||
// iterate over the remaining key assignment replacements | ||
IterateKeyAssignmentReplacements(chainID, func( | ||
pAddr sdk.ConsAddress, | ||
prevCKey tmprotocrypto.PublicKey, | ||
power int64, | ||
) (stop bool) { | ||
// set the old consumer key's power to 0 | ||
newUpdates = append(newUpdates, abci.ValidatorUpdate{ | ||
PubKey: prevCKey, | ||
Power: 0, | ||
}) | ||
// set the new consumer key's power to the power in key assignment replacement | ||
newConsumerKey := GetValidatorConsumerPubKey(chainID, pAddr) | ||
newUpdates = append(newUpdates, abci.ValidatorUpdate{ | ||
PubKey: newConsumerKey, | ||
Power: power, | ||
}) | ||
return false | ||
}) | ||
|
||
// remove all the key assignment replacements | ||
|
||
return newUpdates | ||
} | ||
``` | ||
|
||
On receiving a `SlashPacket` from a consumer chain with id `chainID` for a infraction of a validator `data.Validator`: | ||
```golang | ||
func HandleSlashPacket(chainID string, data ccv.SlashPacketData) (success bool, err error) { | ||
// ... | ||
// the slash packet validator address may be known only on the consumer chain; | ||
// in this case, it must be mapped back to the consensus address on the provider chain | ||
consumerAddr := sdk.ConsAddress(data.Validator.Address) | ||
providerAddr, found := GetValidatorByConsumerAddr(chainID, consumerAddr) | ||
if !found { | ||
// the validator has the same key on the consumer as on the provider | ||
providerAddr = consumer | ||
} | ||
// ... | ||
} | ||
``` | ||
|
||
On receiving a `VSCMatured`: | ||
```golang | ||
func OnRecvVSCMaturedPacket(packet channeltypes.Packet, data ccv.VSCMaturedPacketData) exported.Acknowledgement { | ||
// ... | ||
// prune previous consumer validator address that are no longer needed | ||
consumerAddrs := GetConsumerAddrsToPrune(chainID, data.ValsetUpdateId) | ||
for _, addr := range consumerAddrs { | ||
DeleteValidatorByConsumerAddr(chainID, addr) | ||
} | ||
DeleteConsumerAddrsToPrune(chainID, data.ValsetUpdateId) | ||
// ... | ||
} | ||
``` | ||
|
||
On stopping a consumer chain: | ||
```golang | ||
func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, closeChan bool) (err error) { | ||
// ... | ||
// deletes all the state needed for key assignments on this consumer chain | ||
// ... | ||
} | ||
``` | ||
|
||
## Consequences | ||
|
||
### Positive | ||
|
||
- Validators can use different consensus keys on the consumer chains. | ||
|
||
### Negative | ||
|
||
- None | ||
|
||
### Neutral | ||
|
||
- The consensus state necessary to create a client to the consumer chain must use the hash returned by the `MakeConsumerGenesis` method as the `nextValsHash`. | ||
- The consumer chain can no longer check the initial validator set against the consensus state on `InitGenesis`. | ||
|
||
## References | ||
|
||
* [Key assignment issue](https://github.com/cosmos/interchain-security/issues/26) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
# ADR {ADR-NUMBER}: {TITLE} | ||
|
||
## Changelog | ||
* {date}: {changelog} | ||
|
||
## Status | ||
|
||
> A decision may be "proposed" if it hasn't been agreed upon yet, or "accepted" once it is agreed upon. If a later ADR changes or reverses a decision, it may be marked as "deprecated" or "superseded" with a reference to its replacement. | ||
|
||
{Deprecated|Proposed|Accepted} | ||
|
||
## Context | ||
|
||
> This section contains all the context one needs to understand the current state, and why there is a problem. It should be as succinct as possible and introduce the high level idea behind the solution. | ||
|
||
## Decision | ||
|
||
> This section explains all of the details of the proposed solution, including implementation details. | ||
It should also describe affects / corollary items that may need to be changed as a part of this. | ||
If the proposed change will be large, please also indicate a way to do the change to maximize ease of review. | ||
(e.g. the optimal split of things to do between separate PR's) | ||
|
||
## Consequences | ||
|
||
> This section describes the consequences, after applying the decision. All consequences should be summarized here, not just the "positive" ones. | ||
|
||
### Positive | ||
|
||
### Negative | ||
|
||
### Neutral | ||
|
||
## References | ||
|
||
> Are there any relevant PR comments, issues that led up to this, or articles referrenced for why we made the given design choice? If so link them here! | ||
|
||
* {reference link} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this ADR, or ADRs in our repo in general? This is from a template, which is also new. Where did it come from? Different organisations have different ideas of ADRs.
The general breakdown into subheading ('On receiving a
VSCMatured
:', 'On stopping a consumer chain:' ect) is good, and the consequences and other written sections are good.The part that is not clear to me is if the parts which are almost all code snippets are useful. I have questions. Should this be maintained? Who is the intended audience? Why would they prefer to read this instead of code? (Especially if we directed the energy away from this towards making the code more readable)?
I am still left with questions, which I believe an ADR should at least try to answer. What were the other alternative approaches to this solution that were considered and why were they not taken? What business or product goal does this solve? What security concerns arise from this feature? What testing concerns arise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed a place where to add the main logic behind the key assignment design. Having a random file for each feature doesn't seem like a good practice.
ADRs should describe features that are added to the repo. Unlike the spec, the ADRs should address the dependencies to Cosmos SDK and Tendermint.
https://github.com/cosmos/ibc-go/tree/main/docs/architecture
Yes.
Engineers.
The implementation contains a lot of boiler plate code that is hard to parse for somebody not used to the SDK. We need a more concise description of a feature.
Had we had ADRs before, every approach taken but not finalized would have been its own ADR. Or the same ADR with a commit history. Not sure that it makes sense to consumer resources to document design decisions not taken.
Feel free to expand the ADR with all this info if you feel it's needed. Even better, open an issue beforehand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest opening a PR into the main/ that adds this info about ADR's (and the template) to the top level README or somewhere else. Then, new ADR's can be assessed to see if they do what they are supposed to.