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

Add current relay block to ValidationParams #879

Closed
wants to merge 4 commits into from

Conversation

coriolinus
Copy link
Contributor

Passing along the current relay block here enables progress on https://github.com/paritytech/cumulus/tree/prgn-upgrade-parachain, which targets paritytech/cumulus#59.

Resolving the circular dependency was relatively simple: just move
a bunch of parachain-specific stuff from the parachain package into
the primitives package, then fix up the Cargo manifests. Then,
publicly import all the stuff which was moved so that we don't break
any external packages which depend on parachain.

We have a deprecation notice on that `pub use` statement because
downstream consumers should depend on items in the right place.
Unfortunately, it doesn't actually do anything due to
rust-lang/rust#47236. Still, we'll
leave it in against the day that bug gets fixed.

Adding current_relay_block to ValidationParams is only part of the
work, of course: we now need to go back to where that struct is
instantiated, and insert it there, tracing it back until we get to
some kind of relay chain instance from which we can get the actual
current value and insert it appropriately.
…d relay chain

The parachain validation logic trends heavily toward functions, not
structs with methods. This is probably a good thing for efficiency,
but does make it harder to incorporate an external piece of state like
the current relay chain block number: we have to change several
functions signatures and their call sites, not just add a field
to a struct.

This commit partially traces back one of the chains of compilation
errors resulting from missing function parameters, adding parameters
to parent functions appropriately. At
`validation::collation::collation_fetch`, the parameters change from
type `BlockNumber` to type `F: Fn() -> BlockNumber`, because that loop
looks like a place where the relay chain might potentially change
during execution.

Unfortunately, this doesn't work as-is right now, because within
`validation::validation_service::launch_work`, we get errors that
`get_current_relay_block` may not live long enough. The compiler
suggests requiring a `'static` lifetime, which feels like a
strategy which won't work unless the struct where the relay chain
block number is actually stored is also `'static`. Adding a `'a`
lifetime tied to `&'a self` doesn't work either. It looks like the
restriction comes from `futures::task::Spawn`, whose future must
apparently conform to `'static`.

We could just pass the current block number all the way from the
top of the function chain, but it's not yet obvious how these functions
are called by the relay chain, and at what points the current block
might change during execution.

Therefore, questions:

- Can we assume that the relay chain current block will not change
  during execution of this function chain? If yes, we can just
  pass the block number, which is `Copy` and therefore in `'static`.
- If no, is there a reasonable way to get a `'static` function from
  the relay chain so we can still use this strategy to get the
  current relay block number?
- If no, how should we propagate this data? Channels maybe?
- If we retain the closure strategy, is it a problem to call the
  non-async function `get_current_relay_block` within
  `async fn collation_fetch`? Given that async closures are not
  yet stable, do we have the option not to?
Turns out we already had a client object available which implemented
the necessary trait; the difficulty was just that I didn't know that
a. the trait existed, or that
b. the client in our possession implemented that trait.

This does make things much neater; it just means propagating the
trait bound backwards as implemented here.
@coriolinus coriolinus requested a review from bkchr March 5, 2020 12:59
@parity-cla-bot
Copy link

It looks like @coriolinus signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

validation/src/validation_service/mod.rs Outdated Show resolved Hide resolved
parachain/src/lib.rs Outdated Show resolved Hide resolved
@rphmeier
Copy link
Contributor

rphmeier commented Mar 5, 2020

We should pass not the "current" relay block, but the header corresponding to the relay_parent in the CandidateReceipt/CollationInfo, as that is the block the parablock is executing in the context of.

Although I don't follow exactly what is gained by passing the header. It seems passing the relay_parent hash would be enough, as then any parablock wanting to include the header could just include the header in its PoVBlock and check it against that hash.

@rphmeier rphmeier added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 5, 2020
@rphmeier
Copy link
Contributor

rphmeier commented Mar 5, 2020

I've tagged this as inprogress. Based on my reading of the PR, it seems incomplete, but correct the label if not correct (generally better to label these yourself!)

@@ -18,6 +18,7 @@ sp-io = { git = "https://github.com/paritytech/substrate", branch = "cumulus-bra
lazy_static = { version = "1.4.0", optional = true }
parking_lot = { version = "0.10.0", optional = true }
log = { version = "0.4.8", optional = true }
polkadot-primitives = { path = "../primitives", default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to bring polkadot-primitives in as a dependency. It depends on substrate primitives crates that we don't want to force all parachains to link to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The motivation there was to enable the use polkadot_primitives::BlockNumber import. If we replace the block number with the relay chain header, we still have to either use polkadot_primitives::Header, or make the ValidationParams struct generic.

I think you're right: if we have a generic ValidationParams, then we don't need most of the refactor in b4fb14b, which reduces the changes necessary to make all this work.

Copy link
Contributor

@rphmeier rphmeier Mar 9, 2020

Choose a reason for hiding this comment

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

I don't really like the idea of ValidationParams being generic, since we know what the types are going to be and are just getting around some type-fu.

I'd suggest instead breaking down polkadot-primitives into multiple crates and bringing in a crate which does not import Substrate WASM externalities.

@coriolinus
Copy link
Contributor Author

After more discussion, it appears that it should be possible to accomplish what I need without any of these changes.

@coriolinus coriolinus closed this Mar 6, 2020
@coriolinus coriolinus deleted the prgn-current-relay-block branch March 6, 2020 15:08
HCastano added a commit to HCastano/polkadot that referenced this pull request Apr 15, 2021
5330d84e CLI: naming clean-up. (paritytech#897)
f99f2225 Westend<>Rococo Headers Relay (paritytech#875)
72c9117b Use complex headers+messages relay in test deployments (paritytech#905)
48423d5b Stop recursing when creating test headers (paritytech#906)
f8586fd4 Fix outstanding bridge names. (paritytech#901)
54b683b3 Complex headers+messages Millau<->Rialto relay (paritytech#878)
c0e77ca1 fix message generator scripts (paritytech#900)
debf3a82 Use Substrate state_getReadProof RPC method to get storage proofs (paritytech#893)
c3fa7216 Support more than `u8::max_value` GRANDPA validators (paritytech#896)
e5cb87f9 Grandpa Pallet Pruning (paritytech#890)
0b6a8920 RestartNeeded is a connection error (paritytech#894)
2cf5fa26 CLI: Estimate Fee (paritytech#888)
7dace624 CLI: Send Message (paritytech#886)
f8eaecfa CLI: Encode Message (paritytech#889)
1610f868 Bump `jsonrpsee` to Alpha.3 (paritytech#892)
d665b531 Use new Cargo feature resolver (paritytech#891)
ce2ee6ed Rialto Millau Maintenance Dashboard (paritytech#881)
7c585ce8 Revert to older nightly. (paritytech#887)
73a0470e Adding GrandpaJustification custom type (paritytech#882)
b9ccea9c Install CA certificates in relay images (paritytech#880)
ec7841a2 fix widget names (paritytech#879)
REVERT: 746a4027 Accidentally committed `cargo-expand`ed code 🤦
REVERT: 1a5d09c5 Add note to more closely match `initialize` Call variant
REVERT: fdd6e6b3 Add `submit_finality_proof` mock Call variant
REVERT: 768b053e Simplify the Rococo and Westend signing params
REVERT: 62aca80e Add Westend<>Rococo variants to `relay_headers`
REVERT: 0bcb0f51 Add Westend<>Rococo variants to `init_bridge`
REVERT: 01d1305f Use mock Westend and Rococo finaltiy tx calls
REVERT: fb34b9dd Add modules for Rococo<>Westend header sync

git-subtree-dir: bridges
git-subtree-split: 5330d84e9511e38cf9d9ec765bee865fedd4b260
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants