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

Apply updates recursively #32

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Apply updates recursively #32

wants to merge 2 commits into from

Conversation

dapplion
Copy link

@dapplion dapplion commented Jan 22, 2024

Motivation

Current main branch has a foot gun for nested List/Vectors. The code below will panic on tree_hash_root because l_sub still has pending updates. Ethereum consensus does have many nested complex types that involve mutability, but it would be nice to remove the possibility of runtime errors if there's not much overhead.

let mut l = List::<List<u64, U16>, U16>::default();

l.push(<_>::default()).unwrap();
let l_sub = l.get_mut(0).unwrap();
l_sub.push(1).unwrap();
let v = l_sub.get_mut(0).unwrap();
assert_eq!(*v, 1);
// l_sub.apply_updates().unwrap(); <<- Needs this line to not panic
l.apply_updates().unwrap();
l.tree_hash_root()

Impl

This PR introduces a new trait PendingUpdates that all values of List / Vector must implement. I'll take suggestions for a better name, since this will be very prevalent in the Lighthouse codebase.

@michaelsproul comments that the compiler may optimize an apply implementation inside of the for_each_mut loop away. However I'm not sure if that's the case with a Result involved + the current impl. Open to feedback.

Next

  • derive macro to implement PendingUpdates for structs

@dapplion dapplion requested review from macladson and michaelsproul and removed request for macladson January 22, 2024 05:52
@michaelsproul michaelsproul self-assigned this Jan 31, 2024
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