-
Notifications
You must be signed in to change notification settings - Fork 20.2k
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
core: implement EIP-3651, warm coinbase #25819
Conversation
core/state_transition.go
Outdated
@@ -320,6 +320,10 @@ func (st *StateTransition) TransitionDb() (*ExecutionResult, error) { | |||
// Set up the initial access list. | |||
if rules.IsBerlin { | |||
st.state.PrepareAccessList(msg.From(), msg.To(), vm.ActivePrecompiles(rules), msg.AccessList()) | |||
// EIP-3651 warm COINBASE | |||
if rules.IsShanghai { | |||
st.state.AddAddressToAccessList(st.evm.Context.Coinbase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not super-elegant, adding it like this means it's a journalled change. So it would be nicer to somehow integrate more closely in the "constructor" PrepareAccessList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there are a few other places where PrepareAccessList
is invoked (e.g. core/vm/runtime/runtime.go:Create
) which also needs to do this extra step, and are currently not.
If we turn it into only one step, then we don't have to worry about missing the warm-coinbase extra-step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about changing the PrepareAccessList
into
func (s *StateDB) PrepareAccessList(sender, coinbase common.Address, dst *common.Address, precompiles []common.Address, list types.AccessList, rules params.Rules) {
I.e: adding coinbase common.Address
, and also adding rules params.Rules
, so the method can add the coinbase if we're in shanghai.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the transient eip pr, the signature is changed (https://github.com/ethereum/go-ethereum/pull/26003/files#diff-c3757dc9e9d868f63bc84a0cc67159c1d5c22cc5d8c9468757098f0492e0658cR1071) , into
func (s *StateDB) Prepare(rules params.Rules, sender common.Address, dst *common.Address, precompiles []common.Address, list types.AccessList) {
So you only need to add the coinbase
I have now pushed a couple of changes,
|
@@ -1068,25 +1068,29 @@ func (s *StateDB) Commit(deleteEmptyObjects bool) (common.Hash, error) { | |||
// | |||
// Potential EIPs: | |||
// - Reset transient storage(1153) | |||
func (s *StateDB) Prepare(rules params.Rules, sender common.Address, dst *common.Address, precompiles []common.Address, list types.AccessList) { | |||
func (s *StateDB) Prepare(rules params.Rules, sender, coinbase common.Address, dst *common.Address, precompiles []common.Address, list types.AccessList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, can you also add the Shanghai Fork information here:
e.g.
Shanghai fork:
- Add coinbase to access list(3651)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nitpick, I feel like it's safe to not journal initial accessList change. These change will never be rollback IIUC.
But yeah, we can run the PR on a synced node, just ensure nothing broken?
It's chugging along now, started at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, can't approve since I'm author
Follow-up
All good |
tests/state_test.go
Outdated
@@ -28,6 +28,7 @@ import ( | |||
"testing" | |||
"time" | |||
|
|||
"github.com/ethereum/go-ethereum/common" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> build/cache/golangci-lint-1.49.0-linux-amd64/golangci-lint run --config .golangci.yml ./...
tests/state_test.go:31:2: "github.com/ethereum/go-ethereum/common" imported but not used (typecheck)
"github.com/ethereum/go-ethereum/common"
^
util.go:48: exit status 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implements EIP-3651, "Warm Coinbase", for Shanghai hardfork. Specification: https://eips.ethereum.org/EIPS/eip-3651.
* core: implement EIP-3651, warm coinbase (ethereum#25819) Implements EIP-3651, "Warm Coinbase", for Shanghai hardfork. Specification: https://eips.ethereum.org/EIPS/eip-3651. * improve test * update to shanghai * trigger ci * fix comments --------- Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
Implements EIP-3651, "Warm Coinbase", for Shanghai hardfork. Specification: https://eips.ethereum.org/EIPS/eip-3651.
Implements https://eips.ethereum.org/EIPS/eip-3651 for Shanghai.