fix: add max results error and default raw codec for eth_getLogs #12671

akaladarshi · 2024-11-06T06:12:46Z

Related Issues

#12636

Proposed Changes

Adds raw codec while filtering ethereum logs
Error out if we hit max result

Checklist

Before you mark the PR ready for review, please make sure that:

Commits have a clear commit message.
PR title conforms with contribution conventions
Update CHANGELOG.md or signal that this change does not need it per contribution conventions
New features have usage guidelines and / or documentation updates in
- Lotus Documentation
- Discussion Tutorials
Tests exist for new functionality or change in behavior
CI is green

rvagg · 2024-11-07T03:43:53Z

chain/index/events.go

@@ -563,6 +566,9 @@ func makePrefillFilterQuery(f *EventFilter) ([]any, string, error) {
 				clauses = append(clauses, "("+strings.Join(subclauses, " OR ")+")")
 			}
 		}
+	} else {
+		clauses = append(clauses, "ee.codec=?")
+		values = append(values, uint64(multicodec.Raw))


This won't work because we call this method from both the eth APIs and also GetActorEventsRaw and SubscribeActorEventsRaw and these APIs want any codec if they don't specify a KeysWithCodec.

I think what you might need to do is extend EventFilter to add a OnlyRaw, or WithCodec or something like that so that when this gets called from the Eth APIs, that can be set so we can restrict to Raw only.

Yeah while testing I realised this. So after going over the code I agree with you on extending the EventFilter.

I am in favour of adding an optional field in EventFilter suggesting, filtering based on this codec if keys are not available.

Codec *uint64 // Optional codec filter, used only if keys are not available

Note: We can also use array of codecs, but I don't think it will be useful because we only use two codecs (CBOR and RAW) to filter from.

yeah, for now just Codec is fine, "only used if KeysWithCodec is not set"

rvagg · 2024-11-18T04:55:36Z

CHANGELOG.md

@@ -27,7 +27,7 @@
 - Try harder in the F3 participation loop to participate using the same lotus node ([filecoin-project/lotus#12664](https://github.com/filecoin-project/lotus/pull/12664)).
 - The mining loop will now correctly "stick" to the same upstream lotus node for all operations pertaining to mining a single block ([filecoin-project/lotus#12665](https://github.com/filecoin-project/lotus/pull/12665)).
 - Make the ordering of event output for `eth_` APIs and `GetActorEventsRaw` consistent, sorting ascending on: epoch, message index, event index and original event entry order. ([filecoin-project/lotus#12623](https://github.com/filecoin-project/lotus/pull/12623))
-
+- Return error after hitting max results in event filtering and add default `raw` codec for eth_getLogs. ([filecoin-project/lotus#12671](https://github.com/filecoin-project/lotus/pull/12671))


Suggested change

- Return error after hitting max results in event filtering and add default `raw` codec for eth_getLogs. ([filecoin-project/lotus#12671](https://github.com/filecoin-project/lotus/pull/12671))

- Event APIs (`GetActorEventsRaw`, `SubscribeActorEventsRaw`, `eth_getLogs`, `eth_newFilter`, etc.) will now return an error when a request matches more than `MaxFilterResults` (default: 10,000) rather than silently truncating the results. Also apply an internal event matcher for `eth_getLogs` (etc.) to avoid builtin actor events on database query so as not to include them in `MaxFilterResults` calculation. ([filecoin-project/lotus#12671](https://github.com/filecoin-project/lotus/pull/12671))

Try to think about the change from a user perspective:

They primarily want to know what impact it has on them, so speaking in terms of API is usually best if we're touching that

They're generally not knowledgeable about internal details, and the raw vs cbor thing is pretty esoteric, so we can side-step that.

rvagg · 2024-11-18T05:04:42Z

chain/index/events.go

@@ -22,6 +22,8 @@ import (
 	"github.com/filecoin-project/lotus/chain/types"
 )

+var ErrMaxResultsReached = xerrors.New("max results limit reached, results truncated")


Suggested change

var ErrMaxResultsReached = xerrors.New("max results limit reached, results truncated")

var ErrMaxResultsReached = fmt.Errorf("filter matches too many events, try a more restricted filter")

rvagg · 2024-11-18T05:08:24Z

node/impl/full/eth.go

+	var codec uint64
+	// if no keys are specified, we can use the codec filter to make sure we only get `raw` encoded events
+	if len(pf.keys) == 0 {
+		codec = uint64(multicodec.Raw)
+	}
+
 	ef := &index.EventFilter{
 		MinHeight:     pf.minHeight,
 		MaxHeight:     pf.maxHeight,
 		TipsetCid:     pf.tipsetCid,
 		Addresses:     pf.addresses,
 		KeysWithCodec: pf.keys,
+		Codec:         codec,
 		MaxResults:    e.EventFilterManager.MaxFilterResults,
 	}


Suggested change

var codec uint64

// if no keys are specified, we can use the codec filter to make sure we only get `raw` encoded events

if len(pf.keys) == 0 {

codec = uint64(multicodec.Raw)

}

ef := &index.EventFilter{

MinHeight: pf.minHeight,

MaxHeight: pf.maxHeight,

TipsetCid: pf.tipsetCid,

Addresses: pf.addresses,

KeysWithCodec: pf.keys,

Codec: codec,

MaxResults: e.EventFilterManager.MaxFilterResults,

}

ef := &index.EventFilter{

MinHeight: pf.minHeight,

MaxHeight: pf.maxHeight,

TipsetCid: pf.tipsetCid,

Addresses: pf.addresses,

KeysWithCodec: pf.keys,

Codec: multicodec.Raw,

MaxResults: e.EventFilterManager.MaxFilterResults,

}

you can avoid the conditional, it's always Raw regardless. Look in keysToKeysWithCodec and you'll see even there if they supply a key then it's going to be set to Raw anyway.

I was thinking, since Codec is an optional field, so we should set only in certain condition (pf.keys is not there).

@rvagg Let me know what you think about the above, otherwise I will make your suggested changes.

I think that regardless of what KeysWithCodec contains, we're safe to always set it to Raw. Which is fine since the contract says that it's only going to be used if KeysWithCodec is not set.

But also, 0 is a legitimate multicodec code too, so you're even setting it by not setting it. If we want to make it truly optional then it should probably be a pointer, but that gets ugly and it's really unnecessary for this.

I think if it was a public-facing API then we might want to be a bit more careful with the layout of it. Consider, for example, we have TipsetCid conflicting with MinHeight and MaxHeight - if you supply the former then the latter two won't be read. If this were a public API maybe we would come up with a better union representation for this.

rvagg · 2024-11-18T05:09:01Z

chain/index/interface.go

@@ -47,6 +47,8 @@ type EventFilter struct {

 	KeysWithCodec map[string][]types.ActorEventBlock // map of key names to a list of alternate values that may match
 	MaxResults    int                                // maximum number of results to collect, 0 is unlimited
+
+	Codec uint64 // optional codec filter, only used if KeysWithCodec is not set


Suggested change

Codec uint64 // optional codec filter, only used if KeysWithCodec is not set

Codec multicodec.Code // optional codec filter, only used if KeysWithCodec is not set

akaladarshi added 2 commits November 6, 2024 11:20

apply raw codec filter for eth get logs

67c4b72

return error when we hit max filter result

0d7956c

rvagg reviewed Nov 7, 2024

View reviewed changes

akaladarshi added 2 commits November 7, 2024 14:21

add optional field codec in event filter

4f9b754

add tests for filter codec and max results

6cab093

akaladarshi marked this pull request as ready for review November 7, 2024 09:01

update changelog

8cca81c

akaladarshi requested a review from rvagg November 7, 2024 09:06

rvagg reviewed Nov 18, 2024

View reviewed changes

BigLep assigned akaladarshi Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add max results error and default raw codec for eth_getLogs #12671

fix: add max results error and default raw codec for eth_getLogs #12671

akaladarshi commented Nov 6, 2024

rvagg Nov 7, 2024

akaladarshi Nov 7, 2024 •

edited

Loading

rvagg Nov 7, 2024

rvagg Nov 18, 2024 •

edited

Loading

rvagg Nov 18, 2024

rvagg Nov 18, 2024

akaladarshi Nov 18, 2024 •

edited

Loading

rvagg Nov 18, 2024

rvagg Nov 18, 2024

	- Return error after hitting max results in event filtering and add default `raw` codec for eth_getLogs. ([filecoin-project/lotus#12671](https://github.com/filecoin-project/lotus/pull/12671))
	- Event APIs (`GetActorEventsRaw`, `SubscribeActorEventsRaw`, `eth_getLogs`, `eth_newFilter`, etc.) will now return an error when a request matches more than `MaxFilterResults` (default: 10,000) rather than silently truncating the results. Also apply an internal event matcher for `eth_getLogs` (etc.) to avoid builtin actor events on database query so as not to include them in `MaxFilterResults` calculation. ([filecoin-project/lotus#12671](https://github.com/filecoin-project/lotus/pull/12671))

	var ErrMaxResultsReached = xerrors.New("max results limit reached, results truncated")
	var ErrMaxResultsReached = fmt.Errorf("filter matches too many events, try a more restricted filter")

	Codec uint64 // optional codec filter, only used if KeysWithCodec is not set
	Codec multicodec.Code // optional codec filter, only used if KeysWithCodec is not set

fix: add max results error and default raw codec for eth_getLogs #12671

Are you sure you want to change the base?

fix: add max results error and default raw codec for eth_getLogs #12671

Conversation

akaladarshi commented Nov 6, 2024

Related Issues

Proposed Changes

Checklist

rvagg Nov 7, 2024

Choose a reason for hiding this comment

akaladarshi Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

rvagg Nov 7, 2024

Choose a reason for hiding this comment

rvagg Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

rvagg Nov 18, 2024

Choose a reason for hiding this comment

rvagg Nov 18, 2024

Choose a reason for hiding this comment

akaladarshi Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

rvagg Nov 18, 2024

Choose a reason for hiding this comment

rvagg Nov 18, 2024

Choose a reason for hiding this comment

akaladarshi Nov 7, 2024 •

edited

Loading

rvagg Nov 18, 2024 •

edited

Loading

akaladarshi Nov 18, 2024 •

edited

Loading