Domains: Move trace root capture from runtime to client for efficiency #3338

vedhavyas · 2025-01-08T10:29:20Z

We have been capturing the trace roots in the Domain runtime before executing the next trasaction. This became problematic during our benchmark tests where calculating storage root is essentially time taking process since it does the calculate the intermediate roots from full set of changes from beginning instead of from the previous root. More on this here

This PR essentially moves capturing the trace roots by creating triebackend over uncommiteed changes to ensure the diff set is minimal during the state root calculation.

~~I have also took the opportunity to clean up a TODO in the Fraud proof where post state root was returned from the Domain runtime. This can be calculated on verifier from the overlay changes.~~
Update: unfortunately, I found an interesting deviation that needs more research. Will provide a new PR when that TODO can be fixed and it does not relates to this PR anyway

Code contributor checklist:

I have read, understood and followed contributing guide

teor2345

Just wanted to check something that could impact performance (and correctness). Please feel free to ignore my review if it’s not ready yet!

teor2345 · 2025-01-08T23:49:58Z

domains/client/block-builder/src/custom_api.rs

+            Ok(Some(v))
+        } else {
+            Ok(self.backend.get(key, prefix)?)
+        }


Are we missing a “tombstone” check here?

If a key is present in the backend, but deleted in the overlay, this method returns Some(old) - the old value from the backend. But the correct value would be None, because the key has been deleted in the overlay, and will be deleted in the backend once consolidated.

(If we never delete any keys, or we always “delete” by overwriting with another value, then there’s no bug here. But I just wanted to check.)

Good question.

We are not missing any tombstone checks here. If the key is deleted, it will have a default value.

If the key is deleted, it will have a default value.

Just to be sure you are not talking about ValueQuery here, right? because ValueQuery/OptionQuery just define returning T::default() or None if the key-value does no exist in the state, it doesn't mean actually storing a T::default() in the state.

So just to confirm my understanding here: if a key is deleted in the overlay, the overlay will return Some(default), and we’ll never get to the backend returning Some(old)?

Yes. If the key is written(new/overwrite/delete) during the execution, then the memory DB will have the key present. If the value was not written to, then overlay memory db will not have it and we fallback to supporting backend

As a quick note: this is not new code and has been in the in the codebase for quite sometime powering our invalid state transaition Fraud proof :)

Yes, I noticed the code had been moved 🙂

So if the key is deleted in the overlay, it will be present and set to what value?
(It can’t be None, because then we’d ignore the deletion, and return the outdated value from the backend.)

Or should we be using contains() in the if condition, and get() to retrieve the value (which can be None for keys deleted in the overlay)?

That would reliably find deleted keys, if they hadn’t already been purged.

Either way, do we have a test for this behaviour?

I couldn’t find it documented clearly in the TrieBackendStorage API reference, but they do say to use contains() (and not get()) to check for existence:
https://docs.rs/memory-db/0.32.0/memory_db/struct.MemoryDB.html#

I couldn’t find it documented clearly in the TrieBackendStorage API reference, but they do say to use contains() (and not get()) to check for existence:

Ideally does not matter since get returns an optional value.

we do have implicit tests for this through XDM and pallet domains(not sure yet.)
I did test locally sometime back creating a pallet specifically meant to check the overlay deletion and addition. These never made it. You can actually test this by creating a few storage types and update and check them during the block hooks, initialize and finalize.
This will clearly shows if the deletion works

…dApi. These are essentially same roots as tested before the commit

vedhavyas · 2025-01-09T14:22:46Z

Benchmark results until this point

Tx in block	1000	2000	4000	5000	6000	8000
Domain block execution in MS	468	1354	4595	6667	9298	17059
Domain block import in MS	73	399	1947	15703	28678	62564
Total Time in MS	543	1755	6545	22385	37991	79623

vedhavyas · 2025-01-09T16:46:19Z

Benchmark results until this point

Tx in block	1000	2000	4000	5000	6000	8000
Domain block execution in MS	501	1237	4174	6387	9564	15478
Domain block import in MS	8	43	197	370	766	794
Total Time in MS	512	1320	4374	6766	10333	16285

teor2345

This makes sense to me, but I just want to double-check a few things. It would be great to have another review, too!

teor2345 · 2025-01-09T18:58:51Z

crates/sp-domains/src/core_api.rs

@@ -28,9 +28,6 @@ sp_api::decl_runtime_apis! {
            tx_range: &U256,
        ) -> bool;

-        /// Returns the intermediate storage roots in an encoded form.
-        fn intermediate_roots() -> Vec<[u8; 32]>;


Do we need to bump the API version when removing a method?

I think this means old client code will refuse to run with the new runtime due to a missing function, but that’s what we want, right? We want users to upgrade their nodes.

Yes, we do client upgrades first before runtime releases. So all the clients will use client side intermediate roots and once runtime is upgraded the function completely disappears.

I still like to check running the client with new change on taurus to ensure both give same roots. I have verified these locally but verifying on taurus is never a bad thing :)

domains/client/block-builder/src/custom_api.rs

domains/pallets/executive/src/lib.rs

vedhavyas

Great questions @teor2345 !
I trying to still optimise further and will spend more time today on this. In mean time feel free to review and run these changes locally if you prefer :)

vedhavyas · 2025-01-10T05:12:50Z

crates/sp-domains/src/core_api.rs

@@ -28,9 +28,6 @@ sp_api::decl_runtime_apis! {
            tx_range: &U256,
        ) -> bool;

-        /// Returns the intermediate storage roots in an encoded form.
-        fn intermediate_roots() -> Vec<[u8; 32]>;


Yes, we do client upgrades first before runtime releases. So all the clients will use client side intermediate roots and once runtime is upgraded the function completely disappears.

I still like to check running the client with new change on taurus to ensure both give same roots. I have verified these locally but verifying on taurus is never a bad thing :)

domains/client/block-builder/src/custom_api.rs

vedhavyas · 2025-01-10T11:06:22Z

This is good a review now.
I do not see further optimisations but if you find anything I have missed, let me know

teor2345

Sorry to keep going on about deleted keys, but the only docs I could find suggested using a different method to check for existence.

Also it seems like the new timer calls could impact our benchmarks on some platforms and architectures.

teor2345 · 2025-01-12T21:47:16Z

domains/client/block-builder/src/custom_api.rs

+            Ok(Some(v))
+        } else {
+            Ok(self.backend.get(key, prefix)?)
+        }


Yes, I noticed the code had been moved 🙂

So if the key is deleted in the overlay, it will be present and set to what value?
(It can’t be None, because then we’d ignore the deletion, and return the outdated value from the backend.)

Or should we be using contains() in the if condition, and get() to retrieve the value (which can be None for keys deleted in the overlay)?

That would reliably find deleted keys, if they hadn’t already been purged.

Either way, do we have a test for this behaviour?

I couldn’t find it documented clearly in the TrieBackendStorage API reference, but they do say to use contains() (and not get()) to check for existence:
https://docs.rs/memory-db/0.32.0/memory_db/struct.MemoryDB.html#

domains/client/block-builder/src/custom_api.rs

domains/client/block-builder/src/lib.rs

teor2345

These changes look good to me, as long as we're sure about layers and deletion

vedhavyas · 2025-01-17T07:44:43Z

as long as we're sure about layers and deletion

Fair. Added a test pallet that ensures this check and fails on any incorrect changes going forward @teor2345

@NingLin-P

Thanks for the solution @NingLin-P

teor2345 reviewed Jan 8, 2025

View reviewed changes

vedhavyas force-pushed the storage_root_changes branch from 52d6c2e to d2a9284 Compare January 9, 2025 11:50

vedhavyas added 7 commits January 9, 2025 17:21

move delta backend from execution prover to custom api

8a56468

add trie backend api

1b578e7

cleanup block builder

ac1d95c

collect intermediate roots and add missing inherent_extrinsics api call

90a28c2

intergate TrieBackendApi into domain block builder

f7026d3

replace runtime intermediate roots with roots derived from TrieBacken…

c34427f

…dApi. These are essentially same roots as tested before the commit

remove Intermediate roots from pallet executive

fa4fce9

vedhavyas force-pushed the storage_root_changes branch from d2a9284 to fa4fce9 Compare January 9, 2025 11:51

vedhavyas marked this pull request as ready for review January 9, 2025 11:51

vedhavyas requested a review from NingLin-P as a code owner January 9, 2025 11:51

avoid unnecessary clone of InmemoryDB

dfc3827

deduplicate storage changes instead of simp,y appending them

cae4f90

teor2345 reviewed Jan 9, 2025

View reviewed changes

vedhavyas commented Jan 10, 2025

View reviewed changes

hold runtime code and some small cleanup

cc6f251

teor2345 reviewed Jan 12, 2025

View reviewed changes

update struct comment and remove debug points

ae83448

teor2345 previously approved these changes Jan 15, 2025

View reviewed changes

add storage overlay checks pallet

c3ee9a3

vedhavyas dismissed teor2345’s stale review via c3ee9a3 January 17, 2025 07:43

vedhavyas requested a review from nazar-pc as a code owner January 17, 2025 07:43

vedhavyas requested a review from teor2345 January 17, 2025 07:44

purge all the zero referenced nodes post consolidation.

3019471

Thanks for the solution @NingLin-P

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domains: Move trace root capture from runtime to client for efficiency #3338

Domains: Move trace root capture from runtime to client for efficiency #3338

vedhavyas commented Jan 8, 2025 •

edited

Loading

teor2345 left a comment

teor2345 Jan 8, 2025

vedhavyas Jan 9, 2025

NingLin-P Jan 9, 2025

teor2345 Jan 9, 2025

vedhavyas Jan 10, 2025

teor2345 Jan 12, 2025

vedhavyas Jan 14, 2025

vedhavyas commented Jan 9, 2025

vedhavyas commented Jan 9, 2025

teor2345 left a comment

teor2345 Jan 9, 2025

vedhavyas Jan 10, 2025

vedhavyas left a comment

vedhavyas Jan 10, 2025

vedhavyas commented Jan 10, 2025

teor2345 left a comment •

edited

Loading

teor2345 Jan 12, 2025

teor2345 left a comment

vedhavyas commented Jan 17, 2025

Domains: Move trace root capture from runtime to client for efficiency #3338

Are you sure you want to change the base?

Domains: Move trace root capture from runtime to client for efficiency #3338

Conversation

vedhavyas commented Jan 8, 2025 • edited Loading

Code contributor checklist:

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedhavyas commented Jan 9, 2025

vedhavyas commented Jan 9, 2025

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedhavyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vedhavyas commented Jan 10, 2025

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

vedhavyas commented Jan 17, 2025

vedhavyas commented Jan 8, 2025 •

edited

Loading

teor2345 left a comment •

edited

Loading