From b28e176ffcb609d877baeb71395fdb8078e2083a Mon Sep 17 00:00:00 2001 From: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> Date: Mon, 5 Dec 2022 17:11:41 -0800 Subject: [PATCH] requested key format changes (#560) changes --- x/ccv/provider/keeper/relay.go | 1 + x/ccv/provider/keeper/throttle.go | 8 ++-- x/ccv/provider/keeper/throttle_test.go | 57 +++++++++++++++++++------- x/ccv/provider/types/keys.go | 21 ++++++---- x/ccv/provider/types/keys_test.go | 13 +++--- x/ccv/provider/types/throttle.go | 11 ++++- 6 files changed, 77 insertions(+), 34 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index a014cbcec8..0631e3725b 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -253,6 +253,7 @@ func (k Keeper) OnRecvSlashPacket(ctx sdk.Context, packet channeltypes.Packet, d k.QueuePendingSlashPacketEntry(ctx, providertypes.NewSlashPacketEntry( ctx.BlockTime(), // recv time chainID, // consumer chain id that sent the packet + packet.Sequence, // IBC sequence number of the packet data.Validator.Address)) // Queue slash packet data in the same (consumer chain specific) queue as vsc matured packet data, diff --git a/x/ccv/provider/keeper/throttle.go b/x/ccv/provider/keeper/throttle.go index 85a5932594..7bfbeebd9e 100644 --- a/x/ccv/provider/keeper/throttle.go +++ b/x/ccv/provider/keeper/throttle.go @@ -180,10 +180,10 @@ func (k Keeper) GetSlashMeterAllowance(ctx sdktypes.Context) sdktypes.Int { // related to jailing/tombstoning over time. This "parent" queue is used to coordinate the order of slash packet handling // between chains, whereas the chain specific queue is used to coordinate the order of slash and vsc matured packets // relevant to each chain. -func (k Keeper) QueuePendingSlashPacketEntry(ctx sdktypes.Context, entry providertypes.SlashPacketEntry) { +func (k Keeper) QueuePendingSlashPacketEntry(ctx sdktypes.Context, + entry providertypes.SlashPacketEntry) { store := ctx.KVStore(k.storeKey) key := providertypes.PendingSlashPacketEntryKey(entry) - // Note: Val address is stored as value to assist in debugging. This could be removed for efficiency. store.Set(key, entry.ValAddr) } @@ -206,9 +206,9 @@ func (k Keeper) IteratePendingSlashPacketEntries(ctx sdktypes.Context, iterator := sdktypes.KVStorePrefixIterator(store, []byte{providertypes.PendingSlashPacketEntryBytePrefix}) defer iterator.Close() for ; iterator.Valid(); iterator.Next() { - recvTime, chainID := providertypes.ParsePendingSlashPacketEntryKey(iterator.Key()) + recvTime, chainID, ibcSeqNum := providertypes.ParsePendingSlashPacketEntryKey(iterator.Key()) valAddr := iterator.Value() - entry := providertypes.NewSlashPacketEntry(recvTime, chainID, valAddr) + entry := providertypes.NewSlashPacketEntry(recvTime, chainID, ibcSeqNum, valAddr) stop := cb(entry) if stop { break diff --git a/x/ccv/provider/keeper/throttle_test.go b/x/ccv/provider/keeper/throttle_test.go index ab6b5d7384..657a08521e 100644 --- a/x/ccv/provider/keeper/throttle_test.go +++ b/x/ccv/provider/keeper/throttle_test.go @@ -616,15 +616,17 @@ func TestPendingSlashPacketEntries(t *testing.T) { defer ctrl.Finish() // Consistent time for "now" - now := time.Now() + now := time.Now().UTC() entries := providerKeeper.GetAllPendingSlashPacketEntries(ctx) require.Equal(t, 0, len(entries)) // Queue 3 entries for chainIDs 0, 1, 2 for i := 0; i < 3; i++ { - entry := providertypes.NewSlashPacketEntry(now, - fmt.Sprintf("chain-%d", i), ed25519.GenPrivKey().PubKey().Address()) + entry := providertypes.NewSlashPacketEntry(now.Local(), + fmt.Sprintf("chain-%d", i), + 8, // all with seq = 8 + ed25519.GenPrivKey().PubKey().Address()) providerKeeper.QueuePendingSlashPacketEntry(ctx, entry) } entries = providerKeeper.GetAllPendingSlashPacketEntries(ctx) @@ -632,8 +634,10 @@ func TestPendingSlashPacketEntries(t *testing.T) { // Queue 3 entries for chainIDs 0, 1, 2 an hour later for i := 0; i < 3; i++ { - entry := providertypes.NewSlashPacketEntry(now.Add(time.Hour), - fmt.Sprintf("chain-%d", i), ed25519.GenPrivKey().PubKey().Address()) + entry := providertypes.NewSlashPacketEntry(now.Add(time.Hour).Local(), + fmt.Sprintf("chain-%d", i), + 9, // all with seq = 9 + ed25519.GenPrivKey().PubKey().Address()) providerKeeper.QueuePendingSlashPacketEntry(ctx, entry) } @@ -653,8 +657,10 @@ func TestPendingSlashPacketEntries(t *testing.T) { // Queue 3 entries for chainIDs 5, 6, 7 another hour later for i := 0; i < 3; i++ { - entry := providertypes.NewSlashPacketEntry(now.Add(2*time.Hour), - fmt.Sprintf("chain-%d", i+5), ed25519.GenPrivKey().PubKey().Address()) + entry := providertypes.NewSlashPacketEntry(now.Add(2*time.Hour).Local(), + fmt.Sprintf("chain-%d", i+5), + 7697, // all with seq = 7697 + ed25519.GenPrivKey().PubKey().Address()) providerKeeper.QueuePendingSlashPacketEntry(ctx, entry) } @@ -676,6 +682,29 @@ func TestPendingSlashPacketEntries(t *testing.T) { require.True(t, slices.Contains(thirdChainIdSet, "chain-6")) require.True(t, slices.Contains(thirdChainIdSet, "chain-7")) + // Assert each field is as expected for all 9 entries + for idx, entry := range entries { + switch idx { + case 0, 1, 2: + require.Equal(t, now, entry.RecvTime) + require.Equal(t, fmt.Sprintf("chain-%d", idx), entry.ConsumerChainID) + require.Equal(t, uint64(8), entry.IbcSeqNum) + require.NotEmpty(t, entry.ValAddr) + case 3, 4, 5: + require.Equal(t, now.Add(time.Hour), entry.RecvTime) + require.Equal(t, fmt.Sprintf("chain-%d", idx-3), entry.ConsumerChainID) + require.Equal(t, uint64(9), entry.IbcSeqNum) + require.NotEmpty(t, entry.ValAddr) + case 6, 7, 8: + require.Equal(t, now.Add(2*time.Hour), entry.RecvTime) + require.Equal(t, fmt.Sprintf("chain-%d", idx-6+5), entry.ConsumerChainID) + require.Equal(t, uint64(7697), entry.IbcSeqNum) + require.NotEmpty(t, entry.ValAddr) + default: + t.Fatalf("unexpected entry index %d", idx) + } + } + // Test the callback break functionality of the iterator entries = []providertypes.SlashPacketEntry{} providerKeeper.IteratePendingSlashPacketEntries(ctx, func(entry providertypes.SlashPacketEntry) bool { @@ -701,13 +730,13 @@ func TestPendingSlashPacketEntryDeletion(t *testing.T) { // Instantiate entries in the expected order we wish to get them back as (ordered by recv time) entries = []providertypes.SlashPacketEntry{} - entries = append(entries, providertypes.NewSlashPacketEntry(now, "chain-0", ed25519.GenPrivKey().PubKey().Address())) - entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(time.Hour).UTC(), "chain-1", ed25519.GenPrivKey().PubKey().Address())) - entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(2*time.Hour).Local(), "chain-2", ed25519.GenPrivKey().PubKey().Address())) - entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(3*time.Hour).In(time.FixedZone("UTC-8", -8*60*60)), "chain-3", ed25519.GenPrivKey().PubKey().Address())) - entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(4*time.Hour).Local(), "chain-4", ed25519.GenPrivKey().PubKey().Address())) - entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(5*time.Hour).UTC(), "chain-5", ed25519.GenPrivKey().PubKey().Address())) - entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(6*time.Hour).Local(), "chain-6", ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now, "chain-0", 1, ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(time.Hour).UTC(), "chain-1", 178, ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(2*time.Hour).Local(), "chain-2", 89, ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(3*time.Hour).In(time.FixedZone("UTC-8", -8*60*60)), "chain-3", 23423, ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(4*time.Hour).Local(), "chain-4", 323, ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(5*time.Hour).UTC(), "chain-5", 18, ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(6*time.Hour).Local(), "chain-6", 2, ed25519.GenPrivKey().PubKey().Address())) // Instantiate shuffled copy of above slice shuffledEntries := append([]providertypes.SlashPacketEntry{}, entries...) diff --git a/x/ccv/provider/types/keys.go b/x/ccv/provider/types/keys.go index e718b081ed..d9b4c68b40 100644 --- a/x/ccv/provider/types/keys.go +++ b/x/ccv/provider/types/keys.go @@ -275,10 +275,6 @@ func ParsePendingPacketDataKey(key []byte) (string, uint64, error) { } // PendingSlashPacketEntryKey returns the key for storing a pending slash packet entry. -// -// Note: It's not expected for a single consumer chain to send a slash packet for the same validator more than once -// in the same block. Hence why this key should ber unique per slash packet. However, if a malicious consumer did send -// duplicate slash packets in the same block, the slash packet entry would simply be overwritten. func PendingSlashPacketEntryKey(packetEntry SlashPacketEntry) []byte { timeBz := sdk.FormatTimeBytes(packetEntry.RecvTime) timeBzL := len(timeBz) @@ -286,27 +282,34 @@ func PendingSlashPacketEntryKey(packetEntry SlashPacketEntry) []byte { []byte{PendingSlashPacketEntryBytePrefix}, sdk.Uint64ToBigEndian(uint64(timeBzL)), timeBz, - HashBytes(packetEntry.ValAddr), + sdk.Uint64ToBigEndian(packetEntry.IbcSeqNum), []byte(packetEntry.ConsumerChainID), ) } // ParsePendingSlashPacketEntryKey returns the received time and chainID for a pending slash packet entry key -func ParsePendingSlashPacketEntryKey(bz []byte) (time.Time, string) { +func ParsePendingSlashPacketEntryKey(bz []byte) ( + recvTime time.Time, consumerChainID string, ibcSeqNum uint64) { + // Prefix is in first byte expectedPrefix := []byte{PendingSlashPacketEntryBytePrefix} if prefix := bz[:1]; !bytes.Equal(prefix, expectedPrefix) { panic(fmt.Sprintf("invalid prefix; expected: %X, got: %X", expectedPrefix, prefix)) } + // 8 bytes for uint64 storing timestamp length timeBzL := sdk.BigEndianToUint64(bz[1:9]) recvTime, err := sdk.ParseTimeBytes(bz[9 : 9+timeBzL]) if err != nil { panic(err) } - // ChainID is stored after 32 byte hashed validator address - chainID := string(bz[9+int(timeBzL)+32:]) - return recvTime, chainID + + ibcSeqNum = sdk.BigEndianToUint64(bz[9+timeBzL : 9+timeBzL+8]) + + // ChainID is stored after 8 byte ibc seq num + chainID := string(bz[9+timeBzL+8:]) + + return recvTime, chainID, ibcSeqNum } // ConsumerValidatorsKey returns the key under which the diff --git a/x/ccv/provider/types/keys_test.go b/x/ccv/provider/types/keys_test.go index 449c150b13..b6ecf553d5 100644 --- a/x/ccv/provider/types/keys_test.go +++ b/x/ccv/provider/types/keys_test.go @@ -179,19 +179,20 @@ func TestPendingPacketDataKeyAndParse(t *testing.T) { func TestPendingSlashPacketEntryKeyAndParse(t *testing.T) { now := time.Now() entries := []providertypes.SlashPacketEntry{} - entries = append(entries, providertypes.NewSlashPacketEntry(now, "chain-0", ed25519.GenPrivKey().PubKey().Address())) - entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(2*time.Hour), "chain-7896978", ed25519.GenPrivKey().PubKey().Address())) - entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(3*time.Hour), "chain-1", ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now, "chain-0", 2, ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(2*time.Hour), "chain-7896978", 3, ed25519.GenPrivKey().PubKey().Address())) + entries = append(entries, providertypes.NewSlashPacketEntry(now.Add(3*time.Hour), "chain-1", 4723894, ed25519.GenPrivKey().PubKey().Address())) for _, entry := range entries { key := providertypes.PendingSlashPacketEntryKey(entry) require.NotEmpty(t, key) timeBzL := len(sdk.FormatTimeBytes(entry.RecvTime)) - // This key should be of set length: prefix + 8 + timeBzL + hashed valAddr + chainID - require.Equal(t, 1+8+timeBzL+32+len(entry.ConsumerChainID), len(key)) - parsedRecvTime, parsedChainID := providertypes.ParsePendingSlashPacketEntryKey(key) + // This key should be of set length: prefix + 8 + timeBzL + 8 + chainID + require.Equal(t, 1+8+timeBzL+8+len(entry.ConsumerChainID), len(key)) + parsedRecvTime, parsedChainID, parsedIBCSeqNum := providertypes.ParsePendingSlashPacketEntryKey(key) require.Equal(t, entry.RecvTime, parsedRecvTime) require.Equal(t, entry.ConsumerChainID, parsedChainID) + require.Equal(t, entry.IbcSeqNum, parsedIBCSeqNum) } } diff --git a/x/ccv/provider/types/throttle.go b/x/ccv/provider/types/throttle.go index 9ca945d18d..62f2fe95fd 100644 --- a/x/ccv/provider/types/throttle.go +++ b/x/ccv/provider/types/throttle.go @@ -7,18 +7,27 @@ import ( // A persisted queue entry indicating that a slash packet data instance needs to be handled type SlashPacketEntry struct { // Block time that slash packet was received by provider chain. + // This field is used for store key iteration ordering. RecvTime time.Time // The consumer that sent the packet. ConsumerChainID string + // The IBC sequence number of the recv packet. + // This field is used in the store key to ensure uniqueness. + IbcSeqNum uint64 // The byte address of the validator being slashed. + // This field is used to obtain validator power in HandlePendingSlashPackets. + // It is not used in the store key, but is persisted in value bytes, + // see QueuePendingSlashPacketEntry. ValAddr []byte } // NewSlashPacketEntry creates a new SlashPacketEntry. -func NewSlashPacketEntry(recvTime time.Time, consumerChainID string, valAddr []byte) SlashPacketEntry { +func NewSlashPacketEntry(recvTime time.Time, consumerChainID string, + ibcSeqNum uint64, valAddr []byte) SlashPacketEntry { return SlashPacketEntry{ RecvTime: recvTime.UTC(), // UTC prevents serialization inconsistencies ConsumerChainID: consumerChainID, + IbcSeqNum: ibcSeqNum, ValAddr: valAddr, } }