singular batch from op-node is log without the transaction info#392
singular batch from op-node is log without the transaction info#392shenkeyao merged 7 commits intocelo-integration-rebase-16.2from
singular batch from op-node is log without the transaction info#392Conversation
|
Claude finished @shenkeyao's task in 1m 9s —— View job Code Review: PR #392
Overall: Looks good. The change is clean and well-motivated — logging tx hashes instead of raw bytes is the right call for both readability and avoiding DataDog truncation. A few observations:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the logging mechanism in AttributesQueue.NextAttributes to log transaction hashes instead of raw transaction bytes, aiming to prevent log truncation in external aggregators. A review comment highlights concerns regarding the performance overhead of RLP decoding within the derivation pipeline's hot path and notes that silently skipping malformed transactions could result in inconsistent or misleading log data.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
jjeangal
left a comment
jjeangal
left a comment
There was a problem hiding this comment.
Little comments but fine to approve
| aq.batch = batch | ||
| aq.concluding = concluding | ||
| aq.log.Info("singular batch from op-node is ", "batch", aq.batch, "concluding", concluding) | ||
| // Log tx hashes instead of raw bytes, since hashes are compact whereas raw tx bytes can |
There was a problem hiding this comment.
Little nit: we can simplify this comment.
| if err := tx.UnmarshalBinary(rawTx); err == nil { | ||
| txHashes = append(txHashes, tx.Hash()) | ||
| } else { | ||
| aq.log.Warn("failed to unmarshal transaction", "index", i, "err", err) |
There was a problem hiding this comment.
Could that not spam a lot of logs if there are errors?
There was a problem hiding this comment.
It wouldn't be too spammy since there would be just one log for each malformed transaction, but on second thought, it doesn't make much sense to log a malformed transaction anyway, so I've removed it!
shenkeyao
merged commit 8fd7cec
into
celo-integration-rebase-16.2
* Add log * Add hashes * Update op-node/rollup/derive/attributes_queue.go Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * Fix lint * Simplify comment, remove malformed tx log --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
2 participants
Closes https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1212148617379345?focus=true.
This PR:
This PR does not: