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.

Sign up for GitHub

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

internal/ethapi: fix defaults for blob fields #29037

Merged
merged 4 commits into from
Feb 21, 2024
Merged

internal/ethapi: fix defaults for blob fields #29037

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3cc8afb
internal/ethapi: fix defaults for blob fields
s1na Feb 20, 2024
1474a8f
internal/ethapi: fix defaults for blob fields
s1na Feb 20, 2024
f97408f
rm cancun check
s1na Feb 20, 2024
ce80a95
Update internal/ethapi/transaction_args.go
s1na Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions internal/ethapi/transaction_args.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,20 @@ func (args *TransactionArgs) setDefaults(ctx context.Context, b Backend) error {

// setFeeDefaults fills in default fee values for unspecified tx fields.
func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) error {
head := b.CurrentHeader()
// Sanity check the EIP-4844 fee parameters.
if args.BlobFeeCap != nil && args.BlobFeeCap.ToInt().Sign() == 0 {
return errors.New("maxFeePerBlobGas must be non-zero")
s1na marked this conversation as resolved.
Show resolved Hide resolved
}
if b.ChainConfig().IsCancun(head.Number, head.Time) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This part I don't agree with. Geth should always be able to create blob transactions, it shouldn't depend on the chain head for that. If there is a restriction about sending them, it will be enforced by the txpool, but AFAIK even the pool accepts them before Cancun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I liked to have it because setDefaults doubles as a pre-validation for the EVM sometimes (createAccessList). Removed that check.

if err := args.setCancunFeeDefaults(ctx, head, b); err != nil {
return err
}
} else {
if args.BlobFeeCap != nil {
return errors.New("maxFeePerBlobGas is not valid before Cancun is active")
}
}
// If both gasPrice and at least one of the EIP-1559 fee parameters are specified, error.
if args.GasPrice != nil && (args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil) {
return errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified")
Expand All @@ -186,7 +200,6 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) erro
// other tx values. See https://github.com/ethereum/go-ethereum/pull/23274
// for more information.
eip1559ParamsSet := args.MaxFeePerGas != nil && args.MaxPriorityFeePerGas != nil

// Sanity check the EIP-1559 fee parameters if present.
if args.GasPrice == nil && eip1559ParamsSet {
if args.MaxFeePerGas.ToInt().Sign() == 0 {
Expand All @@ -198,13 +211,7 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) erro
return nil // No need to set anything, user already set MaxFeePerGas and MaxPriorityFeePerGas
}

// Sanity check the EIP-4844 fee parameters.
if args.BlobFeeCap != nil && args.BlobFeeCap.ToInt().Sign() == 0 {
return errors.New("maxFeePerBlobGas must be non-zero")
}

// Sanity check the non-EIP-1559 fee parameters.
head := b.CurrentHeader()
isLondon := b.ChainConfig().IsLondon(head.Number)
if args.GasPrice != nil && !eip1559ParamsSet {
// Zero gas-price is not allowed after London fork
Expand All @@ -215,21 +222,14 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) erro
}

// Now attempt to fill in default value depending on whether London is active or not.
if b.ChainConfig().IsCancun(head.Number, head.Time) {
if err := args.setCancunFeeDefaults(ctx, head, b); err != nil {
return err
}
} else if isLondon {
if args.BlobFeeCap != nil {
return errors.New("maxFeePerBlobGas is not valid before Cancun is active")
}
if isLondon {
// London is active, set maxPriorityFeePerGas and maxFeePerGas.
if err := args.setLondonFeeDefaults(ctx, head, b); err != nil {
return err
}
} else {
if args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil || args.BlobFeeCap != nil {
return errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active")
if args.MaxFeePerGas != nil || args.MaxPriorityFeePerGas != nil {
return errors.New("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active")
Comment on lines +225 to +226
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this clause is also part of what @fjl would oppose (and I think I agree)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm so it's a good question what to do in this case. Should we set gasPrice or no?

}
// London not active, set gas price.
price, err := b.SuggestGasTipCap(ctx)
Expand Down Expand Up @@ -286,6 +286,10 @@ func (args *TransactionArgs) setLondonFeeDefaults(ctx context.Context, head *typ

// setBlobTxSidecar adds the blob tx
func (args *TransactionArgs) setBlobTxSidecar(ctx context.Context, b Backend) error {
isCancun := b.ChainConfig().IsCancun(b.CurrentHeader().Number, b.CurrentHeader().Time)
if !isCancun && (args.Blobs != nil || args.Commitments != nil || args.Proofs != nil || args.BlobHashes != nil) {
return errors.New("blobs are only valid after Cancun is active")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be removed. But just so I understand, this would happen if the user passes in sidecar, but geth is not in cancun mode?
IF so, I think we should let the user proceed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Exactly if user sends in blobs to compute the proofs and commitments, however chain is still behind cancun.

// No blobs, we're done.
if args.Blobs == nil {
return nil
Expand Down
26 changes: 19 additions & 7 deletions internal/ethapi/transaction_args_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,14 @@ func TestSetFeeDefaults(t *testing.T) {
"legacy",
&TransactionArgs{MaxFeePerGas: maxFee},
nil,
errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active"),
errors.New("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active"),
},
{
"dynamic fee tx pre-London, priorityFee set",
"legacy",
&TransactionArgs{MaxPriorityFeePerGas: fortytwo},
nil,
errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active"),
errors.New("maxFeePerGas and maxPriorityFeePerGas are not valid before London is active"),
},
{
"dynamic fee tx, maxFee < priorityFee",
Expand Down Expand Up @@ -219,7 +219,7 @@ func TestSetFeeDefaults(t *testing.T) {
"legacy",
&TransactionArgs{BlobFeeCap: fortytwo},
nil,
errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active"),
errors.New("maxFeePerBlobGas is not valid before Cancun is active"),
},
{
"set gas price and maxFee for blob transaction",
Expand All @@ -235,6 +235,13 @@ func TestSetFeeDefaults(t *testing.T) {
&TransactionArgs{BlobHashes: []common.Hash{}, BlobFeeCap: (*hexutil.Big)(big.NewInt(4)), MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
nil,
},
{
"fill maxFeePerBlobGas when dynamic fees are set",
"cancun",
&TransactionArgs{BlobHashes: []common.Hash{}, MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
&TransactionArgs{BlobHashes: []common.Hash{}, BlobFeeCap: (*hexutil.Big)(big.NewInt(4)), MaxFeePerGas: maxFee, MaxPriorityFeePerGas: fortytwo},
nil,
},
}

ctx := context.Background()
Expand All @@ -244,11 +251,16 @@ func TestSetFeeDefaults(t *testing.T) {
}
got := test.in
err := got.setFeeDefaults(ctx, b)
if err != nil && err.Error() == test.err.Error() {
// Test threw expected error.
if err != nil {
if test.err == nil {
t.Fatalf("test %d (%s): unexpected error: %s", i, test.name, err)
} else if err.Error() != test.err.Error() {
t.Fatalf("test %d (%s): unexpected error: (got: %s, want: %s)", i, test.name, err, test.err)
}
// Matching error.
continue
} else if err != nil {
t.Fatalf("test %d (%s): unexpected error: %s", i, test.name, err)
} else if test.err != nil {
t.Fatalf("test %d (%s): expected error: %s", i, test.name, test.err)
}
if !reflect.DeepEqual(got, test.want) {
t.Fatalf("test %d (%s): did not fill defaults as expected: (got: %v, want: %v)", i, test.name, got, test.want)
Expand Down
Loading