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

Fix getAccountProof for Bonsai #5647

Closed

Conversation

gfukushima
Copy link
Contributor

PR description

This PR fix the eth_getProof RPC method when using Bonsai by adding the worldStateProof to BonsaiWorldStateProvider.
By passing a snapshot of the ws that we want the proof for the BonsaiWorldStateProvider should be able to make normal usage of the worldStateProof to generate the proofs.

Fixed Issue(s)

Part of #5377

…ateProvider.

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
@github-actions
Copy link

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

@gfukushima gfukushima requested a review from matkt June 28, 2023 00:54
Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

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

seems to be good

do we can add test for bonsai in this class WorldStateProofProviderTest. the current tests are only using forest

you can use this class as example SnapWorldDownloadStateTest we are using Parameterized to call the same tests with bonsai and forest

@gfukushima gfukushima added the TeamGroot GH issues worked on by Groot Team label Jun 29, 2023
@gfukushima gfukushima requested a review from matkt July 5, 2023 03:00
Comment on lines 76 to 83
if (dataStorageFormat.equals(DataStorageFormat.BONSAI)) {
worldStateStorage =
new BonsaiWorldStateKeyValueStorage(
new InMemoryKeyValueStorageProvider(), new NoOpMetricsSystem());
} else {
worldStateStorage = new WorldStateKeyValueStorage(new InMemoryKeyValueStorage());
}

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
if (dataStorageFormat.equals(DataStorageFormat.BONSAI)) {
worldStateStorage =
new BonsaiWorldStateKeyValueStorage(
new InMemoryKeyValueStorageProvider(), new NoOpMetricsSystem());
} else {
worldStateStorage = new WorldStateKeyValueStorage(new InMemoryKeyValueStorage());
}
worldStateStorage =
switch (dataStorageFormat) {
case BONSAI -> new BonsaiWorldStateKeyValueStorage(
new InMemoryKeyValueStorageProvider(), new NoOpMetricsSystem());
case FOREST -> new WorldStateKeyValueStorage(new InMemoryKeyValueStorage());
};

Comment on lines +121 to +124
if (location.isEmpty()) {
updater.saveWorldState(Bytes.EMPTY, worldStateTrie.getRootHash(), value);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

we have to save the state root and the blockhash in specific keys. when we are doing an isWorldStateAvailable it is this key that are checked. So if this keys are empty the isWorldStateAvailable will return false

Copy link
Contributor

@siladu siladu Jul 6, 2023

Choose a reason for hiding this comment

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

Maybe worth a comment explaining this?
Presume it's a bonsai-only issue?

Signed-off-by: Gabriel Fukushima <gabrielfukushima@gmail.com>
@matkt
Copy link
Contributor

matkt commented Jul 6, 2023

can we verify this GetProof method (compare with other client ?) from what i see getProof doesn't look good. I worked on this part a long time ago but I have a doubt now

Normally GetProof must return the siblings at each level in order to validate the proof. That doesn't seem like it regarding the code. I implemented a getProof method in another component so we can fix that easily but just wanted to be sure i'm not missing something

IMO it should be L1+L2+X for X

*         o
*        / \
*       o   o (L1)
*       / \
*  (L2)o  x

looking at the code it is L1+L2+X

*   (L1)  o
*        / \
*   (L2)o   o 
*       / \
*      o   x 

@gfukushima
Copy link
Contributor Author

I've tested and we do provide the right proofs but there's seems to be an issue for proofs there are not the latest state in bonsai. Will move this PR to draft until I get back to it.

@gfukushima gfukushima marked this pull request as draft July 26, 2023 02:42
@gfukushima gfukushima removed the TeamGroot GH issues worked on by Groot Team label Jul 26, 2023
@gfukushima
Copy link
Contributor Author

Closing this until I get back to it.

@gfukushima gfukushima closed this Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants