Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass state version for extrinsic root derivation through Config #1

Merged
merged 1 commit into from
Sep 27, 2023
Merged

Pass state version for extrinsic root derivation through Config #1

merged 1 commit into from
Sep 27, 2023

Conversation

vedhavyas
Copy link
Member

@vedhavyas vedhavyas commented Sep 22, 2023 •
edited
Loading

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context,
including:

  • What does this PR do?
    • Adds Extrinsinc root state version as config parameter so that users can pick V0/V1 depending on the use case.
  • Why are these changes needed?
    • Currently extrinsic root is derived using StateVersion::V0. In order to generate the extrinsic root, we would need to have access to full extrinsic data. We have a use case where, domains extrinsic root need to be derived on consensus runtime and passing all the extrinsic data through Fraud proof will exceed block limit. We are planning to use StateVersion::V1 so that we can just use extrinsic hashes if the extrinsic data is more than 32 bytes.
  • How were these changes implemented and what do they affect?
    • Config parameter for ExtrinsicRootStateVersion can be passed through runtime and I have set it to StateVersion::V0 so this should not change the extrinsic root derivation.

nazar-pc
nazar-pc reviewed Sep 22, 2023
View reviewed changes
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a use case where, domains extrinsic root need to be derived on consensus runtime

Why? This feels wrong on the surface.

We would like to inject parent state root so that we can verify the StorageProof in the runtime using the given State root. This would avoid us doing the verification on the client side instead.

Where on the client exactly, in which specific case, does it remove all client-side verification? You can achieve the same with simple inherent extrinsic if you just want to include parent state root in the ancestor block. I don't see why it needs to be upstreamed.

substrate/frame/system/src/tests.rs Outdated Show resolved Hide resolved
substrate/primitives/transaction-pool/src/runtime_api.rs Outdated Show resolved Hide resolved
vedhavyas
vedhavyas commented Sep 23, 2023
View reviewed changes
Copy link
Member Author

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This feels wrong on the surface.

We discussed this already. We want to derive extrinsic root of the domain block during fraud proof verification.

Where on the client exactly, in which specific case, does it remove all client-side verification? You can achieve the same with simple inherent extrinsic if you just want to include parent state root in the ancestor block. I don't see why it needs to be upstreamed.

Yes. you are correct. I'll try that route and remove the second commit

substrate/frame/system/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/system/src/tests.rs Outdated Show resolved Hide resolved
@vedhavyas vedhavyas force-pushed the state_version_and_parent_state_root_injection branch from e504a94 to d9bdb2b Compare September 23, 2023 07:02
@vedhavyas vedhavyas force-pushed the state_version_and_parent_state_root_injection branch from d9bdb2b to fb2fd5d Compare September 23, 2023 08:32
nazar-pc
nazar-pc approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very reasonable.

Please update PR title and let's submit this upstream before merging (while we're working on PR that uses this in subspace/subspace) to see their response. Looks like fairly non-controversial to me.

@nazar-pc
Copy link
Member

BTW, looks like StateVersion::V1 has more implications than just using extrinsic hashed rather than values, but I don't think I have knowledge of what exactly.

@vedhavyas
Copy link
Member Author

BTW, looks like StateVersion::V1 has more implications than just using extrinsic hashed rather than values, but I don't think I have knowledge of what exactly.

Any specific point where you see this implication? Anyway lets get the upstream response

@vedhavyas vedhavyas changed the title Pass state version for extrinsic root derivation through Config and inject parent state root Pass state version for extrinsic root derivation through Config Sep 25, 2023
@nazar-pc
Copy link
Member

Any specific point where you see this implication?

Nothing specific, I just checked and saw that the data structure is used in multiple places across the project, but didn't dig into where exactly and how. It is an assumption.

@vedhavyas vedhavyas force-pushed the state_version_and_parent_state_root_injection branch from fb2fd5d to 4ece868 Compare September 27, 2023 09:13
@vedhavyas vedhavyas force-pushed the state_version_and_parent_state_root_injection branch from 4ece868 to 1f217f3 Compare September 27, 2023 10:28
@vedhavyas vedhavyas merged commit a20ecc4 into subspace-v1 Sep 27, 2023
3 of 6 checks passed
@vedhavyas vedhavyas deleted the state_version_and_parent_state_root_injection branch September 27, 2023 10:30
@nazar-pc
Copy link
Member

First attempt to land this upstream: paritytech#1691
Second: paritytech#1968

shamil-gadelshin pushed a commit to shamil-gadelshin/polkadot-sdk that referenced this pull request Apr 3, 2024
@AndreiEres @sandreim
subsystem-bench: Prepare CI output (paritytech#3158)
9e6298e 
1. Benchmark results are collected in a single struct.
2. The output of the results is prettified.
3. The result struct used to save the output as a yaml and store it in
artifacts in a CI job.

```
$ cargo run -p polkadot-subsystem-bench --release -- test-sequence --path polkadot/node/subsystem-bench/examples/availability_read.yaml | tee output.txt
$ cat output.txt

polkadot/node/subsystem-bench/examples/availability_read.yaml autonomys#1

Network usage, KiB                     total   per block
Received from peers               510796.000  170265.333
Sent to peers                        221.000      73.667

CPU usage, s                           total   per block
availability-recovery                 38.671      12.890
Test environment                       0.255       0.085


polkadot/node/subsystem-bench/examples/availability_read.yaml autonomys#2

Network usage, KiB                     total   per block
Received from peers               413633.000  137877.667
Sent to peers                        353.000     117.667

CPU usage, s                           total   per block
availability-recovery                 52.630      17.543
Test environment                       0.271       0.090


polkadot/node/subsystem-bench/examples/availability_read.yaml autonomys#3

Network usage, KiB                     total   per block
Received from peers               424379.000  141459.667
Sent to peers                        703.000     234.333

CPU usage, s                           total   per block
availability-recovery                 51.128      17.043
Test environment                       0.502       0.167

```

```
$ cargo run -p polkadot-subsystem-bench --release -- --ci test-sequence --path polkadot/node/subsystem-bench/examples/availability_read.yaml | tee output.txt
$ cat output.txt
- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml autonomys#1'
  network:
  - resource: Received from peers
    total: 509011.0
    per_block: 169670.33333333334
  - resource: Sent to peers
    total: 220.0
    per_block: 73.33333333333333
  cpu:
  - resource: availability-recovery
    total: 31.845848445
    per_block: 10.615282815
  - resource: Test environment
    total: 0.23582828799999941
    per_block: 0.07860942933333313

- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml autonomys#2'
  network:
  - resource: Received from peers
    total: 411738.0
    per_block: 137246.0
  - resource: Sent to peers
    total: 351.0
    per_block: 117.0
  cpu:
  - resource: availability-recovery
    total: 18.93596025099999
    per_block: 6.31198675033333
  - resource: Test environment
    total: 0.2541994199999979
    per_block: 0.0847331399999993

- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml autonomys#3'
  network:
  - resource: Received from peers
    total: 424548.0
    per_block: 141516.0
  - resource: Sent to peers
    total: 703.0
    per_block: 234.33333333333334
  cpu:
  - resource: availability-recovery
    total: 16.54178526900001
    per_block: 5.513928423000003
  - resource: Test environment
    total: 0.43960946299999537
    per_block: 0.14653648766666513
```

---------

Co-authored-by: Andrei Sandu <54316454+sandreim@users.noreply.github.com>
vedhavyas pushed a commit that referenced this pull request Apr 11, 2024
@HCastano @jimpo @bkchr
Move substrate-bridge-relay into repository (#1)
ebbc472 
* Initial commit. CLI which parses RPC urls.

* Establish ws connections and make simple RPC requests.

* Complete bridge setup.

* Process subscription events.

* Ctrl-C handler.

* Write a bare-bones README and copy in design doc.

* Modularize code a little bit.

* Communicate with each chain in a separate task.

* Parse headers from RPC subscription notifications.

* Send (fake) extrinsics across bridge channels.

And now it's deadlocked.

* Fix deadlock.

* Clarify in README that this is not-in-progress.

* Move everything into a single folder

* Move Substrate relay into appropriate folder

* Get the Substrate Relay node compiling

* Update Cargo.lock

* Use new composite accounts from Substrate

* Remove specification document

It has been moved to the Wiki on the Github repo.

* Update author + remove comments

* Use latest master for jsonrpsee

Required renaming some stuff (e.g Client -> RawClient)

Co-authored-by: Jim Posen <jim.posen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nazar-pc nazar-pc nazar-pc approved these changes

@NingLin-P NingLin-P Awaiting requested review from NingLin-P

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vedhavyas @nazar-pc @NingLin-P