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

all: implement EIP-1153 transient storage #26003

Merged
merged 8 commits into from
Nov 16, 2022
Merged

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Oct 17, 2022

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

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#1

Link 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

30M gas * 1 TSTORE / 100 gas * 32 bytes / 1 TSTORE * 1MB / 2^20 bytes ~= 9.15MB

Co-authored-by: Sara Reynolds snreynolds2506@gmail.com

Copy link
Contributor

@holiman holiman left a 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.

Comment on lines 225 to 227
func (ch transientStorageChange) dirtied() *common.Address {
return ch.account
}
Copy link
Contributor

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.

Copy link
Contributor

@holiman holiman Oct 17, 2022

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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

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 agree with this, a dirty account implies that it was mutated in some way

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 agree with this, a dirty account implies that it was mutated in some way

98eff44 has dirtied() return nil

core/vm/opcodes.go Outdated Show resolved Hide resolved
@tynes
Copy link
Contributor Author

tynes commented Oct 23, 2022

Thank you for the review @holiman. I've addressed your feedback and moved the API away from the stateObject.

To run the ethereum/tests, I used the following setup:

./retesteth/retesteth -t GeneralStateTests/stEIP1153 -- \
    --testpath <path to ethereum/tests> \
    --all --vmtrace --clients t8ntooleip --singlenet London

Note that I was having trouble with the London+1153 syntax, so I just built evm with 1153 activated in London to run these tests for simplicity. Not sure why it was giving me an error

Copy link
Contributor

@holiman holiman left a 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

core/state/transient_storage.go Outdated Show resolved Hide resolved
core/vm/instructions.go Outdated Show resolved Hide resolved
core/state/statedb.go Show resolved Hide resolved
core/state/statedb.go Show resolved Hide resolved
core/state/transient_storage.go Show resolved Hide resolved
core/state/transient_storage.go Show resolved Hide resolved
core/state/transient_storage.go Show resolved Hide resolved
@holiman holiman changed the title eip1153: transient storage all: implement EIP-1153 transient storage Nov 11, 2022
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

tynes and others added 8 commits November 16, 2022 09:57
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.
@holiman holiman added this to the 1.11.0 milestone Nov 16, 2022
@holiman holiman merged commit b4ea2bf into ethereum:master Nov 16, 2022
tynes added a commit to tynes/erigon that referenced this pull request Nov 26, 2022
Implements EIP-1153 "Transient Storage"

https://eips.ethereum.org/EIPS/eip-1153

Based on ethereum/go-ethereum#26003
yperbasis added a commit to erigontech/erigon that referenced this pull request May 1, 2023
Port ethereum/go-ethereum#26003

---------

Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023
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>
NazariiDenha pushed a commit to scroll-tech/go-ethereum that referenced this pull request Apr 15, 2024
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>
NazariiDenha pushed a commit to scroll-tech/go-ethereum that referenced this pull request Apr 25, 2024
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>
Thegaram pushed a commit to scroll-tech/go-ethereum that referenced this pull request Apr 28, 2024
* 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>
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 27, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Oct 2, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Oct 16, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Oct 17, 2024
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants