feat: eth: harden event processing #10215
Conversation
Stebalien
force-pushed
the
steb/txhash-topic
branch
from
405e261
to
280fcba
Compare
| ) | ||
| for _, entry := range entries { | ||
| // Check if the key is t1..t4 | ||
| if len(entry.Key) == 2 && "t1" <= entry.Key && entry.Key <= "t4" { |
There was a problem hiding this comment.
I would still like to hoist the key names as global vars. Can use EthTopicStart and EthTopicEnd, since we are now using range semantics. It just feels wrong to have constants buried inline here.
There was a problem hiding this comment.
That would be kind of dangerous. E.g., if someone decides to extend this to t12, it'll break.
There was a problem hiding this comment.
You mean that the value being declared as a global variable will make it more prominent and subject to inadvertent changes? We can make it private to avoid programmatic mutations, but developer error can happen anywhere in the code. (not strongly held, we usually just define fixed arbitrary values as constants in most codebases)
There was a problem hiding this comment.
No, I'm concerned about code distance. I want anyone changing them to read the code that uses them.
| for len(topics) <= idx { | ||
| topics = append(topics, ethtypes.EthHash{}) | ||
| } |
There was a problem hiding this comment.
Nit: we can actually pre-initialize the slice as we expect exactly len(entries) - 1 topics. Then we can fail if we go beyond that.
There was a problem hiding this comment.
We could probably also remove the topicsFoundCount counter.
There was a problem hiding this comment.
I'm currently allowing for arbitrary entries.
There was a problem hiding this comment.
I.e., len(entries) > len(topics).
This ensures they're always 32 bytes and padded, as required.
We now enforce the following rules: 1. No duplicate topics or data. 2. Topics must have 32 byte keys. 3. Topics may not be skipped. (e.g., no t1 & t3 without a t2). 4. Raw codecs. We _don't_ require that topics/data be emitted in any specific order. We _skip_ events with unknown keys. We _drop_ events that violate the above rules.
We can remove these later as we add more event types, but this will aid in debugging.
Stebalien
force-pushed
the
steb/txhash-topic
branch
from
280fcba
to
cc302dd
Compare
Stebalien
force-pushed
the
steb/txhash-topic
branch
from
6f7180a
to
b0d917d
Compare
This commit hardens event processing: