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

refactor state diffs not to require two states in memory #4986

Merged
merged 1 commit into from
May 30, 2023
Merged

refactor state diffs not to require two states in memory #4986

merged 1 commit into from
May 30, 2023

Conversation

tersec
Copy link
Contributor

@tersec tersec commented May 23, 2023

It wasn't feasible to integrate before into the rest of Nimbus without substantially increasing memory consumption due to holding a previous state in memory. However, the diff didn't use the vast majority of the previous state, so this uses a small, intermediate object which can be safely buffered.

With 600k validators, this object is still not tiny, but it's not part of the serialized compatibility surface, so can be optimized as warranted by either using a bitmap (8x less memory, less than 100KiB, constant regardless of what kind of validators exist) or optimizing by omitting certain kinds of withdrawal credentials (needs more than 1 bit of memory, because then some sort of either sparse or offset representation has to be used; a PackedSet would be reasonable).

Either way, this is simple, reasonably efficient in both time and space, and uses only built-in data structures.

@github-actions
Copy link

Unit Test Results

         9 files  ±0    1 071 suites  ±0   41m 10s ⏱️ + 4m 39s
  3 682 tests ±0    3 403 ✔️ ±0  279 💤 ±0  0 ±0 
15 649 runs  ±0  15 344 ✔️ ±0  305 💤 ±0  0 ±0 

Results for commit 8a3b3df. ± Comparison against base commit 8db65ef.

BeaconStateDiffPreSnapshot(
eth1_data_votes_recent:
if state0.eth1_data_votes.len > 0:
state0.eth1_data_votes[^1 .. ^1]
Copy link
Contributor

@zah zah May 30, 2023

Choose a reason for hiding this comment

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

The logic here is quite non-obvious. It would be useful to add a reference to replaceOrAddEncodeEth1Votes, explaining what the plan is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zah zah merged commit c9f1bf2 into unstable May 30, 2023
@zah zah deleted the lUw branch May 30, 2023 08:55
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