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

Update amt for_each caching mechanisms and usages #847

Merged
merged 6 commits into from
Nov 16, 2020
Merged

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Nov 12, 2020

Summary of changes
Changes introduced in this pull request:

  • We were doing too little reads on one message on height 3080, this solves the issue
    • This just marks the mutated node as unchanged, which would lead to unexpected behaviour, if it wasn't removed directly after (which is the case here)
  • Changes gas charge names to match go casing, for a cleaner diff when interop

This PR does not yet solve the issue of gas diff on mutation instead of removals, because we do the mutation in place (as it should be if the go implementation was caching and iterating correctly) but they set the value while the amt is being iterated over (memory unsafe). I will find a way around this, but it might involve an inefficient Clone, because we can't exactly pull the element from the amt to set it back into the amt. I will open this PR when I have a functional workaround and tested adequately.

This now solves the issue marked above. I've documented that it isn't a perfect solution, since the reads and writes are in different order, which could cause a consensus fault if an out of gas error is hit in the middle of this. We cannot match exactly without memory unsafe usage or an extensive implementation of this, so I think this is a good enough solution for now. I will try to make changes upstream/open a FIP to make their caching more efficient or fix this specific issue.

Currently resyncing to check to make sure no regressions

Go equivalent tests: austinabell/go-amt-ipld@7471915

Reference issue to close (if applicable)

Closes

Other information and links

@austinabell austinabell marked this pull request as ready for review November 13, 2020 15:03
@austinabell austinabell added Interop Interop with Lotus/specs-actors and testing Status: Needs Review labels Nov 13, 2020
Copy link
Contributor

@timvermeulen timvermeulen left a comment

Choose a reason for hiding this comment

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

This looks reasonable!

This was referenced Nov 13, 2020
@austinabell austinabell merged commit af28ef4 into main Nov 16, 2020
@austinabell austinabell deleted the austin/amtfixes branch November 16, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Interop Interop with Lotus/specs-actors and testing Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants