Repository navigation
fix(ethmonitor): handle Avalanche settled headers (ACP-194) and stop failing on a full publish queue - #221
Conversation
Avalanche Helicon headers carry settledHeight, and their logsBloom covers the blocks they settle rather than the block's own logs. Detect them from the raw block JSON, stop using the bloom to validate their logs, and check an empty eth_getLogs result against the block's receipts, which wait for execution, using the receipt logs when they disagree. eth_getBlockReceipts is tried first; per-transaction receipts are fetched in bounded batches when the node doesn't support it, or for a single call when it answers with a non-2xx error.
…ng on a full queue A block whose logs keep failing used to hold the publish queue until it overflowed into ErrFatal(ErrQueueFull), on any chain. After MaxLogsAttempts (default 10) failed fetches its logs are now taken from its receipts; outside settled-header chains, empty receipts for a block with a set bloom are rejected. A block still unresolved when it leaves the chain cache is published with empty logs, with an Error log and an alert, instead of stopping the monitor. This changes behaviour for every consumer: queue overflow is no longer fatal by default. Options.StrictLogs restores the previous behaviour for consumers that would rather stop than publish unconfirmed logs.
b545794 to
2a7e529
Compare
py-zoid
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a7e529761
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // On settled-header chains eth_getLogs does not wait for execution, so | ||
| // an empty result may be premature. Receipt calls do wait, so confirm | ||
| // it against them before it can be cached. | ||
| if settled && len(topics) == 0 && len(logs) == 0 { |
There was a problem hiding this comment.
When a cache backend returns an existing [] entry for a settled-header block, the getter—and therefore this receipt check—is skipped; the cache-hit path merely unmarshals with expectLogs == false and marks the block ready. Such an entry can come from an older monitor that cached a premature empty response for a zero-bloom settled header, causing the upgraded monitor to publish the block without its actual logs until cache expiry. Apply the receipt confirmation to cached empty responses as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c7206b3. Details on @patrislav's thread below.
patrislav
left a comment
patrislav
left a comment
There was a problem hiding this comment.
Thanks for this, the Avalanche analysis is right and the tests are thorough. I ran the ethmonitor package with -race on the PR head and everything passes. CI is green.
Verified
- Lock ordering is safe: the queue lock is only ever taken inside the chain lock.
logsAttemptslives on the chain's copy, which is the same pointer the queue holds, so eviction keeps the count.checkBlockReceiptsdecodable threshold matches what ethrpc actually drops, types above 2.matchTopicsfollows geth filter semantics.- Strict mode does not re-enable the bloom check on settled chains. I ran the settled end-to-end test with
StrictLogs = trueand it passes.
…retry rate-limited receipts - A cached empty logs result for a settled-header block is now checked against the block's receipts, and replaced with the receipt logs when they disagree. Entries written by monitors that did not confirm empty results, such as earlier versions sharing the cache, were otherwise accepted until they expired. - Block.LogsUnconfirmed is set on blocks published without confirmed logs, after eth_getLogs and receipts both failed for the block's whole stay in the chain cache, so consumers can tell them apart from blocks without logs. It is kept in block snapshots and by Blocks.Copy. - eth_getBlockReceipts errors in non-2xx responses no longer trigger the per-transaction receipts fallback, which multiplied requests against rate-limited providers. A method-not-found error is recognised at any HTTP status; any other error is retried.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7206b3007
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if len(receipts) < numTxs || decodable > numTxs { | ||
| return fmt.Errorf("%d receipts for %d transactions", len(receipts), numTxs) | ||
| } |
There was a problem hiding this comment.
When a block mixes transactions decoded by ethrpc with skipped types above DynamicFeeTxType, a partial eth_getBlockReceipts response can satisfy this count check. For example, a block with two decoded transactions and one skipped transaction is accepted if the response contains one decoded receipt and the skipped receipt, because len(receipts) == numTxs and decodable < numTxs; the block is then marked ready using logs that omit the missing decoded transaction. Validate the returned TransactionHash values against every transaction in block.Transactions() rather than relying only on these counts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b38f14f. checkBlockReceipts now requires a receipt for every decoded transaction, matched by transaction hash, on top of the count checks. A new test, i_block_receipts_missing_decoded_tx_with_typed_receipt, covers your example: two decoded transactions, with the second one's receipt replaced by a type-3 receipt. That block is no longer accepted. The per-tx fallback already fetches each receipt by its transaction hash, so it needed no change.
…ky receipts fallback - An empty transaction list now counts as an empty block only when the header carries the empty transactions root. Headers always include transactionsRoot, so the zero-root case only served test blocks, which now use the real empty root. - Note that once eth_getBlockReceipts is found unsupported, the monitor uses per-transaction receipts for the rest of its lifetime.
|
Thanks for the thorough review, @patrislav.
The description is updated to match. |
eth_getBlockReceipts results were only checked by count, so a partial response could pass when a receipt for a transaction type ethrpc does not decode stood in for a missing one, and the block was accepted without that transaction's logs. Every decoded transaction must now have a receipt, matched by transaction hash.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b38f14f3a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if len(receipts) < len(txs) || decodable > len(txs) { | ||
| return fmt.Errorf("%d receipts for %d transactions", len(receipts), len(txs)) |
There was a problem hiding this comment.
When a block contains transaction types that ethrpc skips, this lower bound only reflects the decoded transactions. If eth_getBlockReceipts returns every decoded receipt but omits a skipped transaction's receipt—for example, one legacy and one type-3 transaction with only the legacy receipt returned—these checks and the hash loop both pass, and a premature empty eth_getLogs response is accepted without the skipped transaction's logs. Query the block's actual transaction count on this path too and require the receipt count to match it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, but I'm keeping it as a known limitation rather than fixing it. It needs a node returning a partial eth_getBlockReceipts and the missing receipt belonging to a transaction type ethrpc doesn't decode. Closing it would add an eth_getBlockTransactionCountByHash call to every receipts lookup, plus extra error handling, for a case we haven't seen. 013e2e9 documents it in checkBlockReceipts. The more likely variant, a missing receipt for a decoded transaction, is caught since b38f14f.
Summary
Two related changes to
ethmonitor. They solve different problems and are split into separate commits.1. Handle Avalanche settled block headers (ACP-194 / Helicon)
Since Avalanche's Helicon upgrade (Continuous Execution; Fuji block 57403047, C-Chain mainnet block 95921105), a block is accepted before it is executed. Each header carries a new
settledHeightfield, and itslogsBloom/receiptsRootcover the blocks it settles ((parent.settledHeight, settledHeight], about 5s behind), not the block's own logs.eth_getLogsstill returns the block's own logs, and does not wait for execution.ethmonitor treated "bloom set but
eth_getLogsreturned[]" as an untrustworthy response and retried the block forever. The block blocked the publish queue until it overflowed intoErrFatal(ErrQueueFull), so monitors on Avalanche C-Chain and Fuji crash repeatedly. The opposite case was a silent data-loss path: a header that settles nothing has a zero bloom, so a premature[]for a block that does have logs was accepted.settledHeightpresent and non-null). No chain-ID list is needed.eth_getLogsresult for a block with transactions is checked against the block's receipts, which wait for execution, and the receipt logs are used when they disagree. An unconfirmed[]is never accepted or cached.eth_getBlockReceipts. If the node answers that the method doesn't exist, at any HTTP status, the monitor falls back toeth_getTransactionReceiptin bounded batches of 20, pluseth_getBlockTransactionCountByHash, so transactions ethrpc can't decode can't silently lose logs. Other receipts errors, including rate limits, are retried.Chains without
settledHeightare untouched by this part.2. Stop failing on a full publish queue (all chains)
Independent of Avalanche: on any chain, a block whose logs keep failing (for example a provider that keeps erroring on
eth_getLogs) held the queue untilErrFatal(ErrQueueFull).MaxLogsAttemptsfailed fetches (default 10), the block's logs are taken from its receipts. On chains without settled headers, empty receipts for a block whose bloom is set are rejected, which keeps the original bloom guard.BlockRetentionLimitblocks) is published with empty logs andLogsUnconfirmedset, with an Error log and anAlerteralert, instead of stopping the monitor.eth_getLogsresult for a block with transactions adds one receipts lookup, about 3–4% more requestseth_getLogsfails, recovers withinMaxLogsAttemptseth_getLogskeeps failing, receipts workErrFatalonce the queue overflowseth_getLogsand receipts both fail for the whole cache windowErrFatalLogsUnconfirmed+ Error log + alert (StrictLogs: stillErrFatal)Queue overflow caused by unresolved logs is no longer fatal by default. A consumer can now receive a block published without confirmed logs (only after
eth_getLogsand receipts have both failed for the block's whole stay in the chain cache; it's flagged withLogsUnconfirmed, logged at Error and alerted).New options and fields:
Options.MaxLogsAttempts(default 10): failedeth_getLogsattempts before falling back to receipts.Options.StrictLogs(default false): set it to restore the previous behaviour. Unconfirmed logs are never published, and overflow returnsErrFatal(ErrQueueFull). Consumers that persist progress and would rather stop than record a gap should consider it.Block.LogsUnconfirmed: set on a block published without confirmed logs (default mode only). ItsLogsis empty, but the block may have had logs.No API breaks:
filterLogsis unexported,Blockgains an unexported field and the newLogsUnconfirmedfield, and theOptionschanges are additive.Known limitations
LogTopicsset) don't get the receipts check, so a premature empty filtered result is accepted. This is pre-existing: filtered empty results were never validated.BlockRetentionLimit + TrailNumBlocksBehindHead <= 5, the queue can still overflow in default mode, because the chain retains at least 10 blocks while the queue cap is2 × BlockRetentionLimit.backfillChainLogsretries stuck blocks under the chain lock, and each can now cost aneth_getLogsplus a receipts call (the pattern is pre-existing).Testing
go test ./ethmonitor/ -count=1 -race -skip TestMonitorBasicpasses, and each commit builds and passes on its own. New tests:TestHasSettledHeight,TestSettledHeadersDetection: detection, including nested and in-string keys,nulland malformed JSON.TestSettledEmptyLogResponses: receipts confirmation, receipt logs, caching and replay, block-receipts fallback and memoisation, transient errors, validation failures, and batches staying under a 40-request node limit.TestIsMethodNotFoundError: only real method-not-found errors stopeth_getBlockReceiptsfrom being used, at any HTTP status.[]recheck (j_premature_empty_cached_by_peer_*) and the HTTP 429 and 400 receipts cases inTestSettledEmptyLogResponses.TestBlockLogsUnconfirmedSurvivesCopyAndSnapshot: the new flag survivesBlocks.Copyand block snapshots.TestLogsAttemptsFallBackToReceipts: theMaxLogsAttemptsfallback, topic filtering, the bloom guard on receipts, and one receipts call per attempt.TestMonitorUnresolvedBlocksAreNotFatal: end to end, default mode publishes with one Error log and one alert per block, and strict mode returnsErrFatal+ErrQueueFull.TestMonitorSettledChainDoesNotStall: end to end, a settled chain with set blooms and emptyeth_getLogspublishes every block. The control case (nosettledHeight, strict) reproduces the old overflow.go test ./...also fails ingo-ethereum/core/typesandgo-ethereum/rpc; those tests fail the same way onmasterand are untouched here.