Skip to content
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

Fee Market Updates #17

Merged
merged 2 commits into from
Sep 19, 2022
Merged

Fee Market Updates #17

merged 2 commits into from
Sep 19, 2022

Conversation

Inphi
Copy link
Collaborator

@Inphi Inphi commented Sep 8, 2022

Implements the full fee market spec.

@Inphi Inphi force-pushed the inphi/fee-hdr branch 2 times, most recently from e14ad39 to 8e7bdb1 Compare September 12, 2022 14:32
@Inphi Inphi marked this pull request as ready for review September 12, 2022 14:33
- and update the blob fee rules to the full version
@Inphi
Copy link
Collaborator Author

Inphi commented Sep 12, 2022

cc: @dgcoffman

Copy link
Collaborator

@roberto-bayardo roberto-bayardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some initial comments/questions

core/types/transaction.go Outdated Show resolved Hide resolved
crypto/kzg/util.go Outdated Show resolved Hide resolved
params/protocol_params.go Show resolved Hide resolved
@@ -139,8 +139,8 @@ func Transaction(ctx *cli.Context) error {
r.Address = sender
}
// Check intrinsic gas
if gas, err := core.IntrinsicGas(tx.Data(), tx.AccessList(), len(tx.BlobVersionedHashes()), tx.To() == nil,
chainConfig.IsHomestead(new(big.Int)), chainConfig.IsIstanbul(new(big.Int))); err != nil {
if gas, err := core.IntrinsicGas(tx.Data(), tx.AccessList(), len(tx.BlobVersionedHashes()), 0, tx.To() == nil,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know a lot about this utility (looks like it's for testing?) but thought I'd ask if we might get into trouble by always using 0 for excess blobs. Should there maybe at least be a comment saying we always set it to 0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The utility is used to validate transactions. Up until EIP-4844, the intrinsic gas did not rely on the prestate of the execution. I imagine the utility may be updated in the future to take an excessBlobs parameter, but for now I think using zero is a nice default.

I agree that there should be a comment explaining this. Will add.

core/state_transition.go Outdated Show resolved Hide resolved
return adjusted - params.TargetBlobsPerBlock
}

// FakeExponential approximates 2 ** (num / denom)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we adopt George's proposed taylor expansion version already or will you update if/when it gets merged into the spec?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a PR for the spec changes? I'll update this once there's one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't see one yet... this was where I saw it, but makes sense to wait if/when. adietrichs/EIPs@5f91002

@Inphi Inphi merged commit d8e92d1 into mdehoog:eip-4844 Sep 19, 2022
@Inphi Inphi deleted the inphi/fee-hdr branch September 19, 2022 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants