Repository navigation
fix(event): limit contract ABI size when decoding events - #7091
0xbigapple wants to merge 4 commits into
Conversation
Matching a log to an event (ContractTriggerCapsule and BlockEventGet) rebuilds and hashes a signature string for every ABI entry, synchronously during block application. An ABI is bounded only by the transaction size limit, so the work of decoding a log grows with the ABI with no useful bound of its own. Add size checks to ContractEventParserAbi and apply them on the event-subscription path, which is node-local: it affects only what subscribers receive, not block validity. The check reads each log's own ABI with no shared state, so one contract or transaction cannot affect decoding of another: - an ABI with more than MAX_ABI_ENTRIES entries, or more than MAX_ABI_EVENTS non-anonymous events, is not decoded; its logs are delivered as raw contract logs; - an entry with more than MAX_ABI_ENTRY_PARAMS parameters, or an identifier longer than MAX_ABI_IDENTIFIER_LENGTH, is skipped so its signature is never built. Real contracts stay well within these limits; an over-limit ABI simply has its logs delivered undecoded, which subscribers can still decode client-side.
waynercheung
left a comment
There was a problem hiding this comment.
Thanks for the PR. Keeping these limits node-local is appropriate, and the mainnet distribution provides useful compatibility evidence.
[SHOULD] The guards reduce the work for oversized ABIs, but accepted ABIs still rebuild and hash event signatures for every log. On the block-application path, version 0 decodes synchronously while pushBlock holds the Manager monitor. Version 1 decodes inside synchronized (manager) in BlockEventLoad.load(), using the same monitor. Expensive decoding can therefore delay block processing in either version. Please provide reproducible stress results for in-limit ABIs with log-heavy blocks on both versions, including processing latency and allocation/GC behavior. Microbenchmarks show the remaining repeated work, but they do not by themselves establish node-level availability.
[SHOULD] Please qualify the raw-log fallback in the description and release note. On both paths, the fallback only reaches subscribers that have the corresponding contractlog/soliditylog trigger enabled. Event-only subscribers stop receiving the affected events, and they do not automatically receive the raw logs instead.
[DISCUSS] Version 0 applies matchFilter after decoding (ContractTriggerCapsule.java:138), while version 1 filters first (BlockEventGet.java:102). Moving the version 0 filter ahead of decoding would avoid work for excluded logs. This predates the PR and can be handled separately.
| && !ContractEventParserAbi.hasTooManyEntries(abi) | ||
| && !ContractEventParserAbi.hasTooManyEvents(abi) |
There was a problem hiding this comment.
[DISCUSS] Both paths rebuild and hash event signatures for every log, even when the ABI is shared within a transaction. A transaction/block-scoped signature index could avoid this repeated work. A longer-lived cache would need bounded capacity, correct ABI identity/reorg handling, and preservation of first-match behavior for duplicate signatures. This optimization can be a follow-up. The immediate request is the threshold validation described in the main comment.
There was a problem hiding this comment.
Good suggestion — caching the per-ABI signatures would cut the repeated hashing. Happy to discuss further.
There was a problem hiding this comment.
Thanks. One note for the follow-up: moving decoding off the lock or making it asynchronous keeps the total CPU the same, so under a sustained stream it turns the stall into an unbounded backlog. Version 0 is also synchronous on purpose (Manager.java:2483: it avoids the race between an async queue and the reorg cache clearing). The repeated work itself goes away with the index: all logs of a contract within a transaction share the same ABI instance (abiMap in LogInfoTriggerParser, and in BlockEventGet.parseLogs), so a transaction-scoped topic0 -> entry map built once would replace 12,000 x 512 signature hashes with 512 for the case you measured. I'd suggest the follow-up start there.
| @Test | ||
| public void normalSizedAbiIsDecoded() { | ||
| assertFalse(hasTooManyEntries(abiWithEntries(MAX_ABI_ENTRIES))); | ||
| } | ||
|
|
||
| @Test | ||
| public void oversizedAbiIsNotDecoded() { | ||
| assertTrue(hasTooManyEntries(abiWithEntries(MAX_ABI_ENTRIES + 1))); |
There was a problem hiding this comment.
[SHOULD] These tests only exercise the predicates, so removing either call-site guard would leave them green. Please add behavior tests for both production paths covering normal decoding, over-limit fallback, and raw-data preservation. Each over-limit case should contain a matching event/topic0 that would decode without the guard.
Also, abiWithEntries(1024) creates 1,024 events. That exceeds the event limit, so the ABI in normalSizedAbiIsDecoded would not be decoded in production, despite the test name. Please use mixed function/event ABIs to cover the two count limits independently:
- 1,023 functions + 1 event (1,024 entries): still decoded;
- 1,024 functions + 1 event: only the entry limit is exceeded;
- 513 events + a few functions (at most 1,024 entries): only the event limit is exceeded.
Please also cover an oversized entry followed by a valid matching entry, to verify that only the oversized entry is skipped.
There was a problem hiding this comment.
Done in a82450d.
Behavior tests — ContractAbiSizeLimitTest covers both paths (ContractTriggerCapsule, BlockEventGet): normal decoding, over-limit fallback to a raw log with the raw topics/data preserved, and an oversized entry followed by a valid matching entry where only the oversized one is skipped. Each over-limit case carries a topic0 matching an event present in the ABI, so it would decode without the guard — removing either call-site guard fails these tests.
Predicate tests — ContractEventParserAbiTest now uses mixed function/event ABIs to exercise the two count limits independently: 1,023 functions + 1 event (decoded), 1,024 functions + 1 event (only the entry limit exceeded), 513 events + 3 functions (only the event limit exceeded).
Thanks for the careful review. You're right that the limits don't address the per-log work for accepted ABIs. As you noted, decoding is synchronous on the block-application path in both versions (version 0 in Here is data from a local environment (macOS, JDK 17, Each transaction emits ~12,000
The two columns measure different things. On v0, decoding runs inside On GC and memory: no Full GC or Separately, I'll qualify the raw-log fallback in the description and release note: it only reaches subscribers that have the |
| && !ArrayUtils.isEmpty(topics.get(0).getData()) | ||
| && !ContractEventParserAbi.hasTooManyEntries(abi) | ||
| && !ContractEventParserAbi.hasTooManyEvents(abi) | ||
| && Args.getInstance().getStorage().isContractParseSwitch()) { |
There was a problem hiding this comment.
Small suggestion: could we check isContractParseSwitch() before hasTooManyEvents(abi), both here and in BlockEventGet.processTrigger (v1)? The current order can still scan the ABI when parsing is disabled. Reordering the existing condition would avoid that work while keeping the raw-log path below intact.
There was a problem hiding this comment.
Good catch. Fixed in 9b78007 — moved isContractParseSwitch() ahead of the two count checks in both paths, so a parsing-disabled node short-circuits before any ABI scan.
… paths Also make the predicate tests exercise the entry-count and event-count limits independently (mixed function/event ABIs).
8eb4ae4 to
a82450d
Compare
Short-circuit isContractParseSwitch() ahead of hasTooManyEntries/ hasTooManyEvents in both decode paths so a parsing-disabled node skips the O(n) ABI scan per log.
waynercheung
left a comment
There was a problem hiding this comment.
Thanks for the updates and the stress data. The raw-log compatibility note, the call-site guard tests and the parse-switch ordering are resolved. Caching, async decoding and moving matchFilter are fine as follow-ups and are not needed here.
[SHOULD] Two test gaps remain (verified with mutants):
- The fallback tests build the log with empty
data(trigger()usesnew byte[0]), so "raw data is preserved" compares two empty strings. ReplacingsetData(logInfo.getHexData())withsetData("")on either path still passes. Please use non-empty data. - Version 0 has no "oversized entry followed by a valid matching entry" case (only
v1_oversizedEntrySkippedButValidEntryMatchesexists), so changingcontinuetobreakinContractTriggerCapsulestill passes.
[NIT] "Follow up: None" reads as if the decode cost is fully addressed. Your stress results show in-limit load can still delay block processing, so a sentence on the planned follow-up in the description would help.
Use non-empty log data so the raw-log data-preservation assertions compare real hex, and add a v0 oversized-then-valid-entry case so a continue->break mutation in ContractTriggerCapsule is caught.
Thanks — both gaps fixed, each confirmed against the mutant you described.
Also updated the description's "Follow up" to note the residual in-limit case and the planned event-processing follow-up. |
|
[SHOULD] Please supplement the ABI statistics for both mainnet and Nile with the snapshot block height, sample count, and observed maxima for each limit enforced by this PR. You can also offer the scanning script/command, but it is not requeired. If any limit is exceeded, please provide the affected-contract count and representative addresses. Also, please clarify whether the statistics cover currently retained ABIs or all historical deployments, and scope the compatibility claim accordingly. |
What does this PR do?
Adds size checks to
ContractEventParserAbiand applies them on the event-subscription path (version 0 inContractTriggerCapsule.processTrigger, version 1 inBlockEventGet.processTrigger) so a contract ABI is size-checked before it is decoded. The check is node-local and reads each log's own ABI with no shared state:MAX_ABI_ENTRIES(1024) entries, or more thanMAX_ABI_EVENTS(512) non-anonymous events, is not decoded; its logs fall back to raw contract logs, which are delivered to subscribers that have thecontractlog/soliditylogtrigger enabled;MAX_ABI_ENTRY_PARAMS(64) parameters, or an identifier longer thanMAX_ABI_IDENTIFIER_LENGTH(256), is skipped.Why are these changes required?
Decoding a log into an event scans the whole contract ABI, synchronously during block application. Because the ABI is deployer-supplied and bounded only by the transaction size, this cost has no useful bound of its own on an event-subscribing node.
The limits were set from the distribution of deployed contract ABIs (mainnet
abistore, 714,295 contracts with a non-empty ABI); no real contract reaches them:The two count checks bound different costs:
MAX_ABI_ENTRIESis an O(1) check on how many entries are iterated (functions included);MAX_ABI_EVENTSis a tighter cap on just the non-anonymous events, which are the ones that actually hash a signature.This PR has been tested by:
ContractEventParserAbiTestcovers the predicate boundaries, with the entry-count and event-count limits exercised independently (mixed function/event ABIs).ContractAbiSizeLimitTestadds behavior tests on both decode paths (version 0ContractTriggerCapsule, version 1BlockEventGet): normal decoding, over-limit fallback to raw contract logs with the raw data preserved, and an oversized entry skipped while a valid matching entry still decodes. Each over-limit case carries a topic0 that would decode without the guard, so removing a call-site guard fails the test.Follow up
These limits bound the per-ABI decode cost, but not the total in-limit event volume, so a sustained stream of log-dense blocks can still delay event processing on a subscribing node. A transaction-scoped topic0 -> entry index — reusing the shared ABI instance so each contract's logs are hashed once per transaction instead of per log — is left as a follow-up.
Extra details
Node-local and non-consensus. Release note: an over-limit ABI is not decoded into events — its logs are delivered as raw contract logs, which reach subscribers that have the
contractlog/soliditylogtrigger enabled; a subscriber with onlycontractevent/solidityeventstops receiving the affected events and does not automatically receive the raw logs.