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

mod subsystem-util #1376

Merged
merged 32 commits into from
Jul 14, 2020
Merged

mod subsystem-util #1376

merged 32 commits into from
Jul 14, 2020

Conversation

coriolinus
Copy link
Contributor

@coriolinus coriolinus commented Jul 8, 2020 •
edited
Loading

Includes #1312 so it can refactor that PR as the common methods are extracted.

  • JobCanceler
  • basic runtime requests
  • fancy runtime requests via util::Validator
  • job management with util::JobManager

Intentionally deferred work:

  • how precisely to handle incoming messages which don't have a parent hash

@coriolinus coriolinus added A3-in_progress Pull request is in progress. No review needed at this stage. 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 Jul 8, 2020
@coriolinus coriolinus self-assigned this Jul 8, 2020
@coriolinus coriolinus changed the title subsystem-util crate mod subsystem-util Jul 8, 2020
ordian
ordian reviewed Jul 10, 2020
View reviewed changes
node/subsystem/src/util.rs Outdated Show resolved Hide resolved
@coriolinus
Add subsystem-util crate.
aee251a 
Start by moving the JobCanceler here.
@coriolinus
copy utility functions for requesting runtime data; generalize
1719606
@coriolinus
convert subsystem-util from crate to module in subsystem
bc5efd0 
The point of making a sub-crate is to ensure that only the necessary
parts of a program get compiled; if a dependent package needed only
subsystem-util, and not subsystem, then subsystem wouldn't need to
be compiled.

However, that will never happen: subsystem-util depends on
subsystem::messages, so subsystem will always be compiled.

Therefore, it makes more sense to add it as a module in the existing
crate than as a new and distinct crate.
@coriolinus
make runtime request sender type generic
145dd58
@coriolinus
candidate backing subsystem uses util for api requests
e76239d
@coriolinus
add struct Validator representing the local validator
be2be35 
This struct can be constructed when the local node is a validator;
the constructor fails otherwise. It stores a bit of local data, and
provides some utility methods.
@coriolinus
add alternate constructor for better efficiency
32e5c18
@coriolinus
refactor candidate backing to use utility methods
5d8a8af
@coriolinus
fix test breakage caused by reordering tests
447ab4a
@coriolinus
restore test which accidentally got deleted during merge
1d59e66
@coriolinus
start extracting jobs management into helper traits + structs
fa46a8a
@coriolinus
use util::{JobHandle, Jobs} in CandidateBackingSubsystem
36b3367
@coriolinus
implement generic job-manager subsystem impl
91e301b 
This means that the work of implementing a subsystem boils down
to implementing the job, and then writing an appropriate
type definition, i.e.

pub type CandidateBackingSubsystem<Spawner, Context> =
	util::JobManager<Spawner, Context, CandidateBackingJob>;
@coriolinus
add hash-extraction helper to messages
3a47236
@coriolinus
fix errors caused by improper rebase
38110d5
coriolinus
coriolinus commented Jul 10, 2020
View reviewed changes
node/subsystem/src/util.rs Outdated Show resolved Hide resolved
node/subsystem/src/util.rs Outdated Show resolved Hide resolved
@coriolinus coriolinus marked this pull request as ready for review July 10, 2020 14:37
@github-actions github-actions bot removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Jul 10, 2020
drahnr
drahnr reviewed Jul 10, 2020
View reviewed changes
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

I very much like it, this reduces re-impl of the same code and reduces subsystem impl chores significantly! Could you potentially add a few tests showcasing the usage and information flow from handle_incoming to handle_outgoing?

@coriolinus
Copy link
Contributor Author

Could you potentially add a few tests showcasing the usage and information flow from handle_incoming to handle_outgoing?

Yes, though I'm getting to the end of the work day now. I'll handle testing, and the Stream impl (which I think is a good idea) on Monday morning.

drahnr
drahnr reviewed Jul 10, 2020
View reviewed changes
node/subsystem/src/util.rs Outdated Show resolved Hide resolved
montekki
montekki reviewed Jul 10, 2020
View reviewed changes
Copy link
Contributor

@montekki montekki left a comment

Choose a reason for hiding this comment

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

Overall a great effort; I am going to take another look after some time

node/core/backing/src/lib.rs Show resolved Hide resolved
node/core/backing/src/lib.rs Show resolved Hide resolved
node/core/backing/src/lib.rs Show resolved Hide resolved
node/subsystem/src/messages.rs Show resolved Hide resolved
node/subsystem/src/util.rs Outdated Show resolved Hide resolved
rphmeier
rphmeier reviewed Jul 12, 2020
View reviewed changes
Copy link
Contributor

@rphmeier rphmeier left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The approach looks fine to me, modulo nits left by others. I didn't look closely at the changes to candidate-backing.

One concern I have is that refactoring based on only a single example of usage can lead us to abstractions that do not actually encompass our range of desired behavior. Usually I would prefer to wait until we had 3 or more targets to refactor in order to have a sample of what abstractions are needed. So I expect to merge this in anticipation of the interface needing to be grown based on future needs.

@coriolinus
rename: handle_unhashed_msg -> handle_orphan_msg
fa595c2
@coriolinus
impl Stream for Jobs<Spawner, Job>
940c111 
This turns out to be relatively complicated and requires some
unsafe code, so we'll want either detailed review, or to choose
to revert this commit.
ordian
ordian reviewed Jul 13, 2020
View reviewed changes
node/subsystem/src/util.rs Outdated Show resolved Hide resolved
@coriolinus
add missing documentation for public items
11772f5
@coriolinus
use pin-project to eliminate unsafe code from this codebase
dc9e12c
@coriolinus
rename SenderMessage -> FromJob
802981a
@coriolinus
reenvision the subsystem requests as an extension trait
a5639e3 
This works within `util.rs`, but fails in `core/backing/src/lib.rs`,
because we don't actually create the struct soon enough. Continuing
down this path would imply substantial rewriting.
@coriolinus
Revert "reenvision the subsystem requests as an extension trait"
e7f5276 
This reverts commit a5639e3.

The fact is, the new API is more complicated to no real benefit.
@coriolinus
Copy link
Contributor Author

@rphmeier

Would really prefer to avoid unsafe code if possible.

Makes sense to me. Is the library introduced in dc9e12c a suitable replacement? It gets all unsafe code out of our codebase.

@rphmeier
Copy link
Contributor

Yeah, dc9e12c looks good to me.

@coriolinus
apply suggested futuresunordered join_all impl
0eaca1b
@coriolinus
CandidateValidationMessage variants have no top-level relay parents
863d733
@coriolinus
rename handle_orphan_msg -> handle_unanchored_msg
1e14746
@coriolinus
make most node-core-backing types private
e3f76a4 
Now the only public types exposed in that module are
CandidateBackingSubsystem and ToJob. While ideally we could reduce
the public interface to only the former type, that doesn't work
because ToJob appears in the public interface of CandidateBackingSubsystem.

This also involves changing the definition of CandidateBackingSubsystem;
it is no longer a typedef, but a struct wrapping the job manager.
rphmeier
rphmeier approved these changes Jul 14, 2020
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.

Thanks, looks great.

@rphmeier rphmeier merged commit d768411 into master Jul 14, 2020
@rphmeier rphmeier deleted the prgn-subsystem-util branch July 14, 2020 13:36
pepyakin added a commit that referenced this pull request May 4, 2022
@pepyakin
Squashed 'bridges/' changes from f220d2fccab..d2faa6b2600
c0c1eee 
d2faa6b2600 Update spec_version for Rococo (#1387)
b3d701124fe Remove support for encoded-call messaging from relay and runtime integration code (#1376)
7f1c4af6650 fix copypaste (#1386)
4e195205ae2 re-enable BEEFY alarms for Rialto (#1385)
072ae865d6b fix for "`/root` is not writable." during deployments startup (#1384)
3ab1810b071 fix daily gitlab build (#1383)
3317b8a6811 Update Substrate/Polkadot refs for latest BEEFY + xcm-v3 capability (#1381)
ebfa9f2fef8 remove vault from ci (#1382)
82136eb42e3 Switch to gav-xcm-v3 branch to be able to test bridges + XCMv3 integration (#1378)
aa8896475b6 Revert "mention encoded-calls-messaging tag"
80c0f7ee05d mention encoded-calls-messaging tag
c7c6f0ce5e8 Revert "add api data() for inbound_lane (#1373)" (#1375)
6ef6bcc0169 FinalityEngine in substrate relay (#1374)
82698e3e082 add api data() for inbound_lane (#1373)
74a48878f86 pub use WeightInfo in Grandpa + Messsages pallets (#1370)
2cc27b7abb5 Update Substrate/Polkadot/Cumulus references (#1364)
9f3ffcd59c7 [ci] Use bridges-ci:production image in the pipeline (#1365)
61766e31f2e Few typos and clippy fixes (#1362)
REVERT: f220d2fccab Polkadot staging update (#1356)
REVERT: 92ddc3ea7a8 Polkadot-staging update (#1305)
REVERT: 29eecdf1fa1 Merge pull request #1294 from paritytech/polkadot-staging-update
REVERT: 173d2d82297 Merge pull request #1280 from paritytech/polkadot-staging-update
REVERT: 54146416e7f Merge pull request #1276 from paritytech/polkadot-staging-update
REVERT: df701181745 Merge branch 'master' into polkadot-staging-update
REVERT: f704a741ee8 Polkadot staging update (#1270)
REVERT: 1602249f0a2 Enable Beefy debug logs in test deployment (#1237)
REVERT: c61d240b474 Fix storage parameter name computation (#1238)
REVERT: 96d3808e88f Integrate BEEFY with Rialto & Millau runtimes (#1227)
REVERT: f75a1bdd9ba update dependencies (#1229)
REVERT: 957da038547 Add mut support (#1232)
REVERT: 8062637289f fixed set_operational in GRANDPA pallet (#1226)
REVERT: 14b36ca4eef Add CODEOWNERS file (#1219)
REVERT: 3bec15766f6 Unify metric names (#1209)
REVERT: 0e839d2423e remove abandoned exchange relay (#1217)
REVERT: 2c91c6815cf Remove unused `relays/headers` (#1216)
REVERT: 80b1e65db82 Remove unused PoA<>Substrate bridge (#1210)
REVERT: f36f76fc2a7 Fix UI deployment. (#1211)
REVERT: fc0b65365bb Add `AtLeast32BitUnsigned` for MessageLance::SourceChainBalance (#1207)

git-subtree-dir: bridges
git-subtree-split: d2faa6b2600fea77d121f3c0767cf59211e597a3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ordian ordian ordian left review comments

@drahnr drahnr drahnr left review comments

@montekki montekki montekki left review comments

@rphmeier rphmeier rphmeier approved these changes

Assignees

@coriolinus coriolinus

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@coriolinus @rphmeier @montekki @drahnr @ordian