core/types: Populate Jovian receipt fields #710
Conversation
sebastianst
force-pushed
the
seb/jovian-receipts
branch
from
406344f to
741994a
Compare
sebastianst
force-pushed
the
seb/jovian-receipts
branch
from
741994a to
954954c
Compare
| rs[i].OperatorFeeConstant = gasParams.operatorFeeConstant | ||
| } | ||
| } | ||
| if config.IsOptimismBedrock(new(big.Int).SetUint64(blockNumber)) && len(txs) >= 2 { |
There was a problem hiding this comment.
Condition changed: we still need the config.Optimism != nil, correct? Suggesting we leave the condition unchanged since it doesn't seem to have an impact on this PR.
| if config.IsOptimismBedrock(new(big.Int).SetUint64(blockNumber)) && len(txs) >= 2 { | |
| if config.Optimism != nil && len(txs) >= 2 && config.IsOptimismBedrock(new(big.Int).SetUint64(blockNumber)) { |
There was a problem hiding this comment.
I think the condition is equivalent because of this:
Lines 907 to 910 in 96738d2
| rs[i].OperatorFeeScalar = u32ptrTou64ptr(gasParams.operatorFeeScalar) | ||
| rs[i].OperatorFeeConstant = gasParams.operatorFeeConstant | ||
| } | ||
| if config.IsJovian(blockTime) { |
There was a problem hiding this comment.
nit: we can have isJovian := config.IsJovian(blockTime) at the top of the function to avoid checking on every receipt.
| rs[i].OperatorFeeConstant = gasParams.operatorFeeConstant | ||
| } | ||
| if config.IsJovian(blockTime) { | ||
| rs[i].DAFootprintGasScalar = &daFootprintGasScalar |
There was a problem hiding this comment.
It feels a tad wasteful to include the scalar (a block-level attribute) in every receipt when it can be statically inferred from the BlobGasUsed and the transaction.
I see we include other block-level attributes like the operatorFeeScalar in receipts, although I'm not sure if they can be inferred like the daFootprintGasScalar.
Any thoughts?
There was a problem hiding this comment.
Just noting also that we should specify whatever behaviour we want to enshrine in the specs, as was done here https://specs.optimism.io/protocol/isthmus/exec-engine.html#receipts.
| ) | ||
|
|
||
| // deriveOPStackFields derives the OP Stack specific fields for each receipt. | ||
| // It must only be called for blocks with at least two transactions. |
There was a problem hiding this comment.
nit: it will still work even if there is only the system deposit transaction right?
| // It must only be called for blocks with at least two transactions. | |
| // It must only be called for blocks with at least one transaction (the system deposit). |
There was a problem hiding this comment.
I think the system deposit (l1 attribs) tx would be skipped below because of the isDepositTx check.
I'm generally unsure about this requirement, it does match the condition in the calling function DeriveFields(), but I don't understand it. We already skip deposit transactions in the loop below, so I don't know why we can't just call this unconditionally (perhaps it is a performance optimisation?) And what if we had multiple deposits in a block, we would still call this function and potentially skip all the transactions in the loop.
There was a problem hiding this comment.
so I don't know why we can't just call this unconditionally (perhaps it is a performance optimisation?)
It would panic since we read txs[0].Data(). Ofc we can guard against this by checking the length of the txs slice, but I don't want to bikeshed this comment too much: as long as it accurately documents the behavior I'm happy.
There was a problem hiding this comment.
Yeah completely empty tx slice would be problematic, but shouldn't happen on an optimism chain, so the tx length check in the caller seems redundant to me still. Doesn't need to block this PR since I think it preserves original semantics, but I would like to either understand this better or circle back and change it.
There was a problem hiding this comment.
The genesis block might be an edge case.
|
This is a bit challenging to review since there are intersecting refactors in the same commit as the consensus changes. If you don't mind splitting this PR into separate commits @sebastianst, I'd be happy to give this another look. I gave it a first pass and tried to manually diff the changes locally, but I could have missed something. |
|
I was hoping this tool would make validating the test file migration easier https://app.semanticdiff.com/gh/ethereum-optimism/op-geth/pull/710 but it doesn't help much. |
3 participants
Description
Populates Jovian receipt fields during receipt field derivation:
daFootprintGasScalarandblobGasUsed(to individual transaction DA footprint).Note that most of the diff comes from moving code around into new filed to reduce the upstream diff and isolate OP Stack changes more into its own files to reduce merge conflicts.
Tests
Added new test for Jovian receipts derivation. Also refactored the existing tests to be less repetitive and fixed a few things, like proper clearing of all OP Stack additional receipt fields.
Metadata
Fixes #708