From eaac80837cccd192417e0a7de9d4e0deb75bae02 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Fri, 17 May 2024 14:11:33 -0500 Subject: [PATCH 1/3] Add Payload.Address() to extract payload address This commit adds Payload.Address(). It can be used by programs to reduce overhead of extracting address from payload key. --- ledger/common/convert/convert.go | 16 ++--- ledger/common/convert/convert_test.go | 26 ++++---- ledger/trie.go | 31 +++++++++ ledger/trie_encoder.go | 91 ++++++++++++++++++++------- ledger/trie_test.go | 29 ++++++++- 5 files changed, 144 insertions(+), 49 deletions(-) diff --git a/ledger/common/convert/convert.go b/ledger/common/convert/convert.go index 05e8f3ac39e..d81912f0cc3 100644 --- a/ledger/common/convert/convert.go +++ b/ledger/common/convert/convert.go @@ -7,14 +7,6 @@ import ( "github.com/onflow/flow-go/model/flow" ) -const ( - KeyPartOwner = uint16(0) - // Deprecated: KeyPartController was only used by the very first - // version of Cadence for access control, which was later retired - _ = uint16(1) // DO NOT REUSE - KeyPartKey = uint16(2) -) - // UnexpectedLedgerKeyFormat is returned when a ledger key is not in the expected format var UnexpectedLedgerKeyFormat = fmt.Errorf("unexpected ledger key format") @@ -23,8 +15,8 @@ var UnexpectedLedgerKeyFormat = fmt.Errorf("unexpected ledger key format") func LedgerKeyToRegisterID(key ledger.Key) (flow.RegisterID, error) { parts := key.KeyParts if len(parts) != 2 || - parts[0].Type != KeyPartOwner || - parts[1].Type != KeyPartKey { + parts[0].Type != ledger.KeyPartOwner || + parts[1].Type != ledger.KeyPartKey { return flow.RegisterID{}, fmt.Errorf("ledger key %s: %w", key.String(), UnexpectedLedgerKeyFormat) } @@ -39,11 +31,11 @@ func RegisterIDToLedgerKey(registerID flow.RegisterID) ledger.Key { return ledger.Key{ KeyParts: []ledger.KeyPart{ { - Type: KeyPartOwner, + Type: ledger.KeyPartOwner, Value: []byte(registerID.Owner), }, { - Type: KeyPartKey, + Type: ledger.KeyPartKey, Value: []byte(registerID.Key), }, }, diff --git a/ledger/common/convert/convert_test.go b/ledger/common/convert/convert_test.go index fa615c78a51..4933c9b7fc8 100644 --- a/ledger/common/convert/convert_test.go +++ b/ledger/common/convert/convert_test.go @@ -18,11 +18,11 @@ func TestLedgerKeyToRegisterID(t *testing.T) { key := ledger.Key{ KeyParts: []ledger.KeyPart{ { - Type: convert.KeyPartOwner, + Type: ledger.KeyPartOwner, Value: []byte(expectedRegisterID.Owner), }, { - Type: convert.KeyPartKey, + Type: ledger.KeyPartKey, Value: []byte("key"), }, }, @@ -45,11 +45,11 @@ func TestLedgerKeyToRegisterID_Global(t *testing.T) { key := ledger.Key{ KeyParts: []ledger.KeyPart{ { - Type: convert.KeyPartOwner, + Type: ledger.KeyPartOwner, Value: []byte(""), }, { - Type: convert.KeyPartKey, + Type: ledger.KeyPartKey, Value: []byte("uuid"), }, }, @@ -77,7 +77,7 @@ func TestLedgerKeyToRegisterID_Error(t *testing.T) { Value: []byte("owner"), }, { - Type: convert.KeyPartKey, + Type: ledger.KeyPartKey, Value: []byte("key"), }, }, @@ -93,13 +93,13 @@ func TestRegisterIDToLedgerKey(t *testing.T) { expectedKey := ledger.Key{ KeyParts: []ledger.KeyPart{ { - Type: convert.KeyPartOwner, + Type: ledger.KeyPartOwner, // Note: the owner field is extended to address length during NewRegisterID // so we have to do the same here Value: []byte(registerID.Owner), }, { - Type: convert.KeyPartKey, + Type: ledger.KeyPartKey, Value: []byte("key"), }, }, @@ -114,11 +114,11 @@ func TestRegisterIDToLedgerKey_Global(t *testing.T) { expectedKey := ledger.Key{ KeyParts: []ledger.KeyPart{ { - Type: convert.KeyPartOwner, + Type: ledger.KeyPartOwner, Value: []byte(""), }, { - Type: convert.KeyPartKey, + Type: ledger.KeyPartKey, Value: []byte("uuid"), }, }, @@ -135,8 +135,8 @@ func TestPayloadToRegister(t *testing.T) { p := ledger.NewPayload( ledger.NewKey( []ledger.KeyPart{ - ledger.NewKeyPart(convert.KeyPartOwner, []byte(expected.Owner)), - ledger.NewKeyPart(convert.KeyPartKey, []byte(expected.Key)), + ledger.NewKeyPart(ledger.KeyPartOwner, []byte(expected.Owner)), + ledger.NewKeyPart(ledger.KeyPartKey, []byte(expected.Key)), }, ), value, @@ -152,8 +152,8 @@ func TestPayloadToRegister(t *testing.T) { p := ledger.NewPayload( ledger.NewKey( []ledger.KeyPart{ - ledger.NewKeyPart(convert.KeyPartOwner, []byte("")), - ledger.NewKeyPart(convert.KeyPartKey, []byte("uuid")), + ledger.NewKeyPart(ledger.KeyPartOwner, []byte("")), + ledger.NewKeyPart(ledger.KeyPartKey, []byte("uuid")), }, ), value, diff --git a/ledger/trie.go b/ledger/trie.go index 1228dd64b1b..94224dc7185 100644 --- a/ledger/trie.go +++ b/ledger/trie.go @@ -13,6 +13,7 @@ import ( "github.com/onflow/flow-go/ledger/common/bitutils" "github.com/onflow/flow-go/ledger/common/hash" + "github.com/onflow/flow-go/model/flow" ) // Path captures storage path of a payload; @@ -237,6 +238,14 @@ func (k encKey) DeepCopy() encKey { return newK } +const ( + KeyPartOwner = uint16(0) + // Deprecated: KeyPartController was only used by the very first + // version of Cadence for access control, which was later retired + _ = uint16(1) // DO NOT REUSE + KeyPartKey = uint16(2) +) + // Payload is the smallest immutable storable unit in ledger type Payload struct { // encKey is key encoded using PayloadVersion. @@ -330,6 +339,28 @@ func (p *Payload) EncodedKey() []byte { return p.encKey } +// Address returns: +// - (address, nil) if the payload is for an account, the account address is returned +// - (flow.EmptyAddress, nil) if the payload is not for an account (global register) +// - (flow.EmptyAddress, err) if running into any exception +// The zero address is used for global Payloads and is not an actual account +func (p *Payload) Address() (flow.Address, error) { + if p == nil { + return flow.EmptyAddress, fmt.Errorf("failed to get payload address: payload is nil") + } + if len(p.encKey) == 0 { + return flow.EmptyAddress, fmt.Errorf("failed to get payload address: encoded key is empty") + } + b, found, err := decodeKeyPartValueByType(p.encKey, KeyPartOwner, true, PayloadVersion) + if err != nil { + return flow.EmptyAddress, err + } + if !found { + return flow.EmptyAddress, fmt.Errorf("failed to find address by type %d", KeyPartOwner) + } + return flow.BytesToAddress(b), nil +} + // Value returns payload value. // CAUTION: do not modify returned value because it shares underlying data with payload value. func (p *Payload) Value() Value { diff --git a/ledger/trie_encoder.go b/ledger/trie_encoder.go index 27bbf9e9a84..c4c50a63d7f 100644 --- a/ledger/trie_encoder.go +++ b/ledger/trie_encoder.go @@ -148,28 +148,28 @@ func DecodeKeyPart(encodedKeyPart []byte) (*KeyPart, error) { } // decode the key part content (zerocopy) - key, err := decodeKeyPart(rest, true, version) + kpt, kpv, err := decodeKeyPart(rest, true, version) if err != nil { return nil, fmt.Errorf("error decoding key part: %w", err) } - return key, nil + return &KeyPart{Type: kpt, Value: kpv}, nil } // decodeKeyPart decodes inp into KeyPart. If zeroCopy is true, KeyPart // references data in inp. Otherwise, it is copied. -func decodeKeyPart(inp []byte, zeroCopy bool, _ uint16) (*KeyPart, error) { +func decodeKeyPart(inp []byte, zeroCopy bool, _ uint16) (uint16, []byte, error) { // read key part type and the rest is the key item part kpt, kpv, err := utils.ReadUint16(inp) if err != nil { - return nil, fmt.Errorf("error decoding key part (content): %w", err) + return 0, nil, fmt.Errorf("error decoding key part (content): %w", err) } if zeroCopy { - return &KeyPart{Type: kpt, Value: kpv}, nil + return kpt, kpv, nil } v := make([]byte, len(kpv)) copy(v, kpv) - return &KeyPart{Type: kpt, Value: v}, nil + return kpt, v, nil } // EncodeKey encodes a key into a byte slice @@ -240,6 +240,63 @@ func DecodeKey(encodedKey []byte) (*Key, error) { return key, nil } +func decodeKeyPartValueByType(inp []byte, typ uint16, zeroCopy bool, version uint16) ([]byte, bool, error) { + // Read number of key parts + numOfParts, rest, err := utils.ReadUint16(inp) + if err != nil { + return nil, false, fmt.Errorf("error decoding number of key parts: %w", err) + } + + for i := 0; i < int(numOfParts); i++ { + var kpt uint16 + var kpv []byte + + kpt, kpv, rest, err = decodeKeyPartWithEncodedSizeInfo(rest, zeroCopy, version) + if err != nil { + return nil, false, err + } + if kpt == typ { + return kpv, true, nil + } + } + + return nil, false, nil +} + +func decodeKeyPartWithEncodedSizeInfo( + inp []byte, + zeroCopy bool, + version uint16, +) ( + // kp KeyPart, + kpt uint16, + kpv []byte, + rest []byte, + err error, +) { + + // Read encoded key part size + kpEncSize, rest, err := utils.ReadUint32(inp) + if err != nil { + return 0, nil, nil, fmt.Errorf("error decoding key part: %w", err) + } + + // Read encoded key part + var kpEnc []byte + kpEnc, rest, err = utils.ReadSlice(rest, int(kpEncSize)) + if err != nil { + return 0, nil, nil, fmt.Errorf("error decoding key part: %w", err) + } + + // Decode encoded key part + kpType, kpValue, err := decodeKeyPart(kpEnc, zeroCopy, version) + if err != nil { + return 0, nil, nil, fmt.Errorf("error decoding key part: %w", err) + } + + return kpType, kpValue, rest, nil +} + // decodeKey decodes inp into Key. If zeroCopy is true, returned key // references data in inp. Otherwise, it is copied. func decodeKey(inp []byte, zeroCopy bool, version uint16) (*Key, error) { @@ -257,27 +314,15 @@ func decodeKey(inp []byte, zeroCopy bool, version uint16) (*Key, error) { key.KeyParts = make([]KeyPart, numOfParts) for i := 0; i < int(numOfParts); i++ { - var kpEncSize uint32 - var kpEnc []byte - // read encoded key part size - kpEncSize, rest, err = utils.ReadUint32(rest) - if err != nil { - return nil, fmt.Errorf("error decoding key (content): %w", err) - } - - // read encoded key part - kpEnc, rest, err = utils.ReadSlice(rest, int(kpEncSize)) - if err != nil { - return nil, fmt.Errorf("error decoding key (content): %w", err) - } + var kpt uint16 + var kpv []byte - // decode encoded key part - kp, err := decodeKeyPart(kpEnc, zeroCopy, version) + kpt, kpv, rest, err = decodeKeyPartWithEncodedSizeInfo(rest, zeroCopy, version) if err != nil { - return nil, fmt.Errorf("error decoding key (content): %w", err) + return nil, err } - key.KeyParts[i] = *kp + key.KeyParts[i] = KeyPart{kpt, kpv} } return key, nil } diff --git a/ledger/trie_test.go b/ledger/trie_test.go index 8edc97b32a7..85a5a9a0be3 100644 --- a/ledger/trie_test.go +++ b/ledger/trie_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/fxamacker/cbor/v2" + "github.com/onflow/flow-go/model/flow" "github.com/stretchr/testify/require" ) @@ -308,26 +309,52 @@ func TestPayloadKey(t *testing.T) { k, err := p.Key() require.NoError(t, err) require.Equal(t, Key{}, k) + + _, err = p.Address() + require.Error(t, err) }) t.Run("empty payload", func(t *testing.T) { p := Payload{} k, err := p.Key() require.NoError(t, err) require.Equal(t, Key{}, k) + + _, err = p.Address() + require.Error(t, err) }) t.Run("empty key", func(t *testing.T) { p := NewPayload(Key{}, Value{}) k, err := p.Key() require.NoError(t, err) require.Equal(t, Key{}, k) + + _, err = p.Address() + require.Error(t, err) + }) + t.Run("global key", func(t *testing.T) { + key := Key{KeyParts: []KeyPart{{Type: 0, Value: []byte{}}, {Type: 1, Value: []byte("def")}}} + value := Value([]byte{0, 1, 2}) + p := NewPayload(key, value) + k, err := p.Key() + require.NoError(t, err) + require.Equal(t, key, k) + + addr, err := p.Address() + require.NoError(t, err) + require.Equal(t, flow.EmptyAddress, addr) }) t.Run("key", func(t *testing.T) { - key := Key{KeyParts: []KeyPart{{Type: 0, Value: []byte("abc")}, {Type: 1, Value: []byte("def")}}} + address := []byte{1, 2, 3, 4, 5, 6, 7, 8} + key := Key{KeyParts: []KeyPart{{Type: 0, Value: address}, {Type: 1, Value: []byte("def")}}} value := Value([]byte{0, 1, 2}) p := NewPayload(key, value) k, err := p.Key() require.NoError(t, err) require.Equal(t, key, k) + + addr, err := p.Address() + require.NoError(t, err) + require.Equal(t, flow.Address(address), addr) }) } From 4951b915453ab532185e0627999167565a95ecb3 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Fri, 17 May 2024 14:34:54 -0500 Subject: [PATCH 2/3] Replace PayloadToAddress() with Payload.Address() This reduces overhead of decoding and creating payload key. --- cmd/util/ledger/util/payload_grouping.go | 28 +----------------------- ledger/common/convert/convert_test.go | 5 ++--- 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/cmd/util/ledger/util/payload_grouping.go b/cmd/util/ledger/util/payload_grouping.go index a472998fa14..5237e48d9b2 100644 --- a/cmd/util/ledger/util/payload_grouping.go +++ b/cmd/util/ledger/util/payload_grouping.go @@ -10,7 +10,6 @@ import ( "github.com/rs/zerolog" "github.com/onflow/flow-go/ledger" - "github.com/onflow/flow-go/ledger/common/convert" "github.com/onflow/flow-go/model/flow" ) @@ -56,7 +55,7 @@ func (g *PayloadAccountGrouping) Next() (*PayloadAccountGroup, error) { } g.current++ - address, err := PayloadToAddress(g.payloads[accountStartIndex]) + address, err := g.payloads[accountStartIndex].Address() if err != nil { return nil, fmt.Errorf("failed to get address from payload: %w", err) } @@ -125,31 +124,6 @@ func GroupPayloadsByAccount( } } -// PayloadToAddress takes a payload and return: -// - (address, nil) if the payload is for an account, the account address is returned -// - (common.ZeroAddress, nil) if the payload is not for an account -// - (common.ZeroAddress, err) if running into any exception -// The zero address is used for global Payloads and is not an actual account -func PayloadToAddress(p *ledger.Payload) (flow.Address, error) { - k, err := p.Key() - if err != nil { - return flow.EmptyAddress, fmt.Errorf("could not find key for payload: %w", err) - } - - id, err := convert.LedgerKeyToRegisterID(k) - if err != nil { - return flow.EmptyAddress, fmt.Errorf("error converting key to register ID") - } - - if len([]byte(id.Owner)) != flow.AddressLength { - return flow.EmptyAddress, nil - } - - address := flow.BytesToAddress([]byte(id.Owner)) - - return address, nil -} - type sortablePayloads []*ledger.Payload func (s sortablePayloads) Len() int { diff --git a/ledger/common/convert/convert_test.go b/ledger/common/convert/convert_test.go index 4933c9b7fc8..dca50f1f19d 100644 --- a/ledger/common/convert/convert_test.go +++ b/ledger/common/convert/convert_test.go @@ -5,7 +5,6 @@ import ( "github.com/stretchr/testify/require" - "github.com/onflow/flow-go/cmd/util/ledger/util" "github.com/onflow/flow-go/ledger" "github.com/onflow/flow-go/ledger/common/convert" "github.com/onflow/flow-go/model/flow" @@ -34,7 +33,7 @@ func TestLedgerKeyToRegisterID(t *testing.T) { p := ledger.NewPayload(key, ledger.Value("value")) - address, err := util.PayloadToAddress(p) + address, err := p.Address() require.NoError(t, err) require.Equal(t, registerID.Owner, flow.AddressToRegisterOwner(address)) @@ -62,7 +61,7 @@ func TestLedgerKeyToRegisterID_Global(t *testing.T) { p := ledger.NewPayload(key, ledger.Value("value")) - address, err := util.PayloadToAddress(p) + address, err := p.Address() require.NoError(t, err) require.Equal(t, registerID.Owner, flow.AddressToRegisterOwner(address)) From 53c94b2b3d77af4cf2c6ffa511e4c9101329e241 Mon Sep 17 00:00:00 2001 From: Faye Amacker <33205765+fxamacker@users.noreply.github.com> Date: Fri, 17 May 2024 14:49:06 -0500 Subject: [PATCH 3/3] Lint --- ledger/trie_test.go | 3 ++- module/state_synchronization/indexer/indexer_core_test.go | 4 ++-- storage/pebble/bootstrap_test.go | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/ledger/trie_test.go b/ledger/trie_test.go index 85a5a9a0be3..3b5e6031da4 100644 --- a/ledger/trie_test.go +++ b/ledger/trie_test.go @@ -5,8 +5,9 @@ import ( "testing" "github.com/fxamacker/cbor/v2" - "github.com/onflow/flow-go/model/flow" "github.com/stretchr/testify/require" + + "github.com/onflow/flow-go/model/flow" ) // TestPayloadEquals tests equality of payloads. It tests: diff --git a/module/state_synchronization/indexer/indexer_core_test.go b/module/state_synchronization/indexer/indexer_core_test.go index 5e01e40d41c..5fd93d4b824 100644 --- a/module/state_synchronization/indexer/indexer_core_test.go +++ b/module/state_synchronization/indexer/indexer_core_test.go @@ -640,11 +640,11 @@ func ledgerPayloadWithValuesFixture(owner string, key string, value []byte) *led k := ledger.Key{ KeyParts: []ledger.KeyPart{ { - Type: convert.KeyPartOwner, + Type: ledger.KeyPartOwner, Value: []byte(owner), }, { - Type: convert.KeyPartKey, + Type: ledger.KeyPartKey, Value: []byte(key), }, }, diff --git a/storage/pebble/bootstrap_test.go b/storage/pebble/bootstrap_test.go index bf35ec915b3..f4d0e6ddaee 100644 --- a/storage/pebble/bootstrap_test.go +++ b/storage/pebble/bootstrap_test.go @@ -231,8 +231,8 @@ func randomRegisterPayloads(n uint16) []ledger.Payload { o := make([]byte, 0, 8) o = binary.BigEndian.AppendUint16(o, n) k := ledger.Key{KeyParts: []ledger.KeyPart{ - {Type: convert.KeyPartOwner, Value: o}, - {Type: convert.KeyPartKey, Value: o}, + {Type: ledger.KeyPartOwner, Value: o}, + {Type: ledger.KeyPartKey, Value: o}, }} // values are always 'v' for ease of testing/checking v := ledger.Value{defaultRegisterValue}