chore(lib/trie): add tests for ParityTech/trie proofs are incompatible with gossamer/lib/trie #2329

[thomasmodeneis]

I am adding the failing test case that was initially reported on #2329 so that it can be used as base for the fix and further validation.

Changes

This PR adds test case for the reported issue #2329

Tests

 go test -test.timeout=0 -run TestTrieProofParityTechCompatible -v github.com/ChainSafe/gossamer/lib/trie/...

Issues

#2329

Primary Reviewer

@timwu20
@EclesioMeloJunior

[codecov]

Codecov Report

Merging #2477 (4abd7fc) into development (98400b3) will increase coverage by 0.03%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           development    #2477      +/-   ##
===============================================
+ Coverage        61.58%   61.62%   +0.03%     
===============================================
  Files              215      215              
  Lines            28184    28205      +21     
===============================================
+ Hits             17357    17381      +24     
  Misses            9066     9066              
+ Partials          1761     1758       -3     

Flag	Coverage Δ
unit-tests	61.62% <ø> (+0.03%)	⬆️

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

Impacted Files	Coverage Δ
dot/state/block_notify.go	81.90% <0.00%> (-8.58%)	⬇️
dot/network/light.go	85.25% <0.00%> (-0.80%)	⬇️
dot/digest/digest.go	66.53% <0.00%> (+0.59%)	⬆️
lib/blocktree/blocktree.go	54.71% <0.00%> (+1.08%)	⬆️
dot/state/epoch.go	49.29% <0.00%> (+1.21%)	⬆️
dot/network/notifications.go	65.17% <0.00%> (+1.37%)	⬆️
dot/network/block_announce.go	63.20% <0.00%> (+4.80%)	⬆️
dot/network/inbound.go	100.00% <0.00%> (+14.03%)	⬆️

---

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 98400b3...4abd7fc. Read the comment docs.

[timwu20]

should this be fixed now?

[thomasmodeneis]

Hello @timwu20, thanks for reaching out.
This PR does not intend to provide a fix for the issue. IMHO the fix should come as part of a resolution for the following issue: #2329 ; I understand that @qdm12 and @EclesioMeloJunior are on top of the problem already.

[qdm12]

I'm working on upgrading the trie.
Let's keep this PR opened for now, and we can merge it before the trie upgrade so I can use those values in my branch.

[seunlanlege]

@qdm12 do you think this test should be passing now? After the changes you've made to the trie

[qdm12]

No, not yet. Don't worry I'll let you know once it's done. You can also subscribe to #2418 which will be closed once it's implemented.

[seunlanlege]

Gently pinging @qdm12 . Is there any hope to merge this soon-ish?

[qdm12]

I'm still working on it, I understand it has been several months now, sorry about that.
There were many pieces not adapted for a versioned trie (and hybrid trie) nor for the v1 state trie (such as proof verification).
Just to be sure, you are only using this to verify proofs correct? If so, I will try to focus on that part first.

[seunlanlege]

Just to be sure, you are only using this to verify proofs correct?

yes we're only using this for proof verification.

[qdm12]

On which chain do you have this problem? The v1 trie is not enabled on west end nor kusama nor polkadot for now (although it will be), so I am doubting it is related? On top of that, the v1 trie has to do with hashed subvalues and not partial keys really.

I have been digging into the test code here, rewriting it using newer code:

func TestXxx(t *testing.T) {
	key, err := hex.DecodeString("f0c365c3cf59d671eb72da0e7a4113c4bbd108c4899964f707fdaffb82636065")
	require.NoError(t, err)

	root, err := hex.DecodeString("5e1eb8e577ea88deaa94b456da24ab0c9f4c0c6d9372af1568edd7aeef3b4c4e")
	require.NoError(t, err)

	//nolint:lll
	bytes1, err := hex.DecodeString("80fffd8028b54b9a0a90d41b7941c43e6a0597d5914e3b62bdcb244851b9fc806c28ea2480e2f0847174b6f8ea15133a8d70de58d1a6174b7542e8e12028154c611bc3ee5280ddd81bdda149a8bc6990d3548a719d4a90ddbe5ea4a598211aacf6afd0b23bf58038fe7e08c8e684bd600f25631f32e6510ed7d37f43fce0d5aa974009857aeb5b80aafc60caa3519d4b861e6b8da226266a15060e2071bba4184e194da61dfb208e80b34a4ee6e2f949f58b7cb7f4a7fb1aaea8cdc2a5cb27557d32da7096fdf157c58024a760a8f6c27928ae9e2fed9968bc5f6e17c3ae647398d8a615e5b2bb4b425f8085a0da830399f25fca4b653de654ffd3c92be39f3ae4f54e7c504961b5bd00cf80c2d44d371e5fc1f50227d7491ad65ad049630361cefb4ab1844831237609f08380c8ae6a1e8df858b43e050a3959a25b90d711413ee1a863622c3914d45250738980b5955ff982ab818fcba39b2d507a6723504cef4969fc7c722ee175df95a33ae280509bb016f2887d12137e73d26d7ddcd7f9c8ff458147cb9d309494655fe68de180009f8697d760fbe020564b07f407e6aad58ba9451b3d2d88b3ee03e12db7c47480952dcc0804e1120508a1753f1de4aa5b7481026a3320df8b48e918f0cecbaed380fff4f175da5ff30200fabfdc2bbdd45f864d84f339ec2432f80b5749ac35bbfc")
	require.NoError(t, err)

	// Root branch with partial key b""
	// Full key is b""
	n1, err := node.Decode(bytes.NewReader(bytes1))
	require.NoError(t, err)
	_, _ = n1.CalculateMerkleValue()
	t.Log("N1:", n1)

	// Branch 2 with partial key b"", child 0 of branch 3 below.
	// Full key is b""
	//nolint:lll
	bytes2, err := hex.DecodeString("9ec365c3cf59d671eb72da0e7a4113c41002505f0e7b9012096b41c4eb3aaf947f6ea429080000685f0f1f0515f462cdcf84e0f1d6045dfcbb2056145f077f010000")
	require.NoError(t, err)

	n2, err := node.Decode(bytes.NewReader(bytes2))
	require.NoError(t, err)
	_, _ = n2.CalculateMerkleValue()
	t.Log("N2:", n2)

	// Branch 3 with partial key b"", child 15 of root branch
	// Full key is b""
	//nolint:lll
	bytes3, err := hex.DecodeString("80050880149156720805d0ad098ae52fcffae34ff637b1d1f1a0fa8e7f94201b8615695580c1638f702aaa71e4b78cc8538ecae03e827bb494cc54279606b201ec071a5e24806d2a1e6d5236e1e13c5a5c84831f5f5383f97eba32df6f9faf80e32cf2f129bc")
	require.NoError(t, err)

	n3, err := node.Decode(bytes.NewReader(bytes3))
	require.NoError(t, err)
	_, _ = n3.CalculateMerkleValue()
	t.Log("N3:", n3)

	proof := [][]byte{
		bytes1, bytes2, bytes3,
	}

	trie, err := buildTrie(proof, root)

	t.Log("TRIE:", trie)
	require.NoError(t, err)

	value := trie.Get(key)
	// TODO add a concrete expected value once this test is fixed.
	require.NotEmpty(t, value)
}

So we have the root node (N1), and two branches N2 and N3, but that key f0c365c3cf59d671eb72da0e7a4113c4bbd108c4899964f707fdaffb82636065 is simply not there. Are you missing nodes in the proof maybe? Or did I miss something? 🤔

[qdm12]

I also checked the variant header byte for each proof node is 0b1000_0000 (so 10 so branch with value variant), so it has nothing to do with the v1 trie which has new headers. Other inlined nodes are just leaves (01) so nothing with v1 either.

[qdm12]

we did use different test data, I tested your data with the substrate trie algorithm and the unit testing results were the same as yours and it also failed.

So closing this, feel free to re-open a similar PR if you find a substrate generated (v0) trie proof not validating with Gossamer (make sure you use v0.7.0 or development branch).