iroha icon indicating copy to clipboard operation
iroha copied to clipboard

Performance degradation in executor for permission check

Open Erigara opened this issue 1 year ago • 10 comments

Right now we use the following queries during permission check:

  • FindDomainById - to check domain owner
  • FindAssetDefinitonId - to check asset definiton owner
  • FindPermissionsByAccountId - to check for existence of specific permission

All this queries degrade linearly with growth of Domain, AssetDefiniton or amout of Permissions given to the account. This happens because when we cross wasm boundary we have to serialize + fully clone the data into the wasm + deserialize it inside wasm.

We should figure out a way to check this in O(1) or O(logn) time.

Erigara avatar Jun 21 '24 10:06 Erigara

Just a thought but maybe we can store our entites in zerocopy way so that we can omit costly serialization and deserialization? smt like apache arrow But this would be a lot of changes...

Erigara avatar Jun 21 '24 10:06 Erigara

Just a thought but maybe we can store our entites in zerocopy way so that we can omit costly serialization and deserialization? smt like apache arrow But this would be a lot of changes...

even if we used zero-copy serialization formats, data would still have to be copied into linear memory and the degradation would still be linear

mversic avatar Jul 01 '24 21:07 mversic

i agree, still it would lower the overhead, sadly we can't share the data with executor

Erigara avatar Jul 02 '24 06:07 Erigara

i agree, still it would lower the overhead, sadly we can't share the data with executor

a big change like this can only be done if we determine serialization is the bottleneck

mversic avatar Jul 02 '24 08:07 mversic

I agree, out of curiosity i've executed validate_block benchmark with wasm profiling support: results.

cd default_executor
RUSTFLAGS="-C force-frame-pointers=on" cargo +nightly build -Z build-std -Z build-std-features=panic_immediate_abort -Z unstable-options --target wasm32-unknown-unknown --release
cp target/wasm32-unknown-unknown/release/iroha_default_executor.wasm ../configs/swarm/executor.wasm
cd ..
RUSTFLAGS="-C force-frame-pointers=on" cargo build --example validate_blocks -Z build-std --target aarch64-apple-darwin  --profile profiling --features profiling
samply record -r 999 ./target/aarch64-apple-darwin/profiling/examples/validate_blocks

From the results we can see that we spending around 60% of the time inside executor + around 1/3 of all stacks contain decode on there path.

Erigara avatar Jul 02 '24 09:07 Erigara

To have more accurate results we can create benchmark which would replay blocks from the block store.

Unfortunately pprof which we use to profile iroha in CI doesn't support profiling wasm...

Erigara avatar Jul 02 '24 09:07 Erigara

Remove the need to query domains and asset definitions

Checking if being owner could be replaced with checking if having an equivalent role. A domain registration, for example, could involve registering role of the domain owner and granting the role to the domain registrant

Remove the need to query multiple permissions

Multiple permissions could be represented as a single "entities trie". This is an analogy and an expansion of this suggestion for metadata. Also, state diff before and after transaction could be represented by the same trie. This means transaction validation would be done after execution by comparing the permission trie and the diff trie i.e. pass if the former is superset of the latter. The permission trie needs to be read from the state before the transaction execution (StateBlock), so regulation here is that instructions and queries cannot succeed/fail depending on preceding grant/revoke instructions in the same transaction.

Contrary to such a lazy validation of instructions, queries in a transaction should be eagerly validated, otherwise it leads ABA problem: B denotes a transient state where the query authority has granted privileges to himself. He can gain some extra knowledge from whether the transaction passes (after ABA) or fails (after ABC) by branching the final state based on the query result.

Btw a diff trie would become a data event trie, so this would be equivalent to batch emission of events

s8sato avatar Jul 03 '24 16:07 s8sato

This means transaction validation would be done after execution by comparing the permission trie and the diff trie i.e. pass if the former is superset of the latter.

This might have negative impact on a performance in two ways:

  1. Iroha have to do some work before realizing it was unnecessary
  2. As our state is designed right now, discarding transaction is not free because we have to undo every change done so far

So worst case scenario would be where malicious user try do make as much changes as possible within given transaction.

Erigara avatar Jul 04 '24 06:07 Erigara

Yeah, reversing the order of validation and execution may not be a good idea: it takes away the opportunity for early returns, and would require at least other deterrents such as pay-as-you-go transaction fees

s8sato avatar Jul 04 '24 07:07 s8sato

This can be considered a general issue related to reducing FFI calls, not just for permission queries. Possible solutions:

  • #5357
  • #5358

s8sato avatar Mar 17 '25 05:03 s8sato