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.

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

Use cached session index to obtain executor params #1190

Merged
merged 12 commits into from
Sep 1, 2023

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

Closes #579

To validate a candidate, approval voting and dispute coordinator subsystems currently rely on the candidate validation subsystem, which requests executor parameters at a session determined by the candidate's relay parent. This is unreliable, as the state at that relay parent may already be pruned. Still, we want to be able to apply slashes in that case.

This PR makes use of the internal cache maintained by approval voting and dispute distribution subsystems to reliably obtain session index even if the state in question isn't there. Then, the execution parameters are requested and provided to the candidate validation subsystem, which doesn't have to request them on its own anymore.

The positive side effect is that the candidate validation subsystem is returning to its pre-#6161 state; that is, it expects the exhaustive validation data from a requesting subsystem instead of doing its own data collection.

The backing subsystem, also affected by the change, does not maintain any cache of session indexes as it doesn't need it, as the state at that stage is guaranteed to be alive. A session index is always known when a backing job is created. Executor parameters are requested before performing the actual validation using that session index.

A rather large volume of changes is dictated by the fact that every test doing candidate validation needs to be fixed, along with the backing, approval, and dispute tests. The actual logic changes are quite compact.

@s0me0ne-unkn0wn s0me0ne-unkn0wn changed the title Import changes from archieved repo Use cached session index to obtain executor params Aug 27, 2023
@bkchr
Copy link
Member

bkchr commented Sep 1, 2023

Wouldn't it just make more sense to store the ExecutorParams as part of the AvailabilityData? I mean we already store all the relevant data as part of AvailabilityData to make the validation "stateless"? I find it weird to now switch to a model where we store parts of the data in the availability store and other is taken from somewhere else.

@rphmeier
Copy link
Contributor

rphmeier commented Sep 1, 2023

Wouldn't it just make more sense to store the ExecutorParams as part of the AvailabilityData? I mean we already store all the relevant data as part of AvailabilityData to make the validation "stateless"

That carries a pretty high overhead (as it'd be duplicated for every candidate in the session), and recent sessions' info should be generally available. But yes, it'd be more foolproof.

* master: (25 commits)
  fix typos (#1339)
  Use bandersnatch-vrfs with locked dependencies ref (#1342)
  Bump bs58 from 0.4.0 to 0.5.0 (#1293)
  Contracts: `seal0::balance` should return the free balance (#1254)
  Logs: add extra debug log for negative rep changes (#1205)
  Added short-benchmarks for cumulus (#1183)
  [xcm-emulator] Improve hygiene and clean up (#1301)
  Bump the known_good_semver group with 1 update (#1347)
  Renames API (#1186)
  Rename `polkadot-parachain` to `polkadot-parachain-primitives` (#1334)
  Add README to project root (#1253)
  Add environmental variable to track decoded instructions (#1320)
  Fix polkadot-node-core-pvf-prepare-worker build with jemalloc (#1315)
  Sassafras primitives (#1249)
  Restructure `dispatch` macro related exports (#1162)
  backing: move the min votes threshold to the runtime (#1200)
  Bump zstd from 0.11.2+zstd.1.5.2 to 0.12.4 (#1326)
  Remove `substrate_test_utils::test` (#1321)
  remove disable-runtime-api (#1328)
  [ci] add more jobs for pipeline cancel, cleanup (#1314)
  ...
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Added my suggestion in #1355. Otherwise, I think this is strictly better than status quo, so approving.

@eskimor
Copy link
Member

eskimor commented Sep 1, 2023

bot merge

@command-bot
Copy link

command-bot bot commented Sep 1, 2023

@eskimor Unknown command "merge". Refer to help docs and/or source code.

@eskimor eskimor enabled auto-merge (squash) September 1, 2023 17:21
@bkchr
Copy link
Member

bkchr commented Sep 1, 2023

That carries a pretty high overhead (as it'd be duplicated for every candidate in the session), and recent sessions' info should be generally available. But yes, it'd be more foolproof.

Why not store the execution parameters on their own in the availability store and let the availability data reference it using a hash? Then we don't need to duplicate in every availability data.

@eskimor eskimor merged commit a2b6470 into master Sep 1, 2023
4 checks passed
@eskimor eskimor deleted the s0me0ne/candidate-session-index branch September 1, 2023 18:07
@eskimor
Copy link
Member

eskimor commented Sep 1, 2023

How would that be better than having them stored on chain? The chain will keep the data far longer than we would in the availability store. The problem (that has been fixed) was that we depended on the state of a particular relay parent to be available. I don't see a problem to rely on long lived data on chain. We already rely on the chain for getting the PVF for example. We also always need access to session information. Hence putting execution environment parameters into availability would not gain us anything in terms of robustness.

Also it should really be fine, assuming we have probabilistic finality maintained in all cases.

Mooaar tests as pointed out by @rphmeier are in order though.

@rphmeier
Copy link
Contributor

rphmeier commented Sep 1, 2023

Yes. The actual issue is that we were attempting to read out the state of a specific relay parent, whereas our approach here is usually to guarantee that the necessary data will be available in every relay chain state for the foreseeable future.

ordian added a commit that referenced this pull request Sep 7, 2023
* master: (25 commits)
  Markdown linter (#1309)
  Update `fmt` file and some authors (#1379)
  Bump the known_good_semver group with 1 update (#1375)
  Bump proc-macro-warning from 0.4.1 to 0.4.2 (#1376)
  feat: add futures api to `TransactionPool` (#1348)
  Ensure cumulus/bridges is ignored by formatter and run it (#1369)
  substrate: chain-spec paths corrected in zombienet tests (#1362)
  contracts: Update to wasmi 0.31 (#1350)
  [improve docs]: Template pallet (#1280)
  [xcm-emulator] Unignore cumulus integration tests (#1247)
  Fix wrong ref counting (#1358)
  Use cached session index to obtain executor params (#1190)
  fix typos (#1339)
  Use bandersnatch-vrfs with locked dependencies ref (#1342)
  Bump bs58 from 0.4.0 to 0.5.0 (#1293)
  Contracts: `seal0::balance` should return the free balance (#1254)
  Logs: add extra debug log for negative rep changes (#1205)
  Added short-benchmarks for cumulus (#1183)
  [xcm-emulator] Improve hygiene and clean up (#1301)
  Bump the known_good_semver group with 1 update (#1347)
  ...
Daanvdplas pushed a commit that referenced this pull request Sep 11, 2023
* Import changes from archieved repo

* Revert erroneous changes

* Fix more tests

* Resolve discussions

* Fix MORE tests

* approval-voting: launch_approval better interface (#1355)

---------

Co-authored-by: Javier Viola <javier@parity.io>
Co-authored-by: ordian <noreply@reusable.software>
Co-authored-by: ordian <write@reusable.software>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/stalled-parachains-on-kusama-post-mortem/3998/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve candidate validation robustness for missing relay chain blocks
7 participants