Don't include `:code` by default in storage proofs by bkchr · Pull Request #5060 · paritytech/substrate

bkchr · 2020-02-25T20:45:27Z

This pr changes the behavior of automatically including :code in storage proofs. Before this pr, the :code was extracted by the wasm-executor before executing a function in the runtime. Now, the runtime code is passed as a parameter from the outside. This makes it possible to extract the :code from the backend, before the storage recorder tracks all accesses to the backend (which resulted in inclusion of :code in the proof).

However, the execution_proof function of Client still adds the :code to the proof, this is required by the light client, until #5047 is implemented.

Closes: paritytech/cumulus#8

the proof

…runtime-everytime

bkchr · 2020-02-25T20:46:35Z

client/src/client.rs

+		// backwards compatible.
+		//
+		// TODO: Remove when solved: https://github.com/paritytech/substrate/issues/5047
+		let code_proof = self.read_proof(


@svyatonik here I'm currently forcing that the :code is still added to the proof.

svyatonik · 2020-02-26T06:26:06Z

client/block-builder/src/lib.rs

+		assert!(
+			backend.storage(&sp_core::storage::well_known_keys::CODE)
+				.unwrap_err()
+				.contains("Database missing expected key"),


Probably just leave .unwrap_err() - comparing strings seems a bit fragile?

Actually I wanted to match on the error, however it is a string already. The full error message is much longer, but this part is the important one (as long as no one changes the display impl for the error, it should be fine).

…runtime-everytime

pepyakin

Looks great! A couple of nits

client/executor/src/wasm_runtime.rs

pepyakin · 2020-02-26T09:20:15Z

primitives/state-machine/src/backend.rs

+	where H::Out: Encode,
+{
+	let code = backend.storage(well_known_keys::CODE)
+			.ok()


Isn't this overindented?

pepyakin · 2020-02-26T09:21:32Z

primitives/state-machine/src/backend.rs

+	let code = backend.storage(well_known_keys::CODE)
+			.ok()
+			.flatten()
+			.ok_or_else(|| "`:code` not found")?;


nit: .ok_or can work here as well

Suggested change

.ok_or_else(|| "`:code` not found")?;

.ok_or("`:code` not found")?;

primitives/core/src/traits.rs

Co-Authored-By: Sergei Pepyakin <sergei@parity.io>

…runtime-everytime

* Adds test to verify that the runtime currently is always contained in the proof * Start passing the runtime wasm code from the outside * Fix compilation * More build fixes * Make the test work as expected now :) * Last fixes * Fixes benchmarks * Review feedback * Apply suggestions from code review Co-Authored-By: Sergei Pepyakin <sergei@parity.io> * Review feedback * Fix compilation Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com>

arkpar · 2020-03-04T23:17:18Z

So now to make a call one is required to query code from storage and keep it in memory to avoid re-querying it each time? This just wastes memory. Why not just add filter to the proof recorder?

arkpar · 2020-03-04T23:19:16Z

This pr changes the behavior of automatically including :code in storage proofs. Before this pr, the :code was extracted by the wasm-executor before executing a function in the runtime.

That's not true. It was only extracted if the hash does not match the available instance. When there's matching version of compiled code or wasmi instance we don't need to keep original WASM bytecode in memory.

bkchr · 2020-03-04T23:50:44Z

This pr changes the behavior of automatically including :code in storage proofs. Before this pr, the :code was extracted by the wasm-executor before executing a function in the runtime.

That's not true. It was only extracted if the hash does not match the available instance. When there's matching version of compiled code or wasmi instance we don't need to keep original WASM bytecode in memory.

Yeah okay, after checking again the code, I found the code that caches the storage_hash. I thought that this wasn't done. However, given this code, the :code is also cached.

I don't think that this wastes any more memory than the implementation before. We don't cache the code anywhere else as before.

Why not just add filter to the proof recorder?

This doesn't work, as you also can have legit accesses from the runtime to :code and that requires that the proof recorder records them. From my point of view neither the state machine nor the executor should care where the runtime code comes from. It is just instructed to execute the given code and should not be responsible for fetching it from the backend.

arkpar · 2020-03-05T07:34:17Z

This doesn't work, as you also can have legit accesses from the runtime to :code and that requires that the proof recorder records them.

Does it? Whoever verifies the proof has the code anyway and they can just return it when it is queried from the proof storage. Is that right @svyatonik?

From my point of view neither the state machine nor the executor should care where the runtime code comes from. It is just instructed to execute the given code and should not be responsible for fetching it from the backend.

Needlessly fetching the code on each call is expensive. The executor decides when it actually needs the code and should use either externalities or some other interface to fetch it only when needed

svyatonik · 2020-03-05T07:39:19Z

Does it? Whoever verifies the proof has the code anyway and they can just return it when it is queried from the proof storage. Is that right @svyatonik?

No - there's no code on light client and that's why tthe ":code" was a part of execution proof since very beginning. There's an issue to add code caching #5047 , mentioned in the PR description.

arkpar · 2020-03-05T08:05:05Z

No - there's no code on light client and that's why tthe ":code" was a part of execution proof since very beginning. There's an issue to add code caching #5047 , mentioned in the PR description.

I'm asking about parachain validation here. The proof is useless without the code. So if it is excluded from the proof whoever is verifying it must have the code from some other source already. That also means when you have "legit accesses from the runtime to :code" they can just use the code they already have and there's no need to include it in the proof. Is this technically correct?

bkchr · 2020-03-05T08:11:31Z

For parachain validation, the runtime code is part of the validation blob that is stored on the relay chain. So, we don't need :code to be part of the proof.

This reverts commit a193a19.

…5136) This reverts commit a193a19.

…h#5060)" (paritytech#5136) This reverts commit a193a19.

* Adds test to verify that the runtime currently is always contained in the proof * Start passing the runtime wasm code from the outside * Fix compilation * More build fixes * Make the test work as expected now :) * Last fixes * Fixes benchmarks * Review feedback * Apply suggestions from code review Co-Authored-By: Sergei Pepyakin <sergei@parity.io> * Review feedback * Fix compilation Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com>

* Don't include `:code` by default in storage proofs (#5060) * Adds test to verify that the runtime currently is always contained in the proof * Start passing the runtime wasm code from the outside * Fix compilation * More build fixes * Make the test work as expected now :) * Last fixes * Fixes benchmarks * Review feedback * Apply suggestions from code review Co-Authored-By: Sergei Pepyakin <sergei@parity.io> * Review feedback * Fix compilation Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com> * Fix compilation and change the way `RuntimeCode` works * Fix tests * Switch to `Cow` Co-authored-by: Benjamin Kampmann <ben@gnunicorn.org> Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com>

* Don't include `:code` by default in storage proofs (paritytech#5060) * Adds test to verify that the runtime currently is always contained in the proof * Start passing the runtime wasm code from the outside * Fix compilation * More build fixes * Make the test work as expected now :) * Last fixes * Fixes benchmarks * Review feedback * Apply suggestions from code review Co-Authored-By: Sergei Pepyakin <sergei@parity.io> * Review feedback * Fix compilation Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com> * Fix compilation and change the way `RuntimeCode` works * Fix tests * Switch to `Cow` Co-authored-by: Benjamin Kampmann <ben@gnunicorn.org> Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com>

* Adds test to verify that the runtime currently is always contained in the proof * Start passing the runtime wasm code from the outside * Fix compilation * More build fixes * Make the test work as expected now :) * Last fixes * Fixes benchmarks * Review feedback * Apply suggestions from code review Co-Authored-By: Sergei Pepyakin <sergei@parity.io> * Review feedback * Fix compilation Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com>

…h#5060)" (paritytech#5136) This reverts commit a193a19.

* Don't include `:code` by default in storage proofs (paritytech#5060) * Adds test to verify that the runtime currently is always contained in the proof * Start passing the runtime wasm code from the outside * Fix compilation * More build fixes * Make the test work as expected now :) * Last fixes * Fixes benchmarks * Review feedback * Apply suggestions from code review Co-Authored-By: Sergei Pepyakin <sergei@parity.io> * Review feedback * Fix compilation Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com> * Fix compilation and change the way `RuntimeCode` works * Fix tests * Switch to `Cow` Co-authored-by: Benjamin Kampmann <ben@gnunicorn.org> Co-authored-by: Sergei Pepyakin <s.pepyakin@gmail.com>

bkchr added 8 commits February 13, 2020 10:30

Adds test to verify that the runtime currently is always contained in

e27a0ed

the proof

Start passing the runtime wasm code from the outside

fa1e568

Fix compilation

90daba0

Merge remote-tracking branch 'origin/master' into bkchr-dont-include-…

7e9ad60

…runtime-everytime

More build fixes

785c68a

Merge remote-tracking branch 'origin/master' into bkchr-dont-include-…

20c6b90

…runtime-everytime

Make the test work as expected now :)

070bb70

Last fixes

c4bcb45

bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes labels Feb 25, 2020

bkchr requested a review from svyatonik February 25, 2020 20:45

bkchr requested review from kianenigma and pepyakin as code owners February 25, 2020 20:45

bkchr commented Feb 25, 2020

View reviewed changes

svyatonik reviewed Feb 26, 2020

View reviewed changes

svyatonik approved these changes Feb 26, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into bkchr-dont-include-…

4419f97

…runtime-everytime

pepyakin approved these changes Feb 26, 2020

View reviewed changes

bkchr and others added 6 commits February 26, 2020 11:24

Fixes benchmarks

d23fcac

Review feedback

d9acf70

Apply suggestions from code review

ca87055

Co-Authored-By: Sergei Pepyakin <sergei@parity.io>

Review feedback

5431be3

Merge remote-tracking branch 'origin/master' into bkchr-dont-include-…

a307928

…runtime-everytime

Fix compilation

877ea10

bkchr added this to the 2.0 milestone Mar 4, 2020

bkchr added A8-looksgood and removed A0-please_review Pull request needs code review. labels Mar 4, 2020

gnunicorn merged commit a193a19 into master Mar 4, 2020

gnunicorn deleted the bkchr-dont-include-runtime-everytime branch March 4, 2020 19:26

bkchr added a commit that referenced this pull request Mar 5, 2020

Revert "Don't include :code by default in storage proofs (#5060)"

fc583ab

This reverts commit a193a19.

arkpar mentioned this pull request Mar 5, 2020

Revert "Don't include :code by default in storage proofs (#5060)" #5136

Merged

arkpar added a commit that referenced this pull request Mar 5, 2020

Revert "Don't include :code by default in storage proofs (#5060)" (#…

1d3d688

…5136) This reverts commit a193a19.

General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Mar 6, 2020

Revert "Don't include :code by default in storage proofs (paritytec…

358968c

…h#5060)" (paritytech#5136) This reverts commit a193a19.

bkchr mentioned this pull request Mar 7, 2020

Don't include :code by default in storage proofs #5179

Merged

General-Beck pushed a commit to General-Beck/substrate that referenced this pull request Mar 17, 2020

Revert "Don't include :code by default in storage proofs (paritytec…

1be8cc3

…h#5060)" (paritytech#5136) This reverts commit a193a19.

	.ok_or_else(\|\| "`:code` not found")?;
	.ok_or("`:code` not found")?;

Conversation

bkchr commented Feb 25, 2020

Uh oh!

bkchr Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

svyatonik Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

bkchr Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

pepyakin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pepyakin Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

pepyakin Feb 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arkpar commented Mar 4, 2020

Uh oh!

arkpar commented Mar 4, 2020

Uh oh!

bkchr commented Mar 4, 2020

Uh oh!

arkpar commented Mar 5, 2020

Uh oh!

svyatonik commented Mar 5, 2020

Uh oh!

arkpar commented Mar 5, 2020

Uh oh!

bkchr commented Mar 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants