-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gas mechanics phase 1 implementation. #1471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first round of superficial review.
return &gPool, nil | ||
} | ||
|
||
func CalculateL1GasUsed(data []byte, overhead *big.Int) *big.Int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a comment.
If I were to guess, this tries to estimate how much it will cost storing this tx to the l1.
If yes, I don't think it's working correctly.
Because the transaction will be compressed and encrypted.
I would say, it can only work by estimating. Taking the size, reducing it to 66% and then multiply by the cost of a non-zero byte
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how optimism does it. I'm not sure if taking the size directly and assuming it is all 1s is the correct approach to go, cause zero to ones ratio should somewhat be maintained after compression, right?
Nvm keep forgetting about encryption
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few more comments
ReceiptHash common.Hash `json:"receiptsRoot"` | ||
Number *big.Int `json:"number"` // height of the batch | ||
SequencerOrderNo *big.Int `json:"sequencerOrderNo"` // multiple batches can be created with the same height in case of L1 reorgs. The sequencer is responsible for including all of them in the rollups. | ||
GasLimit uint64 `json:"gasLimit"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our maximum gas limit per batch should be the one from ethereum/12.
our batch time =1s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the gas limit of a batch fills up, it should put all unexecuted transactions back in the mempool and return the batch
} | ||
accBalance := stateDB.GetBalance(*sender) | ||
|
||
cost, err := executor.gasOracle.GetGasCostForTx(tx, block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's call this EstimateL1StorageGasCost
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Why this change is needed
In order to pay for L1 fees and L2 computational costs, we need to introduce gas mechanics feature.
What changes were made as part of this PR
Support is added for depositing Ethereum. When transactions get put in a batch, funds are deducted to pay for rollup publishing costs. Withdrawals do not populate merkle proofs in the receipts, thus do not work with this PR. Base fee is also disabled.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks