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

core/state: move state account to core/types + abstracted "write account to trie" #23567

Merged
merged 10 commits into from
Sep 28, 2021

Conversation

gballet
Copy link
Member

@gballet gballet commented Sep 13, 2021

The verkle trie and state expiry EIPs propose a different way of storing the accounts. As a preparatory work, this PR abstracts account is stored inside the tree, and the implementation details are left to the tree code itself.

Note this is useful beyond verkle trees, as any other tree format will eventually require this change if they are to cohabit with MPTs during the transition period.

@@ -319,7 +319,7 @@ func handleMessage(backend Backend, peer *Peer) error {
if err != nil {
return p2p.Send(peer.rw, StorageRangesMsg, &StorageRangesPacket{ID: req.ID})
}
var acc state.Account
var acc accounT.Account
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var acc accounT.Account
var acc account.Account

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that intentional? That's a definite "Nope" IMO

This comment was marked as spam.

@@ -319,7 +319,7 @@ func handleMessage(backend Backend, peer *Peer) error {
if err != nil {
return p2p.Send(peer.rw, StorageRangesMsg, &StorageRangesPacket{ID: req.ID})
}
var acc state.Account
var acc accounT.Account
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that intentional? That's a definite "Nope" IMO

@@ -31,6 +31,7 @@ import (
"github.com/ethereum/go-ethereum/common/math"
"github.com/ethereum/go-ethereum/core/rawdb"
"github.com/ethereum/go-ethereum/core/state"
accounT "github.com/ethereum/go-ethereum/core/state/account"
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no!!

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yes :trollface:

Copy link
Member Author

Choose a reason for hiding this comment

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

seriously though, there is clearly a name clash, if you have a better naming suggestion, I'm happy to update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I do have a better sugggestion. Essentially, what you're doing is creating a separate package which contains the state types. As of right now, there's only one type. So how about having an account.go inside core/state/types.?

trie/secure_trie.go Outdated Show resolved Hide resolved
core/state/account/account.go Outdated Show resolved Hide resolved
Comment on lines 132 to 134
func (s *stateObject) EncodeRLP(w io.Writer) error {
return rlp.Encode(w, s.data)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Before you remove it completely, might be better to place a panic in it, to make sure we never ever use it aside from the locations where you've alredy replaced it with rlp.Encode(object.data)

light/trie.go Outdated Show resolved Hide resolved
trie/secure_trie.go Outdated Show resolved Hide resolved
Co-authored-by: Martin Holst Swende <martin@swende.se>
@gballet gballet force-pushed the try-update-account branch 2 times, most recently from 068db08 to 260fb94 Compare September 17, 2021 18:13
Comment on lines 124 to 125
func (s *stateObject) EncodeRLP(io.Writer) error {
panic("deprecated code, should not be called")
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be reverted

@fjl fjl removed the status:triage label Sep 21, 2021
@@ -85,6 +87,21 @@ func (t *SecureTrie) TryGetNode(path []byte) ([]byte, int, error) {
return t.trie.TryGetNode(path)
}

// TryUpdate account will abstract the write of an account to the
// secure trie.
func (t *SecureTrie) TryUpdateAccount(key []byte, acc *types.StateAccount) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method uses a pointer acc *types.StateAccount, but trie.TryUpdateAccount uses account types.StateAccount. Is there any particular reason not to be consistent?

}
if err = s.trie.TryUpdate(addr[:], data); err != nil {
if err := s.trie.TryUpdateAccount(addr[:], &obj.data); err != nil {
//if err := s.trie.TryUpdateAccount(addr[:], obj.data); 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.

remove this line

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.

One remaining question from my side, but I think it's ok. LGTM

@@ -130,7 +122,7 @@ func newObject(db *StateDB, address common.Address, data Account) *stateObject {

// EncodeRLP implements rlp.Encoder.
func (s *stateObject) EncodeRLP(w io.Writer) error {
return rlp.Encode(w, s.data)
return rlp.Encode(w, &s.data)
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes.. something? Not sure if it makes any difference -- but is it safe?

Copy link
Member Author

Choose a reason for hiding this comment

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

it won't make any difference because this EncodeRLP is not called anymore. I don't think that it will hurt in any case, and would save a copy when called.

@holiman holiman changed the title core/state: abstracted "write account to trie" method core/state: move state account to core/types + abstracted "write account to trie" Sep 28, 2021
@holiman holiman merged commit 443afc9 into ethereum:master Sep 28, 2021
@holiman holiman added this to the 1.10.9 milestone Sep 28, 2021
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Sep 28, 2021
…unt to trie" (ethereum#23567)

* core/state: abstracted "write account to trie" method

* fix appveyor build

* Apply suggestions from code review

Co-authored-by: Martin Holst Swende <martin@swende.se>

* review feedback

* core/state/accounts: move Account to core/types

* core/types: rename Account -> StateAccount

* core/state: restore EncodeRLP for stateObject

* core/types: add the missing file

* more review feedback

* more review feedback

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Felix Lange <fjl@twurst.com>
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
…unt to trie" (ethereum#23567)

* core/state: abstracted "write account to trie" method

* fix appveyor build

* Apply suggestions from code review

Co-authored-by: Martin Holst Swende <martin@swende.se>

* review feedback

* core/state/accounts: move Account to core/types

* core/types: rename Account -> StateAccount

* core/state: restore EncodeRLP for stateObject

* core/types: add the missing file

* more review feedback

* more review feedback

Co-authored-by: Martin Holst Swende <martin@swende.se>
Co-authored-by: Felix Lange <fjl@twurst.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants