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

ViewDU container rebinds node with read-only operations #379

Open
twoeths opened this issue Jul 9, 2024 · 2 comments · May be fixed by #417
Open

ViewDU container rebinds node with read-only operations #379

twoeths opened this issue Jul 9, 2024 · 2 comments · May be fixed by #417
Assignees

Comments

@twoeths
Copy link
Contributor

twoeths commented Jul 9, 2024

Describe the bug

Consider this unit test here in #378

  it("should not recompute hashTreeRoot() when no fields is changed", () => {
    const root = state.hashTreeRoot();
    // this causes viewsChanged inside BeaconState container
    state.validators.length;
    state.balances.length;
    // but we should not recompute root, should get from cache instead
    const root2 = state.hashTreeRoot();
    expect(root2).to.equal(root, "should not recompute hashTreeRoot() when no fields are changed");
  });

the BeaconState container tracked validators and length as "viewsChanged" and it rebinds the underlying tree to compute new root with same nodes, which results in the same value but different tree/nodes, and we need to rehash the different nodes

note that #378 implemented a cache that returns the same root for multiple ViewDU.hashTreeRoot() calls

Expected behavior

  • according to the design of ssz v2, BeaconState container should still track validators and balances as "viewsChanged" so that when we modify them they will escalate the change to BeaconState. However when we commit those viewDUs, if we see no change we should not rebind the nodes, and it should return the same cached root

Steps to Reproduce

@philknows
Copy link
Member

@twoeths Can this be closed via #380?

@twoeths
Copy link
Contributor Author

twoeths commented Oct 9, 2024

@twoeths Can this be closed via #380?

@philknows not yet since I merged it to the te/batch_hash_tree_root branch. Will close it once we merge to master

@twoeths twoeths linked a pull request Oct 21, 2024 that will close this issue
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 a pull request may close this issue.

2 participants