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

internal/ethapi: fix panic in accesslist creation #23225

Merged
merged 4 commits into from Jul 28, 2021

Conversation

@MariusVanDerWijden
Copy link
Member

@MariusVanDerWijden MariusVanDerWijden commented Jul 16, 2021

closes #23223

Copy link
Contributor

@holiman holiman left a comment

I guess it fixes the panic, which is good, but I'm a bit unsure of the semantics here. So we tell it to apply a tx with 0 tipcap and 0 maxTipand0` gas -- won't it panic on invalid basefee?

Copy link
Contributor

@holiman holiman left a comment

I guess it's fine since we have nobasefee=true set

if gasPrice == nil {
gasPrice = new(big.Int)
}
msg := types.NewMessage(args.from(), args.To, uint64(*args.Nonce), args.Value.ToInt(), uint64(*args.Gas), gasPrice, big.NewInt(0), big.NewInt(0), args.data(), accessList, false)

I suggest assigning it outside of the for loop

	// gas price fallback
	if args.GasPrice == nil {
		args.GasPrice = (*hexutil.Big)(big.NewInt(0))
	}

if gasPrice == nil {
gasPrice = new(big.Int)
}
msg := types.NewMessage(args.from(), args.To, uint64(*args.Nonce), args.Value.ToInt(), uint64(*args.Gas), gasPrice, big.NewInt(0), big.NewInt(0), args.data(), accessList, false)
Copy link
Contributor

@rjl493456442 rjl493456442 Jul 19, 2021

Why don't you use the args.toMessage() function instead? Perhaps you can re-assign the accessList after converting it to the message. So that you don't need to handle the gasPrice.

Copy link
Member Author

@MariusVanDerWijden MariusVanDerWijden Jul 19, 2021

Yep using to message now, and set the accesslist beforehand

@holiman
Copy link
Contributor

@holiman holiman commented Jul 27, 2021

internal/ethapi/api.go:1446:4: ineffectual assignment to gasPrice (ineffassign)

internal/ethapi/api.go Outdated Show resolved Hide resolved
@holiman
Copy link
Contributor

@holiman holiman commented Jul 27, 2021

--- FAIL: TestGethClient (0.01s)

    --- FAIL: TestGethClient/TestAccessList (0.00s)

        gethclient_test.go:146: unexpected error: failed to apply transaction: 0xf1896776ada91d0cbcc861fac937cd06b86aba11cc55ba1fb264188e41e9dbbf err: max fee per gas less than block base fee: address 0x71562b71999873DB5b286dF957af199Ec94617F7, maxFeePerGas: 1 baseFee: 765625000

@@ -96,7 +96,7 @@ func (al accessList) equal(other accessList) bool {
func (al accessList) accessList() types.AccessList {
acl := make(types.AccessList, 0, len(al))
for addr, slots := range al {
tuple := types.AccessTuple{Address: addr}
tuple := types.AccessTuple{Address: addr, StorageKeys: []common.Hash{}}
Copy link
Member Author

@MariusVanDerWijden MariusVanDerWijden Jul 27, 2021

This fixes an issue where sometimes the AL tracer would produce an accesslist with nil StorageKeys. This would then fail the marshaling/unmarshaling

@holiman holiman merged commit 2faf796 into ethereum:master Jul 28, 2021
2 checks passed
@holiman holiman added this to the 1.10.7 milestone Jul 28, 2021
sidhujag added a commit to sidhujag/go-ethereum that referenced this issue Jul 28, 2021
* internal/ethapi: revert + fix properly in al tracer

* internal/ethapi: use toMessage instead of creating new message

* internal/ethapi: remove ineffassign

* core: fix invalid unmarshalling, fix test

Co-authored-by: Martin Holst Swende <martin@swende.se>
reds pushed a commit to reds/go-ethereum that referenced this issue Aug 28, 2021
* internal/ethapi: revert + fix properly in al tracer

* internal/ethapi: use toMessage instead of creating new message

* internal/ethapi: remove ineffassign

* core: fix invalid unmarshalling, fix test

Co-authored-by: Martin Holst Swende <martin@swende.se>
i-norden added a commit to vulcanize/go-ethereum that referenced this issue Sep 10, 2021
* internal/ethapi: revert + fix properly in al tracer

* internal/ethapi: use toMessage instead of creating new message

* internal/ethapi: remove ineffassign

* core: fix invalid unmarshalling, fix test

Co-authored-by: Martin Holst Swende <martin@swende.se>
atif-konasl added a commit to lukso-network/pandora-execution-engine that referenced this issue Oct 15, 2021
* internal/ethapi: revert + fix properly in al tracer

* internal/ethapi: use toMessage instead of creating new message

* internal/ethapi: remove ineffassign

* core: fix invalid unmarshalling, fix test

Co-authored-by: Martin Holst Swende <martin@swende.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants