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

add get proof for bonsai #5919

Merged
merged 8 commits into from
Sep 24, 2023
Merged

Conversation

matkt
Copy link
Contributor

@matkt matkt commented Sep 21, 2023

PR description

PR that adds getproof for Bonsai. With this implementation, we can serve the getproof for historical states and not just the head.

Fixed Issue(s)

fixes #5874

Signed-off-by: Karim TAAM <[email protected]>
@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

@matkt matkt requested review from garyschulte and jflo September 21, 2023 07:34
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

LGTM

final Hash worldStateRoot,
final Address accountAddress,
final List<UInt256> accountStorageKeys) {
return worldStateProof.getAccountProof(worldStateRoot, accountAddress, accountStorageKeys);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use this.worldStateRootHash instead? passing the state root makes sense for forest, but not really for bonsai.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you right I modified the code, I moved the getProof to archive again with some modification. it will normally makes sense now . if you can review again

@@ -28,7 +28,6 @@ public void setup() throws Exception {
}

public static Object[][] specs() {
return findSpecFiles(
new String[] {"eth"}, "getProof"); // getProof is not working with bonsai trie
return findSpecFiles(new String[] {"eth"});
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

LGTM. non-blocking question regarding arbitrary state root in bonsai. Otherwise 🚢

Signed-off-by: Karim TAAM <[email protected]>
Signed-off-by: Karim TAAM <[email protected]>
@macfarla macfarla merged commit 52795b6 into hyperledger:main Sep 24, 2023
jflo pushed a commit to jflo/besu that referenced this pull request Nov 10, 2023
* add get proof for bonsai

Signed-off-by: Karim TAAM <[email protected]>

* fix review

Signed-off-by: Karim TAAM <[email protected]>

* remove logs

Signed-off-by: Karim TAAM <[email protected]>

---------

Signed-off-by: Karim TAAM <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
NickSneo pushed a commit to NickSneo/besu that referenced this pull request Nov 12, 2023
* add get proof for bonsai

Signed-off-by: Karim TAAM <[email protected]>

* fix review

Signed-off-by: Karim TAAM <[email protected]>

* remove logs

Signed-off-by: Karim TAAM <[email protected]>

---------

Signed-off-by: Karim TAAM <[email protected]>
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.

eth_getProof: Account not found
4 participants