Resolving Compile Errors in Core Time Prep Tests#1793
Conversation
# Description This PR introduces two changes: - the previous `Tracing` trait has been modified to accept contract address instead of code hash (seems to be way more convenient) - a new trait `CallInterceptor` that allows intercepting contract calls; in particular the default implementation for `()` will just proceed in a standard way (after compilation optimizations, there will be no footprint of that); however, implementing type might decide to mock invocation and return `ExecResult` instead Note: one might try merging `before_call` and `intercept_call`. However, IMHO this would be bad, since it would mix two completely different abstractions - tracing without any effects and actual intervention into execution process. This will unblock working on mocking contracts utility in drink and similar tools (use-ink/drink#33) # Checklist - [x] My PR includes a detailed description as outlined in the "Description" section above - [ ] My PR follows the [labeling requirements](https://github.com/paritytech/polkadot-sdk/blob/master/docs/CONTRIBUTING.md#process) of this project (at minimum one label for `T` required) - [x] I have made corresponding changes to the documentation (if applicable) - [x] I have added tests that prove my fix is effective or that my feature works (if applicable)
closes #158. partially addresses #226. Instead of fragile calculation of current balance by looking at `free balance - ED`, Nomination Pool now freezes ED in the pool reward account to restrict an account from going below minimum balance. This also has a nice side effect that if ED changes, we know how much is the imbalance in ED frozen in the pool and the current required ED. A pool operator can diligently top up the pool with the deficit in ED or vice versa, withdraw the excess they transferred to the pool. ## Notable changes - New call `adjust_pool_deposit`: Allows to top up the deficit or withdraw the excess deposited funds to the pool. - Uses Fungible trait (instead of Currency trait). Since NP was not doing any locking/reserving previously, no migration is needed for this. - One time migration of freezing ED from each of the existing pools (not very PoV friendly but fine for relay chain).
Derive `RuntimeDebug\Eq\PartialEq` but do not bound any generics. This achieved by using their equivalent no bound versions: `EqNoBound\PartialEqNoBound\RuntimeDebugNoBound`. Deriving with `Debug`, `Eq`, and `PartialEq` for the `Debt` and `Credit` type aliases of `Imbalance` is not feasible due to the `OnDrop` and `OppositeOnDrop` generic types lacking implementations of the same traits. This absence posed challenges in testing and any scenarios that demanded the traits implementations for the type.
Co-authored-by: Bastian Köcher <git@kchr.de>
What does this PR do? - Introduced the TotalValueLocked storage for nomination-pools. - introduced a slashing api in mock.rs - additional test for tracking a slashing event towards a pool without sub-pools - migration for the nomination-pools (V6 to V7) with `VersionedMigration` Why are these changes needed? this is the continuation of the work by @kianenigma in this [PR](paritytech/substrate#13319) How were these changes implemented and what do they affect? - It's an extra StorageValue that's modified whenever funds flow in or out of staking for any of the `bonded_account` of `BondedPools` - The `PoolSlashed`event is now emitted even when no `SubPools` are found Closes #155 KSM: HHEEgVzcqL3kCXgsxSfJMbsTy8dxoTctuXtpY94n4s8F4pS --------- Co-authored-by: Liam Aharon <liam.aharon@hotmail.com> Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com> Co-authored-by: Ankan <ankan.anurag@gmail.com> Co-authored-by: command-bot <>
Moving a few pallets to the latest and greatest `derive_impl` to give it a try. Part of #171 --------- Co-authored-by: Adrian Catangiu <adrian@parity.io> Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
…o CI (#1344) Makes SPs first class citizens along with the relay chains in the context of our CI runtime upgrade checks. ## Code changes - Sets missing current storage version in `uniques` pallet - Adds multisig V1 migration to run where it was missing - Removes executed migration whos pre/post hooks were failing from collectives runtime - Initializes storage versions for SP pallets added after genesis - Originally I was going to wait for #1297 to be merged so this wouldn't need to be done manually, but it doesn't seem like it'll be merged any time soon so I've decided to set them manually to unblock this ## CI changes - Removed dependency of `westend` runtime upgrades being complete prior to other ones running. I assume it is supposed to cache the `try-runtime` build for a performance benefit, but it seems it wasn't working. Maybe someone from the CI team can look into this or explain why it needs to be there? - Adds check-runtime-migration jobs for Parity asset-hubs, bridge-hubs and contract chains - Updated VARIABLES to accomodate the `kusama-runtime` package being renamed to `staging-kusama-runtime` in #1241 - Added `EXTRA_ARGS` variable to `check-runtime-migration`, and set `--no-weight-warnings` to the relay chain runtime upgrade checks (relay chains don't have weight restrictions).
…tem (#1761) re: https://forum.polkadot.network/t/blocknumber-vs-timestamps-should-we-abandon-blocktimes-altogether/4077 This exposes the `LastRelayChainBlockNumber` storage member of `cumulus-pallet-parachain-system` with a getter and alters the behavior of this storage item to only be updated in `on_finalize` to ensure a consistent value throughout `on_initialize` and within transactions. Parachains, especially with features such as asynchronous backing and agile coretime, should not use the parachain block number as a clock. Any feature of Polkadot intended to optimize core utilization and parachain coretime consumption is likely to worsen this clock as it is practically applied.
Since the hash rules of this part of the `pallet_prefix/storage_prefix` are always fixed, we can put the runtime calculation into compile time. --- polkadot address: 15ouFh2SHpGbHtDPsJ6cXQfes9Cx1gEFnJJsJVqPGzBSTudr --------- Co-authored-by: Juan <juangirini@gmail.com> Co-authored-by: command-bot <> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Branislav Kontur <bkontur@gmail.com>
## [Updated review bot version](677610b) updated version to version `2.0.1` which contains paritytech/review-bot#90, a fix for the team members not being fetch in its totality. ## [Updated review-bot.yml minApprovals convention](b144683) Renamed `min_approvals` to `minApprovals`. A breaking change in paritytech/review-bot#86 which was done to standarize all the cases (so now everything is camelCase)
Applied changes from the [User Update Guide](https://docs.google.com/document/d/1WQijD3bZTCsudOyPcDvugv659nCa2hEp2b_8eRU0h-Q), diverging in the node side where service.rs is different for `polkadot-parachain` than in the parachain template.
…kadot-sdk into brad-core-time-tests
BradleyOlson64
had a problem deploying
to
master
GitHub Actions
Failure
BradleyOlson64
had a problem deploying
to
master
GitHub Actions
Failure
eskimor
left a comment
eskimor
left a comment
There was a problem hiding this comment.
Good work @BradleyOlson64 ! Left a few comments. I think we should untangle tests a bit. It should not be that we need to expose assigner internals to scheduler tests.
| } | ||
|
|
||
| #[cfg(test)] | ||
| pub fn add_on_demand_order( |
Might panic at runtime if legacy parachain assignment provider is not configured correctly.
| } | ||
|
|
||
| impl<T: Config> AssignmentProvider<BlockNumber> for Pallet<T> { | ||
| type AssignmentType = TestUnifiedAssignment; |
There was a problem hiding this comment.
This does not make any sense. This mock assignment provider exists to test scheduler functionality. If we want to test migrations from old to new we should do that in the unifying assignment provider itself. Why would we duplicate real internal functionality that should not matter to the outside in a mock?
There was a problem hiding this comment.
The main reason is so that push_back_assignment knows how to behave. It is a part of the AssignmentProvider interface, so I can't change its signature. And it only has the parameter AssignmentType to tell it how to behave. Using an AssignmentType enum seems elegant enough though perhaps it could be made simpler still. A bunch of tests were failing before I swapped in TestUnifiedAssignment.
I'm open to other options if there's something you'd prefer!
There was a problem hiding this comment.
It occurs to me that checking whether the ParaId in a V0Assignment corresponds to a parachain would likely be sufficient. I'll try that, and get rid of TestUnifiedAssignment if so.
There was a problem hiding this comment.
The point is: This interface does not tell anything about the nature of the assignment provider. If assignments would need to be of a certain type, the architecture would be completely broken. The only requirement is that it implements Assignment.
BradleyOlson64
had a problem deploying
to
master
GitHub Actions
Failure
BradleyOlson64
had a problem deploying
to
master
GitHub Actions
Failure
|
|
||
| fn push_back_assignment(assignment: Self::AssignmentType) { | ||
| if <paras::Pallet<T>>::is_parachain(assignment.para_id()) { | ||
| } else { |
There was a problem hiding this comment.
For assignments corresponding to parachains we act as if we are in a real ParachainsAssigner, where there is no order queue to push back onto.
This keeps bulk parachain assignments that are pushed back from accidentally adding to the on-demand queue.
There was a problem hiding this comment.
I'll add more comments describing what's going on here and throughout the mock assigner.
There was a problem hiding this comment.
But why? You seem to be basically reproducing the real functionality .... why have the mock in the first place then?
There was a problem hiding this comment.
Perhaps I misinterpreted this:
“We could and likely should use a test config that uses a mock test assignment provider, so we don't need to expose internals of the used assignment provider here”
I took this to mean, “Let’s construct a mock assignment provider with minimum necessary complexity to do the following.
- Allow all scheduler tests to compile and pass without re-writing them.
- Avoid exposing any internals of actual assigners to those tests.
It seems there’s a third criterion I’m not sure can be met without giving up the re-writing part of the first:
3. Avoid replicating any large portion of the existing assigner logic.
There was a problem hiding this comment.
I’ll do some thinking about how the scheduler tests could be rewritten to be assigner agnostic.
There was a problem hiding this comment.
No, 3 is not directly the requirement. The requirement is, that if the interna of the assignment provider matter, then the scheduler tests are broken and should be fixed. Any assignment provider internal functionality that is tested there, should be tested in the assignment provider itself.
| // These numbers are set via config in individual scheduler tests. Just pass them through | ||
| // here. | ||
| fn get_provider_config(core_idx: CoreIndex) -> AssignmentProviderConfig<BlockNumber> { | ||
| if Self::is_legacy_core(&core_idx) { |
There was a problem hiding this comment.
on_demand_claims_are_pruned_after_timing_out uses these values in the on-demand case to inform testing. Providing bulk config values for bulk core_idxs should keep some logic errors from popping up if we decide to test bulk core timeouts in the future.
| // Create a new assignment to pass if core belongs to bulk buyer, else pop | ||
| // on demand assignment from front of queue. | ||
| fn pop_assignment_for_core(core_idx: CoreIndex) -> Option<Self::AssignmentType> { | ||
| if Self::is_legacy_core(&core_idx) { |
There was a problem hiding this comment.
This assigner is fulfilling the role of either a ParachainsAssigner or an OnDemandAssigner for each call. By calling is_legacy_core we determine which way to act using the input core_idx. The idea is that for a bulk time core index this assigner will always behave like a ParachainsAssigner. The same applies to on-demand core indices.
The follow up question to this would be, "Why do we need the MockAssigner to act like any particular kind of assigner." The answer is that the assigner is how we control the flow of eventual ParasEntries in scheduler tests as well as the AssignmentProviderConfig telling scheduler tests how to handle them.
We test the scheduler in scenarios where it needs to behave as if it has a real OnDemandAssigner: on_demand_claims_are_pruned_after_timing_out and next_up_on_time_out_reuses_claim_if_nothing_queued.
We also test it in scenarios where it needs to behave as if it has a ParachainsAssigner:
next_up_on_time_out_is_parachain_always
There was a problem hiding this comment.
In this specific instance, for on-demand core indices, we need to drain an entry from the queue each time so that we can test scenarios where the scheduler becomes drained of ParasEntrys
There was a problem hiding this comment.
We test the scheduler in scenarios where it needs to behave as if it has a real OnDemandAssigner: on_demand_claims_are_pruned_after_timing_out and next_up_on_time_out_reuses_claim_if_nothing_queued.
Yeah, this is what is wrong. It is valuable though for the scheduler to have tests, checking whether it behaves correctly if there are no more assignments on a core. This can be achieved for example by just having a queue of assignments for each core in the mock assigner which can be populated by tests as they need to. This would indeed be quite similar to what on-demand is doing, but that is just coincidence not the design. Does this make sense?
|
Commenting to highlight, I'm greatly shortening |
eskimor
left a comment
eskimor
left a comment
There was a problem hiding this comment.
Nice! Thanks @BradleyOlson64 !
|
|
||
| impl ParachainsAssignment { | ||
| fn new(para_id: ParaId) -> Self { | ||
| pub(super) fn new(para_id: ParaId) -> Self { |
There was a problem hiding this comment.
If we moved the parachains test parachains_assigner_pop_assignment_is_always_some one level down (to here), then we would not need to break encapsulation and could keep this private.
|
|
||
| // cores 0 and 1 are occupied by lease holding parachains. cores 2 and 3 are occupied by | ||
| // on-demand parachain claims. core 4 was free. | ||
| // cores 0 and 1 are occupied by on-demand claims. core 2 was free. |
There was a problem hiding this comment.
I think this comment is outdated. (No on-demand here)
| } | ||
|
|
||
| T::AssignmentProvider::report_processed(dropped.assignment); | ||
| for _ in 0..num_dropped { |
There was a problem hiding this comment.
Note: I refactored/cleaned up this a bit more.
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
The CI pipeline was cancelled due to failure one of the required jobs. |
20 participants
This leaves one build error I wasn't sure how to handle in builder.rs.
No guarantees all the tests still pass, but I did my best to swap in the new types without altering any logic.