CompactProof: Deduplicate values - #233
Conversation
encode_compact emits a detached value node once per referencing trie node, so a value shared by N keys is sent N times (paritytech/polkadot-sdk#12565). Add encode_compact_skip_duplicate_values to emit each distinct value once; repeats are emitted unmodified and stay decodable by existing hash-keyed decoders. For prefixed databases, decode_compact_from_iter_with_known_values re-inserts deduplicated values at every referencing position.
A trailing attached value node was not counted in the returned used-item count, so decoding concatenated encodings at that offset re-read the value bytes as a node.
trie_codec_proof only ran ExtensionLayout (inline values), so value detachment and deduplication were never fuzzed. Round-trip the deduplicating encoding into hash-keyed and prefixed databases, add a hashed-value target with heavily shared values, and a deterministic smoke test so the assertions run in CI without libFuzzer. Also fix the fuzz crate's stale memory-db path dependency version.
An old decoder inserts a deduplicated value once, so its refcount in a hash-keyed database understates the referencing nodes; a consumer consolidating removals could drop a still-referenced value. The re-inserting decoder restores exact parity. Assert full database equality (entries and refcounts, hash-keyed and prefixed) between the plain and deduplicated encodings.
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
You can do the de-duplication on the node level and not just on the value level.
Vendor the compact-proof decoder from the released trie-db 0.31.0 verbatim into the test crate as a frozen snapshot of deployed decoder behavior, and assert that encodings produced by encode_compact_skip_duplicate_values decode with it into a hash-keyed database with every entry readable. This pins the backward compatibility the deduplicating encoder relies on, instead of leaving it as an argument in documentation.
encode_compact_skip_duplicates now emits each distinct trie node once, not just each detached value. A later occurrence of an already-emitted subtree keeps a plain hash reference (like any reference outside the partial trie), so the encoding never grows. The decoder threads a known-items map and re-inserts skipped subtrees at every position, reconstructing the same database as an un-deduplicated encoding.
We now also deduplicate nodes |
|
@cheme Would be super nice if you could support us with a review here :) |
Sure, will look at it (may not be able to do tomorrow, but will try to do before next week). |
cheme
left a comment
cheme
left a comment
There was a problem hiding this comment.
Using seen_hashes make sense to me, I think it is good (should even be extended node hashes).
What I am not too sure, is why we try to use prefixedmemorydb or have proper rc count.
|
@cheme @bkchr Sorry for the delay (was off for some time), did another pass and removed the RC reconstruction and subtree reinsertion during decoding. Substrate does not need it at all, and it was complicating the code. The reinsertion had another problem, a deduplicated proof emits each shared subtree once, but the reinserting decoder replayed it at every referencing position. Nested duplication makes that unfold exponentially costing exponential decode time. To fix this we would have needed to introduce some kind of decoding budget on the decoder side. Not worth it IMO. And if needed, it is still possible to arrive at a correct PrefixedDB from the HashDB. |
cheme
left a comment
cheme
left a comment
There was a problem hiding this comment.
Looks good to me.
Note that I did not go through all testing and fuzzing code path, from a quick glance, maybe there is a bit of code that is not so needed anymore (regarding latest simplification).
| last_entry.child_index += 1; | ||
| } else { | ||
| return Ok((node_hash, i + 1)) | ||
| return Ok((node_hash, i + 1 + attached_node)) |
There was a problem hiding this comment.
I guess this change means major version update.
lexnv
left a comment
lexnv
left a comment
There was a problem hiding this comment.
Had a brief look, this also incorporates the hashing optimization via Copy (ie TrieHash<L>)
One tiny thing that can be a followup would be to turn Clippy/Fmt unused/wrongly formatting warnings into errors at compile time such the CI would fail instead of the bot leaving comments on the PR, worth considering later 🙏
5 participants
Summary
encode_compactre-emits a shared detached value once per referencing node, causing proof size to grow with reference count instead of value count. Adds opt-inencode_compact_skip_duplicate_valuesto dedupe on encode.decode_compact_from_iter(anddecode_compact) now always handle both old-style and deduplicated proofs correctly. Also fixes an off-by-one indecode_compact_from_iter's returned item count when the last node has an attached value.Compatibility
encode_compact/decode_compact_from_iterbehavior unchanged — new functions are additive.MemoryDB(verified against 0.31.0) — but not into aPrefixedKeyDB, which needs the value re-inserted per prefix.get/containsonly checkrc > 0); This matters if you later do balanced insert/remove against that decoded DB.decode_compact's item count is now correct when the last node has an attached value — previously it was off by one in that case.