Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

add additional assurances to create_inherent #4349

Merged
merged 17 commits into from
Nov 24, 2021
Merged

add additional assurances to create_inherent #4349

merged 17 commits into from
Nov 24, 2021

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Nov 22, 2021

Ref #4028
Closes #4335

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Nov 22, 2021
@drahnr drahnr self-assigned this Nov 22, 2021
@drahnr drahnr added the B0-silent Changes should not be mentioned in any release notes label Nov 22, 2021
rphmeier
rphmeier reviewed Nov 23, 2021
View reviewed changes
runtime/parachains/src/paras_inherent.rs
);
log::debug!(
target: LOG_TARGET,
"[enter] bitfields.len(): {}, backed_candidates.len(): {}, disputes.len() {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

based on experience, you're going to want the parent header hash here at some point.

rphmeier
rphmeier reviewed Nov 23, 2021
View reviewed changes
runtime/parachains/src/paras_inherent.rs Outdated
!candidate_has_concluded_invalid_dispute(backed_candidate.candidate.hash())
let mut index = 0;
backed_candidates.retain(move |backed_candidate| {
let ret = !candidate_has_concluded_invalid_dispute_or_is_invalid(index, backed_candidate);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, this is important as it also implicitly should cause the sanitize_bitfields changes to strip out any un-scheduled cores.

rphmeier
rphmeier reviewed Nov 23, 2021
View reviewed changes
runtime/parachains/src/paras_inherent.rs Outdated

let scheduled = <scheduler::Pallet<T>>::scheduled();

let relay_parent_number = now - One::one();

let checker_ctx = CandidateCheckContext::<T>::new(now, relay_parent_number);
Copy link
Contributor

Choose a reason for hiding this comment

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

I could definitely see CandidateCheckContext being updated for contextual execution, although we might need to instantiate it on a candidate-by-candidate basis.

rphmeier
rphmeier approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

Changeset looks good. A few grumbles.

emostov
emostov reviewed Nov 23, 2021
View reviewed changes
runtime/parachains/src/paras_inherent.rs
parent_header: inherent_data.parent_header,
}
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

At what point will we remove the "training wheels" and no longer call enter_inner?

Copy link
Contributor Author

@drahnr drahnr Nov 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Once we refactored this entire thing, I am not happy with it - it's too complex and things are still very cross dependent.

@drahnr drahnr added C1-low PR touches the given topic and has a low impact on builders. A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Nov 24, 2021
@drahnr drahnr requested a review from ordian November 24, 2021 12:02
@ordian
Copy link
Member

ordian commented Nov 24, 2021

could you merge master? the diff looks wrong

drahnr and others added 16 commits November 24, 2021 13:17
@drahnr
add additional validity checks
d54fbcf
@drahnr
simplify shuffling
585f16d
@drahnr
Closes potential OOB weight
e421e7b
@drahnr
improve docs
f55b459
@drahnr
fooo
92b1254
@drahnr
remove obsolete comment
06d9c3f
@drahnr
move filtering into the rollback-transaction
720ff45 
Technically this is not necessary but avoids future footguns.
@drahnr
move check up and avoid duplicate checks
c59b710
@drahnr
refactor: make sure backed candidates are sane, even more
a6d022b
@drahnr @emostov
doc wording
25509e9 
Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
@drahnr
refactor: avoid const generics for sake of wasm size
6b041d5 
`true` -> `FullCheck::Skip`, `false` -> `FullCheck::Yes`.
@drahnr
chore: unify CandidateCheckContext instance names
bf5c52e
@drahnr
refactor: introduce IndexedRetain for Vec<T>
43bef25
@drahnr
chore: make tests prefix free
9b25114
@drahnr
doc: re-introduce removed comment
ab394dc
@drahnr
refactor: remove another const generic to save some wasm size
6cec917
@drahnr
Copy link
Contributor Author

drahnr commented Nov 24, 2021

I force pushed and the diff looks OK again.

@drahnr
Copy link
Contributor Author

drahnr commented Nov 24, 2021

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 0609dc7 into master Nov 24, 2021
@paritytech-processbot paritytech-processbot bot deleted the bernhard-more-assurances branch November 24, 2021 14:11
@pepyakin pepyakin mentioned this pull request Nov 25, 2021
Closed
drahnr added a commit that referenced this pull request Nov 26, 2021
@drahnr @emostov
add additional assurances to create_inherent (#4349)
4ea424d 
* minor: move checks into separate fn

* add additional validity checks

* simplify shuffling

* Closes potential OOB weight

* improve docs

* fooo

* remove obsolete comment

* move filtering into the rollback-transaction

Technically this is not necessary but avoids future footguns.

* move check up and avoid duplicate checks

* refactor: make sure backed candidates are sane, even more

* doc wording

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* refactor: avoid const generics for sake of wasm size

`true` -> `FullCheck::Skip`, `false` -> `FullCheck::Yes`.

* chore: unify `CandidateCheckContext` instance names

* refactor: introduce `IndexedRetain` for `Vec<T>`

* chore: make tests prefix free

* doc: re-introduce removed comment

* refactor: remove another const generic to save some wasm size

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
@drahnr drahnr mentioned this pull request Nov 26, 2021
Merged
drahnr added a commit that referenced this pull request Nov 26, 2021
@drahnr @emostov
add additional assurances to create_inherent (#4349)
306394f 
* minor: move checks into separate fn

* add additional validity checks

* simplify shuffling

* Closes potential OOB weight

* improve docs

* fooo

* remove obsolete comment

* move filtering into the rollback-transaction

Technically this is not necessary but avoids future footguns.

* move check up and avoid duplicate checks

* refactor: make sure backed candidates are sane, even more

* doc wording

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* refactor: avoid const generics for sake of wasm size

`true` -> `FullCheck::Skip`, `false` -> `FullCheck::Yes`.

* chore: unify `CandidateCheckContext` instance names

* refactor: introduce `IndexedRetain` for `Vec<T>`

* chore: make tests prefix free

* doc: re-introduce removed comment

* refactor: remove another const generic to save some wasm size

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
ordian pushed a commit that referenced this pull request Nov 26, 2021
@drahnr @emostov @ordian
add additional assurances to create_inherent (#4349)
072019b 
* minor: move checks into separate fn

* add additional validity checks

* simplify shuffling

* Closes potential OOB weight

* improve docs

* fooo

* remove obsolete comment

* move filtering into the rollback-transaction

Technically this is not necessary but avoids future footguns.

* move check up and avoid duplicate checks

* refactor: make sure backed candidates are sane, even more

* doc wording

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* refactor: avoid const generics for sake of wasm size

`true` -> `FullCheck::Skip`, `false` -> `FullCheck::Yes`.

* chore: unify `CandidateCheckContext` instance names

* refactor: introduce `IndexedRetain` for `Vec<T>`

* chore: make tests prefix free

* doc: re-introduce removed comment

* refactor: remove another const generic to save some wasm size

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
@chevdor chevdor added this to the v0.9.13 milestone Nov 26, 2021
ordian pushed a commit that referenced this pull request Nov 29, 2021
@drahnr @rphmeier
@emostov @Lldenaurois @bkchr @pepyakin
[BACKPORT] backport changes for v0.9.13 (#4385)
c0d7044 
* move paras inherent filtering to runtime (#4028)

* move things around, add filter methods��

* validator keys, modify availability bitfields according to disputes

* simplify, keep the filter -> sanitize generic for both usecases

* minor

* assure tests still work, reduce changeset

* integration

* start entropy passing

* fixins

* compile, 1 failing test

* filter with coverage

* fixins

* Update runtime/parachains/src/paras_inherent.rs

Co-authored-by: Robert Habermeier <rphmeier@gmail.com>

* slip of the pen

* improve test cases

* misc

* fix

* fixins

* test avoid extra into() calls in assert_noop!

* chores

* ff

* test fixup superfluous into call

* chore: pfmt

* improve apply_block_weight_limit to try to maximize the number of sufficiently backed

blocks and add extra bitfields in a round-robin fashion

* new code treats the lack of backed candidates as ok

* Use vrf based entropy

* fixup vrf random

* add warn

* slip of the pen

* fixup

* assure ordering

* rethink apply_weights

* mock

* use a closure as predicate check

* extract and use DisputedBitfield

* chore: simplify

* remove stray dbg

* chore: fmt

* address feedback

* fix test, halfway there

* stage1

* dbg stuff

* make group selection align

* fix session index

* fix wrongly returned candidates

* cleanup

* chore fmt

* fix ensure check

* make good case test work

* more tests for bitfields

* create sanitize_backed_candidates

* fixup tests

* update guide

* add check referenced in the guide

* improve weights code

* fmt

* fixins

* Update roadmap/implementers-guide/src/runtime/inclusion.md

Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>

* compiling + address review

* add comments

* fix weight calc

* address review comments and test failure

* fix

* fix: condition

* Fix random_sel function

* Fix overlength block check

* Zeke + Ladi commit for disputes filtering + integration test builder + runtime benchmarks + integration tests

* Add benchmarks for code upgrades

* Code upgrade bench; Feature gate TestWeightInfo

* Try and make CI happier

* Feature gate enter test to not(benchmarks)

* Make sure no unused imports/fn

* refactor, re-use, the beginning

* Fix issue with frame benchmarking dep compilation

* More precise feature gating for some derives

* integrate piece-wise

* foo

* fixins

* chore fmt

* fixins

* rename const generic

* Update runtime/parachains/src/paras_inherent.rs

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* Fix compilation

* limit to test

* remove unused spam slots

* spellcheck

* remove a tick, fix a typo

* Add Code upgrade weights

* comment improvements + >=

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* remove another tick

* Update runtime/parachains/src/paras_inherent/benchmarking.rs

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* saturating fixins + some spaces

* fix

* benchmarking - preliminary results

* Add training wheels

* Refactor some early exit logic for enter

* Gracefully handle filtering bitfields & candidates (#4280)

This updates the logic for sanitize_bitfields and sanitize_backed_candidates to never error when there is an issue, but instead to simply skip the problematic items.

* Refactor inherent data weight limiting logic (#4287)

* Apply suggestions from code review

* Update runtime/parachains/src/builder.rs

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* Update runtime/parachains/src/builder.rs

* Update runtime/parachains/src/paras_inherent.rs

* final pass

* Run cargo +nightly-2021-10-29 fmt

* Update implementors guide with `sanitize_*` & `enter` (#4294)

* Make spell check happier

* Make wasm runtimes compile with benchmarks enabled (#4303)

* comment stuff out, use old toml

* Seems to be working?

* Remove feature gating from builder

* Remove commented out stuff

* Remove generic from digest

* Update weight files for runtime

Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Lldenaurois <Ljdenaurois@gmail.com>
Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
Co-authored-by: Bastian Köcher <info@kchr.de>

* prefer code upgrades in inherent filtering (#4334)

* impl prefered items

Closes #4330

* do not stop attempting to select, just because one did not fit

* doc

* prefered -> preferred

* missing usage of the preferred indices

* sigh

* shuffle is not available for chacha

* remove duplicate weight addition

* ref vs no ref

* add additional assurances to `create_inherent` (#4349)

* minor: move checks into separate fn

* add additional validity checks

* simplify shuffling

* Closes potential OOB weight

* improve docs

* fooo

* remove obsolete comment

* move filtering into the rollback-transaction

Technically this is not necessary but avoids future footguns.

* move check up and avoid duplicate checks

* refactor: make sure backed candidates are sane, even more

* doc wording

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* refactor: avoid const generics for sake of wasm size

`true` -> `FullCheck::Skip`, `false` -> `FullCheck::Yes`.

* chore: unify `CandidateCheckContext` instance names

* refactor: introduce `IndexedRetain` for `Vec<T>`

* chore: make tests prefix free

* doc: re-introduce removed comment

* refactor: remove another const generic to save some wasm size

Co-authored-by: Zeke Mostov <z.mostov@gmail.com>

* Inherent filtering follow up (#4305)

* Add feature more feature gating for benchmarking + tests

* New line

* cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=kusama-dev --steps=50 --repeat=20 --pallet=runtime_parachains::paras_inherent --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/kusama/src/weights/runtime_parachains_paras_inherent.rs

* cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=polkadot-dev --steps=50 --repeat=20 --pallet=runtime_parachains::paras_inherent --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/polkadot/src/weights/runtime_parachains_paras_inherent.rs

* Do not assume we use max validators per core

* Use kusama weights for rococo (hopefully temp)

* cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=runtime_parachains::paras_inherent --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/runtime_parachains_paras_inherent.rs

* Add more validity votes when neccesary

* Some fixes for the last commit

* Restore westend weights

* cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=runtime_parachains::paras_inherent --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/runtime_parachains_paras_inherent.rs

* Revert bad westend weights write

* Make sure to update val idx before skipping

* Fix validity vote range to max at group size'

* Temp setup for rococo

* cargo run --quiet --release --features runtime-benchmarks -- benchmark --chain=rococo-dev --steps=50 --repeat=20 --pallet=runtime_parachains::paras_inherent --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./runtime/rococo/src/weights/runtime_parachains_paras_inherent.rs --header=./file_header.txt

* Augment generated Rococo weights

* Make it compile

* Revert range for enter_backed_candidates_variable

* Delete runtime/kusama/src/weights/runtime_paras_paras_inherent.rs

Co-authored-by: Parity Bot <admin@parity.io>

* prepare worker: Catch unexpected unwinds (#4304)

* prepare worker: Catch unexpected unwinds

* Use more specific wording for unknown panic payload

* Treat non-deterministic prep errors as internal errors (#4364)

Closes #4293

This PR changes the way how we treat a certain subset of PVF preparation
errors. Specifically, now only the deterministic errors are treated as
invalid candidates. That is, the errors that are easily
attributable to either the the PVF contents or the wasmtime code, but
not e.g. I/O errors that could be triggered by the OS (insufficient
memory, disk failure, too much load, etc). The latter are treated as
internal errors and thus do not trigger the disputes.

Co-authored-by: Robert Habermeier <rphmeier@gmail.com>
Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Co-authored-by: Lldenaurois <Ljdenaurois@gmail.com>
Co-authored-by: Zeke Mostov <z.mostov@gmail.com>
Co-authored-by: Bastian Köcher <info@kchr.de>
Co-authored-by: Parity Bot <admin@parity.io>
Co-authored-by: Sergei Shulepov <sergei@parity.io>
@redzsina redzsina added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Dec 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier approved these changes

@emostov emostov emostov approved these changes

@ordian ordian Awaiting requested review from ordian

Assignees

@drahnr drahnr

Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited.
Projects
None yet
Milestone
v0.9.13
Development

Successfully merging this pull request may close these issues.

Handle overly frequent upgrades earlier
6 participants
@drahnr @ordian @rphmeier @emostov @chevdor @redzsina