From 9c4a3b9b3c24744c7efe2f925a404798b0b12e47 Mon Sep 17 00:00:00 2001 From: Thomas Bruyelle Date: Wed, 25 May 2022 15:02:32 +0200 Subject: [PATCH 1/4] fix: investigate iris bug insert balances fails because the amount field has non-UTF-8 characters. the test added in this commit takes balance trace from various chain and demonstrate a clear difference in the value of the trace for Iris. The next question is why? --- tracelistener/processor/bank_test.go | 50 +++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/tracelistener/processor/bank_test.go b/tracelistener/processor/bank_test.go index 802c7db5..5f023ebc 100644 --- a/tracelistener/processor/bank_test.go +++ b/tracelistener/processor/bank_test.go @@ -1,20 +1,68 @@ package processor import ( + "encoding/hex" + "encoding/json" + "fmt" "testing" "github.com/stretchr/testify/require" "go.uber.org/zap" + sdk "github.com/cosmos/cosmos-sdk/types" + bankTypes "github.com/cosmos/cosmos-sdk/x/bank/types" + gaia "github.com/cosmos/gaia/v6/app" models "github.com/emerishq/demeris-backend-models/tracelistener" "github.com/emerishq/tracelistener/tracelistener" "github.com/emerishq/tracelistener/tracelistener/config" "github.com/emerishq/tracelistener/tracelistener/processor/datamarshaler" ) -func TestBankProcessorOwnsKey(t *testing.T) { +func TestFix794(t *testing.T) { + ops := []string{ + // Iris1 + "{\"operation\":\"write\",\"key\":\"AhT+5cRMA/vuTTyGA9Qqcf/L5yTzpA==\",\"value\":\"CiQKBXVpcmlzEhs1MDk2MjUxNDM1MDYwNjM3ODgwNTA2Nzg0NTU=\",\"metadata\":{\"blockHeight\":15065683,\"txHash\":\"31647EB774FDC743E067FA459AA05CD5B0F315431CCCA54F98D877D7C26BCFC4\"}}", + // Iris2 + "{\"operation\":\"write\",\"key\":\"AhT+5cRMA/vuTTyGA9Qqcf/L5yTzpA==\",\"value\":\"CiQKBXVpcmlzEhs1MDk2MjU4MDE3MjExOTM0Njg5NTk2NDM3Njk=\",\"metadata\":{\"blockHeight\":15065683,\"txHash\":\"31647EB774FDC743E067FA459AA05CD5B0F315431CCCA54F98D877D7C26BCFC4\"}}", + // Iris3 read + "{\"operation\":\"read\",\"key\":\"AhT+5cRMA/vuTTyGA9Qqcf/L5yTzpA==\",\"value\":\"CiQKBXVpcmlzEhs1MDk2MjUxNDM1MDYwNjM3ODgwNTA2Nzg0NTU=\",\"metadata\":{\"blockHeight\":15065683,\"txHash\":\"31647EB774FDC743E067FA459AA05CD5B0F315431CCCA54F98D877D7C26BCFC4\"}}", + // osmosis1 + "{\"operation\":\"write\",\"key\":\"AhTxgpZ221d2gulE/DST1FG2f/Pin3Vvc21v\",\"value\":\"CgV1b3NtbxIFMTExODI=\",\"metadata\":{\"blockHeight\":4533309,\"txHash\":\"989E5A9E0B87B7A7ED696E965A0CCD66B97E493F30EA102101DEE2807B4C875A\"}}", + //osmosis2 + "{\"operation\":\"write\",\"key\":\"AhSgqrCihDYKl/VG5eiryfgJBa0QtHVvc21v\",\"value\":\"CgV1b3NtbxIHNzI2NTM5MA==\",\"metadata\":{\"blockHeight\":4533309,\"txHash\":\"989E5A9E0B87B7A7ED696E965A0CCD66B97E493F30EA102101DEE2807B4C875A\"}}", + // gaia1 + "{\"operation\":\"write\",\"key\":\"AhT7U2l8ff5EAeaLXC+PAenSuJDN1HVhdG9t\",\"value\":\"CgV1YXRvbRIFNTY0Mzc=\",\"metadata\":{\"blockHeight\":10625077,\"txHash\":\"03CCF5BEA0D76759CD7DB8674BC243E00DBB050A56C7A7D983F270BFF17F1DC0\"}}", + // crescent1 + "{\"operation\":\"write\",\"key\":\"AhSTNUhFAwJ0zUvxaGq9YKso7FLhp3VjcmU=\",\"value\":\"CgR1Y3JlEgw0MTQyNjkzMzA4NTQ=\",\"metadata\":{\"blockHeight\":607479,\"txHash\":\"F02890B45998C1E70628D91421021E51B81375308B95793E8F1D2DED26DCE508\"}}", + } d := bankProcessor{} + for i := 0; i < len(ops); i++ { + var tr tracelistener.TraceOperation + err := json.Unmarshal([]byte(ops[i]), &tr) + require.NoError(t, err) + // fmt.Printf("TR %#v\n", tr) + // fmt.Println("OWN", d.OwnsKey(tr.Key)) + if !d.OwnsKey(tr.Key) { + continue + } + + addrBytes, err := bankTypes.AddressFromBalancesStore(tr.Key[1:]) + require.NoError(t, err) + + hAddr := hex.EncodeToString(addrBytes) + + coin := sdk.Coin{ + Amount: sdk.NewInt(0), + } + err = gaia.MakeEncodingConfig().Marshaler.Unmarshal(tr.Value, &coin) + require.NoError(t, err) + fmt.Printf("%d) KEY=%q COINS %q %q\n", i, hAddr, coin.Denom, coin.Amount) + } +} + +func TestBankProcessorOwnsKey(t *testing.T) { + d := bankProcessor{} tests := []struct { name string prefix []byte From edef1fcfb9ce629723dd982b337dff8cb12279a4 Mon Sep 17 00:00:00 2001 From: Thomas Bruyelle Date: Mon, 30 May 2022 14:23:58 +0200 Subject: [PATCH 2/4] test: add cases for DataMarshaler.Bank --- .../processor/datamarshaler/impl_v44.go | 2 +- .../processor/datamarshaler/impl_v44_test.go | 96 +++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) diff --git a/tracelistener/processor/datamarshaler/impl_v44.go b/tracelistener/processor/datamarshaler/impl_v44.go index fb9f16ef..9ab9545d 100644 --- a/tracelistener/processor/datamarshaler/impl_v44.go +++ b/tracelistener/processor/datamarshaler/impl_v44.go @@ -72,7 +72,7 @@ func (d DataMarshaler) Bank(data tracelistener.TraceOperation) (models.BalanceRo } if err := getCodec().Unmarshal(data.Value, &coins); err != nil { - return models.BalanceRow{}, err + return models.BalanceRow{}, fmt.Errorf("cannot unmarshal balance coin: %w", err) } // Since SDK 0.44.x x/bank now deletes keys from store when the balance is 0 diff --git a/tracelistener/processor/datamarshaler/impl_v44_test.go b/tracelistener/processor/datamarshaler/impl_v44_test.go index 0cb4a51b..3631c4b3 100644 --- a/tracelistener/processor/datamarshaler/impl_v44_test.go +++ b/tracelistener/processor/datamarshaler/impl_v44_test.go @@ -5,10 +5,106 @@ package datamarshaler import ( "testing" + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/x/bank/types" clientTypes "github.com/cosmos/ibc-go/v2/modules/core/02-client/types" + models "github.com/emerishq/demeris-backend-models/tracelistener" + "github.com/emerishq/tracelistener/tracelistener" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap" ) +func TestDataMarshalerBank(t *testing.T) { + var ( + coins = sdk.Coin{ + Denom: "uatom", + Amount: sdk.NewInt(100), + } + coinBz, _ = coins.Marshal() + ) + tests := []struct { + name string + tr tracelistener.TraceOperation + expectedError string + expectedBalanceRow models.BalanceRow + }{ + { + name: "fail: key doesn't have address", + tr: tracelistener.TraceOperation{ + Operation: tracelistener.WriteOp.String(), + Key: types.BalancesPrefix, + }, + expectedError: "cannot parse address from balance store key, invalid key", + }, + { + name: "fail: key have wrong address length", + tr: tracelistener.TraceOperation{ + Operation: tracelistener.WriteOp.String(), + Key: append(types.BalancesPrefix, []byte{4, 'a', 'd', 'd'}...), + }, + expectedError: "cannot parse address from balance store key, invalid key", + }, + { + name: "ok: value is empty", + tr: tracelistener.TraceOperation{ + Operation: tracelistener.WriteOp.String(), + Key: append(types.BalancesPrefix, []byte{3, 'a', 'd', 'd'}...), + }, + expectedBalanceRow: models.BalanceRow{}, + }, + { + name: "ok: value is not a valid coin", + tr: tracelistener.TraceOperation{ + Operation: tracelistener.WriteOp.String(), + Key: append(types.BalancesPrefix, []byte{3, 'a', 'd', 'd'}...), + Value: []byte("\n$\n\x05uiris\x12\x1b509625143506063788050678455"), + }, + }, + { + name: "ok: value is not a valid coin but operation is delete", + tr: tracelistener.TraceOperation{ + Operation: tracelistener.DeleteOp.String(), + Key: append(types.BalancesPrefix, []byte{3, 'a', 'd', 'd'}...), + }, + expectedBalanceRow: models.BalanceRow{ + Address: "616464", + Amount: "0", + }, + }, + { + name: "ok", + tr: tracelistener.TraceOperation{ + Operation: tracelistener.WriteOp.String(), + Key: append(types.BalancesPrefix, []byte{3, 'a', 'd', 'd'}...), + Value: coinBz, + }, + expectedBalanceRow: models.BalanceRow{ + Address: "616464", + Amount: "100uatom", + Denom: "uatom", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + dm := NewDataMarshaler(zap.L().Sugar()) + + row, err := dm.Bank(tt.tr) + + if tt.expectedError != "" { + require.EqualError(err, tt.expectedError) + return + } + require.NoError(err) + assert.Equal(tt.expectedBalanceRow, row) + }) + } + +} + func Test_ParseIBCChainID(t *testing.T) { tt := []struct { name string From 8c0c7df41ff4d1f6dee81318ddbcab7122dc0992 Mon Sep 17 00:00:00 2001 From: Thomas Bruyelle Date: Mon, 30 May 2022 14:32:55 +0200 Subject: [PATCH 3/4] fix: invalid balance coin should trigger an error By rejecting the trace during the processing phrase, we allow an other processor to handle it, and we also prevent a database error that occurs during the insert when the unmarshaled coin contains invalid UTF-8 characters (see emerishq/demeris-backend#794). --- tracelistener/processor/datamarshaler/impl_v44.go | 4 ++-- tracelistener/processor/datamarshaler/impl_v44_test.go | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/tracelistener/processor/datamarshaler/impl_v44.go b/tracelistener/processor/datamarshaler/impl_v44.go index 9ab9545d..c4b4559c 100644 --- a/tracelistener/processor/datamarshaler/impl_v44.go +++ b/tracelistener/processor/datamarshaler/impl_v44.go @@ -79,14 +79,14 @@ func (d DataMarshaler) Bank(data tracelistener.TraceOperation) (models.BalanceRo // (picture someone who sends all their balance to another address). // To work around this issue, we don't return when coin is invalid when data.Operation is "delete", // and we set balance == 0 instead. - if !coins.IsValid() { + if err := coins.Validate(); err != nil { if data.Operation == tracelistener.DeleteOp.String() { // rawAddress still contains the length prefix, so we have to jump it by // reading 1 byte after len(addrBytes) denom := rawAddress[len(addrBytes)+1:] coins.Denom = string(denom) } else { - return models.BalanceRow{}, nil + return models.BalanceRow{}, fmt.Errorf("invalid balance coin: %w", err) } } diff --git a/tracelistener/processor/datamarshaler/impl_v44_test.go b/tracelistener/processor/datamarshaler/impl_v44_test.go index 3631c4b3..c125b250 100644 --- a/tracelistener/processor/datamarshaler/impl_v44_test.go +++ b/tracelistener/processor/datamarshaler/impl_v44_test.go @@ -46,12 +46,12 @@ func TestDataMarshalerBank(t *testing.T) { expectedError: "cannot parse address from balance store key, invalid key", }, { - name: "ok: value is empty", + name: "fail: value is empty", tr: tracelistener.TraceOperation{ Operation: tracelistener.WriteOp.String(), Key: append(types.BalancesPrefix, []byte{3, 'a', 'd', 'd'}...), }, - expectedBalanceRow: models.BalanceRow{}, + expectedError: "invalid balance coin: invalid denom: ", }, { name: "ok: value is not a valid coin", @@ -60,6 +60,7 @@ func TestDataMarshalerBank(t *testing.T) { Key: append(types.BalancesPrefix, []byte{3, 'a', 'd', 'd'}...), Value: []byte("\n$\n\x05uiris\x12\x1b509625143506063788050678455"), }, + expectedError: "invalid balance coin: invalid denom: \n\x05uiris\x12\x1b509625143506063788050678455", }, { name: "ok: value is not a valid coin but operation is delete", From a3479774b060d6c65d0b815427f229d241bd41b7 Mon Sep 17 00:00:00 2001 From: Thomas Bruyelle Date: Mon, 30 May 2022 14:35:07 +0200 Subject: [PATCH 4/4] chore: remove investigation test --- tracelistener/processor/bank_test.go | 49 ---------------------------- 1 file changed, 49 deletions(-) diff --git a/tracelistener/processor/bank_test.go b/tracelistener/processor/bank_test.go index 5f023ebc..aae9e50d 100644 --- a/tracelistener/processor/bank_test.go +++ b/tracelistener/processor/bank_test.go @@ -1,66 +1,17 @@ package processor import ( - "encoding/hex" - "encoding/json" - "fmt" "testing" "github.com/stretchr/testify/require" "go.uber.org/zap" - sdk "github.com/cosmos/cosmos-sdk/types" - bankTypes "github.com/cosmos/cosmos-sdk/x/bank/types" - gaia "github.com/cosmos/gaia/v6/app" models "github.com/emerishq/demeris-backend-models/tracelistener" "github.com/emerishq/tracelistener/tracelistener" "github.com/emerishq/tracelistener/tracelistener/config" "github.com/emerishq/tracelistener/tracelistener/processor/datamarshaler" ) -func TestFix794(t *testing.T) { - ops := []string{ - // Iris1 - "{\"operation\":\"write\",\"key\":\"AhT+5cRMA/vuTTyGA9Qqcf/L5yTzpA==\",\"value\":\"CiQKBXVpcmlzEhs1MDk2MjUxNDM1MDYwNjM3ODgwNTA2Nzg0NTU=\",\"metadata\":{\"blockHeight\":15065683,\"txHash\":\"31647EB774FDC743E067FA459AA05CD5B0F315431CCCA54F98D877D7C26BCFC4\"}}", - // Iris2 - "{\"operation\":\"write\",\"key\":\"AhT+5cRMA/vuTTyGA9Qqcf/L5yTzpA==\",\"value\":\"CiQKBXVpcmlzEhs1MDk2MjU4MDE3MjExOTM0Njg5NTk2NDM3Njk=\",\"metadata\":{\"blockHeight\":15065683,\"txHash\":\"31647EB774FDC743E067FA459AA05CD5B0F315431CCCA54F98D877D7C26BCFC4\"}}", - // Iris3 read - "{\"operation\":\"read\",\"key\":\"AhT+5cRMA/vuTTyGA9Qqcf/L5yTzpA==\",\"value\":\"CiQKBXVpcmlzEhs1MDk2MjUxNDM1MDYwNjM3ODgwNTA2Nzg0NTU=\",\"metadata\":{\"blockHeight\":15065683,\"txHash\":\"31647EB774FDC743E067FA459AA05CD5B0F315431CCCA54F98D877D7C26BCFC4\"}}", - // osmosis1 - "{\"operation\":\"write\",\"key\":\"AhTxgpZ221d2gulE/DST1FG2f/Pin3Vvc21v\",\"value\":\"CgV1b3NtbxIFMTExODI=\",\"metadata\":{\"blockHeight\":4533309,\"txHash\":\"989E5A9E0B87B7A7ED696E965A0CCD66B97E493F30EA102101DEE2807B4C875A\"}}", - //osmosis2 - "{\"operation\":\"write\",\"key\":\"AhSgqrCihDYKl/VG5eiryfgJBa0QtHVvc21v\",\"value\":\"CgV1b3NtbxIHNzI2NTM5MA==\",\"metadata\":{\"blockHeight\":4533309,\"txHash\":\"989E5A9E0B87B7A7ED696E965A0CCD66B97E493F30EA102101DEE2807B4C875A\"}}", - // gaia1 - "{\"operation\":\"write\",\"key\":\"AhT7U2l8ff5EAeaLXC+PAenSuJDN1HVhdG9t\",\"value\":\"CgV1YXRvbRIFNTY0Mzc=\",\"metadata\":{\"blockHeight\":10625077,\"txHash\":\"03CCF5BEA0D76759CD7DB8674BC243E00DBB050A56C7A7D983F270BFF17F1DC0\"}}", - // crescent1 - "{\"operation\":\"write\",\"key\":\"AhSTNUhFAwJ0zUvxaGq9YKso7FLhp3VjcmU=\",\"value\":\"CgR1Y3JlEgw0MTQyNjkzMzA4NTQ=\",\"metadata\":{\"blockHeight\":607479,\"txHash\":\"F02890B45998C1E70628D91421021E51B81375308B95793E8F1D2DED26DCE508\"}}", - } - d := bankProcessor{} - for i := 0; i < len(ops); i++ { - var tr tracelistener.TraceOperation - err := json.Unmarshal([]byte(ops[i]), &tr) - require.NoError(t, err) - // fmt.Printf("TR %#v\n", tr) - // fmt.Println("OWN", d.OwnsKey(tr.Key)) - if !d.OwnsKey(tr.Key) { - continue - } - - addrBytes, err := bankTypes.AddressFromBalancesStore(tr.Key[1:]) - require.NoError(t, err) - - hAddr := hex.EncodeToString(addrBytes) - - coin := sdk.Coin{ - Amount: sdk.NewInt(0), - } - err = gaia.MakeEncodingConfig().Marshaler.Unmarshal(tr.Value, &coin) - require.NoError(t, err) - fmt.Printf("%d) KEY=%q COINS %q %q\n", i, hAddr, coin.Denom, coin.Amount) - } - -} - func TestBankProcessorOwnsKey(t *testing.T) { d := bankProcessor{} tests := []struct {