fix(trie): decode inline child nodes

[notbdu]

Changes

• Adds handler for decoding inline child nodes based on expected hash length.
• Achieves behavioral parity with the parity implementation here: https://github.com/paritytech/substrate/blob/542d587c3db7e46b6fff423e8efc4f327f7b79a1/primitives/trie/src/node_codec.rs#L141-L145

Tests

• Adds a test for decoding a branch with inlined child nodes.

Issues

• Fixes proof issue: Paritytech/trie proofs are incompatible with gossamer/lib/trie #2329

Primary Reviewer

[CLAassistant]

All committers have signed the CLA.

[qdm12]

Quick question, I guess this inline format cannot be used for branches right? Only for leaves?

[notbdu]

Ah so a few things here I think. Technically there could be, my impression of the logic is that any child node < 32 bytes can be in-lined leaf or branch.

The child parsing seems to be wrong in general? It looks like its currently parsing every child as a leaf node. Although I'm not sure it semantically makes a difference when its just a hash pointer.

Also, there are a few header types that are missing from the impl:

impl Decode for NodeHeader {
    fn decode<I: Input>(input: &mut I) -> Result<Self, codec::Error> {
        let i = input.read_byte()?;
        if i == trie_constants::EMPTY_TRIE {
            return Ok(NodeHeader::Null);
        }
        println!("header byte {:b} {:b}", i, i & (0b11 << 6));
        match i & (0b11 << 6) {
            trie_constants::LEAF_PREFIX_MASK => Ok(NodeHeader::Leaf(decode_size(i, input, 2)?)),
            trie_constants::BRANCH_WITH_MASK => {
                Ok(NodeHeader::Branch(true, decode_size(i, input, 2)?))
            }
            trie_constants::BRANCH_WITHOUT_MASK => {
                Ok(NodeHeader::Branch(false, decode_size(i, input, 2)?))
            }
            trie_constants::EMPTY_TRIE => {
                if i & (0b111 << 5) == trie_constants::ALT_HASHING_LEAF_PREFIX_MASK {
                    Ok(NodeHeader::HashedValueLeaf(decode_size(i, input, 3)?))
                } else if i & (0b1111 << 4) == trie_constants::ALT_HASHING_BRANCH_WITH_MASK {
                    Ok(NodeHeader::HashedValueBranch(decode_size(i, input, 4)?))
                } else {
                    // do not allow any special encoding
                    Err("Unallowed encoding".into())
                }
            }
            _ => unreachable!(),
        }
    }
}

The mask size for hashed leaves and branches are different but after digging through the scale decoder implementation it looks like it handles that internally?

I think it's probably worth looking into some of the other issues but outside the scope of this PR? Maybe just have some open issues for now?

[qdm12]

It looks like its currently parsing every child as a leaf node. Although I'm not sure it semantically makes a difference when its just a hash pointer.

As the decodeBranch comment mentions:

// Note that since the encoded branch stores the hash of the children nodes, we are not
// reconstructing the child nodes from the encoding. This function instead stubs where the
// children are known to be with an empty leaf. The children nodes hashes are then used to
// find other values using the persistent database.

In another PR, it would be interesting to refactor this to load from database as it decodes, especially since you can have in-lined encoded nodes, otherwise it's rather confusing. Created #2375.

there are a few header types that are missing from the impl

That might be the case, I opened #2374, thanks so much for your investigation!

The mask size for hashed leaves and branches are different but after digging through the scale decoder implementation it looks like it handles that internally?

Are you sure? Can you point to some code you've seen? @timwu20 maybe do you have a clue on this?

I think it's probably worth looking into some of the other issues but outside the scope of this PR? Maybe just have some open issues for now?

Totally 👍

[notbdu]

Are you sure? Can you point to some code you've seen? @timwu20 maybe do you have a clue on this?

I think it's probably worth looking into some of the other issues but outside the scope of this PR? Maybe just have some open issues for now?

It looks like that way based on this section of the header parsing code I shared above.

                if i & (0b111 << 5) == trie_constants::ALT_HASHING_LEAF_PREFIX_MASK {
                    Ok(NodeHeader::HashedValueLeaf(decode_size(i, input, 3)?))
                } else if i & (0b1111 << 4) == trie_constants::ALT_HASHING_BRANCH_WITH_MASK {
                    Ok(NodeHeader::HashedValueBranch(decode_size(i, input, 4)?))

https://github.com/paritytech/substrate/blob/345e3b1fcbbb0c479a031c45859d7154e7c4b5f7/primitives/trie/src/node_header.rs#L97-L102

Leaf hash mask:
11100000
Branch hash mask:
11110000

Inisde of decode_size(), the max value of the header size is then 32 and 16 for leaf and branch headers respectively.

[danforbes]

Hi, @notbdu - thank you so much for your contribution! I would love to learn a bit more about your interest in Gossamer 🤓 If you'd like to chat, can you please shoot me an email @ dan.forbes@chainsafe.io?

[notbdu]

Sent an email!

Ah, I've also broken the integration tests 🥲 .

   babe_integration_test.go:119:
               Error Trace:    babe_integration_test.go:119
                                                       babe_integration_test.go:231
               Error:          Received unexpected error:
                               failed to load storage trie from database: cannot load child at index 2 with hash 0x73c356dac7914c2574ed8cacd4e65147bf93ed93b56cce9792c2e2fb846c3941: cannot find child node key 0x in database: Key not found
               Test:           TestService_ProducesBlocks
-- FAIL: TestService_ProducesBlocks (1.38s)

I think the test is expecting all decoded children to be hash pointers but now it's loading the child inline.

[noot]

@notbdu I believe I fixed the integration tests, but can't push to your branch, feel free to add this to *Trie.load in trie/database.go line 187:

		hash := child.GetHash()
		_, isLeaf := child.(*node.Leaf)
		if len(hash) == 0 && isLeaf {
			// node has already been loaded inline
			continue
		}

[noot]

looks good to me!

[notbdu]

Thanks for the reviews @noot, @qdm12 and team!

[notbdu]

Added same fix to getFromDB() as well to fix trie/database unit tests.

[codecov]

Codecov Report

Merging #2369 (5154e16) into development (7f5b5aa) will decrease coverage by 0.03%.
The diff coverage is 43.05%.

@@               Coverage Diff               @@
##           development    #2369      +/-   ##
===============================================
- Coverage        58.59%   58.56%   -0.04%     
===============================================
  Files              214      214              
  Lines            27991    28027      +36     
===============================================
+ Hits             16402    16414      +12     
- Misses            9943     9959      +16     
- Partials          1646     1654       +8     

Flag	Coverage Δ
unit-tests	58.56% <43.05%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/gossamer/flags.go	66.66% <ø> (ø)
dot/config.go	0.00% <0.00%> (ø)
dot/node.go	0.00% <0.00%> (ø)
internal/trie/node/decode.go	96.07% <55.55%> (-3.93%)	⬇️
lib/trie/database.go	52.13% <82.35%> (+1.17%)	⬆️
cmd/gossamer/config.go	75.41% <100.00%> (ø)
cmd/gossamer/export.go	88.46% <100.00%> (ø)
cmd/gossamer/utils.go	74.35% <100.00%> (ø)
lib/runtime/life/exports.go	40.56% <0.00%> (-11.26%)	⬇️
lib/runtime/life/instance.go	55.14% <0.00%> (-6.13%)	⬇️
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2ec68d...5154e16. Read the comment docs.

[qdm12]

Looks good although there is some investigation to be done about missing types and inline encoding/decoding of branches.

[qdm12]

@notbdu I think there are linting issues, can you please fix it so we can merge this? Thanks!

[notbdu]

Deepsource go was complaining of this issue (link from another repo): https://deepsource.io/gh/xiaods/k8e/issue/RVV-B0001/occurrences/

The private method is the lower cased version of the public method.

[qdm12]

@notbdu Oh sorry for the confusion; I was talking about the required linter workflow (golangci-lint named as checks / linting (pull_request)), we don't really use Deepsource and it's not required either.

You have to git rebase development / git merge development, we made a small CI workflow requirement change so you need new changes 🙏 Thanks!!

[notbdu]

@qdm12 sick sick, just rebased!

[qdm12]

Oh-oh there is some decoding/encoding issue it seems: https://github.com/ChainSafe/gossamer/runs/5681474712?check_suite_focus=true

[notbdu]

Ah interesting, TAL now - remember it passing before 🤔 ... haha

[notbdu]

Let me know if this fix maxes sense to you. The already inline loaded nodes were missing cached hash digest and encoding so I'm just caching them here: c94e83c

[kishansagathiya]

thanks @notbdu and @seunlanlege

Great to have contributions like these!

[kishansagathiya]

I think you want to check for len(hash) < hashLength, right?

From what I read, inline nodes only fit less than 32 bytes

[notbdu]

Just FYI, parity's impl is an equality check and it doesn't seem like they have any check if count > H::LENGTH 🤔 .
https://github.com/paritytech/substrate/blob/542d587c3db7e46b6fff423e8efc4f327f7b79a1/primitives/trie/src/node_codec.rs#L141-L145

						children[i] = Some(if count == H::LENGTH {
							NodeHandlePlan::Hash(range)
						} else {
							NodeHandlePlan::Inline(range)
						});

But yea I'm under the impression that inline nodes should be < 32 bytes. Modified the check 👍.

[kishansagathiya]

@danforbes Is this something that should be in the spec? I did a quick search for inline in the spec pdf and did not find anything!

[danforbes]

@kishansagathiya this is the relevant part of the spec (thank you, @FlorianFranzen 🙏🏻) https://w3f.github.io/polkadot-spec/develop/sect-state-storage.html#defn-node-header

[qdm12]

To add to @danforbes's comment, #2418 was created to address this.