Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to block this; but all our code checks for empty values with
!= nil
and although that might the issue here, I don't want to risk other ramifications.Especially:
{0}
for branch-without-value nodes, which is wrong according to spec and substate code. Our decoding would still work since we interpretEOF
and{0}
the same, but it's not necessarily the same with substrate. And it's an unneeded{0}
as well.SubValue != nil
checks[]byte{}
as node values, that's why the trie CI passes whereas really it would fail if test cases were added with[]byte{}
as node value.After re-reading Substrate code and the spec, #2927 is still correct and our code before was basically wrong. It was perhaps making it (still wrongly) thanks to the cached encoding of nodes, which got removed in #2919
As much as I hate to throw the ball elsewhere, I think the problem you are facing is due to something else (and didn't show up because of our wrong node encoding caching before). I'll ask on the elements chat about node encoding/decoding just to double check though.