Skip to content

Add redundant writes to Amt and Hamt to match go impl #729

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

Closed
austinabell opened this issue Oct 5, 2020 · 2 comments · Fixed by #731
Closed

Add redundant writes to Amt and Hamt to match go impl #729

austinabell opened this issue Oct 5, 2020 · 2 comments · Fixed by #731
Labels
Priority: 3 - Medium Nice-to-have, does not impede core functionality

Comments

@austinabell
Copy link
Contributor

Issue summary

Was waiting to create this issue because it was potentially going to change for the Amt, but is no longer the case. This issue is just to track adding in the unnecessary writes through flushing sub nodes on expanding of Amt and Hamt.

This is necessary because of gas charges for blockstore usage in the VM.

Will be done after #719 is finished (wip) to group update needed for Amt as well (and to allow it to be reverted easily in future if changed)

Other information and links

@austinabell austinabell added Priority: 3 - Medium Nice-to-have, does not impede core functionality IPLD labels Oct 5, 2020
@ec2
Copy link
Contributor

ec2 commented Oct 6, 2020

Is there a way to simulate this without redundant writes but rather simulating or calculating the costs and adding it?

@austinabell
Copy link
Contributor Author

Is there a way to simulate this without redundant writes but rather simulating or calculating the costs and adding it?

No, I've thought through this, and the main reasons this is not feasible (or compatible) because:

  • The blockstore interface would have to be expanded to include a new method that does not persist the object
    • This adds overhead and requires a lot of custom logic within Amt and Hamt to be able to "fake" persist the objects, but to keep the functionality consistent with go, the objects would have to be persisted without charge if this sub-tree is flushed
    • Adds a lot of complexity and overhead (as well as would need extremely extensive test coverage to make sure we interop in every possible case)
  • All writes are happening implicitly through the abstract data types, and isn't as simple as manually serializing all objects that would be persisted to charge gas (especially since these data structures have no notion of the gas charging blockstore and this is not trivial)
  • The logic of these structures, wrt what's written and read, is very different when node is flushed as opposed to cached
    • for example if not flushed here, if there is a read on this node, the go implementation doesn't hit a cache as they only keep the cid and charges a read when we wouldn't because we still have the dirty element in cache

TLDR a lot of complexity and overhead that risks consensus faults for an optimization (maybe revisit later, but will most likely be a headache)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 3 - Medium Nice-to-have, does not impede core functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants