Skip to content

internal/ethapi: fix defaults for blob fields #29037

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

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 all 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
36 changes: 17 additions & 19 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,14 @@ 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, if specified, must be non-zero")
}
if err := args.setCancunFeeDefaults(ctx, head, b); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

previously, setCancunFeeDefaults also internally called setLondonFeeDefaults, which makes sense to me -- makes it so it by default recursively does The Right Thing. You changed it so we now must make two explicit invocations.
Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decoupled them. Reason is sometimes we should set cancun fields but we can skip london fields, as they are already provided by the user.

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 it makes sense because setCancunFeeDefaults is now just one step in a series of steps.

return err
}
// 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 +194,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 +205,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 +216,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 All @@ -245,15 +239,19 @@ func (args *TransactionArgs) setFeeDefaults(ctx context.Context, b Backend) erro
func (args *TransactionArgs) setCancunFeeDefaults(ctx context.Context, head *types.Header, b Backend) error {
// Set maxFeePerBlobGas if it is missing.
if args.BlobHashes != nil && args.BlobFeeCap == nil {
var excessBlobGas uint64
if head.ExcessBlobGas != nil {
excessBlobGas = *head.ExcessBlobGas
}
// ExcessBlobGas must be set for a Cancun block.
blobBaseFee := eip4844.CalcBlobFee(*head.ExcessBlobGas)
blobBaseFee := eip4844.CalcBlobFee(excessBlobGas)
// Set the max fee to be 2 times larger than the previous block's blob base fee.
// The additional slack allows the tx to not become invalidated if the base
// fee is rising.
val := new(big.Int).Mul(blobBaseFee, big.NewInt(2))
args.BlobFeeCap = (*hexutil.Big)(val)
}
return args.setLondonFeeDefaults(ctx, head, b)
return nil
}

// setLondonFeeDefaults fills in reasonable default fee values for unspecified fields.
Expand Down
38 changes: 18 additions & 20 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 @@ -207,20 +207,6 @@ func TestSetFeeDefaults(t *testing.T) {
errors.New("both gasPrice and (maxFeePerGas or maxPriorityFeePerGas) specified"),
},
// EIP-4844
{
"set maxFeePerBlobGas pre cancun",
"london",
&TransactionArgs{BlobFeeCap: fortytwo},
nil,
errors.New("maxFeePerBlobGas is not valid before Cancun is active"),
},
{
"set maxFeePerBlobGas pre london",
"legacy",
&TransactionArgs{BlobFeeCap: fortytwo},
nil,
errors.New("maxFeePerGas and maxPriorityFeePerGas and maxFeePerBlobGas are not valid before London is active"),
},
{
"set gas price and maxFee for blob transaction",
"cancun",
Expand All @@ -235,6 +221,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 +237,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