This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Conversation
…bstrate Update Substrate 3e65111
Add cfg_attr ... no_std
This pallet will eventually make life much easier for people attempting to upgrade parachains on their validator nodes, but for the moment, key sections remain unimplemented while dependency details are worked out.
This compiles, which means it's probably mostly correct. However, it's pretty far from being finished. Work yet to come: - Integrate with the democracy pallet somehow to eliminate the requirement for the root user to initiate this process. - Figure out what to do in the event that the parachain blocks and relay chain blocks get out of sync / delayed. - Add testing... somehow. (What's reasonable to test?) Open questions: - Is the block number parameter in `on_initialize` the parachain block number, or the relay chain block number? If, as I suspect, it's the parachain block number, how do we deal with the fact that the real upgrade should happen on a very specific parachain block number? - In line 68, is it reasonable to use `if n >= apply_block`, or should that line require strict equality? - Is it reasonable to store/retrieve `CurrentBlockNumber` on every block, or is there a more streamlined way to pass that data between functions? Maybe it can be inserted into `struct Module` somehow? - Can we somehow parametrize ValidationUpgradeDelayBlocks by T in order to eliminate the `.into()` call?
Currently doesn't build: line 230 is the problem. Removing or
commenting that line results in the new tests failing due to a
missing block number. Adding it, in an attempt to fix the problem,
fails to compile with this error:
```
Compiling parachain-upgrade-pallet v2.0.0 (/home/coriolinus/Documents/Projects/coriolinus/parachain-upgrade-pallet)
error[E0599]: no function or associated item named `set_block_number` found for struct `Module<tests::Test>` in the current scope
--> src/lib.rs:230:21
|
47 | / decl_module! {
48 | | pub struct Module<T: Trait> for enum Call where origin: T::Origin {
49 | | // Initializing events
50 | | // this is needed only if you are using events in your pallet
... |
100 | | }
101 | | }
| |_- function or associated item `set_block_number` not found for this
...
230 | System::set_block_number(123);
| ^^^^^^^^^^^^^^^^
| |
| function or associated item not found in `Module<tests::Test>`
| help: there is a method with a similar name: `current_block_number`
|
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
error: aborting due to previous error
For more information about this error, try `rustc --explain E0599`.
error: could not compile `parachain-upgrade-pallet`.
```
That error is very weird, because the function does in fact exist:
https://github.com/paritytech/substrate/blob/a439a7aa5a9a3df2a42d9b25ea04288d3a0866e8/frame/system/src/lib.rs#L897
Turns out that in fact there was some setup required in order to get everything testing properly, but now we have a set of passing unit tests which test some of the more common error cases.
This currently fails, and I don't yet know why. TODO!
- In particular, only remove the pending validation function from storage when it's time to apply it. - Don't store our own copy of the current block number.
They're defined in System::can_set_code, so may as well use them. Unfortunately, the tests all fail for this right now, and I don't yet understand why. Pushing to get immutable line number references.
Right now, the events struct doesn't seem to contain enough information to validate the particular events that we should have fired. Almost certainly, this is a usage error on my part.
This doesn't change the results, though.
This was complicated to figure out. For the record, testing events requires: - a private module which publicly exports the crate's event type - impl_outer_event macro to construct an enum wrapping the event types from the listed modules - system::Trait and local Trait both declare `type Event = TestEvent;` - (optional) group events with `System::<Test>::initialize(...)` and `System::<Test>::finalize()` calls. It's not entirely clear why both events show up during the initialization phase; my suspicion is that this is an artifact of not mocking a particular extrinsic, such that they end up in initialization by default.
this prepares us to merge this pallet into the cumulus repo
…et into prgn-upgrade-parachain-2 This brings in the parachain upgrade pallet and its history.
This feels like the logical next step, and compiles, at least. Still, there are some big TODOs remaining: - merge the polkadot PR upstream and reset the polkadot branch in `runtime/Cargo.toml` - in `runtime/src/validate_block/implementation.rs:116`, we should almost certainly return `Some(something)` sometime. When, precisely, and how to keep track of the appropriate data are all still open questions.
Hopefully we can upstream `ValidationFunctionParams` into the polkadot trait defs so we can just copy the struct out of `ValidationParams`, but no huge loss if not. This should be more or less everything required at this level. Next up: fix up `pallet-parachain-upgrade` so it reads from `VALIDATION_FUNCTION_PARAMS` to determine upgrade legality and upgrade block, and writes to `NEW_VALIDATION_CODE` when appropriate.
Implements the pallet side of the new flow. Basic tests appear to work. Next up: - make the "real blob" test work - add a bunch of additional tests of all the corners
This test didn't directly test any of the code in this pallet; it existed because we were just copying tests out of the substrate implementation. Now that we have real code of our own to test, (and because it's not compatible with the `BlockTests` abstraction,) we don't need that test anymore. Also added a `Drop` impl to `BlockTests` ensuring they get run at least once.
Contributor
Author
|
The CI failure looks like this: As this PR doesn't directly touch either polkadot or substrate, I'm not sure what to do about that. |
It is very difficult to come up with a coherent theory of what's going on with these default features. Builds were all broken as of 3eb1618. Renaming them in f741cf0 seemed to fix that behavior. Then they broke again locally, prompting aaee1c0. This commit restores the status quo as of f741cf0; with any luck, the build will succeed both locally and in CI.
Contributor
Author
|
Whatever is going on with the build failures, it's not related to the enabled feature set: $ git diff f741cf0..008d3ea '**/Cargo.toml'
$ git diff db26357..008d3ea '**/Cargo.toml'
$Given that there are also no
I still don't know how to fix the CI problem, but I believe I can now confidently state that it is a CI problem. |
This updates several packages, but by inspection, they are all published crates from crates.io; by semver, this should not cause any behavioral changes. This also updates the lockfile format to the new format. The point of this commit is to deal with the fact that `sc-client` no longer exists.
Appropriate weight declarations have changed; this follows them, still using timestamp examples. Note that these weights are almost certainly wrong.
Contributor
Looks like you leaked std somehow. Did you manage to find the issue? |
Contributor
Author
There isn't yet an obvious use case for other modules to get the validation function params from this one, but we may as well support it on the off chance.
This getter allows other modules to simply get the validation function parameters, if they have been updated for this block. Otherwise, it returns None, and they can try again later.
bkchr
reviewed
May 4, 2020
These suggestions should make no semantic difference. Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
These changes affect the semantics of the code; I'll follow up by ensuring that everything still works. Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
bkchr
approved these changes
May 5, 2020
Member
bkchr
left a comment
bkchr
left a comment
There was a problem hiding this comment.
One last nitpick, otherwise it looks good :)
| @@ -0,0 +1,51 @@ | |||
| //! Validation Function Parameters govern the ability of parachains to upgrade their validation functions. | |||
Contributor
Author
There was a problem hiding this comment.
I think I need a VS Code extension that nags me if the license header is missing; I just don't think of that at first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #59.