-
Notifications
You must be signed in to change notification settings - Fork 758
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
client: kaustinen7 verkle testnet preparation #3433
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we look mostly uptodate, i have an open PR regarding system accounts that we needs to be finalized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated this based on latest master and cleaned up the references to stateroot in verifyVerkleProof
@@ -520,13 +500,13 @@ export class StatelessVerkleStateManager implements StateManagerInterface { | |||
* @param {Uint8Array} stateRoot - The stateRoot to verify the executionWitness against | |||
* @returns {boolean} - Returns true if the executionWitness matches the provided stateRoot, otherwise false | |||
*/ | |||
verifyVerkleProof(stateRoot: Uint8Array): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming verifyVerkleProof
on the StatelessVerkleStateManager
to verifyProof
would be more in line with the changes in #3557 and with other current implementations of statemanagers that we have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be something that we'd want to extract to separate helpers, in line with what we've been doing with some other classes (extracting methods into standalone helpers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will open a new issue for this one since we can update it for both of the verkle SM implementations at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if one has already been created, but created #3629 for this.
@gabrocheleau could we skip the failing tests for now and then continue work for the kaustinen7 changes (notably the fix I have for the |
@@ -215,3 +222,46 @@ export const getVerkleTreeKeyForStorageSlot = async ( | |||
|
|||
return concatBytes(getVerkleStem(verkleCrypto, address, treeIndex), toBytes(subIndex)) | |||
} | |||
|
|||
export function decodeVerkleLeafBasicData(encodedBasicData: Uint8Array): VerkleLeafBasicData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note that we should at some point (best still before breaking releases even if not particularly about Verkle) give the Verkle docs a bit more love and e.g. complete and clean up the docs in this Util
module (and e.g. give methods like decodeVerkleLeafBasicData()
some code docs).
Also the Verkle README should giv a somewhat more complete picture on and introduction to the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can give this a look and get some changes started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a basic description and example set up at #3630.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - will address kaustinen7 test data needs in a future PR
This PR makes the following updates to ready our client for Kaustinen stateless syncing:
BASIC_DATA
field. See Update EIP-4762: reworked gas schedule from interop ethereum/EIPs#8550 for more details.