Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Fixing problems in MPT found by light client #1610

Merged
merged 18 commits into from
Oct 12, 2023
Merged

Conversation

miha-stopar
Copy link
Collaborator

@miha-stopar miha-stopar commented Sep 18, 2023

Description

Witness generator fixes:

  • When branch is hashed, its children are stored as hashes. This causes problems when computing the neighbour node which is needed for the case when a node is deleted from a branch with two nodes (the branch dissappears, the remaining node moves one level up).
  • Placeholder storage leaf RLP - when the leaf value is set to zero, the RLP of the placeholder leaf RLP needs to be updated to reflect the shortened value.
  • CreateObject called when there is no storage trie for an account.
  • Setting code hash fixed.

Circuit fix:
When value is set to 0 (which deletes the key) and there are two nodes in the branch, then the node is deleted and the other node moves up one level (replaces the branch). The returned node is then this moved node which doesn't have the value 0 and the lookup into MPT tables fails.
Now the circuit is fixed to ensure the new value in the table is 0.

Besides that, some other problems are fixed which were found by @adria0 when integrating the light client and MPT (these were temporary hacks in MPT for testing purposes that were forgotten to be removed):

  • SetState calls removed in prepare_witness.go calls from
  • oracle.PrefetchStorage call uncommented in prepare_witness.go
  • oracle.PrefetchStorage call uncommented in state/state_object.go updateTrie
  • Account nodes were not appended to all nodes in prepare_witness.go.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

No test until now covered this scenario, now TestNeighbourNodeInHashedBranch has been added.

@miha-stopar miha-stopar changed the title Neighbour node fix for hashed branches Fixing problems in MPTWG found by light client Sep 20, 2023
@ed255 ed255 linked an issue Sep 21, 2023 that may be closed by this pull request
@miha-stopar miha-stopar marked this pull request as ready for review September 28, 2023 11:22
@rrtoledo
Copy link
Contributor

Could you add some unit tests to check that adding/updating/removing is done correctly perhaps?
We could find some test vectors from other MPT libs

@github-actions github-actions bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Sep 28, 2023
@miha-stopar
Copy link
Collaborator Author

Could you add some unit tests to check that adding/updating/removing is done correctly perhaps?
We could find some test vectors from other MPT libs

You mean to the trie? The geth code is used, so it should be ok I think?

@miha-stopar miha-stopar changed the title Fixing problems in MPTWG found by light client Fixing problems in MPT found by light client Sep 28, 2023
@rrtoledo
Copy link
Contributor

Could you add some unit tests to check that adding/updating/removing is done correctly perhaps?
We could find some test vectors from other MPT libs

You mean to the trie? The geth code is used, so it should be ok I think?

Yes, I meant for the trie. I thought not all cases might be covered an error was found (hence the PR) but I guess it is not trie based per se then.

@miha-stopar
Copy link
Collaborator Author

Could you add some unit tests to check that adding/updating/removing is done correctly perhaps?
We could find some test vectors from other MPT libs

You mean to the trie? The geth code is used, so it should be ok I think?

Yes, I meant for the trie. I thought not all cases might be covered an error was found (hence the PR) but I guess it is not trie based per se then.

Yeah, the trie should be ok I think. What this PR solves are some bugs related to the conversion of the getProof result to the MPT witness + MPT table fix for a case when the leaf is deleted and there were just two leaves in the branch.

mpt-witness-generator/witness/leaf.go Outdated Show resolved Hide resolved
mpt-witness-generator/witness/leaf.go Show resolved Hide resolved
mpt-witness-generator/witness/prepare_witness.go Outdated Show resolved Hide resolved
@rrtoledo
Copy link
Contributor

rrtoledo commented Oct 2, 2023

I'm not sure to understand everything you're doing, but here are a few fixes and questions I have.

Copy link
Member

@adria0 adria0 left a comment

Choose a reason for hiding this comment

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

Unable to verify the correctess since I did not work on this part.
Did a very lightweight review in order to merge into main.

Copy link
Member

@adria0 adria0 left a comment

Choose a reason for hiding this comment

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

Tested with zklight branch and works ok

Copy link
Contributor

@rrtoledo rrtoledo left a comment

Choose a reason for hiding this comment

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

LGTM!

@miha-stopar miha-stopar added this pull request to the merge queue Oct 12, 2023
Merged via the queue into main with commit 2176191 Oct 12, 2023
13 checks passed
@miha-stopar miha-stopar deleted the mptwg-neighbour-node branch October 12, 2023 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fixing problems in MPTWG found by light client
3 participants