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.

Sign up for GitHub

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

Jump to bottom

chore(plugin/evm): remove core.BoundedBuffer from atomicTrie #767

Merged
merged 12 commits into from
Feb 10, 2025
Merged

chore(plugin/evm): remove core.BoundedBuffer from atomicTrie #767

merged 12 commits into from
Feb 10, 2025

Conversation

qdm12
Copy link
Collaborator

@qdm12 qdm12 commented Jan 28, 2025

Why this should be merged

  • Removes the atomic trie dependency on the core package
  • Unneeded use of a 1-length bounded buffer

How this works

  1. Add (deep) unit test for atomicTrie.AcceptTrie
    • Tests all happy code paths
    • Tests derefencing occurs by inserting a dirty node and checking the storage size goes back to zero
  2. Remove tipBuffer field, and use the existing lastAcceptedRoot field with a direct call to a.trieDB.Dereference on the previous root.

How this was tested

Test_atomicTrie_AcceptTrie with commit 1, 2 and 3.

Need to be documented?

No

Need to update RELEASES.md?

No

@qdm12 qdm12 force-pushed the qdm12/plugin/evm/atomic-trie-bounded-buffer branch from 73c24a8 to 4887094 Compare January 28, 2025 10:50
@qdm12 qdm12 marked this pull request as ready for review January 28, 2025 10:52
@qdm12 qdm12 requested review from ceyonur, darioush and a team as code owners January 28, 2025 10:52
@qdm12 qdm12 force-pushed the qdm12/plugin/evm/atomic-trie-bounded-buffer branch from 4887094 to e19cf1e Compare January 28, 2025 10:55
darioush
darioush reviewed Jan 28, 2025
View reviewed changes
plugin/evm/atomic_trie_test.go Outdated Show resolved Hide resolved
plugin/evm/atomic_trie_test.go Outdated Show resolved Hide resolved
plugin/evm/atomic_trie_test.go Outdated Show resolved Hide resolved
plugin/evm/atomic_trie_test.go Outdated Show resolved Hide resolved
plugin/evm/atomic_trie_test.go Outdated Show resolved Hide resolved
darioush
darioush previously approved these changes Jan 30, 2025
View reviewed changes
@qdm12 qdm12 enabled auto-merge (squash) January 31, 2025 09:37
qdm12
qdm12 commented Jan 31, 2025
View reviewed changes
plugin/evm/atomic_trie.go Outdated Show resolved Hide resolved
plugin/evm/atomic_trie.go Outdated Show resolved Hide resolved
ceyonur
ceyonur reviewed Jan 31, 2025
View reviewed changes
Copy link
Collaborator

@ceyonur ceyonur left a comment

Choose a reason for hiding this comment

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

overall LGTM, just few test nits

plugin/evm/atomic_trie_test.go Outdated Show resolved Hide resolved
plugin/evm/atomic_trie_test.go Outdated Show resolved Hide resolved
plugin/evm/atomic_trie_test.go Outdated Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/plugin/evm/atomic-trie-bounded-buffer branch from 39911e5 to 292f6b3 Compare January 31, 2025 15:35
@qdm12 qdm12 requested a review from ceyonur January 31, 2025 17:37
@qdm12 qdm12 force-pushed the qdm12/plugin/evm/atomic-trie-bounded-buffer branch from d9530ee to be62f78 Compare February 5, 2025 11:09
ceyonur
ceyonur reviewed Feb 6, 2025
View reviewed changes
plugin/evm/atomic_trie.go Outdated Show resolved Hide resolved
@qdm12 qdm12 requested review from darioush and ceyonur February 7, 2025 07:58
@qdm12 qdm12 merged commit 4b33be0 into master Feb 10, 2025
8 checks passed
@qdm12 qdm12 deleted the qdm12/plugin/evm/atomic-trie-bounded-buffer branch February 10, 2025 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darioush darioush darioush approved these changes

@ceyonur ceyonur ceyonur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qdm12 @darioush @ceyonur