From fcd3d8acf58d1e066435e8f0eceea4515d182294 Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Wed, 8 Feb 2023 23:26:32 +0100 Subject: [PATCH 1/8] v0.37.x:state/kvindexer: port 0.34 query fix (#77) * state/kvindexer: associate event attributes with events (#9759) Co-authored-by: Anca Zamfir Co-authored-by: Sergio Mena Co-authored-by: Romain Ruetschi Co-authored-by: Thane Thomson * Backport kvindexer fix Signed-off-by: Thane Thomson Co-authored-by: Callum Waters Co-authored-by: Thane Thomson * By event search is now default behaviour. Including fixes from PRs added to 0.34 Co-authored-by: Lasaro --- .../77-kvindexer-fix-evattribute-indexing.md | 1 + abci/example/kvstore/kvstore.go | 81 ++++++- abci/example/kvstore/persistent_kvstore.go | 4 + docs/app-dev/indexing-transactions.md | 80 ++++++- rpc/client/main_test.go | 2 + rpc/client/rpc_test.go | 30 ++- state/indexer/block/kv/kv.go | 155 ++++++++++---- state/indexer/block/kv/kv_test.go | 199 ++++++++++++++++++ state/indexer/block/kv/util.go | 116 +++++++++- state/indexer/query_range.go | 56 ++++- state/txindex/kv/kv.go | 162 ++++++++++---- state/txindex/kv/kv_test.go | 178 +++++++++++++++- state/txindex/kv/utils.go | 90 ++++++++ 13 files changed, 1051 insertions(+), 103 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/77-kvindexer-fix-evattribute-indexing.md diff --git a/.changelog/unreleased/bug-fixes/77-kvindexer-fix-evattribute-indexing.md b/.changelog/unreleased/bug-fixes/77-kvindexer-fix-evattribute-indexing.md new file mode 100644 index 00000000000..5033b7f1fbb --- /dev/null +++ b/.changelog/unreleased/bug-fixes/77-kvindexer-fix-evattribute-indexing.md @@ -0,0 +1 @@ +- `[state/kvindexer]` \#77 Fixed the default behaviour of the kvindexer to index and query attributes by events in which they occur. In 0.34.25 this was mitigated by a separated RPC flag. (@jmalicevic) \ No newline at end of file diff --git a/abci/example/kvstore/kvstore.go b/abci/example/kvstore/kvstore.go index f80e64ca244..6f11ce66e81 100644 --- a/abci/example/kvstore/kvstore.go +++ b/abci/example/kvstore/kvstore.go @@ -69,6 +69,9 @@ type Application struct { state State RetainBlocks int64 // blocks to retain after commit (via ResponseCommit.RetainHeight) txToRemove map[string]struct{} + // If true, the app will generate block events in BeginBlock. Used to test the event indexer + // Should be false by default to avoid generating too much data. + genBlockEvents bool } func NewApplication() *Application { @@ -76,6 +79,10 @@ func NewApplication() *Application { return &Application{state: state} } +func (app *Application) SetGenBlockEvents() { + app.genBlockEvents = true +} + func (app *Application) Info(req types.RequestInfo) (resInfo types.ResponseInfo) { return types.ResponseInfo{ Data: fmt.Sprintf("{\"size\":%v}", app.state.Size), @@ -116,6 +123,15 @@ func (app *Application) DeliverTx(req types.RequestDeliverTx) types.ResponseDeli {Key: "noindex_key", Value: "index is working", Index: false}, }, }, + { + Type: "app", + Attributes: []types.EventAttribute{ + {Key: "creator", Value: "Cosmoshi", Index: true}, + {Key: "key", Value: value, Index: true}, + {Key: "index_key", Value: "index is working", Index: true}, + {Key: "noindex_key", Value: "index is working", Index: false}, + }, + }, } return types.ResponseDeliverTx{Code: code.CodeTypeOK, Events: events} @@ -189,7 +205,70 @@ func (app *Application) Query(reqQuery types.RequestQuery) (resQuery types.Respo func (app *Application) BeginBlock(req types.RequestBeginBlock) types.ResponseBeginBlock { app.txToRemove = map[string]struct{}{} - return types.ResponseBeginBlock{} + response := types.ResponseBeginBlock{} + + if !app.genBlockEvents { + return response + } + + if app.state.Height%2 == 0 { + response = types.ResponseBeginBlock{ + Events: []types.Event{ + { + Type: "begin_event", + Attributes: []types.EventAttribute{ + { + Key: "foo", + Value: "100", + Index: true, + }, + { + Key: "bar", + Value: "200", + Index: true, + }, + }, + }, + { + Type: "begin_event", + Attributes: []types.EventAttribute{ + { + Key: "foo", + Value: "200", + Index: true, + }, + { + Key: "bar", + Value: "300", + Index: true, + }, + }, + }, + }, + } + } else { + response = types.ResponseBeginBlock{ + Events: []types.Event{ + { + Type: "begin_event", + Attributes: []types.EventAttribute{ + { + Key: "foo", + Value: "400", + Index: true, + }, + { + Key: "bar", + Value: "300", + Index: true, + }, + }, + }, + }, + } + } + + return response } func (app *Application) ProcessProposal( diff --git a/abci/example/kvstore/persistent_kvstore.go b/abci/example/kvstore/persistent_kvstore.go index 7ebb8880793..e06bfcff855 100644 --- a/abci/example/kvstore/persistent_kvstore.go +++ b/abci/example/kvstore/persistent_kvstore.go @@ -54,6 +54,10 @@ func NewPersistentKVStoreApplication(dbDir string) *PersistentKVStoreApplication } } +func (app *PersistentKVStoreApplication) SetGenBlockEvents() { + app.app.genBlockEvents = true +} + func (app *PersistentKVStoreApplication) SetLogger(l log.Logger) { app.logger = l } diff --git a/docs/app-dev/indexing-transactions.md b/docs/app-dev/indexing-transactions.md index 46f72b94f03..34d2cc8d977 100644 --- a/docs/app-dev/indexing-transactions.md +++ b/docs/app-dev/indexing-transactions.md @@ -15,7 +15,9 @@ the block itself is never stored. Each event contains a type and a list of attributes, which are key-value pairs denoting something about what happened during the method's execution. For more details on `Events`, see the + [ABCI](https://github.com/cometbft/cometbft/blob/main/spec/abci/abci++_basic_concepts.md#events) + documentation. An `Event` has a composite key associated with it. A `compositeKey` is @@ -34,6 +36,9 @@ would be equal to the composite key of `jack.account.number`. By default, CometBFT will index all transactions by their respective hashes and height and blocks by their height. +CometBFT allows for different events within the same height to have +equal attributes. + ## Configuration Operators can configure indexing via the `[tx_index]` section. The `indexer` @@ -67,6 +72,60 @@ for block and transaction events directly against CometBFT's RPC. However, the query syntax is limited and so this indexer type might be deprecated or removed entirely in the future. +**Implementation and data layout** + +The kv indexer stores each attribute of an event individually, by creating a composite key +with +- event type, +- attribute key, +- attribute value, +- event generator (e.g. `EndBlock` and `BeginBlock`) +- the height, and +- event counter. + For example the following events: + +``` +Type: "transfer", + Attributes: []abci.EventAttribute{ + {Key: "sender", Value: "Bob", Index: true}, + {Key: "recipient", Value: "Alice", Index: true}, + {Key: "balance", Value: "100", Index: true}, + {Key: "note", Value: "nothing", Index: true}, + }, + +``` + +``` +Type: "transfer", + Attributes: []abci.EventAttribute{ + {Key: "sender", Value: "Tom", Index: true}, + {Key: "recipient", Value: "Alice", Index: true}, + {Key: "balance", Value: "200", Index: true}, + {Key: "note", Value: "nothing", Index: true}, + }, +``` + +will be represented as follows in the store, assuming these events result from the EndBlock call for height 1: + +``` +Key value +---- event1 ------ +transferSenderBobEndBlock11 1 +transferRecipientAliceEndBlock11 1 +transferBalance100EndBlock11 1 +transferNodeNothingEndblock11 1 +---- event2 ------ +transferSenderTomEndBlock12 1 +transferRecepientAliceEndBlock12 1 +transferBalance200EndBlock12 1 +transferNodeNothingEndblock12 1 + +``` +The event number is a local variable kept by the indexer and incremented when a new event is processed. +It is an `int64` variable and has no other semantics besides being used to associate attributes belonging to the same events within a height. +This variable is not atomically incremented as event indexing is deterministic. **Should this ever change**, the event id generation +will be broken. + #### PostgreSQL The `psql` indexer type allows an operator to enable block and transaction event @@ -122,10 +181,10 @@ func (app *KVStoreApplication) DeliverTx(req types.RequestDeliverTx) types.Resul { Type: "transfer", Attributes: []abci.EventAttribute{ - {Key: []byte("sender"), Value: []byte("Bob"), Index: true}, - {Key: []byte("recipient"), Value: []byte("Alice"), Index: true}, - {Key: []byte("balance"), Value: []byte("100"), Index: true}, - {Key: []byte("note"), Value: []byte("nothing"), Index: true}, + {Key: "sender ", Value: "Bob ", Index: true}, + {Key: "recipient ", Value: "Alice ", Index: true}, + {Key: "balance ", Value: "100 ", Index: true}, + {Key: "note ", Value: "nothing ", Index: true}, }, }, } @@ -168,7 +227,7 @@ a query to `/subscribe` RPC endpoint. Check out [API docs](https://docs.cometbft.com/main/rpc/#subscribe) for more information on query syntax and other options. -## Querying Blocks Events +## Querying Block Events You can query for a paginated set of blocks by their events by calling the `/block_search` RPC endpoint: @@ -177,5 +236,12 @@ You can query for a paginated set of blocks by their events by calling the curl "localhost:26657/block_search?query=\"block.height > 10 AND val_set.num_changed > 0\"" ``` -Check out [API docs](https://docs.cometbft.com/main/rpc/#/Info/block_search) -for more information on query syntax and other options. + +Storing the event sequence was introduced in CometBFT 0.34.26. Before that, up until Tendermint Core 0.34.26, +the event sequence was not stored in the kvstore and events were stored only by height. That means that queries +returned blocks and transactions whose event attributes match within the height but can match across different +events on that height. +This behavior was fixed with CometBFT 0.34.26+. However, if the data was indexed with earlier versions of +Tendermint Core and not re-indexed, that data will be queried as if all the attributes within a height +occurred within the same event. + diff --git a/rpc/client/main_test.go b/rpc/client/main_test.go index 93733537af7..a72a2797978 100644 --- a/rpc/client/main_test.go +++ b/rpc/client/main_test.go @@ -19,6 +19,8 @@ func TestMain(m *testing.M) { } app := kvstore.NewPersistentKVStoreApplication(dir) + // If testing block event generation + // app.SetGenBlockEvents() needs to be called here node = rpctest.StartTendermint(app) code := m.Run() diff --git a/rpc/client/rpc_test.go b/rpc/client/rpc_test.go index 7aacc54c828..b371596f81f 100644 --- a/rpc/client/rpc_test.go +++ b/rpc/client/rpc_test.go @@ -516,6 +516,27 @@ func TestTxSearchWithTimeout(t *testing.T) { require.Greater(t, len(result.Txs), 0, "expected a lot of transactions") } +// This test does nothing if we do not call app.SetGenBlockEvents() within main_test.go +// It will nevertheless pass as there are no events being generated. +func TestBlockSearch(t *testing.T) { + c := getHTTPClient() + + // first we broadcast a few txs + for i := 0; i < 10; i++ { + _, _, tx := MakeTxKV() + + _, err := c.BroadcastTxCommit(context.Background(), tx) + require.NoError(t, err) + } + require.NoError(t, client.WaitForHeight(c, 5, nil)) + // This cannot test match_events as it calls the client BlockSearch function directly + // It is the RPC request handler that processes the match_event + result, err := c.BlockSearch(context.Background(), "begin_event.foo = 100 AND begin_event.bar = 300", nil, nil, "asc") + require.NoError(t, err) + blockCount := len(result.Blocks) + require.Equal(t, blockCount, 0) + +} func TestTxSearch(t *testing.T) { c := getHTTPClient() @@ -536,8 +557,7 @@ func TestTxSearch(t *testing.T) { find := result.Txs[len(result.Txs)-1] anotherTxHash := types.Tx("a different tx").Hash() - for i, c := range GetClients() { - t.Logf("client %d", i) + for _, c := range GetClients() { // now we query for the tx. result, err := c.TxSearch(context.Background(), fmt.Sprintf("tx.hash='%v'", find.Hash), true, nil, nil, "asc") @@ -616,16 +636,17 @@ func TestTxSearch(t *testing.T) { pages = int(math.Ceil(float64(txCount) / float64(perPage))) ) + totalTx := 0 for page := 1; page <= pages; page++ { page := page - result, err := c.TxSearch(context.Background(), "tx.height >= 1", false, &page, &perPage, "asc") + result, err := c.TxSearch(context.Background(), "tx.height >= 1", true, &page, &perPage, "asc") require.NoError(t, err) if page < pages { require.Len(t, result.Txs, perPage) } else { require.LessOrEqual(t, len(result.Txs), perPage) } - require.Equal(t, txCount, result.TotalCount) + totalTx = totalTx + len(result.Txs) for _, tx := range result.Txs { require.False(t, seen[tx.Height], "Found duplicate height %v in page %v", tx.Height, page) @@ -635,6 +656,7 @@ func TestTxSearch(t *testing.T) { maxHeight = tx.Height } } + require.Equal(t, txCount, totalTx) require.Len(t, seen, txCount) } } diff --git a/state/indexer/block/kv/kv.go b/state/indexer/block/kv/kv.go index b9b51cd4f53..709536cdff4 100644 --- a/state/indexer/block/kv/kv.go +++ b/state/indexer/block/kv/kv.go @@ -1,6 +1,7 @@ package kv import ( + "bytes" "context" "errors" "fmt" @@ -26,6 +27,10 @@ var _ indexer.BlockIndexer = (*BlockerIndexer)(nil) // such that matching search criteria returns the respective block height(s). type BlockerIndexer struct { store dbm.DB + + // Add unique event identifier to use when querying + // Matching will be done both on height AND eventSeq + eventSeq int64 } func New(store dbm.DB) *BlockerIndexer { @@ -94,18 +99,39 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, } conditions := q.Syntax() + //conditions, err := q.Conditions() + if err != nil { + return nil, fmt.Errorf("failed to parse query conditions: %w", err) + } + // conditions to skip because they're handled before "everything else" + skipIndexes := make([]int, 0) + + var ok bool - // If there is an exact height query, return the result immediately - // (if it exists). - height, ok := lookForHeight(conditions) - if ok { - ok, err := idx.Has(height) + var heightInfo HeightInfo + // If we are not matching events and block.height occurs more than once, the later value will + // overwrite the first one. + conditions, heightInfo, ok = dedupHeight(conditions) + + // Extract ranges. If both upper and lower bounds exist, it's better to get + // them in order as to not iterate over kvs that are not within range. + ranges, rangeIndexes, heightRange := indexer.LookForRangesWithHeight(conditions) + heightInfo.heightRange = heightRange + + // If we have additional constraints and want to query per event + // attributes, we cannot simply return all blocks for a height. + // But we remember the height we want to find and forward it to + // match(). If we only have the height constraint + // in the query (the second part of the ||), we don't need to query + // per event conditions and return all events within the height range. + if ok && heightInfo.onlyHeightEq { + ok, err := idx.Has(heightInfo.height) if err != nil { return nil, err } if ok { - return []int64{height}, nil + return []int64{heightInfo.height}, nil } return results, nil @@ -113,24 +139,34 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, var heightsInitialized bool filteredHeights := make(map[string][]byte) + if heightInfo.heightEqIdx != -1 { + skipIndexes = append(skipIndexes, heightInfo.heightEqIdx) + } - // conditions to skip because they're handled before "everything else" - skipIndexes := make([]int, 0) - - // Extract ranges. If both upper and lower bounds exist, it's better to get - // them in order as to not iterate over kvs that are not within range. - ranges, rangeIndexes := indexer.LookForRanges(conditions) if len(ranges) > 0 { skipIndexes = append(skipIndexes, rangeIndexes...) for _, qr := range ranges { + // If we have a query range over height and want to still look for + // specific event values we do not want to simply return all + // blocks in this height range. We remember the height range info + // and pass it on to match() to take into account when processing events. + if qr.Key == types.BlockHeightKey && !heightInfo.onlyHeightRange { + // If the query contains ranges other than the height then we need to treat the height + // range when querying the conditions of the other range. + // Otherwise we can just return all the blocks within the height range (as there is no + // additional constraint on events) + + continue + + } prefix, err := orderedcode.Append(nil, qr.Key) if err != nil { return nil, fmt.Errorf("failed to create prefix key: %w", err) } if !heightsInitialized { - filteredHeights, err = idx.matchRange(ctx, qr, prefix, filteredHeights, true) + filteredHeights, err = idx.matchRange(ctx, qr, prefix, filteredHeights, true, heightInfo) if err != nil { return nil, err } @@ -143,7 +179,7 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, break } } else { - filteredHeights, err = idx.matchRange(ctx, qr, prefix, filteredHeights, false) + filteredHeights, err = idx.matchRange(ctx, qr, prefix, filteredHeights, false, heightInfo) if err != nil { return nil, err } @@ -163,7 +199,7 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, } if !heightsInitialized { - filteredHeights, err = idx.match(ctx, c, startKey, filteredHeights, true) + filteredHeights, err = idx.match(ctx, c, startKey, filteredHeights, true, heightInfo) if err != nil { return nil, err } @@ -176,7 +212,7 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, break } } else { - filteredHeights, err = idx.match(ctx, c, startKey, filteredHeights, false) + filteredHeights, err = idx.match(ctx, c, startKey, filteredHeights, false, heightInfo) if err != nil { return nil, err } @@ -185,6 +221,7 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, // fetch matching heights results = make([]int64, 0, len(filteredHeights)) + resultMap := make(map[int64]struct{}) for _, hBz := range filteredHeights { h := int64FromBytes(hBz) @@ -193,7 +230,10 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, return nil, err } if ok { - results = append(results, h) + if _, ok := resultMap[h]; !ok { + resultMap[h] = struct{}{} + results = append(results, h) + } } select { @@ -221,6 +261,7 @@ func (idx *BlockerIndexer) matchRange( startKey []byte, filteredHeights map[string][]byte, firstRun bool, + heightInfo HeightInfo, ) (map[string][]byte, error) { // A previous match was attempted but resulted in no matches, so we return // no matches (assuming AND operand). @@ -229,8 +270,6 @@ func (idx *BlockerIndexer) matchRange( } tmpHeights := make(map[string][]byte) - lowerBound := qr.LowerBoundValue() - upperBound := qr.UpperBoundValue() it, err := dbm.IteratePrefix(idx.store, startKey) if err != nil { @@ -261,17 +300,14 @@ LOOP: continue LOOP } - include := true - if lowerBound != nil && v < lowerBound.(int64) { - include = false - } - - if upperBound != nil && v > upperBound.(int64) { - include = false + if qr.Key != types.BlockHeightKey { + keyHeight, err := parseHeightFromEventKey(it.Key()) + if err != nil || !checkHeightConditions(heightInfo, keyHeight) { + continue LOOP + } } - - if include { - tmpHeights[string(it.Value())] = it.Value() + if checkBounds(qr, v) { + idx.setTmpHeights(tmpHeights, it) } } @@ -300,8 +336,12 @@ LOOP: // Remove/reduce matches in filteredHashes that were not found in this // match (tmpHashes). - for k := range filteredHeights { - if tmpHeights[k] == nil { + for k, v := range filteredHeights { + tmpHeight := tmpHeights[k] + + // Check whether in this iteration we have not found an overlapping height (tmpHeight == nil) + // or whether the events in which the attributed occurred do not match (first part of the condition) + if tmpHeight == nil || !bytes.Equal(tmpHeight, v) { delete(filteredHeights, k) select { @@ -316,6 +356,30 @@ LOOP: return filteredHeights, nil } +func (idx *BlockerIndexer) setTmpHeights(tmpHeights map[string][]byte, it dbm.Iterator) { + // If we return attributes that occur within the same events, then store the event sequence in the + // result map as well + eventSeq, _ := parseEventSeqFromEventKey(it.Key()) + retVal := it.Value() + tmpHeights[string(retVal)+strconv.FormatInt(eventSeq, 10)] = it.Value() + +} + +func checkBounds(ranges indexer.QueryRange, v int64) bool { + include := true + lowerBound := ranges.LowerBoundValue() + upperBound := ranges.UpperBoundValue() + if lowerBound != nil && v < lowerBound.(int64) { + include = false + } + + if upperBound != nil && v > upperBound.(int64) { + include = false + } + + return include +} + // match returns all matching heights that meet a given query condition and start // key. An already filtered result (filteredHeights) is provided such that any // non-intersecting matches are removed. @@ -328,6 +392,7 @@ func (idx *BlockerIndexer) match( startKeyBz []byte, filteredHeights map[string][]byte, firstRun bool, + heightInfo HeightInfo, ) (map[string][]byte, error) { // A previous match was attempted but resulted in no matches, so we return // no matches (assuming AND operand). @@ -346,7 +411,13 @@ func (idx *BlockerIndexer) match( defer it.Close() for ; it.Valid(); it.Next() { - tmpHeights[string(it.Value())] = it.Value() + + keyHeight, err := parseHeightFromEventKey(it.Key()) + if err != nil || !checkHeightConditions(heightInfo, keyHeight) { + continue + } + + idx.setTmpHeights(tmpHeights, it) if err := ctx.Err(); err != nil { break @@ -370,7 +441,11 @@ func (idx *BlockerIndexer) match( defer it.Close() for ; it.Valid(); it.Next() { - tmpHeights[string(it.Value())] = it.Value() + keyHeight, err := parseHeightFromEventKey(it.Key()) + if err != nil || !checkHeightConditions(heightInfo, keyHeight) { + continue + } + idx.setTmpHeights(tmpHeights, it) select { case <-ctx.Done(): @@ -403,7 +478,11 @@ func (idx *BlockerIndexer) match( } if strings.Contains(eventValue, c.Arg.Value()) { - tmpHeights[string(it.Value())] = it.Value() + keyHeight, err := parseHeightFromEventKey(it.Key()) + if err != nil || !checkHeightConditions(heightInfo, keyHeight) { + continue + } + idx.setTmpHeights(tmpHeights, it) } select { @@ -434,8 +513,9 @@ func (idx *BlockerIndexer) match( // Remove/reduce matches in filteredHeights that were not found in this // match (tmpHeights). - for k := range filteredHeights { - if tmpHeights[k] == nil { + for k, v := range filteredHeights { + tmpHeight := tmpHeights[k] + if tmpHeight == nil || !bytes.Equal(tmpHeight, v) { delete(filteredHeights, k) select { @@ -454,6 +534,7 @@ func (idx *BlockerIndexer) indexEvents(batch dbm.Batch, events []abci.Event, typ heightBz := int64ToBytes(height) for _, event := range events { + idx.eventSeq = idx.eventSeq + 1 // only index events with a non-empty type if len(event.Type) == 0 { continue @@ -471,7 +552,7 @@ func (idx *BlockerIndexer) indexEvents(batch dbm.Batch, events []abci.Event, typ } if attr.GetIndex() { - key, err := eventKey(compositeKey, typ, attr.Value, height) + key, err := eventKey(compositeKey, typ, attr.Value, height, idx.eventSeq) if err != nil { return fmt.Errorf("failed to create block index key: %w", err) } diff --git a/state/indexer/block/kv/kv_test.go b/state/indexer/block/kv/kv_test.go index 7e5b8941964..e58b8f9302b 100644 --- a/state/indexer/block/kv/kv_test.go +++ b/state/indexer/block/kv/kv_test.go @@ -122,6 +122,14 @@ func TestBlockIndexer(t *testing.T) { q: query.MustCompile(`block.height > 2 AND end_event.foo <= 8`), results: []int64{4, 6, 8}, }, + "end_event.foo > 100": { + q: query.MustParse("end_event.foo > 100"), + results: []int64{}, + }, + "block.height >= 2 AND end_event.foo < 8": { + q: query.MustParse("block.height >= 2 AND end_event.foo < 8"), + results: []int64{2, 4, 6}, + }, "begin_event.proposer CONTAINS 'FFFFFFF'": { q: query.MustCompile(`begin_event.proposer CONTAINS 'FFFFFFF'`), results: []int64{}, @@ -130,6 +138,197 @@ func TestBlockIndexer(t *testing.T) { q: query.MustCompile(`begin_event.proposer CONTAINS 'FCAA001'`), results: []int64{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}, }, + "end_event.foo CONTAINS '1'": { + q: query.MustParse("end_event.foo CONTAINS '1'"), + results: []int64{1, 10}, + }, + } + + for name, tc := range testCases { + tc := tc + t.Run(name, func(t *testing.T) { + results, err := indexer.Search(context.Background(), tc.q) + require.NoError(t, err) + require.Equal(t, tc.results, results) + }) + } +} + +func TestBlockIndexerMulti(t *testing.T) { + store := db.NewPrefixDB(db.NewMemDB(), []byte("block_events")) + indexer := blockidxkv.New(store) + + require.NoError(t, indexer.Index(types.EventDataNewBlockHeader{ + Header: types.Header{Height: 1}, + ResultBeginBlock: abci.ResponseBeginBlock{ + Events: []abci.Event{}, + }, + ResultEndBlock: abci.ResponseEndBlock{ + Events: []abci.Event{ + { + Type: "end_event", + Attributes: []abci.EventAttribute{ + { + Key: "foo", + Value: "100", + Index: true, + }, + { + Key: "bar", + Value: "200", + Index: true, + }, + }, + }, + { + Type: "end_event", + Attributes: []abci.EventAttribute{ + { + Key: "foo", + Value: "300", + Index: true, + }, + { + Key: "bar", + Value: "500", + Index: true, + }, + }, + }, + }, + }, + })) + + require.NoError(t, indexer.Index(types.EventDataNewBlockHeader{ + Header: types.Header{Height: 2}, + ResultBeginBlock: abci.ResponseBeginBlock{ + Events: []abci.Event{}, + }, + ResultEndBlock: abci.ResponseEndBlock{ + Events: []abci.Event{ + { + Type: "end_event", + Attributes: []abci.EventAttribute{ + { + Key: "foo", + Value: "100", + Index: true, + }, + { + Key: "bar", + Value: "200", + Index: true, + }, + }, + }, + { + Type: "end_event", + Attributes: []abci.EventAttribute{ + { + Key: "foo", + Value: "300", + Index: true, + }, + { + Key: "bar", + Value: "400", + Index: true, + }, + }, + }, + }, + }, + })) + + testCases := map[string]struct { + q *query.Query + results []int64 + }{ + + "query return all events from a height - exact": { + q: query.MustParse("block.height = 1"), + results: []int64{1}, + }, + "query return all events from a height - exact - no match.events": { + q: query.MustParse("block.height = 1"), + results: []int64{1}, + }, + "query return all events from a height - exact (deduplicate height)": { + q: query.MustParse("block.height = 1 AND block.height = 2"), + results: []int64{1}, + }, + "query return all events from a height - exact (deduplicate height) - no match.events": { + q: query.MustParse("block.height = 1 AND block.height = 2"), + results: []int64{1}, + }, + "query return all events from a height - range": { + q: query.MustParse("block.height < 2 AND block.height > 0 AND block.height > 0"), + results: []int64{1}, + }, + "query return all events from a height - range - no match.events": { + q: query.MustParse("block.height < 2 AND block.height > 0 AND block.height > 0"), + results: []int64{1}, + }, + "query return all events from a height - range 2": { + q: query.MustParse("block.height < 3 AND block.height < 2 AND block.height > 0 AND block.height > 0"), + results: []int64{1}, + }, + "query return all events from a height - range 3": { + q: query.MustParse("block.height < 1 AND block.height > 1"), + results: []int64{}, + }, + "query matches fields from same event": { + q: query.MustParse("end_event.bar < 300 AND end_event.foo = 100 AND block.height > 0 AND block.height <= 2"), + results: []int64{1, 2}, + }, + "query matches fields from same event - no match.events": { + q: query.MustParse("end_event.bar < 300 AND end_event.foo = 100 AND block.height > 0 AND block.height <= 2"), + results: []int64{1, 2}, + }, + "query matches fields from multiple events": { + q: query.MustParse("end_event.foo = 100 AND end_event.bar = 400 AND block.height = 2"), + results: []int64{}, + }, + "query matches fields from multiple events 2": { + q: query.MustParse("end_event.foo = 100 AND end_event.bar > 200 AND block.height > 0 AND block.height < 3"), + results: []int64{}, + }, + "query matches fields from multiple events allowed": { + q: query.MustParse("end_event.foo = 100 AND end_event.bar = 400"), + results: []int64{}, + }, + "query matches fields from all events whose attribute is within range": { + q: query.MustParse("block.height = 2 AND end_event.foo < 300"), + results: []int64{2}, + }, + "deduplication test - match.events multiple 2": { + q: query.MustParse("end_event.foo = 100 AND end_event.bar = 400 AND block.height = 2"), + results: []int64{}, + }, + "query using CONTAINS matches fields from all events whose attribute is within range": { + q: query.MustParse("block.height = 2 AND end_event.foo CONTAINS '30'"), + results: []int64{2}, + }, + "query matches all fields from multiple events": { + q: query.MustParse("end_event.bar > 100 AND end_event.bar <= 500"), + results: []int64{1, 2}, + }, + "query matches all fields from multiple events - no match.events": { + q: query.MustParse("end_event.bar > 100 AND end_event.bar <= 500"), + results: []int64{1, 2}, + }, + "query with height range and height equality - should ignore equality": { + q: query.MustParse("block.height = 2 AND end_event.foo >= 100 AND block.height < 2"), + results: []int64{1}, + }, + "query with non-existent field": { + q: query.MustParse("end_event.baz = 100"), + results: []int64{}, + }, + "query with non-existent field ": { + q: query.MustParse("end_event.baz = 100"), + results: []int64{}, + }, } for name, tc := range testCases { diff --git a/state/indexer/block/kv/util.go b/state/indexer/block/kv/util.go index ce6508befdc..90365ff6e55 100644 --- a/state/indexer/block/kv/util.go +++ b/state/indexer/block/kv/util.go @@ -8,9 +8,18 @@ import ( "github.com/google/orderedcode" "github.com/cometbft/cometbft/libs/pubsub/query/syntax" + "github.com/cometbft/cometbft/state/indexer" "github.com/cometbft/cometbft/types" ) +type HeightInfo struct { + heightRange indexer.QueryRange + height int64 + heightEqIdx int + onlyHeightRange bool + onlyHeightEq bool +} + func intInSlice(a int, list []int) bool { for _, b := range list { if b == a { @@ -40,13 +49,14 @@ func heightKey(height int64) ([]byte, error) { ) } -func eventKey(compositeKey, typ, eventValue string, height int64) ([]byte, error) { +func eventKey(compositeKey, typ, eventValue string, height int64, eventSeq int64) ([]byte, error) { return orderedcode.Append( nil, compositeKey, eventValue, height, typ, + eventSeq, ) } @@ -74,18 +84,114 @@ func parseValueFromEventKey(key []byte) (string, error) { height int64 ) - remaining, err := orderedcode.Parse(string(key), &compositeKey, &eventValue, &height, &typ) + _, err := orderedcode.Parse(string(key), &compositeKey, &eventValue, &height, &typ) if err != nil { return "", fmt.Errorf("failed to parse event key: %w", err) } + return eventValue, nil +} + +func parseHeightFromEventKey(key []byte) (int64, error) { + var ( + compositeKey, typ, eventValue string + height int64 + ) + + _, err := orderedcode.Parse(string(key), &compositeKey, &eventValue, &height, &typ) + if err != nil { + return -1, fmt.Errorf("failed to parse event key: %w", err) + } + + return height, nil +} + +func parseEventSeqFromEventKey(key []byte) (int64, error) { + var ( + compositeKey, typ, eventValue string + height int64 + eventSeq int64 + ) + + remaining, err := orderedcode.Parse(string(key), &compositeKey, &eventValue, &height, &typ) + if err != nil { + return 0, fmt.Errorf("failed to parse event key: %w", err) + } + + // This is done to support previous versions that did not have event sequence in their key if len(remaining) != 0 { - return "", fmt.Errorf("unexpected remainder in key: %s", remaining) + remaining, err = orderedcode.Parse(remaining, &eventSeq) + if err != nil { + return 0, fmt.Errorf("failed to parse event key: %w", err) + } + if len(remaining) != 0 { + return 0, fmt.Errorf("unexpected remainder in key: %s", remaining) + } } - return eventValue, nil + return eventSeq, nil +} + +// Remove all occurrences of height equality queries except one. While we are traversing the conditions, check whether the only condition in +// addition to match events is the height equality or height range query. At the same time, if we do have a height range condition +// ignore the height equality condition. If a height equality exists, place the condition index in the query and the desired height +// into the heightInfo struct +func dedupHeight(conditions []query.Condition) (dedupConditions []query.Condition, heightInfo HeightInfo, found bool) { + heightInfo.heightEqIdx = -1 + heightRangeExists := false + var heightCondition []query.Condition + heightInfo.onlyHeightEq = true + heightInfo.onlyHeightRange = true + for _, c := range conditions { + if c.CompositeKey == types.BlockHeightKey { + if c.Op == query.OpEqual { + if found || heightRangeExists { + continue + } else { + heightCondition = append(heightCondition, c) + heightInfo.height = c.Operand.(int64) + found = true + } + } else { + heightInfo.onlyHeightEq = false + heightRangeExists = true + dedupConditions = append(dedupConditions, c) + } + } else { + heightInfo.onlyHeightRange = false + heightInfo.onlyHeightEq = false + dedupConditions = append(dedupConditions, c) + } + } + if !heightRangeExists && len(heightCondition) != 0 { + heightInfo.heightEqIdx = len(dedupConditions) + heightInfo.onlyHeightRange = false + dedupConditions = append(dedupConditions, heightCondition...) + } else { + // If we found a range make sure we set the hegiht idx to -1 as the height equality + // will be removed + heightInfo.heightEqIdx = -1 + heightInfo.height = 0 + heightInfo.onlyHeightEq = false + found = false + } + return dedupConditions, heightInfo, found +} + +func checkHeightConditions(heightInfo HeightInfo, keyHeight int64) bool { + if heightInfo.heightRange.Key != "" { + if !checkBounds(heightInfo.heightRange, keyHeight) { + return false + } + } else { + if heightInfo.height != 0 && keyHeight != heightInfo.height { + return false + } + } + return true } +//nolint:unused,deadcode func lookForHeight(conditions []syntax.Condition) (int64, bool) { for _, c := range conditions { if c.Tag == types.BlockHeightKey && c.Op == syntax.TEq { @@ -93,5 +199,5 @@ func lookForHeight(conditions []syntax.Condition) (int64, bool) { } } - return 0, false + return 0, false, -1 } diff --git a/state/indexer/query_range.go b/state/indexer/query_range.go index c41ac2e1058..881fd4b8770 100644 --- a/state/indexer/query_range.go +++ b/state/indexer/query_range.go @@ -4,6 +4,7 @@ import ( "time" "github.com/cometbft/cometbft/libs/pubsub/query/syntax" + "github.com/cometbft/cometbft/types" ) // QueryRanges defines a mapping between a composite event key and a QueryRange. @@ -75,8 +76,61 @@ func (qr QueryRange) UpperBoundValue() interface{} { } } -// LookForRanges returns a mapping of QueryRanges and the matching indexes in +// LookForRangesWithHeight returns a mapping of QueryRanges and the matching indexes in // the provided query conditions. +func LookForRangesWithHeight(conditions []syntax.Condition) (queryRange QueryRanges, indexes []int, heightRange QueryRange) { + queryRange = make(QueryRanges) + for i, c := range conditions { + heightKey := false + if IsRangeOperation(c.Op) { + r, ok := queryRange[c.CompositeKey] + if !ok { + r = QueryRange{Key: c.CompositeKey} + if c.CompositeKey == types.BlockHeightKey || c.CompositeKey == types.TxHeightKey { + heightRange = QueryRange{Key: c.CompositeKey} + heightKey = true + } + } + + switch c.Op { + case query.OpGreater: + if heightKey { + heightRange.LowerBound = c.Operand + } + r.LowerBound = c.Operand + + case query.OpGreaterEqual: + r.IncludeLowerBound = true + r.LowerBound = c.Operand + if heightKey { + heightRange.IncludeLowerBound = true + heightRange.LowerBound = c.Operand + } + + case query.OpLess: + r.UpperBound = c.Operand + if heightKey { + heightRange.UpperBound = c.Operand + } + + case query.OpLessEqual: + r.IncludeUpperBound = true + r.UpperBound = c.Operand + if heightKey { + heightRange.IncludeUpperBound = true + heightRange.UpperBound = c.Operand + } + } + + queryRange[c.CompositeKey] = r + indexes = append(indexes, i) + } + } + + return queryRange, indexes, heightRange +} + +// Deprecated: This function is not used anymore and will be replaced with LookForRangesWithHeight func LookForRanges(conditions []syntax.Condition) (ranges QueryRanges, indexes []int) { ranges = make(QueryRanges) for i, c := range conditions { diff --git a/state/txindex/kv/kv.go b/state/txindex/kv/kv.go index c4240307032..b2e6a06846b 100644 --- a/state/txindex/kv/kv.go +++ b/state/txindex/kv/kv.go @@ -29,6 +29,8 @@ var _ txindex.TxIndexer = (*TxIndex)(nil) // TxIndex is the simplest possible indexer, backed by key-value storage (levelDB). type TxIndex struct { store dbm.DB + // Number the events in the event list + eventSeq int64 } // NewTxIndex creates new KV indexer. @@ -154,6 +156,7 @@ func (txi *TxIndex) Index(result *abci.TxResult) error { func (txi *TxIndex) indexEvents(result *abci.TxResult, hash []byte, store dbm.Batch) error { for _, event := range result.Result.Events { + txi.eventSeq = txi.eventSeq + 1 // only index events with a non-empty type if len(event.Type) == 0 { continue @@ -171,7 +174,7 @@ func (txi *TxIndex) indexEvents(result *abci.TxResult, hash []byte, store dbm.Ba return fmt.Errorf("event type and attribute key \"%s\" is reserved; please use a different key", compositeTag) } if attr.GetIndex() { - err := store.Set(keyForEvent(compositeTag, attr.Value, result), hash) + err := store.Set(keyForEvent(compositeTag, attr.Value, result, txi.eventSeq), hash) if err != nil { return err } @@ -225,17 +228,35 @@ func (txi *TxIndex) Search(ctx context.Context, q *query.Query) ([]*abci.TxResul // conditions to skip because they're handled before "everything else" skipIndexes := make([]int, 0) + var heightInfo HeightInfo + + // If we are not matching events and tx.height = 3 occurs more than once, the later value will + // overwrite the first one. + conditions, heightInfo = dedupHeight(conditions) + + if !heightInfo.onlyHeightEq { + skipIndexes = append(skipIndexes, heightInfo.heightEqIdx) + } // extract ranges // if both upper and lower bounds exist, it's better to get them in order not // no iterate over kvs that are not within range. - ranges, rangeIndexes := indexer.LookForRanges(conditions) + ranges, rangeIndexes, heightRange := indexer.LookForRangesWithHeight(conditions) + heightInfo.heightRange = heightRange if len(ranges) > 0 { skipIndexes = append(skipIndexes, rangeIndexes...) for _, qr := range ranges { + + // If we have a query range over height and want to still look for + // specific event values we do not want to simply return all + // transactios in this height range. We remember the height range info + // and pass it on to match() to take into account when processing events. + if qr.Key == types.TxHeightKey && !heightInfo.onlyHeightRange { + continue + } if !hashesInitialized { - filteredHashes = txi.matchRange(ctx, qr, startKey(qr.Key), filteredHashes, true) + filteredHashes = txi.matchRange(ctx, qr, startKey(qr.Key), filteredHashes, true, heightInfo) hashesInitialized = true // Ignore any remaining conditions if the first condition resulted @@ -244,13 +265,12 @@ func (txi *TxIndex) Search(ctx context.Context, q *query.Query) ([]*abci.TxResul break } } else { - filteredHashes = txi.matchRange(ctx, qr, startKey(qr.Key), filteredHashes, false) + filteredHashes = txi.matchRange(ctx, qr, startKey(qr.Key), filteredHashes, false, heightInfo) } } } // if there is a height condition ("tx.height=3"), extract it - height := lookForHeight(conditions) // for all other conditions for i, c := range conditions { @@ -259,7 +279,7 @@ func (txi *TxIndex) Search(ctx context.Context, q *query.Query) ([]*abci.TxResul } if !hashesInitialized { - filteredHashes = txi.match(ctx, c, startKeyForCondition(c, height), filteredHashes, true) + filteredHashes = txi.match(ctx, c, startKeyForCondition(c, heightInfo.height), filteredHashes, true, heightInfo) hashesInitialized = true // Ignore any remaining conditions if the first condition resulted @@ -268,19 +288,24 @@ func (txi *TxIndex) Search(ctx context.Context, q *query.Query) ([]*abci.TxResul break } } else { - filteredHashes = txi.match(ctx, c, startKeyForCondition(c, height), filteredHashes, false) + filteredHashes = txi.match(ctx, c, startKeyForCondition(c, heightInfo.height), filteredHashes, false, heightInfo) } } results := make([]*abci.TxResult, 0, len(filteredHashes)) + resultMap := make(map[string]struct{}) RESULTS_LOOP: for _, h := range filteredHashes { + res, err := txi.Get(h) if err != nil { return nil, fmt.Errorf("failed to get Tx{%X}: %w", h, err) } - results = append(results, res) - + hashString := string(h) + if _, ok := resultMap[hashString]; !ok { + resultMap[hashString] = struct{}{} + results = append(results, res) + } // Potentially exit early. select { case <-ctx.Done(): @@ -302,14 +327,9 @@ func lookForHash(conditions []syntax.Condition) (hash []byte, ok bool, err error return } -// lookForHeight returns a height if there is an "height=X" condition. -func lookForHeight(conditions []syntax.Condition) (height int64) { - for _, c := range conditions { - if c.Tag == types.TxHeightKey && c.Op == syntax.TEq { - return int64(c.Arg.Number()) - } - } - return 0 +func (txi *TxIndex) setTmpHashes(tmpHeights map[string][]byte, it dbm.Iterator) { + eventSeq := extractEventSeqFromKey(it.Key()) + tmpHeights[string(it.Value())+eventSeq] = it.Value() } // match returns all matching txs by hash that meet a given condition and start @@ -323,6 +343,7 @@ func (txi *TxIndex) match( startKeyBz []byte, filteredHashes map[string][]byte, firstRun bool, + heightInfo HeightInfo, ) map[string][]byte { // A previous match was attempted but resulted in no matches, so we return // no matches (assuming AND operand). @@ -342,8 +363,15 @@ func (txi *TxIndex) match( EQ_LOOP: for ; it.Valid(); it.Next() { - tmpHashes[string(it.Value())] = it.Value() + // If we have a height range in a query, we need only transactions + // for this height + keyHeight, err := extractHeightFromKey(it.Key()) + if err != nil || !checkHeightConditions(heightInfo, keyHeight) { + continue + } + + txi.setTmpHashes(tmpHashes, it) // Potentially exit early. select { case <-ctx.Done(): @@ -366,7 +394,11 @@ func (txi *TxIndex) match( EXISTS_LOOP: for ; it.Valid(); it.Next() { - tmpHashes[string(it.Value())] = it.Value() + keyHeight, err := extractHeightFromKey(it.Key()) + if err != nil || !checkHeightConditions(heightInfo, keyHeight) { + continue + } + txi.setTmpHashes(tmpHashes, it) // Potentially exit early. select { @@ -394,8 +426,13 @@ func (txi *TxIndex) match( if !isTagKey(it.Key()) { continue } + if strings.Contains(extractValueFromKey(it.Key()), c.Arg.Value()) { - tmpHashes[string(it.Value())] = it.Value() + keyHeight, err := extractHeightFromKey(it.Key()) + if err != nil || !checkHeightConditions(heightInfo, keyHeight) { + continue + } + txi.setTmpHashes(tmpHashes, it) } // Potentially exit early. @@ -426,8 +463,9 @@ func (txi *TxIndex) match( // Remove/reduce matches in filteredHashes that were not found in this // match (tmpHashes). REMOVE_LOOP: - for k := range filteredHashes { - if tmpHashes[k] == nil { + for k, v := range filteredHashes { + tmpHash := tmpHashes[k] + if tmpHash == nil || !bytes.Equal(tmpHash, v) { delete(filteredHashes, k) // Potentially exit early. @@ -453,6 +491,7 @@ func (txi *TxIndex) matchRange( startKey []byte, filteredHashes map[string][]byte, firstRun bool, + heightInfo HeightInfo, ) map[string][]byte { // A previous match was attempted but resulted in no matches, so we return // no matches (assuming AND operand). @@ -461,8 +500,6 @@ func (txi *TxIndex) matchRange( } tmpHashes := make(map[string][]byte) - lowerBound := qr.LowerBoundValue() - upperBound := qr.UpperBoundValue() it, err := dbm.IteratePrefix(txi.store, startKey) if err != nil { @@ -481,18 +518,15 @@ LOOP: if err != nil { continue LOOP } + if qr.Key != types.TxHeightKey { + keyHeight, err := extractHeightFromKey(it.Key()) + if err != nil || !checkHeightConditions(heightInfo, keyHeight) { + continue LOOP + } - include := true - if lowerBound != nil && v < lowerBound.(int64) { - include = false - } - - if upperBound != nil && v > upperBound.(int64) { - include = false } - - if include { - tmpHashes[string(it.Value())] = it.Value() + if checkBounds(qr, v) { + txi.setTmpHashes(tmpHashes, it) } // XXX: passing time in a ABCI Events is not yet implemented @@ -528,8 +562,9 @@ LOOP: // Remove/reduce matches in filteredHashes that were not found in this // match (tmpHashes). REMOVE_LOOP: - for k := range filteredHashes { - if tmpHashes[k] == nil { + for k, v := range filteredHashes { + tmpHash := tmpHashes[k] + if tmpHash == nil || !bytes.Equal(tmpHashes[k], v) { delete(filteredHashes, k) // Potentially exit early. @@ -547,29 +582,49 @@ REMOVE_LOOP: // Keys func isTagKey(key []byte) bool { - return strings.Count(string(key), tagKeySeparator) == 3 + // This should be always 4 if data is indexed together with event sequences + // The check for 3 was added to allow data indexed before (w/o the event number) + // to be retrieved. + numTags := strings.Count(string(key), tagKeySeparator) + return numTags == 4 || numTags == 3 } +func extractHeightFromKey(key []byte) (int64, error) { + parts := strings.SplitN(string(key), tagKeySeparator, -1) + return strconv.ParseInt(parts[2], 10, 64) +} func extractValueFromKey(key []byte) string { - parts := strings.SplitN(string(key), tagKeySeparator, 3) + parts := strings.SplitN(string(key), tagKeySeparator, -1) return parts[1] } -func keyForEvent(key string, value string, result *abci.TxResult) []byte { - return []byte(fmt.Sprintf("%s/%s/%d/%d", +func extractEventSeqFromKey(key []byte) string { + parts := strings.SplitN(string(key), tagKeySeparator, -1) + + if len(parts) == 5 { + return parts[4] + } + return "0" +} +func keyForEvent(key string, value string, result *abci.TxResult, eventSeq int64) []byte { + return []byte(fmt.Sprintf("%s/%s/%d/%d/%d", key, value, result.Height, result.Index, + eventSeq, )) } func keyForHeight(result *abci.TxResult) []byte { - return []byte(fmt.Sprintf("%s/%d/%d/%d", + return []byte(fmt.Sprintf("%s/%d/%d/%d/%d", types.TxHeightKey, result.Height, result.Height, result.Index, + // Added to facilitate having the eventSeq in event keys + // Otherwise queries break expecting 5 entries + 0, )) } @@ -587,3 +642,28 @@ func startKey(fields ...interface{}) []byte { } return b.Bytes() } + +func checkBounds(ranges indexer.QueryRange, v int64) bool { + include := true + lowerBound := ranges.LowerBoundValue() + upperBound := ranges.UpperBoundValue() + if lowerBound != nil && v < lowerBound.(int64) { + include = false + } + + if upperBound != nil && v > upperBound.(int64) { + include = false + } + + return include +} + +//nolint:unused,deadcode +func lookForHeight(conditions []syntax.Condition) (height int64) { + for _, c := range conditions { + if c.CompositeKey == types.TxHeightKey && c.Op == query.OpEqual { + return c.Operand.(int64) + } + } + return 0 +} diff --git a/state/txindex/kv/kv_test.go b/state/txindex/kv/kv_test.go index 166b278ae0b..949dc85a45f 100644 --- a/state/txindex/kv/kv_test.go +++ b/state/txindex/kv/kv_test.go @@ -84,10 +84,13 @@ func TestTxSearch(t *testing.T) { }{ // search by hash {fmt.Sprintf("tx.hash = '%X'", hash), 1}, + // search by hash (lower) + {fmt.Sprintf("tx.hash = '%x'", hash), 1}, // search by exact match (one key) {"account.number = 1", 1}, // search by exact match (two keys) - {"account.number = 1 AND account.owner = 'Ivan'", 1}, + {"account.number = 1 AND account.owner = 'Ivan'", 0}, + {"account.owner = 'Ivan' AND account.number = 1", 0}, // search by exact match (two keys) {"account.number = 1 AND account.owner = 'Vlad'", 0}, {"account.owner = 'Vlad' AND account.number = 1", 0}, @@ -95,30 +98,41 @@ func TestTxSearch(t *testing.T) { {"account.owner = 'Vlad' AND account.number >= 1", 0}, {"account.number <= 0", 0}, {"account.number <= 0 AND account.owner = 'Ivan'", 0}, + {"account.number < 10000 AND account.owner = 'Ivan'", 0}, // search using a prefix of the stored value {"account.owner = 'Iv'", 0}, // search by range {"account.number >= 1 AND account.number <= 5", 1}, + // search by range and another key + {"account.number >= 1 AND account.owner = 'Ivan' AND account.number <= 5", 0}, // search by range (lower bound) {"account.number >= 1", 1}, // search by range (upper bound) {"account.number <= 5", 1}, + {"account.number <= 1", 1}, // search using not allowed key {"not_allowed = 'boom'", 0}, + {"not_allowed = 'Vlad'", 0}, // search for not existing tx result - {"account.number >= 2 AND account.number <= 5", 0}, + {"account.number >= 2 AND account.number <= 5 AND tx.height > 0", 0}, // search using not existing key {"account.date >= TIME 2013-05-03T14:45:00Z", 0}, // search using CONTAINS {"account.owner CONTAINS 'an'", 1}, // search for non existing value using CONTAINS {"account.owner CONTAINS 'Vlad'", 0}, + {"account.owner CONTAINS 'Ivann'", 0}, + {"account.owner CONTAINS 'IIvan'", 0}, + {"account.owner CONTAINS 'Iva n'", 0}, + {"account.owner CONTAINS ' Ivan'", 0}, + {"account.owner CONTAINS 'Ivan '", 0}, // search using the wrong key (of numeric type) using CONTAINS {"account.number CONTAINS 'Iv'", 0}, // search using EXISTS {"account.number EXISTS", 1}, // search using EXISTS for non existing key {"account.date EXISTS", 0}, + {"not_allowed EXISTS", 0}, } ctx := context.Background() @@ -139,6 +153,95 @@ func TestTxSearch(t *testing.T) { } } +func TestTxSearchEventMatch(t *testing.T) { + + indexer := NewTxIndex(db.NewMemDB()) + + txResult := txResultWithEvents([]abci.Event{ + {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "1", Index: true}, {Key: "owner", Value: "Ana", Index: true}}}, + {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "2", Index: true}, {Key: "owner", Value: "Ivan", Index: true}}}, + {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "3", Index: false}, {Key: "owner", Value: "Mickey", Index: false}}}, + {Type: "", Attributes: []abci.EventAttribute{{Key: "not_allowed", Value: "Vlad", Index: true}}}, + }) + + err := indexer.Index(txResult) + require.NoError(t, err) + + testCases := map[string]struct { + q string + resultsLength int + }{ + "Return all events from a height": { + q: "tx.height = 1", + resultsLength: 1, + }, + "Don't match non-indexed events": { + q: "account.number = 3 AND account.owner = 'Mickey'", + resultsLength: 0, + }, + "Return all events from a height with range": { + q: "tx.height > 0", + resultsLength: 1, + }, + "Return all events from a height with range 2": { + q: "tx.height <= 1", + resultsLength: 1, + }, + "Return all events from a height (deduplicate height)": { + q: "tx.height = 1 AND tx.height = 1", + resultsLength: 1, + }, + "Match attributes with height range and event": { + q: "tx.height < 2 AND tx.height > 0 AND account.number > 0 AND account.number <= 1 AND account.owner CONTAINS 'Ana'", + resultsLength: 1, + }, + "Match attributes with multiple CONTAIN and height range": { + q: "tx.height < 2 AND tx.height > 0 AND account.number = 1 AND account.owner CONTAINS 'Ana' AND account.owner CONTAINS 'An'", + resultsLength: 1, + }, + "Match attributes with height range and event - no match": { + q: "tx.height < 2 AND tx.height > 0 AND account.number = 2 AND account.owner = 'Ana'", + resultsLength: 0, + }, + "Match attributes with event": { + q: "account.number = 2 AND account.owner = 'Ana' AND tx.height = 1", + resultsLength: 0, + }, + "Deduplication test - should return nothing if attribute repeats multiple times": { + q: "tx.height < 2 AND account.number = 3 AND account.number = 2 AND account.number = 5", + resultsLength: 0, + }, + "Deduplication test - should return nothing if attribute repeats multiple times with match events": { + q: "tx.height < 2 AND account.number = 3 AND account.number = 2 AND account.number = 5", + resultsLength: 0, + }, + " Match range with match events": { + q: "account.number < 2 AND account.owner = 'Ivan'", + resultsLength: 0, + }, + " Match range with match events 2": { + q: "account.number <= 2 AND account.owner = 'Ivan' AND tx.height > 0", + resultsLength: 1, + }, + } + + ctx := context.Background() + + for _, tc := range testCases { + tc := tc + t.Run(tc.q, func(t *testing.T) { + results, err := indexer.Search(ctx, query.MustParse(tc.q)) + assert.NoError(t, err) + + assert.Len(t, results, tc.resultsLength) + if tc.resultsLength > 0 { + for _, txr := range results { + assert.True(t, proto.Equal(txResult, txr)) + } + } + }) + } +} func TestTxSearchWithCancelation(t *testing.T) { indexer := NewTxIndex(db.NewMemDB()) @@ -242,19 +345,80 @@ func TestTxSearchOneTxWithMultipleSameTagsButDifferentValues(t *testing.T) { txResult := txResultWithEvents([]abci.Event{ {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "1", Index: true}}}, {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "2", Index: true}}}, + {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "3", Index: false}}}, }) err := indexer.Index(txResult) require.NoError(t, err) + testCases := []struct { + q string + found bool + }{ + { + q: "account.number >= 1", + found: true, + }, + { + q: "account.number > 2", + found: false, + }, + { + q: "account.number >= 1 AND tx.height = 3 AND tx.height > 0", + found: true, + }, + { + q: "account.number >= 1 AND tx.height > 0 AND tx.height = 3", + found: true, + }, + + { + q: "account.number >= 1 AND tx.height = 1 AND tx.height = 2 AND tx.height = 3", + found: true, + }, + + { + q: "account.number >= 1 AND tx.height = 3 AND tx.height = 2 AND tx.height = 1", + found: false, + }, + { + q: "account.number >= 1 AND tx.height = 3", + found: false, + }, + { + q: "account.number > 1 AND tx.height < 2", + found: true, + }, + { + q: "account.number >= 2", + found: true, + }, + { + q: "account.number <= 1", + found: true, + }, + { + q: "account.number = 'something'", + found: false, + }, + { + q: "account.number CONTAINS 'bla'", + found: false, + }, + } + ctx := context.Background() - results, err := indexer.Search(ctx, query.MustCompile(`account.number >= 1`)) - assert.NoError(t, err) + for _, tc := range testCases { + results, err := indexer.Search(ctx, query.MustCompile(tc.q)) + assert.NoError(t, err) + len := 0 + if tc.found { + len = 1 + } + assert.Len(t, results, len) + assert.True(t, !tc.found || proto.Equal(txResult, results[0])) - assert.Len(t, results, 1) - for _, txr := range results { - assert.True(t, proto.Equal(txResult, txr)) } } diff --git a/state/txindex/kv/utils.go b/state/txindex/kv/utils.go index 48362bfbc2b..70d656485ac 100644 --- a/state/txindex/kv/utils.go +++ b/state/txindex/kv/utils.go @@ -1,5 +1,22 @@ package kv +import ( + "fmt" + + "github.com/cometbft/cometbft/libs/pubsub/query" + "github.com/cometbft/cometbft/state/indexer" + "github.com/cometbft/cometbft/types" + "github.com/google/orderedcode" +) + +type HeightInfo struct { + heightRange indexer.QueryRange + height int64 + heightEqIdx int + onlyHeightRange bool + onlyHeightEq bool +} + // IntInSlice returns true if a is found in the list. func intInSlice(a int, list []int) bool { for _, b := range list { @@ -9,3 +26,76 @@ func intInSlice(a int, list []int) bool { } return false } + +func ParseEventSeqFromEventKey(key []byte) (int64, error) { + var ( + compositeKey, typ, eventValue string + height int64 + eventSeq int64 + ) + + remaining, err := orderedcode.Parse(string(key), &compositeKey, &eventValue, &height, &typ, &eventSeq) + if err != nil { + return 0, fmt.Errorf("failed to parse event key: %w", err) + } + + if len(remaining) != 0 { + return 0, fmt.Errorf("unexpected remainder in key: %s", remaining) + } + + return eventSeq, nil +} +func dedupHeight(conditions []query.Condition) (dedupConditions []query.Condition, heightInfo HeightInfo) { + heightInfo.heightEqIdx = -1 + heightRangeExists := false + found := false + var heightCondition []query.Condition + heightInfo.onlyHeightEq = true + heightInfo.onlyHeightRange = true + for _, c := range conditions { + if c.CompositeKey == types.TxHeightKey { + if c.Op == query.OpEqual { + if heightRangeExists || found { + continue + } else { + found = true + heightCondition = append(heightCondition, c) + heightInfo.height = c.Operand.(int64) + } + } else { + heightInfo.onlyHeightEq = false + heightRangeExists = true + dedupConditions = append(dedupConditions, c) + } + } else { + heightInfo.onlyHeightRange = false + heightInfo.onlyHeightEq = false + dedupConditions = append(dedupConditions, c) + } + } + if !heightRangeExists && len(heightCondition) != 0 { + heightInfo.heightEqIdx = len(dedupConditions) + heightInfo.onlyHeightRange = false + dedupConditions = append(dedupConditions, heightCondition...) + } else { + // If we found a range make sure we set the height idx to -1 as the height equality + // will be removed + heightInfo.heightEqIdx = -1 + heightInfo.height = 0 + heightInfo.onlyHeightEq = false + } + return dedupConditions, heightInfo +} + +func checkHeightConditions(heightInfo HeightInfo, keyHeight int64) bool { + if heightInfo.heightRange.Key != "" { + if !checkBounds(heightInfo.heightRange, keyHeight) { + return false + } + } else { + if heightInfo.height != 0 && keyHeight != heightInfo.height { + return false + } + } + return true +} From ec82c0c6e7303eeb3f08f2d4b92310c6d32fbea7 Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Tue, 28 Feb 2023 23:44:41 +0100 Subject: [PATCH 2/8] Added fix to query attributes mathing on events --- state/indexer/block/kv/kv.go | 4 +-- state/indexer/block/kv/kv_test.go | 56 +++++++++++++++---------------- state/indexer/block/kv/util.go | 13 +++---- state/indexer/query_range.go | 34 +++++++++---------- state/txindex/kv/kv.go | 4 +-- state/txindex/kv/kv_test.go | 2 +- state/txindex/kv/utils.go | 12 +++---- 7 files changed, 62 insertions(+), 63 deletions(-) diff --git a/state/indexer/block/kv/kv.go b/state/indexer/block/kv/kv.go index 709536cdff4..391ad2d703a 100644 --- a/state/indexer/block/kv/kv.go +++ b/state/indexer/block/kv/kv.go @@ -100,9 +100,7 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, conditions := q.Syntax() //conditions, err := q.Conditions() - if err != nil { - return nil, fmt.Errorf("failed to parse query conditions: %w", err) - } + // conditions to skip because they're handled before "everything else" skipIndexes := make([]int, 0) diff --git a/state/indexer/block/kv/kv_test.go b/state/indexer/block/kv/kv_test.go index e58b8f9302b..dcb7e5f2b22 100644 --- a/state/indexer/block/kv/kv_test.go +++ b/state/indexer/block/kv/kv_test.go @@ -94,10 +94,10 @@ func TestBlockIndexer(t *testing.T) { q *query.Query results []int64 }{ - "block.height = 100": { - q: query.MustCompile(`block.height = 100`), - results: []int64{}, - }, + // "block.height = 100": { + // q: query.MustCompile(`block.height = 100`), + // results: []int64{}, + // }, "block.height = 5": { q: query.MustCompile(`block.height = 5`), results: []int64{5}, @@ -123,11 +123,11 @@ func TestBlockIndexer(t *testing.T) { results: []int64{4, 6, 8}, }, "end_event.foo > 100": { - q: query.MustParse("end_event.foo > 100"), + q: query.MustCompile("end_event.foo > 100"), results: []int64{}, }, "block.height >= 2 AND end_event.foo < 8": { - q: query.MustParse("block.height >= 2 AND end_event.foo < 8"), + q: query.MustCompile("block.height >= 2 AND end_event.foo < 8"), results: []int64{2, 4, 6}, }, "begin_event.proposer CONTAINS 'FFFFFFF'": { @@ -139,7 +139,7 @@ func TestBlockIndexer(t *testing.T) { results: []int64{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}, }, "end_event.foo CONTAINS '1'": { - q: query.MustParse("end_event.foo CONTAINS '1'"), + q: query.MustCompile("end_event.foo CONTAINS '1'"), results: []int64{1, 10}, }, } @@ -246,87 +246,87 @@ func TestBlockIndexerMulti(t *testing.T) { }{ "query return all events from a height - exact": { - q: query.MustParse("block.height = 1"), + q: query.MustCompile("block.height = 1"), results: []int64{1}, }, "query return all events from a height - exact - no match.events": { - q: query.MustParse("block.height = 1"), + q: query.MustCompile("block.height = 1"), results: []int64{1}, }, "query return all events from a height - exact (deduplicate height)": { - q: query.MustParse("block.height = 1 AND block.height = 2"), + q: query.MustCompile("block.height = 1 AND block.height = 2"), results: []int64{1}, }, "query return all events from a height - exact (deduplicate height) - no match.events": { - q: query.MustParse("block.height = 1 AND block.height = 2"), + q: query.MustCompile("block.height = 1 AND block.height = 2"), results: []int64{1}, }, "query return all events from a height - range": { - q: query.MustParse("block.height < 2 AND block.height > 0 AND block.height > 0"), + q: query.MustCompile("block.height < 2 AND block.height > 0 AND block.height > 0"), results: []int64{1}, }, "query return all events from a height - range - no match.events": { - q: query.MustParse("block.height < 2 AND block.height > 0 AND block.height > 0"), + q: query.MustCompile("block.height < 2 AND block.height > 0 AND block.height > 0"), results: []int64{1}, }, "query return all events from a height - range 2": { - q: query.MustParse("block.height < 3 AND block.height < 2 AND block.height > 0 AND block.height > 0"), + q: query.MustCompile("block.height < 3 AND block.height < 2 AND block.height > 0 AND block.height > 0"), results: []int64{1}, }, "query return all events from a height - range 3": { - q: query.MustParse("block.height < 1 AND block.height > 1"), + q: query.MustCompile("block.height < 1 AND block.height > 1"), results: []int64{}, }, "query matches fields from same event": { - q: query.MustParse("end_event.bar < 300 AND end_event.foo = 100 AND block.height > 0 AND block.height <= 2"), + q: query.MustCompile("end_event.bar < 300 AND end_event.foo = 100 AND block.height > 0 AND block.height <= 2"), results: []int64{1, 2}, }, "query matches fields from same event - no match.events": { - q: query.MustParse("end_event.bar < 300 AND end_event.foo = 100 AND block.height > 0 AND block.height <= 2"), + q: query.MustCompile("end_event.bar < 300 AND end_event.foo = 100 AND block.height > 0 AND block.height <= 2"), results: []int64{1, 2}, }, "query matches fields from multiple events": { - q: query.MustParse("end_event.foo = 100 AND end_event.bar = 400 AND block.height = 2"), + q: query.MustCompile("end_event.foo = 100 AND end_event.bar = 400 AND block.height = 2"), results: []int64{}, }, "query matches fields from multiple events 2": { - q: query.MustParse("end_event.foo = 100 AND end_event.bar > 200 AND block.height > 0 AND block.height < 3"), + q: query.MustCompile("end_event.foo = 100 AND end_event.bar > 200 AND block.height > 0 AND block.height < 3"), results: []int64{}, }, "query matches fields from multiple events allowed": { - q: query.MustParse("end_event.foo = 100 AND end_event.bar = 400"), + q: query.MustCompile("end_event.foo = 100 AND end_event.bar = 400"), results: []int64{}, }, "query matches fields from all events whose attribute is within range": { - q: query.MustParse("block.height = 2 AND end_event.foo < 300"), + q: query.MustCompile("block.height = 2 AND end_event.foo < 300"), results: []int64{2}, }, "deduplication test - match.events multiple 2": { - q: query.MustParse("end_event.foo = 100 AND end_event.bar = 400 AND block.height = 2"), + q: query.MustCompile("end_event.foo = 100 AND end_event.bar = 400 AND block.height = 2"), results: []int64{}, }, "query using CONTAINS matches fields from all events whose attribute is within range": { - q: query.MustParse("block.height = 2 AND end_event.foo CONTAINS '30'"), + q: query.MustCompile("block.height = 2 AND end_event.foo CONTAINS '30'"), results: []int64{2}, }, "query matches all fields from multiple events": { - q: query.MustParse("end_event.bar > 100 AND end_event.bar <= 500"), + q: query.MustCompile("end_event.bar > 100 AND end_event.bar <= 500"), results: []int64{1, 2}, }, "query matches all fields from multiple events - no match.events": { - q: query.MustParse("end_event.bar > 100 AND end_event.bar <= 500"), + q: query.MustCompile("end_event.bar > 100 AND end_event.bar <= 500"), results: []int64{1, 2}, }, "query with height range and height equality - should ignore equality": { - q: query.MustParse("block.height = 2 AND end_event.foo >= 100 AND block.height < 2"), + q: query.MustCompile("block.height = 2 AND end_event.foo >= 100 AND block.height < 2"), results: []int64{1}, }, "query with non-existent field": { - q: query.MustParse("end_event.baz = 100"), + q: query.MustCompile("end_event.baz = 100"), results: []int64{}, }, "query with non-existent field ": { - q: query.MustParse("end_event.baz = 100"), + q: query.MustCompile("end_event.baz = 100"), results: []int64{}, }, } diff --git a/state/indexer/block/kv/util.go b/state/indexer/block/kv/util.go index 90365ff6e55..2639e701ba0 100644 --- a/state/indexer/block/kv/util.go +++ b/state/indexer/block/kv/util.go @@ -136,20 +136,21 @@ func parseEventSeqFromEventKey(key []byte) (int64, error) { // addition to match events is the height equality or height range query. At the same time, if we do have a height range condition // ignore the height equality condition. If a height equality exists, place the condition index in the query and the desired height // into the heightInfo struct -func dedupHeight(conditions []query.Condition) (dedupConditions []query.Condition, heightInfo HeightInfo, found bool) { +func dedupHeight(conditions []syntax.Condition) (dedupConditions []syntax.Condition, heightInfo HeightInfo, found bool) { heightInfo.heightEqIdx = -1 heightRangeExists := false - var heightCondition []query.Condition + var heightCondition []syntax.Condition heightInfo.onlyHeightEq = true heightInfo.onlyHeightRange = true for _, c := range conditions { - if c.CompositeKey == types.BlockHeightKey { - if c.Op == query.OpEqual { + if c.Tag == types.BlockHeightKey { + if c.Op == syntax.TEq { if found || heightRangeExists { continue } else { heightCondition = append(heightCondition, c) - heightInfo.height = c.Operand.(int64) + heightInfo.height = int64(c.Arg.Number()) + found = true } } else { @@ -199,5 +200,5 @@ func lookForHeight(conditions []syntax.Condition) (int64, bool) { } } - return 0, false, -1 + return 0, false } diff --git a/state/indexer/query_range.go b/state/indexer/query_range.go index 881fd4b8770..e3cfdc6fda0 100644 --- a/state/indexer/query_range.go +++ b/state/indexer/query_range.go @@ -83,46 +83,46 @@ func LookForRangesWithHeight(conditions []syntax.Condition) (queryRange QueryRan for i, c := range conditions { heightKey := false if IsRangeOperation(c.Op) { - r, ok := queryRange[c.CompositeKey] + r, ok := queryRange[c.Tag] if !ok { - r = QueryRange{Key: c.CompositeKey} - if c.CompositeKey == types.BlockHeightKey || c.CompositeKey == types.TxHeightKey { - heightRange = QueryRange{Key: c.CompositeKey} + r = QueryRange{Key: c.Tag} + if c.Tag == types.BlockHeightKey || c.Tag == types.TxHeightKey { + heightRange = QueryRange{Key: c.Tag} heightKey = true } } switch c.Op { - case query.OpGreater: + case syntax.TGt: if heightKey { - heightRange.LowerBound = c.Operand + heightRange.LowerBound = conditionArg(c) } - r.LowerBound = c.Operand + r.LowerBound = conditionArg(c) - case query.OpGreaterEqual: + case syntax.TGeq: r.IncludeLowerBound = true - r.LowerBound = c.Operand + r.LowerBound = conditionArg(c) if heightKey { heightRange.IncludeLowerBound = true - heightRange.LowerBound = c.Operand + heightRange.LowerBound = conditionArg(c) } - case query.OpLess: - r.UpperBound = c.Operand + case syntax.TLt: + r.UpperBound = conditionArg(c) if heightKey { - heightRange.UpperBound = c.Operand + heightRange.UpperBound = conditionArg(c) } - case query.OpLessEqual: + case syntax.TLeq: r.IncludeUpperBound = true - r.UpperBound = c.Operand + r.UpperBound = conditionArg(c) if heightKey { heightRange.IncludeUpperBound = true - heightRange.UpperBound = c.Operand + heightRange.UpperBound = conditionArg(c) } } - queryRange[c.CompositeKey] = r + queryRange[c.Tag] = r indexes = append(indexes, i) } } diff --git a/state/txindex/kv/kv.go b/state/txindex/kv/kv.go index b2e6a06846b..8b8921c5f8f 100644 --- a/state/txindex/kv/kv.go +++ b/state/txindex/kv/kv.go @@ -661,8 +661,8 @@ func checkBounds(ranges indexer.QueryRange, v int64) bool { //nolint:unused,deadcode func lookForHeight(conditions []syntax.Condition) (height int64) { for _, c := range conditions { - if c.CompositeKey == types.TxHeightKey && c.Op == query.OpEqual { - return c.Operand.(int64) + if c.Tag == types.TxHeightKey && c.Op == syntax.TEq { + height = int64(c.Arg.Number()) } } return 0 diff --git a/state/txindex/kv/kv_test.go b/state/txindex/kv/kv_test.go index 949dc85a45f..7c7eca2e38b 100644 --- a/state/txindex/kv/kv_test.go +++ b/state/txindex/kv/kv_test.go @@ -230,7 +230,7 @@ func TestTxSearchEventMatch(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.q, func(t *testing.T) { - results, err := indexer.Search(ctx, query.MustParse(tc.q)) + results, err := indexer.Search(ctx, query.MustCompile(tc.q)) assert.NoError(t, err) assert.Len(t, results, tc.resultsLength) diff --git a/state/txindex/kv/utils.go b/state/txindex/kv/utils.go index 70d656485ac..3f00d342be4 100644 --- a/state/txindex/kv/utils.go +++ b/state/txindex/kv/utils.go @@ -3,7 +3,7 @@ package kv import ( "fmt" - "github.com/cometbft/cometbft/libs/pubsub/query" + cmtsyntax "github.com/cometbft/cometbft/libs/pubsub/query/syntax" "github.com/cometbft/cometbft/state/indexer" "github.com/cometbft/cometbft/types" "github.com/google/orderedcode" @@ -45,22 +45,22 @@ func ParseEventSeqFromEventKey(key []byte) (int64, error) { return eventSeq, nil } -func dedupHeight(conditions []query.Condition) (dedupConditions []query.Condition, heightInfo HeightInfo) { +func dedupHeight(conditions []cmtsyntax.Condition) (dedupConditions []cmtsyntax.Condition, heightInfo HeightInfo) { heightInfo.heightEqIdx = -1 heightRangeExists := false found := false - var heightCondition []query.Condition + var heightCondition []cmtsyntax.Condition heightInfo.onlyHeightEq = true heightInfo.onlyHeightRange = true for _, c := range conditions { - if c.CompositeKey == types.TxHeightKey { - if c.Op == query.OpEqual { + if c.Tag == types.TxHeightKey { + if c.Op == cmtsyntax.TEq { if heightRangeExists || found { continue } else { found = true heightCondition = append(heightCondition, c) - heightInfo.height = c.Operand.(int64) + heightInfo.height = int64(c.Arg.Number()) } } else { heightInfo.onlyHeightEq = false From bb38a3cd9a8a37f7c1667fa3c545ef3cb24e0021 Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Sat, 25 Feb 2023 13:07:06 +0100 Subject: [PATCH 3/8] txindex v0.37.x: Enable the indexer to parse slashes in query (#382) * txindexer handles slashes in event value * Update .changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md Co-authored-by: Thane Thomson --------- Co-authored-by: Thane Thomson --- .../382-txindexer-fix-slash-parsing.md | 1 + state/txindex/kv/kv.go | 44 +++++++++++++------ state/txindex/kv/kv_test.go | 21 ++++++--- 3 files changed, 46 insertions(+), 20 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md diff --git a/.changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md b/.changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md new file mode 100644 index 00000000000..71afe7bb307 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md @@ -0,0 +1 @@ +- `[state/kvindexer]` \#382 Resolved crashes when event values contained slashes, introduced after adding event sequences in \#77. (@jmalicevic) \ No newline at end of file diff --git a/state/txindex/kv/kv.go b/state/txindex/kv/kv.go index 8b8921c5f8f..0fe4a33f7e3 100644 --- a/state/txindex/kv/kv.go +++ b/state/txindex/kv/kv.go @@ -21,7 +21,8 @@ import ( ) const ( - tagKeySeparator = "/" + tagKeySeparator = "/" + eventSeqSeparator = "$es$" ) var _ txindex.TxIndexer = (*TxIndex)(nil) @@ -582,49 +583,64 @@ REMOVE_LOOP: // Keys func isTagKey(key []byte) bool { - // This should be always 4 if data is indexed together with event sequences - // The check for 3 was added to allow data indexed before (w/o the event number) - // to be retrieved. + // Normally, if the event was indexed with an event sequence, the number of + // tags should 4. Alternatively it should be 3 if the event was not indexed + // with the corresponding event sequence. However, some attribute values in + // production can contain the tag separator. Therefore, the condition is >= 3. numTags := strings.Count(string(key), tagKeySeparator) - return numTags == 4 || numTags == 3 + return numTags >= 3 } func extractHeightFromKey(key []byte) (int64, error) { parts := strings.SplitN(string(key), tagKeySeparator, -1) - return strconv.ParseInt(parts[2], 10, 64) + + return strconv.ParseInt(parts[len(parts)-2], 10, 64) } func extractValueFromKey(key []byte) string { - parts := strings.SplitN(string(key), tagKeySeparator, -1) - return parts[1] + keyString := string(key) + parts := strings.SplitN(keyString, tagKeySeparator, -1) + partsLen := len(parts) + value := strings.TrimPrefix(keyString, parts[0]+tagKeySeparator) + + suffix := "" + suffixLen := 2 + + for i := 1; i <= suffixLen; i++ { + suffix = tagKeySeparator + parts[partsLen-i] + suffix + } + return strings.TrimSuffix(value, suffix) + } func extractEventSeqFromKey(key []byte) string { parts := strings.SplitN(string(key), tagKeySeparator, -1) - if len(parts) == 5 { - return parts[4] + lastEl := parts[len(parts)-1] + + if strings.Contains(lastEl, eventSeqSeparator) { + return strings.SplitN(lastEl, eventSeqSeparator, 2)[1] } return "0" } func keyForEvent(key string, value string, result *abci.TxResult, eventSeq int64) []byte { - return []byte(fmt.Sprintf("%s/%s/%d/%d/%d", + return []byte(fmt.Sprintf("%s/%s/%d/%d%s", key, value, result.Height, result.Index, - eventSeq, + eventSeqSeparator+strconv.FormatInt(eventSeq, 10), )) } func keyForHeight(result *abci.TxResult) []byte { - return []byte(fmt.Sprintf("%s/%d/%d/%d/%d", + return []byte(fmt.Sprintf("%s/%d/%d/%d%s", types.TxHeightKey, result.Height, result.Height, result.Index, // Added to facilitate having the eventSeq in event keys // Otherwise queries break expecting 5 entries - 0, + eventSeqSeparator+"0", )) } diff --git a/state/txindex/kv/kv_test.go b/state/txindex/kv/kv_test.go index 7c7eca2e38b..c254a58c38e 100644 --- a/state/txindex/kv/kv_test.go +++ b/state/txindex/kv/kv_test.go @@ -70,7 +70,7 @@ func TestTxSearch(t *testing.T) { txResult := txResultWithEvents([]abci.Event{ {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "1", Index: true}}}, - {Type: "account", Attributes: []abci.EventAttribute{{Key: "owner", Value: "Ivan", Index: true}}}, + {Type: "account", Attributes: []abci.EventAttribute{{Key: "owner", Value: "/Ivan/", Index: true}}}, {Type: "", Attributes: []abci.EventAttribute{{Key: "not_allowed", Value: "Vlad", Index: true}}}, }) hash := types.Tx(txResult.Tx).Hash() @@ -82,7 +82,7 @@ func TestTxSearch(t *testing.T) { q string resultsLength int }{ - // search by hash + // search by hash {fmt.Sprintf("tx.hash = '%X'", hash), 1}, // search by hash (lower) {fmt.Sprintf("tx.hash = '%x'", hash), 1}, @@ -91,6 +91,7 @@ func TestTxSearch(t *testing.T) { // search by exact match (two keys) {"account.number = 1 AND account.owner = 'Ivan'", 0}, {"account.owner = 'Ivan' AND account.number = 1", 0}, + {"account.owner = '/Ivan/'", 1}, // search by exact match (two keys) {"account.number = 1 AND account.owner = 'Vlad'", 0}, {"account.owner = 'Vlad' AND account.number = 1", 0}, @@ -119,7 +120,7 @@ func TestTxSearch(t *testing.T) { {"account.date >= TIME 2013-05-03T14:45:00Z", 0}, // search using CONTAINS {"account.owner CONTAINS 'an'", 1}, - // search for non existing value using CONTAINS + // search for non existing value using CONTAINS {"account.owner CONTAINS 'Vlad'", 0}, {"account.owner CONTAINS 'Ivann'", 0}, {"account.owner CONTAINS 'IIvan'", 0}, @@ -159,7 +160,7 @@ func TestTxSearchEventMatch(t *testing.T) { txResult := txResultWithEvents([]abci.Event{ {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "1", Index: true}, {Key: "owner", Value: "Ana", Index: true}}}, - {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "2", Index: true}, {Key: "owner", Value: "Ivan", Index: true}}}, + {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "2", Index: true}, {Key: "owner", Value: "/Ivan/.test", Index: true}}}, {Type: "account", Attributes: []abci.EventAttribute{{Key: "number", Value: "3", Index: false}, {Key: "owner", Value: "Mickey", Index: false}}}, {Type: "", Attributes: []abci.EventAttribute{{Key: "not_allowed", Value: "Vlad", Index: true}}}, }) @@ -216,11 +217,19 @@ func TestTxSearchEventMatch(t *testing.T) { resultsLength: 0, }, " Match range with match events": { - q: "account.number < 2 AND account.owner = 'Ivan'", + q: "account.number < 2 AND account.owner = '/Ivan/.test'", resultsLength: 0, }, " Match range with match events 2": { - q: "account.number <= 2 AND account.owner = 'Ivan' AND tx.height > 0", + q: "account.number <= 2 AND account.owner = '/Ivan/.test' AND tx.height > 0", + resultsLength: 1, + }, + " Match range with match events contains with multiple items": { + q: "account.number <= 2 AND account.owner CONTAINS '/Iv' AND account.owner CONTAINS 'an' AND tx.height = 1", + resultsLength: 1, + }, + " Match range with match events contains": { + q: "account.number <= 2 AND account.owner CONTAINS 'an' AND tx.height > 0", resultsLength: 1, }, } From 33fe8a7c613dfb962554d5ef1e3b7c7e8d53a6c2 Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Tue, 28 Feb 2023 23:56:29 +0100 Subject: [PATCH 4/8] Fixed linter --- state/txindex/kv/kv.go | 1 + 1 file changed, 1 insertion(+) diff --git a/state/txindex/kv/kv.go b/state/txindex/kv/kv.go index 0fe4a33f7e3..3030d90d532 100644 --- a/state/txindex/kv/kv.go +++ b/state/txindex/kv/kv.go @@ -679,6 +679,7 @@ func lookForHeight(conditions []syntax.Condition) (height int64) { for _, c := range conditions { if c.Tag == types.TxHeightKey && c.Op == syntax.TEq { height = int64(c.Arg.Number()) + return } } return 0 From f5e60b72e3a202c0578655420089ebe8a39da958 Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Wed, 15 Mar 2023 10:40:41 +0100 Subject: [PATCH 5/8] Fixed code comments --- state/indexer/block/kv/kv.go | 1 - state/indexer/block/kv/kv_test.go | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/state/indexer/block/kv/kv.go b/state/indexer/block/kv/kv.go index 391ad2d703a..27c61c89d96 100644 --- a/state/indexer/block/kv/kv.go +++ b/state/indexer/block/kv/kv.go @@ -99,7 +99,6 @@ func (idx *BlockerIndexer) Search(ctx context.Context, q *query.Query) ([]int64, } conditions := q.Syntax() - //conditions, err := q.Conditions() // conditions to skip because they're handled before "everything else" skipIndexes := make([]int, 0) diff --git a/state/indexer/block/kv/kv_test.go b/state/indexer/block/kv/kv_test.go index dcb7e5f2b22..e8239f12e3a 100644 --- a/state/indexer/block/kv/kv_test.go +++ b/state/indexer/block/kv/kv_test.go @@ -94,10 +94,10 @@ func TestBlockIndexer(t *testing.T) { q *query.Query results []int64 }{ - // "block.height = 100": { - // q: query.MustCompile(`block.height = 100`), - // results: []int64{}, - // }, + "block.height = 100": { + q: query.MustCompile(`block.height = 100`), + results: []int64{}, + }, "block.height = 5": { q: query.MustCompile(`block.height = 5`), results: []int64{5}, From 443cd7bd7e1dc6bdd17b75d79dadc156d9701b46 Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Wed, 15 Mar 2023 10:41:01 +0100 Subject: [PATCH 6/8] Update state/txindex/kv/kv.go Co-authored-by: Sergio Mena --- state/txindex/kv/kv.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/state/txindex/kv/kv.go b/state/txindex/kv/kv.go index 3030d90d532..5ee6226442c 100644 --- a/state/txindex/kv/kv.go +++ b/state/txindex/kv/kv.go @@ -675,11 +675,10 @@ func checkBounds(ranges indexer.QueryRange, v int64) bool { } //nolint:unused,deadcode -func lookForHeight(conditions []syntax.Condition) (height int64) { +func lookForHeight(conditions []syntax.Condition) int64 { for _, c := range conditions { if c.Tag == types.TxHeightKey && c.Op == syntax.TEq { - height = int64(c.Arg.Number()) - return + return int64(c.Arg.Number()) } } return 0 From b108690f53df96954315603b564f0194d753ee2f Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Tue, 21 Mar 2023 09:34:37 +0100 Subject: [PATCH 7/8] Removed unused functions --- state/indexer/block/kv/util.go | 11 ----------- state/txindex/kv/kv.go | 10 ---------- 2 files changed, 21 deletions(-) diff --git a/state/indexer/block/kv/util.go b/state/indexer/block/kv/util.go index 2639e701ba0..9ccb84720fa 100644 --- a/state/indexer/block/kv/util.go +++ b/state/indexer/block/kv/util.go @@ -191,14 +191,3 @@ func checkHeightConditions(heightInfo HeightInfo, keyHeight int64) bool { } return true } - -//nolint:unused,deadcode -func lookForHeight(conditions []syntax.Condition) (int64, bool) { - for _, c := range conditions { - if c.Tag == types.BlockHeightKey && c.Op == syntax.TEq { - return int64(c.Arg.Number()), true - } - } - - return 0, false -} diff --git a/state/txindex/kv/kv.go b/state/txindex/kv/kv.go index 5ee6226442c..17440530428 100644 --- a/state/txindex/kv/kv.go +++ b/state/txindex/kv/kv.go @@ -673,13 +673,3 @@ func checkBounds(ranges indexer.QueryRange, v int64) bool { return include } - -//nolint:unused,deadcode -func lookForHeight(conditions []syntax.Condition) int64 { - for _, c := range conditions { - if c.Tag == types.TxHeightKey && c.Op == syntax.TEq { - return int64(c.Arg.Number()) - } - } - return 0 -} From c6edd5bfebbac43e74d655be41da9a07a833cb03 Mon Sep 17 00:00:00 2001 From: Jasmina Malicevic Date: Tue, 21 Mar 2023 10:16:38 +0100 Subject: [PATCH 8/8] Edited changelog entry --- .../unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md | 1 - .../bug-fixes/423-forwardport-default-kvindexer-behaviour.md | 2 ++ .../bug-fixes/77-kvindexer-fix-evattribute-indexing.md | 1 - 3 files changed, 2 insertions(+), 2 deletions(-) delete mode 100644 .changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md create mode 100644 .changelog/unreleased/bug-fixes/423-forwardport-default-kvindexer-behaviour.md delete mode 100644 .changelog/unreleased/bug-fixes/77-kvindexer-fix-evattribute-indexing.md diff --git a/.changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md b/.changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md deleted file mode 100644 index 71afe7bb307..00000000000 --- a/.changelog/unreleased/bug-fixes/382-txindexer-fix-slash-parsing.md +++ /dev/null @@ -1 +0,0 @@ -- `[state/kvindexer]` \#382 Resolved crashes when event values contained slashes, introduced after adding event sequences in \#77. (@jmalicevic) \ No newline at end of file diff --git a/.changelog/unreleased/bug-fixes/423-forwardport-default-kvindexer-behaviour.md b/.changelog/unreleased/bug-fixes/423-forwardport-default-kvindexer-behaviour.md new file mode 100644 index 00000000000..0563a981af6 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/423-forwardport-default-kvindexer-behaviour.md @@ -0,0 +1,2 @@ +- `[kvindexer]` Forward porting the fixes done to the kvindexer in 0.37 in PR \#77 + ([\#423](https://github.com/cometbft/cometbft/pull/423)) \ No newline at end of file diff --git a/.changelog/unreleased/bug-fixes/77-kvindexer-fix-evattribute-indexing.md b/.changelog/unreleased/bug-fixes/77-kvindexer-fix-evattribute-indexing.md deleted file mode 100644 index 5033b7f1fbb..00000000000 --- a/.changelog/unreleased/bug-fixes/77-kvindexer-fix-evattribute-indexing.md +++ /dev/null @@ -1 +0,0 @@ -- `[state/kvindexer]` \#77 Fixed the default behaviour of the kvindexer to index and query attributes by events in which they occur. In 0.34.25 this was mitigated by a separated RPC flag. (@jmalicevic) \ No newline at end of file