-
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
all: implement EIP-1153 transient storage #26003
Conversation
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.
A few comments. Generally the implementation looks pretty clean.
Also, I don't think there's any point updating the testdata -- not sure what commit you are updating it to, but it's not the latest. If it's some branch with 1153-tests, then that will have to be done later on once it's made it into develop
on tests.
core/state/journal.go
Outdated
func (ch transientStorageChange) dirtied() *common.Address { | ||
return ch.account | ||
} |
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.
We need to look into this: I think it is incorrect to report the account as dirtied by a transient op.
The dirtied
address is added to the journal dirties
, which is later used to figure out which accounts have been modified -- in the sense that the trie representation of the account is changed. But a transient op does not modify the trie representation.
Putting this note here as a todo to investigate a bit further.
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.
Ah, we do need to put it in journal.dirties
after all, because otherwise we won't call finalise
at the end of the tx, and if we don't do that, the transient storage doesn't get reset between transactions.
The cost for this is that we add the address to the prefetcher, which starts loading tries, to speed up the commit-phase later on. And if transient is adopted, then that is unnecessary work which we should avoid.
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.
I'm not so sure that a Storage
on the stateObject
is the best representation of a transient storage. The state-objects live longer than a single transaction. Having the transient on the objects themselves just forces us to iterate through them at the end of each tx, just to clean up.
The transient storage(s) are essentially a transaction-global map of address
--> Storage
. After each transaction, we could just nuke it out and prepare a fresh one.
This is how we did with the accesslist. In statedb.go
:
// Per-transaction access list
accessList *accessList
It uses the 'normal' journal, but it is otherwise it's own separate entity, not tied to stateobjects.
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 makes sense, it feels like it sits at a similar level of abstraction as the access list since it is applied on a per transaction basis. Thank you for the feedback, will update the design to follow this approach
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.
I am of the opinion that dirtied()
should return nil
. Setting a transient storage value does not make an account dirty
, in the trie-sense.
I'm open to being proven wrong
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.
I agree with this, a dirty account implies that it was mutated in some way
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.
I agree with this, a dirty account implies that it was mutated in some way
98eff44 has dirtied()
return nil
Thank you for the review @holiman. I've addressed your feedback and moved the API away from the To run the
./retesteth/retesteth -t GeneralStateTests/stEIP1153 -- \
--testpath <path to ethereum/tests> \
--all --vmtrace --clients t8ntooleip --singlenet London Note that I was having trouble with the |
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, a minor nit or two
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 `TSTORE` and `TLOAD` as specified by the following EIP: https://eips.ethereum.org/EIPS/eip-1153 https://ethereum-magicians.org/t/eip-1153-transient-storage-opcodes/553 Co-authored-by: Sara Reynolds <snreynolds2506@gmail.com>
Moves the transient storage API away from the state object based on PR feedback. Confirm that all ethereum/tests still pass using `retesteth`
Moves the actual opcode implementations to `eips.go` which contains opcodes added through EIPs. Also add method docs to the new functions.
Transient storage doesn't mutate the state so return `nil` instead of returning the account that had transient storage modified.
98eff44
to
5ea11ab
Compare
Port ethereum/go-ethereum#26003 --------- Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
Implements TSTORE and TLOAD as specified by the following EIP: https://eips.ethereum.org/EIPS/eip-1153 https://ethereum-magicians.org/t/eip-1153-transient-storage-opcodes/553 Co-authored-by: Sara Reynolds <snreynolds2506@gmail.com> Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Gary Rong <garyrong0905@gmail.com>
Implements TSTORE and TLOAD as specified by the following EIP: https://eips.ethereum.org/EIPS/eip-1153 https://ethereum-magicians.org/t/eip-1153-transient-storage-opcodes/553 Co-authored-by: Sara Reynolds <snreynolds2506@gmail.com> Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Gary Rong <garyrong0905@gmail.com>
Implements TSTORE and TLOAD as specified by the following EIP: https://eips.ethereum.org/EIPS/eip-1153 https://ethereum-magicians.org/t/eip-1153-transient-storage-opcodes/553 Co-authored-by: Sara Reynolds <snreynolds2506@gmail.com> Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Gary Rong <garyrong0905@gmail.com>
* all: implement EIP-1153 transient storage (ethereum#26003) Implements TSTORE and TLOAD as specified by the following EIP: https://eips.ethereum.org/EIPS/eip-1153 https://ethereum-magicians.org/t/eip-1153-transient-storage-opcodes/553 Co-authored-by: Sara Reynolds <snreynolds2506@gmail.com> Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Gary Rong <garyrong0905@gmail.com> * core/vm: move TSTORE,TLOAD to correct opcode nums (ethereum#27613) * core/vm: move TSTORE,TLOAD to correct opcode nums * core/vm: cleanup * fix tests, rename * goimports * enable 1153 at Curie * bump version * comment fix * improve test * version * testchainconfig * fix another test that affects newly added * fix previous test to clenaup after --------- Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com> Co-authored-by: Sara Reynolds <snreynolds2506@gmail.com> Co-authored-by: Martin Holst Swende <martin@swende.se> Co-authored-by: Gary Rong <garyrong0905@gmail.com> Co-authored-by: Marius van der Wijden <m.vanderwijden@live.de>
Implements
TSTORE
andTLOAD
as specified by the following EIP:https://eips.ethereum.org/EIPS/eip-1153
https://ethereum-magicians.org/t/eip-1153-transient-storage-opcodes/553
This PR includes test coverage of these opcodes.
We are interested in having this included as part of Shanghai, as talked about at the EIP panel at Devcon. There is general community support by application layer developers, see the Ethereum Magicians thread. There are 4 implementations so far, see the links below.
Includes an extensive test suite for
ethereum/tests
here snreynolds/tests#1Link to Shanghai planning issue #25820
Replaces #24463
For additional information, see https://www.eip1153.com/
Additional implementations:
The EIP itself takes into account the maximum amount of memory that can be allocated if an entire block's worth of gas is used to
TSTORE
data, see https://eips.ethereum.org/EIPS/eip-1153#security-considerations.About 9.15MB can be allocated in theory, plus the runtime overhead. cc @MariusVanDerWijden
Co-authored-by: Sara Reynolds snreynolds2506@gmail.com