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.

Sign up for GitHub

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

Jump to bottom

fixing bitfields and heads must be included every block #2955

Closed
wants to merge 1 commit into from
Closed

fixing bitfields and heads must be included every block #2955

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions lib/trie/trie.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,11 +333,6 @@ func (t *Trie) Put(keyLE, value []byte) {

func (t *Trie) insertKeyLE(keyLE, value []byte, deletedMerkleValues map[string]struct{}) {
nibblesKey := codec.KeyLEToNibbles(keyLE)
if len(value) == 0 {
// Force value to be inserted to nil since we don't
// differentiate between nil and empty values.
value = nil
}
Comment on lines -336 to -340
Copy link
Contributor

@qdm12 qdm12 Nov 17, 2022 •
edited
Loading

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:

  • the trie node encoding would result in encoding empty node values as {0} for branch-without-value nodes, which is wrong according to spec and substate code. Our decoding would still work since we interpret EOF and {0} the same, but it's not necessarily the same with substrate. And it's an unneeded {0} as well.
  • pretty much all trie methods rely on SubValue != nil checks
  • Test code assumes there cannot be []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.

t.root, _, _ = t.insert(t.root, nibblesKey, value, deletedMerkleValues)
}

Expand Down