* requiring users to maintain an unchecked invariant is unsafe
* relax trait restrictions on BoundedVec<T, S>
A normal `Vec<T>` can do many things without any particular trait
bounds on `T`. This commit relaxes the bounds on `BoundedVec<T, S>`
to give it similar capabilities.
* Add `BoundedBTreeMap` to `frame_support::storage`
Part of https://github.com/paritytech/substrate/issues/8719.
* max_encoded_len will never encode length > bound
* requiring users to maintain an unchecked invariant is unsafe
* only impl debug when std
* add some marker traits
* add tests
* implement BoundedEncodedLen
* update header
* update imports
* use impl_for_tuples instead of a custom macro
* remove redundant where clause
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
* impl for Compact<T>
* impl BoundedEncodedLen for BoundedVec (#8727)
* impl BoundedEncodedLen for bool
* explicitly implement BoundedEncodedLen for each Compact form
Turns out that u16 doesn't play nicely with the pattern; those values
take two extra bytes, where all other cases take one. :(
* rename BoundedEncodedLen -> MaxEncodedLen
* add tests of compact encoded length
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
* not climate
* explain the intent of the bool in the unsigned phase
* remove glob imports from unsigned.rs
* add OffchainRepeat parameter to ElectionProviderMultiPhase
* migrate core logic from #7976
This is a much smaller diff than that PR contained, but I think
it contains all the essentials.
* improve formatting
* fix test build failures
* cause test to pass
* Apply suggestions from code review
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* collapse imports
* threshold acquired directly within try_acquire_offchain_lock
* add test of resubmission after interval
* add test that ocw can regenerate a failed cache when resubmitting
* ensure that OCW solutions are of the correct round
This should help prevent stale cached solutions from persisting
past the election for which they are intended.
* add test of pre-dispatch round check
* use `RawSolution.round` instead of redundantly externally
* unpack imports
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* rename `OFFCHAIN_HEAD_DB` -> `OFFCHAIN_LOCK`
* rename `mine_call` -> `mine_checked_call`
* eliminate extraneous comma
* check cached call is current before submitting
* remove unused consts introduced by bad merge.
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
* resubmit when our solution beats queued solution
* clear call cache if solution fails to submit
* use local storage; clear on ElectionFinalized
* Revert "use local storage; clear on ElectionFinalized"
This reverts commit 4b46a9388532d0c09b337dc7c7edf76044a6cee8.
* BROKEN: try to filter local events in OCW
* use local storage; simplify score fetching
* fix event filter
* mutate storage instead of setting it
* StorageValueRef::local isn't actually implemented yet
* add logging for some events of interest in OCW miner
* rename kill_solution -> kill_ocw_solution to avoid ambiguity
* defensive err instead of unreachable given unreachable code
* doc punctuation
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* distinguish miner errors between "out of date" and "call invalid"
* downgrade info logs -> debug
* ensure encoded call decodes as a call
* fix semantics of validation of pre-dispatch failure for wrong round
* move score check within `and_then`
* add test that offchain workers clear their cache after election
* ensure that bad ocw submissions are not retained for resubmission
* simplify fn ocw_solution_exists
* add feasibility check when restoring cached solution
should address https://github.com/paritytech/substrate/pull/8290/files#r617533358
restructures how the checks are sequenced, which simplifies legibility.
* simplify checks again
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
* Refactor election solution trimming for efficiency
The previous version always trimmed the `CompactOf<T>` instance,
which was intrinsically inefficient: that's a packed data structure,
which is naturally expensive to edit. It's much easier to edit
the unpacked data structures: the `voters` and `assignments` lists.
* rework length-trim tests to work with the new interface
Test suite now compiles. Tests still don't pass because the macro
generating the compact structure still generates `unimplemented!()`
for the actual `compact_length_of` implementation.
* simplify
* add a fuzzer which can validate `Compact::encoded_size_for`
The `Compact` solution type is generated distinctly for each runtime,
and has both three type parameters and a built-in limit to the number
of candidates that each voter can vote for. Finally, they have an
optional `#[compact]` attribute which changes the encoding behavior.
The assignment truncation algorithm we're using depends on the ability
to efficiently and accurately determine how much space a `Compact`
solution will take once encoded.
Together, these two facts imply that simple unit tests are not
sufficient to validate the behavior of `Compact::encoded_size_for`.
This commit adds such a fuzzer. It is designed such that it is possible
to add a new fuzzer to the family by simply adjusting the
`generate_solution_type` macro invocation as desired, and making a
few minor documentation edits.
Of course, the fuzzer still fails for now: the generated implementation
for `encoded_size_for` is still `unimplemented!()`. However, once
the macro is updated appropriately, this fuzzer family should allow
us to gain confidence in the correctness of the generated code.
* Revert "add a fuzzer which can validate `Compact::encoded_size_for`"
This reverts commit 916038790887e64217c6a46e9a6d281386762bfb.
The design of `Compact::encoded_size_for` is flawed. When `#[compact]`
mode is enabled, every integer in the dataset is encoded using run-
length encoding. This means that it is impossible to compute the final
length faster than actually encoding the data structure, because the
encoded length of every field varies with the actual value stored.
Given that we won't be adding that method to the trait, we won't be
needing a fuzzer to validate its performance.
* revert changes to `trait CompactSolution`
If `CompactSolution::encoded_size_for` can't be implemented in the
way that we wanted, there's no point in adding it.
* WIP: restructure trim_assignments_length by actually encoding
This is not as efficient as what we'd hoped for, but it should still
be better than what it's replacing. Overall efficiency of
`fn trim_assignments_length` is now `O(edges * lg assignments.len())`.
* fix compiler errors
* don't sort voters, just assignments
Sorting the `voters` list causes lots of problems; an invariant that
we need to maintain is that an index into the voters list has a stable
meaning.
Luckily, it turns out that there is no need for the assignments list
to correspond to the voters list. That isn't an invariant, though previously
I'd thought that it was.
This simplifies things; we can just leave the voters list alone,
and sort the assignments list the way that is convenient.
* WIP: add `IndexAssignment` type to speed up repeatedly creating `Compact`
Next up: `impl<'a, T> From<&'a [IndexAssignmentOf<T>]> for Compact`,
in the proc-macro which makes `Compact`. Should be a pretty straightforward
adaptation of `from_assignment`.
* Add IndexAssignment and conversion method to CompactSolution
This involves a bit of duplication of types from
`election-provider-multi-phase`; we'll clean those up shortly.
I'm not entirely happy that we had to add a `from_index_assignments`
method to `CompactSolution`, but we couldn't define
`trait CompactSolution: TryFrom<&'a [Self::IndexAssignment]` because
that made trait lookup recursive, and I didn't want to propagate
`CompactSolutionOf<T> + TryFrom<&[IndexAssignmentOf<T>]>` everywhere
that compact solutions are specified.
* use `CompactSolution::from_index_assignment` and clean up dead code
* get rid of `from_index_assignments` in favor of `TryFrom`
* cause `pallet-election-provider-multi-phase` tests to compile successfully
Mostly that's just updating the various test functions to keep track of
refactorings elsewhere, though in a few places we needed to refactor some
test-only helpers as well.
* fix infinite binary search loop
Turns out that moving `low` and `high` into an averager function is a
bad idea, because the averager gets copies of those values, which
of course are never updated. Can't use mutable references, because
we want to read them elsewhere in the code. Just compute the average
directly; life is better that way.
* fix a test failure
* fix the rest of test failures
* remove unguarded subtraction
* fix npos-elections tests compilation
* ensure we use sp_std::vec::Vec in assignments
* add IndexAssignmentOf to sp_npos_elections
* move miner types to `unsigned`
* use stable sort
* rewrap some long comments
* use existing cache instead of building a dedicated stake map
* generalize the TryFrom bound on CompactSolution
* undo adding sp-core dependency
* consume assignments to produce index_assignments
* Add a test of Assignment -> IndexAssignment -> Compact
* fix `IndexAssignmentOf` doc
* move compact test from sp-npos-elections-compact to sp-npos-elections
This means that we can put the mocking parts of that into a proper
mock package, put the test into a test package among other tests.
Having the mocking parts in a mock package enables us to create a
benchmark (which is treated as a separate crate) import them.
* rename assignments -> sorted_assignments
* sort after reducing to avoid potential re-sort issues
* add runtime benchmark, fix critical binary search error
"Why don't you add a benchmark?", he said. "It'll be good practice,
and can help demonstrate that this isn't blowing up the runtime."
He was absolutely right.
The biggest discovery is that adding a parametric benchmark means that
you get a bunch of new test cases, for free. This is excellent, because
those test cases uncovered a binary search bug. Fixing that simplified
that part of the code nicely.
The other nice thing you get from a parametric benchmark is data about
what each parameter does. In this case, `f` is the size factor: what
percent of the votes (by size) should be removed. 0 means that we should
keep everything, 95 means that we should trim down to 5% of original size
or less.
```
Median Slopes Analysis
========
-- Extrinsic Time --
Model:
Time ~= 3846
+ v 0.015
+ t 0
+ a 0.192
+ d 0
+ f 0
µs
Min Squares Analysis
========
-- Extrinsic Time --
Data points distribution:
v t a d f mean µs sigma µs %
<snip>
6000 1600 3000 800 0 4385 75.87 1.7%
6000 1600 3000 800 9 4089 46.28 1.1%
6000 1600 3000 800 18 3793 36.45 0.9%
6000 1600 3000 800 27 3365 41.13 1.2%
6000 1600 3000 800 36 3096 7.498 0.2%
6000 1600 3000 800 45 2774 17.96 0.6%
6000 1600 3000 800 54 2057 37.94 1.8%
6000 1600 3000 800 63 1885 2.515 0.1%
6000 1600 3000 800 72 1591 3.203 0.2%
6000 1600 3000 800 81 1219 25.72 2.1%
6000 1600 3000 800 90 859 5.295 0.6%
6000 1600 3000 800 95 684.6 2.969 0.4%
Quality and confidence:
param error
v 0.008
t 0.029
a 0.008
d 0.044
f 0.185
Model:
Time ~= 3957
+ v 0.009
+ t 0
+ a 0.185
+ d 0
+ f 0
µs
```
What's nice about this is the clear negative correlation between
amount removed and total time. The more we remove, the less total
time things take.
* make types within `generate_solution_type` macro explicit
Closes#8444.
Just changes the parsing logic for that macro; does not change any
emitted code. The associated types associated with the macro now
require explicit, keyword-style declaration.
**Old**:
```rust
sp_npos_elections::generate_solution_type!(
#[compact]
pub struct TestCompact::<VoterIndex, TargetIndex, PerU16>(16)
);
```
**New**:
```rust
sp_npos_elections::generate_solution_type!(
#[compact]
pub struct TestCompact::<VoterIndex = VoterIndex, CandidateIndex = TargetIndex, Accuracy = PerU16>(16)
);
```
* un-ignore doc-tests
* use new form in bin/node/runtime/
* rename CandidateIndex -> TargetIndex
* add tests demonstrating some potential compile failures
* use more efficient search through candidates in offchain-election
* mark linear accessors as test-only in election-provider-multi-phase
This prevents production code which uses them from compiling.
Also write an efficient helper for getting the target index.
* doc grammar
* use faster target_index_fn in benchmarks
* unbox helper functions
* remove unnecessary import
* write lifetime after primary trait
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* Improve complexity of CompactAssignments::unique_targets
Original implementation was O(n**2). Current impl is O(n log n).
Avoided the original proposed mitigation because it does not retain
the de-duplicating property present in the original implementation.
This implementation does a little more work, but retains that property.
* Explicitly choose sp_std Vec and BTreeSet
Ensures that the macro still works if someone uses it in a context
in which sp_std is not imported or is renamed.
* explicitly use sp_std vectors throughout compact macro
* Add PJR challenge functions
- Updates the PJR check to return a counterexample if one exists
- Adds functions to cheaply check counterexamples
This is in support of off-chain PJR challenges: if a miner discovers
that an accepted election solution does not satisfy PJR, it will be
eligible for substantial rewards. This helps ensure that validator
elections have an absolute quality floor, so even if someone manages
to censor well-behaved solutions to give themselves unfair representation,
we can catch them in the act and penalize them.
* counterexample -> counter_example
* reorganize: high -> low abstraction
* reorganize challenges high -> low abstraction
* add note justifying linear search
* Simplify max_pre_score validation
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* add minor test of pjr challenge validation
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* Apply.
* get rid of glob import
* use meaningful generic type name
* pjr_check operates on `Supports` struct used elsewhere
* improve algorithmic complexity of `prepare_pjr_input`
* fix rustdoc warnings
* improve module docs
* typo
* simplify debug assertion
* add test finding the phase-change threshold value for a constructed scenario
* add more threshold scenarios to disambiguate plausible interpretations
* add link to npos paper reference
* docs: staked_assignment -> supports
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* add utility method for generating npos inputs
* add a fuzzer which asserts that all unbalanced seq_phragmen are PJR
Note that this currently fails. I hope that this can be rectified
by calculating the threshold instead of choosing some arbitrary number.
* assert in all cases, not just debug
* leverage a native solution to choose candidates
* use existing helper methods
* add pjr-check and incorporate into the fuzzer
We should probably have one of the W3F people look at this to ensure
we're not misconstruing any definitions, but this seems like a
fairly straightforward implementation.
* fix compilation errors
* Enable manually setting iteration parameters in single run.
This gives us the ability to reproducably extract cases where
honggfuzz has discovered a panic. For example:
$ cargo run --release --bin phragmen_pjr -- --candidates 569 --voters 100
Tue 23 Feb 2021 11:23:39 AM CET
Compiling bitflags v1.2.1
Compiling unicode-width v0.1.8
Compiling unicode-segmentation v1.7.1
Compiling ansi_term v0.11.0
Compiling strsim v0.8.0
Compiling vec_map v0.8.2
Compiling proc-macro-error-attr v1.0.4
Compiling proc-macro-error v1.0.4
Compiling textwrap v0.11.0
Compiling atty v0.2.14
Compiling heck v0.3.2
Compiling clap v2.33.3
Compiling structopt-derive v0.4.14
Compiling structopt v0.3.21
Compiling sp-npos-elections-fuzzer v2.0.0-alpha.5 (/home/coriolinus/Documents/Projects/paritytech/substrate/primitives/npos-elections/fuzzer)
Finished release [optimized] target(s) in 6.15s
Running `/home/coriolinus/Documents/Projects/paritytech/substrate/target/release/phragmen_pjr -c 569 -v 100`
thread 'main' panicked at 'unbalanced sequential phragmen must satisfy PJR', primitives/npos-elections/fuzzer/src/phragmen_pjr.rs:133:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
This is still not adequate proof that seq_phragmen is broken; it could
very well be that our PJR checker is doing the wrong thing, or we've
somehow missed a parameter of interest. Still, it's concerning.
* update comment verbiage for accuracy
* it is valid in PJR for an elected candidate to have 0 support
* Fix phragmen_pjr fuzzer
It turns out that the fundamental problem causing previous implementations
of the fuzzer to fail wasn't in `seq_phragmen` _or_ in `pjr_check`: it was
in the rounding errors introduced in the various conversions between the
internal data representation and the external one.
Fixing the fuzzer is then simply an issue of using the internal representation
and staying in that representation. However, that leaves the issue that
`seq_phragmen` occasionally produces an output which is technically not
PJR due to rounding errors. In the future we will need to add some kind of
"close-enough" threshold. However, that is explicitly out of scope of
this PR.
* restart ci; it appears to be stalled
* use necessary import for no-std
* use a more realistic distribution of voters and candidates
This isn't ideal; more realistic numbers would be about twice these.
However, either case generation or voting has nonlinear execution
time, and doubling these values brings iteration time from ~20s to
~180s. Fuzzing 6x as fast should make up for fuzzing cases half the size.
* identify specifically which PJR check may fail
* move candidate collection comment into correct place
* standard_threshold: use a calculation method which cannot overflow
* Apply suggestions from code review (update comments)
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* clarify the effectiveness bounds for t-pjr check
* how to spell "committee"
* reorganize: high -> low abstraction
* ensure standard threshold calc cannot panic
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
* Apply suggestions from code review
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: kianenigma <kian.peymani@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
* Create a macro which automates creation of benchmark test suites.
* bump impl_version
* allow unused on test_bench_by_name
* use proper doctest ignore attribute
* Explicitly hand the Module to the test suite
Much better practice than depending on it showing up implicitly in
the namespace.
* explicitly import what we need into `mod tests`
* bench_module is `ident` not `tt`
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
* allow end users to specify arguments for new_test_ext
This turned out to be surprisingly easy. On reflection, it turns out
that of course the compiler can't eagerly evaluate the function call,
but needs to paste it in everywhere desired.
* enable explicitly specifying the path to the benchmarks invocation
also enable optional trailing commas
* Revert "bump impl_version"
This reverts commit 0209e4de33fd43873f8cfc6875815d0fd6151e63.
* list failing benchmark tests and the errors which caused the failure
* harden benchmark tests against internal panics
* suppress warning about ignored profiles
unfortunately, setting the profile here doesn't do anything; we'd
need to set it in every leaf package anyway. However, as this was
just making the default explicit anyway, I think it's safe enough
to remove entirely.
* impl_benchmark_test_suite for assets
* impl_benchmark_test_suite for balances
* impl_benchmark_test_suite for bounties
* impl_benchmark_test_suite for Collective
* impl_benchmark_test_suite for Contracts
* impl_benchmark_test_suite for Democracy
* don't impl_benchmark_test_suite for Elections-Phragmen
* impl_benchmark_test_suite for Identity
Note that Identity tests currently fail. They failed in an identical
way before this change, so as far as I'm concerned, the status quo is
good enough for now.
* impl_benchmark_test_suite for ImOnline
* impl_benchmark_test_suite for indices
For this crate also, the test suite fails identically with and without
this change, so we can say that this change is not the cause of the
tests' failure to compile.
* impl_benchmark_test_suite for lottery
* impl_benchmark_test_suite for merkle-mountain-range
* impl_benchmark_test_suite for Multisig
These tests fail identically with and without the change, so the change
seems unlikely to be the origin of the failures.
* impl_benchmark_test_suite for offences
* impl_benchmark_test_suite for Proxy
Fails identically with and without this change.
* impl_benchmark_test_suite for scheduler
* impl_benchmark_test_suite for session
It turns out to be important to be able to exclude items marked
`#[extra]` sometimes. Who knew?
* impl_benchmark_test_suite for staking
* impl_benchmark_test_suite for system
* impl_benchmark_test_suite for timestamp
* impl_benchmark_test_suite for tips
* impl_benchmark_test_suite for treasury
* impl_benchmark_test_suite for utility
Note that benchmark tests fail identically before and after this change.
* impl_benchmark_test_suite for vesting
* fix wrong module name in impl_benchmark_test_suite in Offences
* address line length nits
* enable optional keyword argument: exec_name
Took a _lot_ of macro-wrangling to get the functionality that I want,
but now you have the option to pass in
```rust
impl_benchmark_test_suite!(
Elections,
crate::tests::ExtBuilder::default().desired_members(13).desired_runners_up(7),
crate::tests::Test,
exec_name = build_and_execute,
);
```
and have it expand out properly. A selected fragment of the expansion:
```rust
fn test_benchmarks() {
crate::tests::ExtBuilder::default()
.desired_members(13)
.desired_runners_up(7)
.build_and_execute(|| {
```
* get rid of dead code
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
* Factor out can_set_code
The motivation for this feature is parachain runtime upgrades, which
happen in two different moments: the initial call, when the checks
should be performed, and at the actual upgrade block. It's much
better and more maintainable to just call `can_set_code` in the
function than to manually copy those checks.
I'm not entirely thrilled with the interface, which requires
cloning the entire code vector in `set_code`. However, it looks
like something in `decl_module!` does not play nicely with
references in its function arguments. If `code` has a lifetime,
it _must_ be named, which gives us this signature.
```rust
pub fn can_set_code<'a>(origin, code: &'a [u8]) {
```
Unfortunately, attempting to compile with that signature generates
a very large and baffling collection of build errors, so I decided
to abandon that path for now.
* make can_set_code non-dispatchable per PR revew
Not only can we now borrow the `code` slice, but we also no longer
need to worry about people sending a `can_set_code` transaction because
of some misunderstanding.
* move into existing impl block
