fixing `bitfields and heads must be included every block` #2955

kishansagathiya · 2022-11-16T12:14:54Z

Changes

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Fixes #2937

Primary Reviewer

@qdm12

codecov · 2022-11-16T12:38:57Z

Codecov Report

Merging #2955 (04653f4) into development (8a17f37) will decrease coverage by 0.07%.
The diff coverage is n/a.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #2955      +/-   ##
===============================================
- Coverage        63.46%   63.38%   -0.08%     
===============================================
  Files              218      218              
  Lines            27505    27500       -5     
===============================================
- Hits             17455    17430      -25     
- Misses            8445     8467      +22     
+ Partials          1605     1603       -2

qdm12 · 2022-11-16T13:19:38Z

lib/trie/trie.go

+	// if len(value) == 0 {
+	// 	// Force value to be inserted to nil since we don't
+	// 	// differentiate between nil and empty values.
+	// 	value = nil
+	// }


Just peeking out of curiosity; we need this and the spec does not differentiate empty byte slices and nil byte slices (it's just 'empty'). The bitfields and heads must be included every block error happened before this change as well AFAIK. Finally, one more note is that before #2927 we would have funny trie root hashes (because of empty byte slices and kinda wrong encoding), maybe that could had been the cause?

The bitfields and heads must be included every block error happened before this change as well AFAIK.

That is quite unlikely, since I tested things to be working on the commit before this (this was tested with westend dev)

kishan@kishan-T480:~/code/gossamer$ git checkout 36ce37d1a358216071c018c7953605903c7adaf4 HEAD is now at 36ce37d1 fix(staging): revise datadog-agent start process (#2935) kishan@kishan-T480:~/code/gossamer$ make gossamer rm -fr ./bin > Building binary... go build -trimpath -o ./bin/gossamer -ldflags="-s -w" ./cmd/gossamer kishan@kishan-T480:~/code/gossamer$ rm -rf /home/kishan/.local/share/gssmr-test-node/chains/ kishan@kishan-T480:~/code/gossamer$ rm -r /tmp/alice /tmp/bob /tmp/charlie /tmp/dave ~/.gossamer/ rm: cannot remove '/tmp/bob': No such file or directory rm: cannot remove '/tmp/charlie': No such file or directory rm: cannot remove '/tmp/dave': No such file or directory rm: cannot remove '/home/kishan/.gossamer/': No such file or directory kishan@kishan-T480:~/code/gossamer$ ./bin/gossamer --key alice init --force --genesis ./chain/westend-dev/westend-dev-spec-raw.json --basepath /tmp/alice 2022-11-16T19:47:02+05:30 INFO 🕸️ initialising node with name unlock-can-60932, id westend_dev, base path /tmp/alice and genesis ./chain/westend-dev/westend-dev-spec-raw.json... node.go:L129 pkg=dot 2022-11-16T19:47:06+05:30 INFO block state hash genesis hash: 0xb967a97c0906f0778912e115fc30e739aa9c274ac90b1ec1951657946c09c41c initialize.go:L108 pkg=state 2022-11-16T19:47:06+05:30 INFO node initialised with name unlock-can-60932, id westend_dev, base path /tmp/alice, genesis ./chain/westend-dev/westend-dev-spec-raw.json, block 0 and genesis hash 0xb967a97c0906f0778912e115fc30e739aa9c274ac90b1ec1951657946c09c41c node.go:L189 pkg=dot kishan@kishan-T480:~/code/gossamer$ ./bin/gossamer --key alice --genesis ./chain/westend-dev/westend-dev-spec-raw.json --babe-lead -port 7011 --basepath /tmp/alice 2022-11-16T19:47:06+05:30 INFO loaded package log configuration: core: INFO, digest: INFO, sync: INFO, network: INFO, rpc: INFO, state: INFO, runtime: INFO, block producer: INFO, finality gadget: INFO config.go:L122 pkg=cmd 2022-11-16T19:47:06+05:30 INFO 🕸️ initialising node services with global configuration name unlock-can-60932, id westend_dev and base path /tmp/alice... node.go:L246 pkg=dot 2022-11-16T19:47:07+05:30 INFO created state service with head 0xb967a97c0906f0778912e115fc30e739aa9c274ac90b1ec1951657946c09c41c, highest number 0 and genesis hash 0xb967a97c0906f0778912e115fc30e739aa9c274ac90b1ec1951657946c09c41c service.go:L162 pkg=state 2022-11-16T19:47:07+05:30 INFO Generating p2p identity with seed 0 and key file /tmp/alice/node.key config.go:L175 pkg=network 2022-11-16T19:47:07+05:30 WARN Bootstrap is enabled but no bootstrap nodes are defined config.go:L141 pkg=network 2022-11-16T19:47:10+05:30 INFO creating runtime with interpreter wasmer... services.go:L108 pkg=dot 2022-11-16T19:47:13+05:30 INFO registered notifications sub-protocol /b967a97c0906f0778912e115fc30e739aa9c274ac90b1ec1951657946c09c41c/grandpa/1 service.go:L505 pkg=network 2022-11-16T19:47:13+05:30 INFO creating BABE service as authority... services.go:L182 pkg=dot 2022-11-16T19:47:13+05:30 INFO keystore with keys [0xc0014043f0] services.go:L190 pkg=dot 2022-11-16T19:47:13+05:30 INFO starting node unlock-can-60932... main.go:L294 pkg=cmd 2022-11-16T19:47:13+05:30 INFO 🕸️ starting node services... node.go:L459 pkg=dot 2022-11-16T19:47:13+05:30 INFO Starting services: [*system.Service *network.Service *digest.Handler *core.Service *grandpa.Service *sync.Service *babe.Service *state.Service] services.go:L57 pkg=dot,services 2022-11-16T19:47:13+05:30 INFO registered notifications sub-protocol /dot/block-announces/1 service.go:L505 pkg=network 2022-11-16T19:47:13+05:30 INFO registered notifications sub-protocol /dot/transactions/1 service.go:L505 pkg=network 2022-11-16T19:47:13+05:30 INFO Started listening on /ip4/127.0.0.1/tcp/7011/p2p/12D3KooWRKka5cAcXg5CQdXuzEfjbEKTACdxSn4Tkw77un3QYx2B service.go:L302 pkg=network 2022-11-16T19:47:13+05:30 INFO Started listening on /ip4/223.178.57.149/tcp/7011/p2p/12D3KooWRKka5cAcXg5CQdXuzEfjbEKTACdxSn4Tkw77un3QYx2B service.go:L302 pkg=network 2022-11-16T19:47:14+05:30 INFO started network service with supported protocols /p2p/id/delta/1.0.0, /ipfs/id/1.0.0, /ipfs/id/push/1.0.0, /ipfs/ping/1.0.0, /b967a97c0906f0778912e115fc30e739aa9c274ac90b1ec1951657946c09c41c/grandpa/1, /dot/sync/2, /dot/light/2, /dot/block-announces/1, /dot/transactions/1 service.go:L322 pkg=network 2022-11-16T19:47:14+05:30 INFO initiating epoch 0 with start slot 278101372 epoch.go:L59 pkg=babe 2022-11-16T19:47:18+05:30 INFO built block 1 with hash 0x6cd9c2abc29fbce7a72d36d90587e58bef3a89eaf9a62a3dcf0c201b2f69f408, state root 0xf6968a8f91a33378935181ab4ed9582c601c9677825d2aa0ddbc056a740867a3, epoch 0 and slot 278101372 babe.go:L539 pkg=babe 2022-11-16T19:47:22+05:30 INFO built block 2 with hash 0xbb6c66a1665ad5cabeaff617c85537b49e8af6cb8bed73475c01a021d9a0b657, state root 0x593723468f27e940e9ee02936b906cdb73e5f27d6f70d2f2e84997924b8b79e6, epoch 0 and slot 278101373 babe.go:L539 pkg=babe 2022-11-16T19:47:28+05:30 INFO built block 3 with hash 0x401b819e22a919fdfda887e03a40fa6f88b7272f2b69af5fe8ccb96e202d9382, state root 0x5e9d4acdce4f057aa50444dbc3d6477d036fd8d377f46ccd0b428bee91d239fb, epoch 0 and slot 278101374 babe.go:L539 pkg=babe ^C2022-11-16T19:47:30+05:30 INFO signal interrupt, shutting down... node.go:L470 pkg=dot 2022-11-16T19:47:30+05:30 INFO Stopping services: [*system.Service *network.Service *digest.Handler *core.Service *grandpa.Service *sync.Service *babe.Service *state.Service] services.go:L70 pkg=dot,services 2022-11-16T19:47:30+05:30 ERROR failed to begin DHT discovery: failed while waiting for peers: context canceled service.go:L315 pkg=network kishan@kishan-T480:~/code/gossamer$

But for now, what I am wondering is how to encode a nil, since rust does not have a nil equivalent. https://blog.knoldus.com/rust-can-never-be-null/

Also, please note that the reason why I have kept this draft is because I am still exploring other solutions.

Yes, just peeking out of curiosity as I said.
Try rebasing development and dropping that commit to check if it works fine?

As you said, Rust doesn't have nil. For now our trie code only have nil node values (and not {}), but that can be changed as well to {} instead of nil, provided we have consistency. Consistency might not even be that needed, but it helps mentally when debugging stuff. What is important is the n.SubValue != nil check in

gossamer/internal/trie/node/encode.go

Lines 40 to 48 in 8a17f37

// Only encode node value if the node is a leaf or

// the node is a branch with a non empty value.

if !nodeIsBranch || (nodeIsBranch && n.SubValue != nil) {

encoder := scale.NewEncoder(buffer)

err = encoder.Encode(n.SubValue)

if err != nil {

return fmt.Errorf("scale encoding value: %w", err)

}

}

You could change that to check for length > 0 instead of a nil check, and that could even (eventually) allow to insert both {} and nil as values in the trie, and get the same encoding/root hash.

dropping that commit has a bunch of conflict. So, instead I removed the part which was likely to create such a change, which is this branch. And this branch works fine.

qdm12 · 2022-11-17T09:33:23Z

lib/trie/trie.go

-	if len(value) == 0 {
-		// Force value to be inserted to nil since we don't
-		// differentiate between nil and empty values.
-		value = nil
-	}


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.

EclesioMeloJunior · 2022-11-22T15:27:41Z

Since #2964 was merged and it points out:

and closes https://github.com/ChainSafe/gossamer/pull/2955

I think we can close this PR, right? @kishansagathiya @qdm12

qdm12 · 2022-11-22T18:17:13Z

Closing this in favor of #2964

qdm12 reviewed Nov 16, 2022

View reviewed changes

don't force trie value to be nil

04653f4

kishansagathiya force-pushed the kishan/fix/encoding branch from 7d71c2f to 04653f4 Compare November 16, 2022 16:26

kishansagathiya marked this pull request as ready for review November 16, 2022 16:26

kishansagathiya requested review from noot, edwardmack, timwu20, EclesioMeloJunior and jimjbrettj as code owners November 16, 2022 16:26

qdm12 suggested changes Nov 17, 2022

View reviewed changes

qdm12 mentioned this pull request Nov 20, 2022

fix(trie): differentiate []byte(nil) and []byte{} for storage values #2964

Merged

qdm12 closed this Nov 22, 2022

qdm12 deleted the kishan/fix/encoding branch November 22, 2022 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing `bitfields and heads must be included every block` #2955

fixing `bitfields and heads must be included every block` #2955

kishansagathiya commented Nov 16, 2022

codecov bot commented Nov 16, 2022 •

edited

Loading

qdm12 Nov 16, 2022

kishansagathiya Nov 16, 2022

kishansagathiya Nov 16, 2022

qdm12 Nov 16, 2022 •

edited

Loading

kishansagathiya Nov 16, 2022

qdm12 Nov 17, 2022 •

edited

Loading

EclesioMeloJunior commented Nov 22, 2022

qdm12 commented Nov 22, 2022

	// Only encode node value if the node is a leaf or
	// the node is a branch with a non empty value.
	if !nodeIsBranch \|\| (nodeIsBranch && n.SubValue != nil) {
	encoder := scale.NewEncoder(buffer)
	err = encoder.Encode(n.SubValue)
	if err != nil {
	return fmt.Errorf("scale encoding value: %w", err)
	}
	}

fixing bitfields and heads must be included every block #2955

fixing bitfields and heads must be included every block #2955

Conversation

kishansagathiya commented Nov 16, 2022

Changes

Tests

Issues

Primary Reviewer

codecov bot commented Nov 16, 2022 • edited Loading

Codecov Report

qdm12 Nov 16, 2022

Choose a reason for hiding this comment

kishansagathiya Nov 16, 2022

Choose a reason for hiding this comment

kishansagathiya Nov 16, 2022

Choose a reason for hiding this comment

qdm12 Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

kishansagathiya Nov 16, 2022

Choose a reason for hiding this comment

qdm12 Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

EclesioMeloJunior commented Nov 22, 2022

qdm12 commented Nov 22, 2022

fixing `bitfields and heads must be included every block` #2955

fixing `bitfields and heads must be included every block` #2955

codecov bot commented Nov 16, 2022 •

edited

Loading

qdm12 Nov 16, 2022 •

edited

Loading

qdm12 Nov 17, 2022 •

edited

Loading