core/state, light, les: make signature of ContractCode hash-independent #27209

gballet · 2023-05-02T12:38:18Z

Like the previous changes to the state.Trie interface, in which functions were modified to use the un-hashed account address instead of its hashed version, the same thing needs to be applied to ContractCode since it takes the hash of the account's address as a parameter.

The only major problem is with LES: I have changed the request object to contain the un-hashed address instead of its hash.

Alternatively, it seems to me that this parameter is only used in LES. If we remove LES, then we can remove this parameter entirely.

holiman · 2023-05-02T13:38:07Z

light/trie.go

@@ -80,14 +80,14 @@ func (db *odrDatabase) ContractCode(addrHash, codeHash common.Hash) ([]byte, err
 		return code, nil
 	}
 	id := *db.id
-	id.AccKey = addrHash[:]
+	id.AccKey = addr[:]


Feels a bit strange to just change a byteslice representing a hash into representing an address. Just changing it doesn't mean it will work, does it?

it won't work if the 'server' doesn't implement this change, but if both client and server do - and if I understand correctly - it will simply replace the hash of the data with the plain address in the request.

I checked that all the locations were updated accordingly, but given the errors in CI, it looks like I missed a point somewhere. I'll wait for Zsolt's input before I fix it, the best approach is still the entire removal of LES if it can be done.

core/state/iterator.go

holiman

LGTM. LES is going through changes anyway, and it's not working as it is, so that shouldn't be a blocker (?)

rjl493456442 · 2023-06-13T03:37:35Z

core/state/sync_test.go

@@ -204,7 +204,7 @@ func testIterativeStateSync(t *testing.T, count int, commit bool, bypath bool) {
 			codeResults = make([]trie.CodeSyncResult, len(codeElements))
 		)
 		for i, element := range codeElements {
-			data, err := srcDb.ContractCode(common.Hash{}, element.code)
+			data, err := srcDb.ContractCode(common.Address{}, element.code)


We must roll out a new snap sync protocol for verkle, later.

In original scheme, contract code is only identified by code hash. While in verkle, contract code is identified by both address and some other information. Therefore, snap protocol has to be changed.

We can just skip it since verkle doesn't support snap sync anyway.

rjl493456442 · 2023-06-13T04:14:35Z

core/state/sync_test.go

@@ -628,13 +622,6 @@ func TestIncompleteStateSync(t *testing.T) {
 		}
 		batch.Write()

-		for _, root := range nodehashes {


It's a very hacky check so that I remove it.

What it does is: iterate the newly added nodes, treat them as the root node of the "sub-trie" and traverse the "sub-trie" respectively. This notion is not compatible in path-based also.

rjl493456442

Lgtm. @holiman Please take another look.

Just one thing to keep in mind: we need to change snap sync protocol for verkle later.

gballet · 2023-06-13T12:14:42Z

core/state/database.go

@@ -208,7 +209,7 @@ func (db *cachingDB) ContractCode(addrHash, codeHash common.Hash) ([]byte, error
 // ContractCodeWithPrefix retrieves a particular contract's code. If the
 // code can't be found in the cache, then check the existence with **new**
 // db scheme.
-func (db *cachingDB) ContractCodeWithPrefix(addrHash, codeHash common.Hash) ([]byte, error) {
+func (db *cachingDB) ContractCodeWithPrefix(_, codeHash common.Hash) ([]byte, error) {


We should make this first parameter a common.Address to be consistent

Good catch!

core/state/iterator.go

rjl493456442 · 2023-06-14T02:10:13Z

@gballet Please take another look. I have fixed all the comments.

Leena8686

Making of signature of ContractCode hash-independent. Thanks for all you'll hard work.

gballet

@rjl493456442 thanks for your changes, they LGTM. I have rebased on top of master in order to fix the merge error.

Leena8686

Make signature of ContractCode hash-independent.

rjl493456442

LGTM!

karalabe · 2023-06-27T16:53:27Z

core/state/iterator.go

@@ -27,7 +28,8 @@ import (
 )

 // nodeIterator is an iterator to traverse the entire state trie post-order,
-// including all of the contract code and contract state tries.
+// including all of the contract code and contract state tries. Preimage is
+// required in order to resolve the contract address.


Are we sure this is safe? Preimages are not enabled by default AFAIK in Geth, so does this mean node iteration all of a sudden breaks with this PR?

State iterator is useful to traverse the whole state, but it's not UX friendly, because the state keys are all hash instead of raw keys. In order to provide some meaningful information, preimages are still required.

However, we already have the state dumper, IMO we should merge these two together and get rid of one of them.

In case of this PR, state iterator is only used in test, to verify the state completeness, which I think it's a nice tool so I kept it.

…nt (ethereum#27209) * core/state, light, les: make signature of ContractCode hash-independent * push current state for feedback * les: fix unit test * core, les, light: fix les unittests * core/state, trie, les, light: fix state iterator * core, les: address comments * les: fix lint --------- Co-authored-by: Gary Rong <garyrong0905@gmail.com>

…ndependent (ethereum#27209)" This reverts commit bbee228.

gballet requested review from zsfelfoldi, rjl493456442, karalabe and holiman as code owners May 2, 2023 12:38

holiman reviewed May 2, 2023

View reviewed changes

gballet commented May 10, 2023

View reviewed changes

core/state/iterator.go Outdated Show resolved Hide resolved

gballet mentioned this pull request May 10, 2023

core/state: unexport NodeIterator #27239

Merged

gballet force-pushed the address-in-contractcode branch from 5f9b950 to 95ee5ab Compare May 15, 2023 08:08

holiman approved these changes May 22, 2023

View reviewed changes

Leena8686 approved these changes Jun 3, 2023

View reviewed changes

rjl493456442 reviewed Jun 13, 2023

View reviewed changes

rjl493456442 force-pushed the address-in-contractcode branch 2 times, most recently from e837e1d to 59bada1 Compare June 13, 2023 04:09

rjl493456442 reviewed Jun 13, 2023

View reviewed changes

rjl493456442 approved these changes Jun 13, 2023

View reviewed changes

gballet commented Jun 13, 2023

View reviewed changes

Leena8686 approved these changes Jun 14, 2023

View reviewed changes

gballet and others added 7 commits June 26, 2023 11:46

core/state, light, les: make signature of ContractCode hash-independent

d8db662

push current state for feedback

09f39ce

les: fix unit test

f5ba414

core, les, light: fix les unittests

6467089

core/state, trie, les, light: fix state iterator

f3c6770

core, les: address comments

8beaead

les: fix lint

b54c50a

gballet force-pushed the address-in-contractcode branch from 4daf648 to b54c50a Compare June 26, 2023 10:01

gballet commented Jun 26, 2023

View reviewed changes

Leena8686 approved these changes Jun 26, 2023

View reviewed changes

rjl493456442 approved these changes Jun 27, 2023

View reviewed changes

gballet added the status:triage label Jun 27, 2023

karalabe reviewed Jun 27, 2023

View reviewed changes

karalabe added this to the 1.12.1 milestone Jun 28, 2023

karalabe merged commit 8bbb16b into ethereum:master Jun 28, 2023

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "core/state, light, les: make signature of ContractCode hash-i…

5fba246

…ndependent (ethereum#27209)" This reverts commit bbee228.

devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023

Revert "core/state, light, les: make signature of ContractCode hash-i…

859b5c3

…ndependent (ethereum#27209)" This reverts commit bbee228.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/state, light, les: make signature of ContractCode hash-independent #27209

core/state, light, les: make signature of ContractCode hash-independent #27209

gballet commented May 2, 2023

holiman May 2, 2023

gballet May 2, 2023

holiman left a comment

rjl493456442 Jun 13, 2023

rjl493456442 Jun 13, 2023

rjl493456442 left a comment

gballet Jun 13, 2023

rjl493456442 Jun 14, 2023

rjl493456442 commented Jun 14, 2023

Leena8686 left a comment

gballet left a comment

Leena8686 left a comment

rjl493456442 left a comment

karalabe Jun 27, 2023

rjl493456442 Jun 28, 2023

rjl493456442 Jun 28, 2023

core/state, light, les: make signature of ContractCode hash-independent #27209

core/state, light, les: make signature of ContractCode hash-independent #27209

Conversation

gballet commented May 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjl493456442 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjl493456442 commented Jun 14, 2023

Leena8686 left a comment

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

Leena8686 left a comment

Choose a reason for hiding this comment

rjl493456442 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment