all: EIP-4844 integration #27511
Closed
all: EIP-4844 integration #27511
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MariusVanDerWijden
force-pushed
the
4844-devnet-6
branch
2 times, most recently
from
e31b0e0 to
a94251c
Compare
karalabe
reviewed
Jun 19, 2023
karalabe
reviewed
Jun 19, 2023
karalabe
reviewed
Jun 19, 2023
MariusVanDerWijden
force-pushed
the
4844-devnet-6
branch
2 times, most recently
from
9581095 to
baa5c0a
Compare
MariusVanDerWijden
force-pushed
the
4844-devnet-6
branch
3 times, most recently
from
ff1b9fd to
ea9cf1b
Compare
holiman
reviewed
Jul 14, 2023
Comment on lines
251
to
252
| blobBalanceCheck.Mul(blobBalanceCheck, st.msg.BlobGasFeeCap) | ||
| balanceCheck.Add(balanceCheck, blobBalanceCheck) | ||
| // Pay for dataGasUsed * actual blob fee | ||
| blobFee := new(big.Int).SetUint64(dataGas) | ||
| blobFee.Mul(blobFee, eip4844.CalcBlobFee(*st.evm.Context.ExcessDataGas)) | ||
| mgval.Add(mgval, blobFee) |
Contributor
There was a problem hiding this comment.
this seems dangerous. See up above?
balanceCheck := mgval
So at this point, the one might alias the other. So IMO let's change that into balanceCheck := new(big.Int).Set(mgval) before this clause, so we don't change one when we want to change the other. Sneaky bugs
fjl
reviewed
Jul 18, 2023
Comment on lines
376
to
389
| if chain.Config().IsCancun(header.Number, header.Time) { | ||
| var blobs int | ||
| for _, tx := range txs { | ||
| blobs += len(tx.BlobHashes()) | ||
| } | ||
| dataGasUsed := uint64(blobs * params.BlobTxDataGasPerBlob) | ||
| header.DataGasUsed = &dataGasUsed | ||
| } |
Contributor
There was a problem hiding this comment.
Is this really the best place to update the header? It kind of seems like this belongs into the state transition, where we usually track the block gas?
miner/payload_building.go
Outdated
| // enough to run. The empty payload can at least make sure there is something | ||
| // to deliver for not missing slot. | ||
| empty, _, err := w.getSealingBlock(args.Parent, args.Timestamp, args.FeeRecipient, args.Random, args.Withdrawals, true) | ||
| empty, _, _, _, _, err := w.getSealingBlock(args.Parent, args.Timestamp, args.FeeRecipient, args.Random, args.Withdrawals, true) |
MariusVanDerWijden
force-pushed
the
4844-devnet-6
branch
from
e03b5ad to
d949917
Compare
MariusVanDerWijden
force-pushed
the
4844-devnet-6
branch
2 times, most recently
from
37c89d9 to
8c9da8e
Compare
lightclient
force-pushed
the
4844-devnet-6
branch
3 times, most recently
from
5273993 to
4a84e82
Compare
lightclient
force-pushed
the
4844-devnet-6
branch
from
6861f29 to
1b5cf90
Compare
lightclient
force-pushed
the
4844-devnet-6
branch
4 times, most recently
from
c81ab7b to
0e54abd
Compare
lightclient
force-pushed
the
4844-devnet-6
branch
from
0e54abd to
eafa695
Compare
lightclient
force-pushed
the
4844-devnet-6
branch
from
eafa695 to
7dba16c
Compare
Contributor
|
We have this in now, and all fixes for Cancun should go to master branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
4844 integration PR