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

Make db initialization configurable #3129

Merged
merged 2 commits into from
Mar 9, 2025
Merged

Conversation

bhartnett
Copy link
Contributor

@bhartnett bhartnett commented Mar 7, 2025

Changes in this PR:

  • Add a flag to make the CommonRef initializeDb() proc configurable (enabled by default) so that it can be disabled when using the Nimbus in-memory evm which doesn't always require the same data to be in the database. For the portal use case, all data is loaded on demand from portal network.

@bhartnett bhartnett requested review from jangko and arnetheduck March 7, 2025 03:50
@@ -453,6 +453,10 @@ proc getCode*(ac: LedgerRef,
returnHash: static[bool] = false): auto =
let acc = ac.getAccount(address, false)
if acc.isNil:
# We need to record that the code was read even if the account doesn't exist
# so that the returned multikeys correctly show that the code lookup occurred
ac.witnessCache[address] = WitnessData(codeTouched: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

For EVM execution without witnessCache, who will clear the table?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the whole witnessCache code path should be put behind compile time flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For EVM execution without witnessCache, who will clear the table?

It appears that the existing code does not clear the witnessCache. Perhaps the previous logic to clear the witnessCache was removed at some point.

Copy link
Contributor Author

@bhartnett bhartnett Mar 7, 2025

Choose a reason for hiding this comment

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

Probably the whole witnessCache code path should be put behind compile time flag.

Great point, I guess this has the risk of impacting performance without the compile time flag. Actually I was thinking about rewriting and improving the witnessCache code to make it better suited to what I'm doing and putting it behind a compile time flag as you suggested.

I'll remove the witness cache change from this PR and do it in a future PR with the improved implementation.

@bhartnett bhartnett changed the title Make db initialization configurable and multikeys improvement Make db initialization configurable Mar 7, 2025
@jangko jangko merged commit b313292 into master Mar 9, 2025
17 checks passed
@jangko jangko deleted the make-db-init-configurable branch March 9, 2025 09:44
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.

2 participants