From 6b23259bcec2cf3487ce00ad3267d8cff08dd8ad Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Nov 2022 18:38:41 +0000 Subject: [PATCH 1/7] (pass) Use specialised data structure --- x/ccv/provider/keeper/key_assignment.go | 56 ++++++++++++++++++------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/x/ccv/provider/keeper/key_assignment.go b/x/ccv/provider/keeper/key_assignment.go index 5d198eb350..01f7da6b70 100644 --- a/x/ccv/provider/keeper/key_assignment.go +++ b/x/ccv/provider/keeper/key_assignment.go @@ -15,7 +15,6 @@ import ( tmprotocrypto "github.com/tendermint/tendermint/proto/tendermint/crypto" ) - type VSCID = uint64 type ProviderPublicKey = tmprotocrypto.PublicKey type ConsumerPublicKey = tmprotocrypto.PublicKey @@ -135,6 +134,12 @@ func (ka *KeyAssignment) PruneUnusedKeys(latestVscid VSCID) { } } +// getProviderKeysForUpdate returns the all the provider keys of validators for which +// an update must be sent. An update must be sent to the consumer if, either: +// 1. The consumer has the validator in its active set AND the assigned key for the +// validator has changed since the last update. +// 2. The voting power of the validator has changed, and the validator is in the active +// set. func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates map[ProviderPublicKey]int64) ([]ProviderPublicKey, map[string]bool) { // Return a list of provider keys that need to be updated @@ -184,36 +189,61 @@ func (ka KeyAssignment) getProviderKeysLastPositiveUpdate(mustCreateUpdate map[s return lastUpdate } +type KeyToPowerMap struct { + inner map[tmprotocrypto.PublicKey]int64 + canonical map[string]tmprotocrypto.PublicKey +} + +func MakeKeyToPowerMap() KeyToPowerMap { + return KeyToPowerMap{ + inner: map[tmprotocrypto.PublicKey]int64{}, + canonical: map[string]tmprotocrypto.PublicKey{}, + } +} + +func (m *KeyToPowerMap) Set(pk tmprotocrypto.PublicKey, power int64) { + s := DeterministicStringify(pk) + if k, found := m.canonical[s]; found { + pk = k + } else { + m.canonical[s] = pk + } + m.inner[pk] = power +} + // do inner work as part of ComputeUpdates -func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[ProviderPublicKey]int64) (consumerUpdates map[ConsumerPublicKey]int64) { +func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[ProviderPublicKey]int64) map[ConsumerPublicKey]int64 { // Init the return value - consumerUpdates = map[ConsumerPublicKey]int64{} + consumerUpdates := MakeKeyToPowerMap() providerKeysForUpdate, mustUpdate := ka.getProviderKeysForUpdate(stakingUpdates) providerKeysLastPositivePowerUpdateMemo := ka.getProviderKeysLastPositiveUpdate(mustUpdate) - canonicalConsumerKey := map[string]ConsumerPublicKey{} - /* Create a deletion (zero power) update for any consumer key known to the consumer - that is no longer in use, or for which the power has changed. + that is no longer in use, or for which the power has changed to zero or a positive + number. + + The purpose of *this* step is to create a clean slate. The *next* step will + amend the updates for any validators with a positive power. */ for i := range providerKeysForUpdate { // For each provider key for which there was already a positive update // create a deletion update for the associated consumer key. pk := providerKeysForUpdate[i] // Avoid taking address to loop variable if lum, found := providerKeysLastPositivePowerUpdateMemo[DeterministicStringify(pk)]; found { - s := DeterministicStringify(*lum.ConsumerKey) - canonicalConsumerKey[s] = *lum.ConsumerKey - consumerUpdates[*lum.ConsumerKey] = 0 cca := TMCryptoPublicKeyToConsAddr(*lum.ConsumerKey) ka.Store.SetConsumerConsAddrToLastUpdateMemo(cca, providertypes.LastUpdateMemo{ConsumerKey: lum.ConsumerKey, ProviderKey: &pk, Vscid: vscid, Power: 0}) + consumerUpdates.Set(*lum.ConsumerKey, 0) } } /* Create a positive power update for any consumer key which is in use. + + The purpose of this step is to ensure that the consumer is sent the latest + positive power update for any validator for which the power is positive. */ for i := range providerKeysForUpdate { pk := providerKeysForUpdate[i] // Avoid taking address to loop variable @@ -243,15 +273,11 @@ func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[Prov } cca := TMCryptoPublicKeyToConsAddr(ck) ka.Store.SetConsumerConsAddrToLastUpdateMemo(cca, providertypes.LastUpdateMemo{ConsumerKey: &ck, ProviderKey: &pk, Vscid: vscid, Power: power}) - if k, found := canonicalConsumerKey[DeterministicStringify(ck)]; found { - consumerUpdates[k] = power - } else { - consumerUpdates[ck] = power - } + consumerUpdates.Set(ck, power) } } - return consumerUpdates + return consumerUpdates.inner } func toMap(providerUpdates []abci.ValidatorUpdate) map[ProviderPublicKey]int64 { From 9ccb6c2485389a064be91ff19a10a638a557be07 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Nov 2022 19:03:25 +0000 Subject: [PATCH 2/7] Adds bespoke maps and sets for dealing with keys --- x/ccv/provider/keeper/key_assignment.go | 150 ++++++++++++++++-------- 1 file changed, 100 insertions(+), 50 deletions(-) diff --git a/x/ccv/provider/keeper/key_assignment.go b/x/ccv/provider/keeper/key_assignment.go index 01f7da6b70..2fb5bcd5ee 100644 --- a/x/ccv/provider/keeper/key_assignment.go +++ b/x/ccv/provider/keeper/key_assignment.go @@ -134,19 +134,97 @@ func (ka *KeyAssignment) PruneUnusedKeys(latestVscid VSCID) { } } +type KeyToPower struct { + inner map[tmprotocrypto.PublicKey]int64 + canonical map[string]tmprotocrypto.PublicKey +} + +func MakeKeyToPower() KeyToPower { + return KeyToPower{ + inner: map[tmprotocrypto.PublicKey]int64{}, + canonical: map[string]tmprotocrypto.PublicKey{}, + } +} + +func (m *KeyToPower) Set(pk tmprotocrypto.PublicKey, power int64) { + stringified := DeterministicStringify(pk) + if k, found := m.canonical[stringified]; found { + pk = k + } else { + m.canonical[stringified] = pk + } + m.inner[pk] = power +} + +type KeyToLastUpdateMemo struct { + inner map[tmprotocrypto.PublicKey]providertypes.LastUpdateMemo + canonical map[string]tmprotocrypto.PublicKey +} + +func MakeKeyToLastUpdateMemo() KeyToLastUpdateMemo { + return KeyToLastUpdateMemo{ + inner: map[tmprotocrypto.PublicKey]providertypes.LastUpdateMemo{}, + canonical: map[string]tmprotocrypto.PublicKey{}, + } +} + +func (m *KeyToLastUpdateMemo) Set(pk tmprotocrypto.PublicKey, memo providertypes.LastUpdateMemo) { + stringified := DeterministicStringify(pk) + if k, found := m.canonical[stringified]; found { + pk = k + } else { + m.canonical[stringified] = pk + } + m.inner[pk] = memo +} + +func (m *KeyToLastUpdateMemo) Get(pk tmprotocrypto.PublicKey) (providertypes.LastUpdateMemo, bool) { + if k, found := m.canonical[DeterministicStringify(pk)]; found { + memo, found := m.inner[k] + if !found { + panic("found canonical key but key not present in inner map") + } + return memo, true + } + return providertypes.LastUpdateMemo{}, false +} + +type KeySet struct { + inner map[tmprotocrypto.PublicKey]struct{} + canonical map[string]tmprotocrypto.PublicKey +} + +func MakeKeySet() KeySet { + return KeySet{ + inner: map[tmprotocrypto.PublicKey]struct{}{}, + canonical: map[string]tmprotocrypto.PublicKey{}, + } +} + +func (s *KeySet) Add(pk tmprotocrypto.PublicKey) { + stringified := DeterministicStringify(pk) + if k, found := s.canonical[stringified]; found { + pk = k + } else { + s.canonical[stringified] = pk + } + s.inner[pk] = struct{}{} +} + +func (s *KeySet) Has(pk tmprotocrypto.PublicKey) bool { + _, found := s.canonical[DeterministicStringify(pk)] + return found +} + // getProviderKeysForUpdate returns the all the provider keys of validators for which // an update must be sent. An update must be sent to the consumer if, either: // 1. The consumer has the validator in its active set AND the assigned key for the // validator has changed since the last update. // 2. The voting power of the validator has changed, and the validator is in the active // set. -func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates map[ProviderPublicKey]int64) ([]ProviderPublicKey, map[string]bool) { +func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates map[ProviderPublicKey]int64) KeySet { - // Return a list of provider keys that need to be updated - keys := []ProviderPublicKey{} - // Key types cannot be used for map lookup so use this string indexed - // map to check if a key is already in the list - included := map[string]bool{} + ret := MakeKeySet() // Get provider keys which the consumer is aware of, because the // last update sent to the consumer was a positive power update @@ -156,8 +234,7 @@ func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates map[ProviderPub if newCk, ok := ka.Store.GetProviderConsAddrToConsumerPublicKey(pca); ok { oldCk := lum.ConsumerKey if !oldCk.Equal(newCk) && 0 < lum.Power { - keys = append(keys, *lum.ProviderKey) - included[DeterministicStringify(*lum.ProviderKey)] = true + ret.Add(*lum.ProviderKey) } } return false @@ -165,23 +242,18 @@ func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates map[ProviderPub // Get provider keys where the validator power has changed for providerPublicKey := range stakingUpdates { - s := DeterministicStringify(providerPublicKey) - if !included[s] { - keys = append(keys, providerPublicKey) - included[s] = true - } + ret.Add(providerPublicKey) } - return keys, included + return ret } -func (ka KeyAssignment) getProviderKeysLastPositiveUpdate(mustCreateUpdate map[string]bool) map[string]providertypes.LastUpdateMemo { - lastUpdate := map[string]providertypes.LastUpdateMemo{} +func (ka KeyAssignment) getProviderKeysLastPositiveUpdate(mustCreateUpdate KeySet) KeyToLastUpdateMemo { + lastUpdate := MakeKeyToLastUpdateMemo() ka.Store.IterateConsumerConsAddrToLastUpdateMemo(func(_ ConsumerConsAddr, lum providertypes.LastUpdateMemo) bool { - s := DeterministicStringify(*lum.ProviderKey) if 0 < lum.Power { - if _, found := mustCreateUpdate[s]; found { - lastUpdate[s] = lum + if mustCreateUpdate.Has(*lum.ProviderKey) { + lastUpdate.Set(*lum.ProviderKey, lum) } } return false @@ -189,36 +261,14 @@ func (ka KeyAssignment) getProviderKeysLastPositiveUpdate(mustCreateUpdate map[s return lastUpdate } -type KeyToPowerMap struct { - inner map[tmprotocrypto.PublicKey]int64 - canonical map[string]tmprotocrypto.PublicKey -} - -func MakeKeyToPowerMap() KeyToPowerMap { - return KeyToPowerMap{ - inner: map[tmprotocrypto.PublicKey]int64{}, - canonical: map[string]tmprotocrypto.PublicKey{}, - } -} - -func (m *KeyToPowerMap) Set(pk tmprotocrypto.PublicKey, power int64) { - s := DeterministicStringify(pk) - if k, found := m.canonical[s]; found { - pk = k - } else { - m.canonical[s] = pk - } - m.inner[pk] = power -} - // do inner work as part of ComputeUpdates func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[ProviderPublicKey]int64) map[ConsumerPublicKey]int64 { // Init the return value - consumerUpdates := MakeKeyToPowerMap() + consumerUpdates := MakeKeyToPower() - providerKeysForUpdate, mustUpdate := ka.getProviderKeysForUpdate(stakingUpdates) - providerKeysLastPositivePowerUpdateMemo := ka.getProviderKeysLastPositiveUpdate(mustUpdate) + providerKeysForUpdate := ka.getProviderKeysForUpdate(stakingUpdates) + providerKeysLastPositivePowerUpdateMemo := ka.getProviderKeysLastPositiveUpdate(providerKeysForUpdate) /* Create a deletion (zero power) update for any consumer key known to the consumer @@ -228,11 +278,11 @@ func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[Prov The purpose of *this* step is to create a clean slate. The *next* step will amend the updates for any validators with a positive power. */ - for i := range providerKeysForUpdate { + for loopPk := range providerKeysForUpdate.inner { // For each provider key for which there was already a positive update // create a deletion update for the associated consumer key. - pk := providerKeysForUpdate[i] // Avoid taking address to loop variable - if lum, found := providerKeysLastPositivePowerUpdateMemo[DeterministicStringify(pk)]; found { + pk := loopPk // Avoid taking the address of the loop variable + if lum, found := providerKeysLastPositivePowerUpdateMemo.Get(pk); found { cca := TMCryptoPublicKeyToConsAddr(*lum.ConsumerKey) ka.Store.SetConsumerConsAddrToLastUpdateMemo(cca, providertypes.LastUpdateMemo{ConsumerKey: lum.ConsumerKey, ProviderKey: &pk, Vscid: vscid, Power: 0}) consumerUpdates.Set(*lum.ConsumerKey, 0) @@ -245,8 +295,8 @@ func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[Prov The purpose of this step is to ensure that the consumer is sent the latest positive power update for any validator for which the power is positive. */ - for i := range providerKeysForUpdate { - pk := providerKeysForUpdate[i] // Avoid taking address to loop variable + for loopPk := range providerKeysForUpdate.inner { + pk := loopPk // Avoid taking the address of the loop variable // For each provider key where there was either // 1) already a positive power update @@ -255,7 +305,7 @@ func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[Prov var power int64 = 0 - if lum, found := providerKeysLastPositivePowerUpdateMemo[DeterministicStringify(pk)]; found { + if lum, found := providerKeysLastPositivePowerUpdateMemo.Get(pk); found { // There was previously a positive power update: copy it. power = lum.Power } From 0e63d51767d95c2e828b5ca24b5bb091a12f6371 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Nov 2022 19:19:47 +0000 Subject: [PATCH 3/7] (pass) use more instances of specialised structures --- x/ccv/provider/keeper/key_assignment.go | 50 ++++++++++++++++--------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/x/ccv/provider/keeper/key_assignment.go b/x/ccv/provider/keeper/key_assignment.go index 2fb5bcd5ee..c673024f9b 100644 --- a/x/ccv/provider/keeper/key_assignment.go +++ b/x/ccv/provider/keeper/key_assignment.go @@ -156,6 +156,17 @@ func (m *KeyToPower) Set(pk tmprotocrypto.PublicKey, power int64) { m.inner[pk] = power } +func (m *KeyToPower) Get(pk tmprotocrypto.PublicKey) (int64, bool) { + if k, found := m.canonical[DeterministicStringify(pk)]; found { + power, found := m.inner[k] + if !found { + panic("found canonical key but key not present in inner map") + } + return power, true + } + return -1, false +} + type KeyToLastUpdateMemo struct { inner map[tmprotocrypto.PublicKey]providertypes.LastUpdateMemo canonical map[string]tmprotocrypto.PublicKey @@ -222,7 +233,7 @@ func (s *KeySet) Has(pk tmprotocrypto.PublicKey) bool { // validator has changed since the last update. // 2. The voting power of the validator has changed, and the validator is in the active // set. -func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates map[ProviderPublicKey]int64) KeySet { +func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates KeyToPower) KeySet { ret := MakeKeySet() @@ -233,6 +244,7 @@ func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates map[ProviderPub pca := TMCryptoPublicKeyToConsAddr(*lum.ProviderKey) if newCk, ok := ka.Store.GetProviderConsAddrToConsumerPublicKey(pca); ok { oldCk := lum.ConsumerKey + // Key changed? Last power was positive? if !oldCk.Equal(newCk) && 0 < lum.Power { ret.Add(*lum.ProviderKey) } @@ -241,7 +253,7 @@ func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates map[ProviderPub }) // Get provider keys where the validator power has changed - for providerPublicKey := range stakingUpdates { + for providerPublicKey := range stakingUpdates.inner { ret.Add(providerPublicKey) } @@ -262,12 +274,15 @@ func (ka KeyAssignment) getProviderKeysLastPositiveUpdate(mustCreateUpdate KeySe } // do inner work as part of ComputeUpdates -func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[ProviderPublicKey]int64) map[ConsumerPublicKey]int64 { +func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates KeyToPower) KeyToPower { - // Init the return value + // Init the return data structure consumerUpdates := MakeKeyToPower() + // Get a set of all the provider validator keys for which an update must be sent providerKeysForUpdate := ka.getProviderKeysForUpdate(stakingUpdates) + + // Get the LAST positive update for each provider validator key that needs an update providerKeysLastPositivePowerUpdateMemo := ka.getProviderKeysLastPositiveUpdate(providerKeysForUpdate) /* @@ -311,7 +326,7 @@ func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[Prov } // There is a new validator power: use it. It takes precedence. - if updatedVotingPower, ok := stakingUpdates[pk]; ok { + if updatedVotingPower, ok := stakingUpdates.Get(pk); ok { power = updatedVotingPower } @@ -327,22 +342,26 @@ func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates map[Prov } } - return consumerUpdates.inner + return consumerUpdates } -func toMap(providerUpdates []abci.ValidatorUpdate) map[ProviderPublicKey]int64 { - ret := map[ProviderPublicKey]int64{} - for _, u := range providerUpdates { - ret[u.PubKey] = u.Power +func (ka *KeyAssignment) ComputeUpdates(vscid VSCID, stakingUpdates []abci.ValidatorUpdate) []abci.ValidatorUpdate { + + // Create a map of provider keys to power + keyToPower := MakeKeyToPower() + for _, u := range stakingUpdates { + keyToPower.Set(u.PubKey, u.Power) } - return ret -} -func fromMap(consumerUpdates map[ConsumerPublicKey]int64) []abci.ValidatorUpdate { + // Get the consumerUpdates to send to the consumer + consumerUpdates := ka.getConsumerUpdates(vscid, keyToPower) + + // Transform the consumer updates back into a list for tendermint ret := []abci.ValidatorUpdate{} - for ck, power := range consumerUpdates { + for ck, power := range consumerUpdates.inner { ret = append(ret, abci.ValidatorUpdate{PubKey: ck, Power: power}) } + // Sort for determinism (TODO:, necessary?) sort.Slice(ret, func(i, j int) bool { if ret[i].Power > ret[j].Power { return true @@ -350,10 +369,7 @@ func fromMap(consumerUpdates map[ConsumerPublicKey]int64) []abci.ValidatorUpdate return ret[i].PubKey.String() > ret[j].PubKey.String() }) return ret -} -func (ka *KeyAssignment) ComputeUpdates(vscid VSCID, stakingUpdates []abci.ValidatorUpdate) (consumerUpdates []abci.ValidatorUpdate) { - return fromMap(ka.getConsumerUpdates(vscid, toMap(stakingUpdates))) } func (ka *KeyAssignment) AssignDefaultsForProviderKeysWithoutExplicitAssignments(updates []abci.ValidatorUpdate) { From 0552c000dc72b6129340ba0a73acbcd000ac758a Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Nov 2022 19:27:04 +0000 Subject: [PATCH 4/7] Adds TestNonDeterministicMapIteration --- x/ccv/provider/keeper/key_assignment_test.go | 21 ++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/x/ccv/provider/keeper/key_assignment_test.go b/x/ccv/provider/keeper/key_assignment_test.go index 296c1a7fd9..5560f1b19a 100644 --- a/x/ccv/provider/keeper/key_assignment_test.go +++ b/x/ccv/provider/keeper/key_assignment_test.go @@ -526,6 +526,27 @@ func TestKeyAssignmentMemo(t *testing.T) { require.True(t, pk.Equal(key(0))) } +func TestNonDeterministicMapIteration(t *testing.T) { + m := map[int]int{} + for j := 0; j < 5; j++ { + m[j] = j + } + k := map[int][]int{} + for i := 0; i < 100; i++ { + k[i] = []int{} + for key := range m { + k[i] = append(k[i], key) + } + } + for j := 0; j < 5; j++ { + for i := 0; i < 99; i++ { + if k[i][j] != k[i+1][j] { + t.Fatal() + } + } + } +} + func TestKeyAssignmentMemoLoopIteration(t *testing.T) { m := providertypes.LastUpdateMemo{} { From ebce38f8c6d2fd3d6cfb6bc013c95b92597cb65d Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Nov 2022 19:27:49 +0000 Subject: [PATCH 5/7] DEL TestNonDeterministicMapIteration --- x/ccv/provider/keeper/key_assignment_test.go | 21 -------------------- 1 file changed, 21 deletions(-) diff --git a/x/ccv/provider/keeper/key_assignment_test.go b/x/ccv/provider/keeper/key_assignment_test.go index 5560f1b19a..296c1a7fd9 100644 --- a/x/ccv/provider/keeper/key_assignment_test.go +++ b/x/ccv/provider/keeper/key_assignment_test.go @@ -526,27 +526,6 @@ func TestKeyAssignmentMemo(t *testing.T) { require.True(t, pk.Equal(key(0))) } -func TestNonDeterministicMapIteration(t *testing.T) { - m := map[int]int{} - for j := 0; j < 5; j++ { - m[j] = j - } - k := map[int][]int{} - for i := 0; i < 100; i++ { - k[i] = []int{} - for key := range m { - k[i] = append(k[i], key) - } - } - for j := 0; j < 5; j++ { - for i := 0; i < 99; i++ { - if k[i][j] != k[i+1][j] { - t.Fatal() - } - } - } -} - func TestKeyAssignmentMemoLoopIteration(t *testing.T) { m := providertypes.LastUpdateMemo{} { From 2a1abad2d4f00bd94494d821d87388a5f482e294 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Nov 2022 19:29:12 +0000 Subject: [PATCH 6/7] Add comment in body of ComputeUpdates --- x/ccv/provider/keeper/key_assignment.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/x/ccv/provider/keeper/key_assignment.go b/x/ccv/provider/keeper/key_assignment.go index c673024f9b..997de54858 100644 --- a/x/ccv/provider/keeper/key_assignment.go +++ b/x/ccv/provider/keeper/key_assignment.go @@ -356,20 +356,22 @@ func (ka *KeyAssignment) ComputeUpdates(vscid VSCID, stakingUpdates []abci.Valid // Get the consumerUpdates to send to the consumer consumerUpdates := ka.getConsumerUpdates(vscid, keyToPower) - // Transform the consumer updates back into a list for tendermint - ret := []abci.ValidatorUpdate{} + // Transform the consumer updates back into a list for sending to the consumer + // The consumer will aggregate updates and ultimately send them to tendermint. + tendermintUpdates := []abci.ValidatorUpdate{} for ck, power := range consumerUpdates.inner { - ret = append(ret, abci.ValidatorUpdate{PubKey: ck, Power: power}) + tendermintUpdates = append(tendermintUpdates, abci.ValidatorUpdate{PubKey: ck, Power: power}) } - // Sort for determinism (TODO:, necessary?) - sort.Slice(ret, func(i, j int) bool { - if ret[i].Power > ret[j].Power { + + // Sort for determinism + sort.Slice(tendermintUpdates, func(i, j int) bool { + if tendermintUpdates[i].Power > tendermintUpdates[j].Power { return true } - return ret[i].PubKey.String() > ret[j].PubKey.String() + return tendermintUpdates[i].PubKey.String() > tendermintUpdates[j].PubKey.String() }) - return ret + return tendermintUpdates } func (ka *KeyAssignment) AssignDefaultsForProviderKeysWithoutExplicitAssignments(updates []abci.ValidatorUpdate) { From 8d060466ebb8d366e49ed602dcb42e0978f7e747 Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 14 Nov 2022 19:46:56 +0000 Subject: [PATCH 7/7] Makes a pass through docstrings --- x/ccv/provider/keeper/key_assignment.go | 68 ++++++++++++++++++++----- 1 file changed, 56 insertions(+), 12 deletions(-) diff --git a/x/ccv/provider/keeper/key_assignment.go b/x/ccv/provider/keeper/key_assignment.go index 997de54858..41cd80392b 100644 --- a/x/ccv/provider/keeper/key_assignment.go +++ b/x/ccv/provider/keeper/key_assignment.go @@ -21,14 +21,6 @@ type ConsumerPublicKey = tmprotocrypto.PublicKey type ProviderConsAddr = sdk.ConsAddress type ConsumerConsAddr = sdk.ConsAddress -func DeterministicStringify(k tmprotocrypto.PublicKey) string { - bz, err := k.Marshal() - if err != nil { - panic(err) - } - return string(bz) -} - func TMCryptoPublicKeyToConsAddr(k tmprotocrypto.PublicKey) sdk.ConsAddress { sdkK, err := sdkcryptocodec.FromTmProtoPublicKey(k) if err != nil { @@ -52,6 +44,9 @@ type Store interface { IterateConsumerPublicKeyToProviderPublicKey(func(ConsumerPublicKey, ProviderPublicKey) bool) } +// KeyAssignment is a wrapper around a storage API which implements the key assignment features +// allowing a provider to assign a consumer key to a provider key and vice versa. +// Please see docs/key-assignment/design.md for more details. type KeyAssignment struct { Store Store } @@ -62,6 +57,9 @@ func MakeKeyAssignment(store Store) KeyAssignment { } } +// SetProviderPubKeyToConsumerPubKey tries to assign a mapping from the provider key to the consumer key. +// The operation can fail if the consumer key is or was recently assigned to by a provider key. This is a +// security feature. func (ka *KeyAssignment) SetProviderPubKeyToConsumerPubKey(pk ProviderPublicKey, ck ConsumerPublicKey) error { if _, ok := ka.Store.GetConsumerPublicKeyToProviderPublicKey(ck); ok { return errors.New(`cannot reuse key which is in use or was recently in use`) @@ -106,14 +104,18 @@ func (ka *KeyAssignment) DeleteProviderKey(pca ProviderConsAddr) error { return nil } +// GetCurrentConsumerPubKeyFromProviderPubKey returns the current consumer key assigned to the provider key. func (ka *KeyAssignment) GetCurrentConsumerPubKeyFromProviderPubKey(pk ProviderPublicKey) (ck ConsumerPublicKey, found bool) { return ka.Store.GetProviderConsAddrToConsumerPublicKey(TMCryptoPublicKeyToConsAddr(pk)) } +// GetProviderPubKeyFromConsumerPubKey returns any provider key which is currently assigned to the consumer key. func (ka *KeyAssignment) GetProviderPubKeyFromConsumerPubKey(ck ConsumerPublicKey) (pk ProviderPublicKey, found bool) { return ka.Store.GetConsumerPublicKeyToProviderPublicKey(ck) } +// GetProviderPubKeyFromConsumerConsAddress returns the provider key which was associated to the consumer key with +// consensus address cca at the last update that used the consumer key. func (ka *KeyAssignment) GetProviderPubKeyFromConsumerConsAddress(cca sdk.ConsAddress) (pk ProviderPublicKey, found bool) { if lum, found := ka.Store.GetConsumerConsAddrToLastUpdateMemo(cca); found { return *lum.ProviderKey, true @@ -121,10 +123,13 @@ func (ka *KeyAssignment) GetProviderPubKeyFromConsumerConsAddress(cca sdk.ConsAd return pk, false } -func (ka *KeyAssignment) PruneUnusedKeys(latestVscid VSCID) { +// PruneUnusedKeys deletes from the store all of the consumer keys which the consumer chain has matured, based +// on latestMatureVscid, and for which it sure that the correct consumer chain can no longer reference the key +// in a downtime or double sign slash instruction. +func (ka *KeyAssignment) PruneUnusedKeys(latestMatureVscid VSCID) { toDel := []ConsumerConsAddr{} ka.Store.IterateConsumerConsAddrToLastUpdateMemo(func(cca ConsumerConsAddr, lum providertypes.LastUpdateMemo) bool { - if lum.Power == 0 && lum.Vscid <= latestVscid { + if lum.Power == 0 && lum.Vscid <= latestMatureVscid { toDel = append(toDel, cca) } return false @@ -134,6 +139,19 @@ func (ka *KeyAssignment) PruneUnusedKeys(latestVscid VSCID) { } } +// DeterministicStringify returns a deterministic string representation of the +// key. This is useful to identify like keys with different representations. +func DeterministicStringify(k tmprotocrypto.PublicKey) string { + bz, err := k.Marshal() + if err != nil { + panic(err) + } + return string(bz) +} + +// KeyToPower is an implementation of a map from a public key to a power. +// It is used because public keys are not comparable in Go, so they cannot be +// used as builtin map keys. type KeyToPower struct { inner map[tmprotocrypto.PublicKey]int64 canonical map[string]tmprotocrypto.PublicKey @@ -146,6 +164,7 @@ func MakeKeyToPower() KeyToPower { } } +// Set is a typical map set operation. func (m *KeyToPower) Set(pk tmprotocrypto.PublicKey, power int64) { stringified := DeterministicStringify(pk) if k, found := m.canonical[stringified]; found { @@ -156,6 +175,7 @@ func (m *KeyToPower) Set(pk tmprotocrypto.PublicKey, power int64) { m.inner[pk] = power } +// Get is a typical map get operation. func (m *KeyToPower) Get(pk tmprotocrypto.PublicKey) (int64, bool) { if k, found := m.canonical[DeterministicStringify(pk)]; found { power, found := m.inner[k] @@ -167,6 +187,10 @@ func (m *KeyToPower) Get(pk tmprotocrypto.PublicKey) (int64, bool) { return -1, false } +// KeyToLastUpdateMemo is an implementation of a map from a public key to a +// memo storing the last update. +// It is used because public keys are not comparable in Go, so they cannot be +// used as builtin map keys. type KeyToLastUpdateMemo struct { inner map[tmprotocrypto.PublicKey]providertypes.LastUpdateMemo canonical map[string]tmprotocrypto.PublicKey @@ -179,6 +203,7 @@ func MakeKeyToLastUpdateMemo() KeyToLastUpdateMemo { } } +// Set is a typical map set operation. func (m *KeyToLastUpdateMemo) Set(pk tmprotocrypto.PublicKey, memo providertypes.LastUpdateMemo) { stringified := DeterministicStringify(pk) if k, found := m.canonical[stringified]; found { @@ -189,6 +214,7 @@ func (m *KeyToLastUpdateMemo) Set(pk tmprotocrypto.PublicKey, memo providertypes m.inner[pk] = memo } +// Get is a typical map get operation. func (m *KeyToLastUpdateMemo) Get(pk tmprotocrypto.PublicKey) (providertypes.LastUpdateMemo, bool) { if k, found := m.canonical[DeterministicStringify(pk)]; found { memo, found := m.inner[k] @@ -200,6 +226,9 @@ func (m *KeyToLastUpdateMemo) Get(pk tmprotocrypto.PublicKey) (providertypes.Las return providertypes.LastUpdateMemo{}, false } +// KeySet is an implementation of a set of public keys. +// It is used because public keys are not comparable in Go +// so they cannot be used as builtin map keys. type KeySet struct { inner map[tmprotocrypto.PublicKey]struct{} canonical map[string]tmprotocrypto.PublicKey @@ -212,6 +241,7 @@ func MakeKeySet() KeySet { } } +// Add is a typical set add operation. func (s *KeySet) Add(pk tmprotocrypto.PublicKey) { stringified := DeterministicStringify(pk) if k, found := s.canonical[stringified]; found { @@ -222,6 +252,7 @@ func (s *KeySet) Add(pk tmprotocrypto.PublicKey) { s.inner[pk] = struct{}{} } +// Add is a typical set has/contains operation. func (s *KeySet) Has(pk tmprotocrypto.PublicKey) bool { _, found := s.canonical[DeterministicStringify(pk)] return found @@ -260,6 +291,8 @@ func (ka *KeyAssignment) getProviderKeysForUpdate(stakingUpdates KeyToPower) Key return ret } +// getProviderKeysLastPositiveUpdate returns a map of provider keys to a memo of the last update +// associated to the key, if that update was positive, and that update is included in mustCreateUpdate. func (ka KeyAssignment) getProviderKeysLastPositiveUpdate(mustCreateUpdate KeySet) KeyToLastUpdateMemo { lastUpdate := MakeKeyToLastUpdateMemo() ka.Store.IterateConsumerConsAddrToLastUpdateMemo(func(_ ConsumerConsAddr, lum providertypes.LastUpdateMemo) bool { @@ -345,6 +378,12 @@ func (ka *KeyAssignment) getConsumerUpdates(vscid VSCID, stakingUpdates KeyToPow return consumerUpdates } +// ComputeUpdates takes a validator power updates as produced by the staking module and returns a set of updates +// which can be sent to the consumer. This is a stateful operation. The updates produced ensure that the consumer +// updates represent a consistent validator set in the presence of validator set changes and validator key +// assignment changes. +// The vscid is used in the pruning mechanism. Positive power updates produced by a call to this function with a +// given vscid will not be prunable until the vscid matures. func (ka *KeyAssignment) ComputeUpdates(vscid VSCID, stakingUpdates []abci.ValidatorUpdate) []abci.ValidatorUpdate { // Create a map of provider keys to power @@ -363,7 +402,7 @@ func (ka *KeyAssignment) ComputeUpdates(vscid VSCID, stakingUpdates []abci.Valid tendermintUpdates = append(tendermintUpdates, abci.ValidatorUpdate{PubKey: ck, Power: power}) } - // Sort for determinism + // Sort for determinism across nodes. sort.Slice(tendermintUpdates, func(i, j int) bool { if tendermintUpdates[i].Power > tendermintUpdates[j].Power { return true @@ -374,6 +413,8 @@ func (ka *KeyAssignment) ComputeUpdates(vscid VSCID, stakingUpdates []abci.Valid return tendermintUpdates } +// AssignDefaultsForProviderKeysWithoutExplicitAssignments assigns the default consumer key to any provider key +// for which a consumer key has not been explicitly assigned. func (ka *KeyAssignment) AssignDefaultsForProviderKeysWithoutExplicitAssignments(updates []abci.ValidatorUpdate) { for _, u := range updates { if _, found := ka.GetCurrentConsumerPubKeyFromProviderPubKey(u.PubKey); !found { @@ -384,12 +425,14 @@ func (ka *KeyAssignment) AssignDefaultsForProviderKeysWithoutExplicitAssignments } } +// AssignDefaultsAndComputeUpdates is a convenience function which calls AssignDefaultsForProviderKeysWithoutExplicitAssignments +// and also computes the latest set of updates to send to the consumer using ComputeUpdates. func (ka *KeyAssignment) AssignDefaultsAndComputeUpdates(vscid VSCID, stakingUpdates []abci.ValidatorUpdate) (consumerUpdates []abci.ValidatorUpdate) { ka.AssignDefaultsForProviderKeysWithoutExplicitAssignments(stakingUpdates) return ka.ComputeUpdates(vscid, stakingUpdates) } -// Returns true iff internal invariants hold +// Returns true iff internal invariants hold. For testing. Expensive. func (ka *KeyAssignment) InternalInvariants() bool { good := true @@ -659,6 +702,7 @@ func (s *KeyAssignmentStore) IterateConsumerConsAddrToLastUpdateMemo(stop func(C } } +// Delete all of the key assignment data associated to the given chain ID. func (k Keeper) DeleteKeyAssignment(ctx sdk.Context, chainID string) { store := ctx.KVStore(k.storeKey) for _, pref := range [][]byte{