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

Verkle checkpoint db tests #3407

Merged
merged 11 commits into from May 9, 2024
Merged

Verkle checkpoint db tests #3407

merged 11 commits into from May 9, 2024

Conversation

scorbajio
Copy link
Contributor

@scorbajio scorbajio commented May 8, 2024

This change adds some unit testing to the verkle package and fixes an issue where db stats aren't being updated in the commit function when write operations are being written to the db.

Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.29%. Comparing base (d24ca11) to head (1003bff).
Report is 4 commits behind head on master.

❗ Current head 1003bff differs from pull request most recent head eed4486. Consider uploading reports for the commit eed4486 to get more accurate results

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 80.79% <ø> (?)
blockchain ?
client 84.23% <ø> (?)
common ?
evm 73.76% <ø> (?)
genesis 99.98% <ø> (?)
statemanager ?
util ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@@ -109,6 +109,7 @@ export class CheckpointDB implements DB {
opts: { keyEncoding: KeyEncoding.Bytes, valueEncoding: ValueEncoding.Bytes },
})
}
this._stats.db.writes += 1
Copy link
Contributor

Choose a reason for hiding this comment

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

In the trie package checkpoint, we only add writes for del operations and put operations. Why do we add it here to commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See bellow for full response: #3407 (comment)

Copy link
Contributor

@gabrocheleau gabrocheleau left a comment

Choose a reason for hiding this comment

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

Nice! Really like the additional tests, and it's great to see you guys jumping in and getting your hands dirty with the verkle work :)

Left one small question concerning the db.writes stats, as there seems to be an inconsistency in the way that we are doing this here vs. in the trie package.

The actual behavior seems to be consistent, but at the trie level, we are incrementing the db.writes count whenever there is a put operations and hasCheckpoints is false. See https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/trie/src/db/checkpoint.ts#L185-L200

I have limited familiarity with that section (i.e. the stats handling) of the code, but shouldn't this be done as low-level as possible (at the level where the actual db writes are happening), to avoid the need to manually increment in multiple places? Curious to hear your thoughts.

Can be merged once that's discussed/resolved.

@scorbajio
Copy link
Contributor Author

Nice! Really like the additional tests, and it's great to see you guys jumping in and getting your hands dirty with the verkle work :)

Left one small question concerning the db.writes stats, as there seems to be an inconsistency in the way that we are doing this here vs. in the trie package.

The actual behavior seems to be consistent, but at the trie level, we are incrementing the db.writes count whenever there is a put operations and hasCheckpoints is false. See https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/trie/src/db/checkpoint.ts#L185-L200

I have limited familiarity with that section (i.e. the stats handling) of the code, but shouldn't this be done as low-level as possible (at the level where the actual db writes are happening), to avoid the need to manually increment in multiple places? Curious to hear your thoughts.

Can be merged once that's discussed/resolved.

I did some more digging into the discrepancy and added some more tests for the CheckpointDB implementation in the trie package as well: It seems like the issue is coming from the batch function, since if checkpoints already exist, each batch operation is performed using the CheckpointDB's put/del functions, which already increment the write stat, but if checkpoints don't exist, the wrapped db's own batch function implementation is used, which doesn't have access to and does not increment the wrapping class' stats property. I've changed both to now increment that stats in the batch function if checkpoints don't exist.

Copy link
Contributor

@gabrocheleau gabrocheleau left a comment

Choose a reason for hiding this comment

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

thanks for addressing the review! lgtm

@gabrocheleau gabrocheleau merged commit ded3c6b into master May 9, 2024
33 of 34 checks passed
@scorbajio scorbajio deleted the verkle-checkpoint-db-tests branch May 9, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants