implement bitfield signing subsystem #1364

coriolinus · 2020-07-06T15:30:06Z

Closes #1239. ~~Includes #1403.~~

node/bitfield-signing/src/lib.rs

roadmap/implementors-guide/src/node/availability/bitfield-signing.md

There were large merge issues with the old bitfield signing PR, so we're just copying all the work from that onto this and restarting. Much of the existing work will be discarded because we now have better tools available, but that's fine.

It's not an ideal implementation--we can make it much more concurrent-- but at least it compiles.

coriolinus · 2020-07-22T12:18:18Z

node/bitfield-signing/src/lib.rs

+	// REVIEW: is it safe to ignore parathreads here, or do they also figure in the availability mapping?
+	if let Some(CoreOccupied::Parachain) = core {


I think that we only care about availability as it relates to parachains, but I'm not certain, and the guide doesn't say (AFAICT).

What in the guide made you think that parathreads don't have to worry about availability? They are occupying availability cores just like parachains.

Anyway, this whole branch is a byproduct of using the wrong type / runtime API. This should be using the CoreState type, not the CoreOccupied type, which shouldn't even be exposed.

coriolinus · 2020-07-22T12:24:22Z

node/bitfield-signing/src/lib.rs

+	// In principle, this work is all concurrent, not parallel. In practice, we can't guarantee it, which is why
+	// we need the mutexes and explicit references above.
+	stream::iter(scheduler_roster.availability_cores.into_iter().enumerate())
+		.for_each_concurrent(None, |(idx, core)| async move {


The move flag here is the reason for the explicit references we construct above, and this appears to work, but it's not a pattern I've seen elsewhere in the codebase. Is there an elegant way to avoid explicit references?

coriolinus · 2020-07-22T12:29:05Z

node/subsystem/src/messages.rs

+	/// Query whether a `PoV` exists within the AV Store.
+	///
+	/// This is useful in cases like bitfield signing, when existence
+	/// matters, but we don't want to necessarily pass around multiple
+	/// megabytes of data to get a single bit of information.
+	QueryPoVAvailable(Hash, oneshot::Sender<bool>),


We could potentially avoid this variant, just using QueryPoV instead. Does it create any problems to create this variant?

I think this message type has some utility, but it's also not what the bitifeld signing subsystem needs.

Bitfield signing is supposed to query for whether we have our specific chunk of the PoV, not the whole PoV itself.

Quoting from the guide's description of bitfield signing:

For each bit in the bitfield, if there is a candidate pending availability, query the Availability Store for whether we have the availability chunk for our validator index.

coriolinus · 2020-07-22T12:29:36Z

node/subsystem/src/messages.rs

@@ -272,6 +293,8 @@ pub enum RuntimeApiRequest {
 	ValidationCode(ParaId, BlockNumber, Option<BlockNumber>, oneshot::Sender<ValidationCode>),
 	/// Get head data for a specific para.
 	HeadData(ParaId, oneshot::Sender<HeadData>),
+	/// Get a the candidate pending availability for a particular parachain by parachain / core index
+	CandidatePendingAvailability(ParaId, oneshot::Sender<Option<CommittedCandidateReceipt>>),


I couldn't see any way around creating this runtime request, but I could have missed something.

Yeah, this not existing is a byproduct of #1419 not being done.

parachain/src/primitives.rs

Co-authored-by: Andronik Ordian <write@reusable.software>

coriolinus · 2020-07-22T14:23:10Z

I am suspicious of the current CI failure messages:

     Updating git repository `https://github.com/paritytech/substrate`
error: failed to get `sc-client-api` as a dependency of package `polkadot-availability-store v0.8.18 (/builds/parity/polkadot/availability-store)`
Caused by:
  failed to load source for dependency `sc-client-api`
Caused by:
  Unable to update https://github.com/paritytech/substrate#5fd98a66
Caused by:
  revspec '5fd98a661d5384a00c40d17c08f28265dae729d4' not found; class=Reference (4); code=NotFound (-3)

I suspect that these are cache failures, so I'm going to try to re-discover the mechanism for clearing the CI cache.

rphmeier · 2020-07-22T21:16:39Z

My intuition would be: merge master and continue. Usually fixes spurious dependency errors like that.

This reverts commit 7fc7780.

This reverts commit f14ffe5.

node/bitfield-signing/src/lib.rs

drahnr · 2020-07-23T13:19:34Z

roadmap/implementers-guide/src/node/availability/bitfield-signing.md

@@ -4,6 +4,10 @@ Validators vote on the availability of a backed candidate by issuing signed bitf

 ## Protocol

+Input:
+
+There is no dedicated input mechanism for bitfield signing. Instead, Bitfield Signing produces a bitfield representing the current state of availability on `StartWork`.


Describing things that are not there is really helpful when reading the guide 👍

drahnr

Looks good to me, a few very minor nitpicks :)

node/bitfield-signing/Cargo.toml

rphmeier · 2020-07-23T15:34:17Z

node/bitfield-signing/src/lib.rs

+	// we have to (cheaply) clone this sender so we can mutate it to actually send anything
+	let mut sender = sender.clone();
+
+	// REVIEW: is it safe to ignore parathreads here, or do they also figure in the availability mapping?


Yes, we need to account for parachains and parathreads.

rphmeier · 2020-07-23T15:36:03Z

node/bitfield-signing/src/lib.rs

+	// request the validator groups so we can get the scheduler roster
+	let (tx, rx) = oneshot::channel();
+	sender
+		.send(RuntimeApi(Request(relay_parent, ValidatorGroups(tx))))


I have a hard time understanding this code. It makes a validator groups request and the rx receives a SchedulerRoster?

(anyway, this is getting the wrong info, as explained in other comments)

rphmeier · 2020-07-23T15:37:46Z

node/bitfield-signing/src/lib.rs

+	},
+	util::{self, JobManager, JobTrait, ToJobTrait, Validator},
+};
+use polkadot_primitives::v1::{AvailabilityBitfield, CoreOccupied, Hash};


I don't think the CoreOccupied type should be used. It shouldn't even be exposed from the runtime, but @montekki added it for candidate backing. Now that there are new runtime APIs, those should be used instead.

Except #1419 hasn't been done yet. That's why I recommended stubbing out an async fn availability_cores { unimplemented!() } in a separate chat. The implementation of that function would be replaced by the actual runtime API in the future.

- use CoreState, not CoreOccupied - query for availability chunks, not the whole PoV - create a stub `fn availability_cores`

* Apply suggestions from #1364 code review - use CoreState, not CoreOccupied - query for availability chunks, not the whole PoV - create a stub `fn availability_cores` * link to issue documenting unimplemented * implement get_availability_cores by adding a new runtime api request * back out an unrelated change properly part of #1404 * av-store: handle QueryChunkAvailability * simplify QueryDataAvailability * remove extraneous whitespace * compact primitive imports

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

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 6, 2020

coriolinus self-assigned this Jul 6, 2020

rphmeier reviewed Jul 6, 2020

View reviewed changes

node/bitfield-signing/src/lib.rs Outdated Show resolved Hide resolved

rphmeier reviewed Jul 6, 2020

View reviewed changes

node/bitfield-signing/src/lib.rs Outdated Show resolved Hide resolved

rphmeier reviewed Jul 6, 2020

View reviewed changes

roadmap/implementors-guide/src/node/availability/bitfield-signing.md Outdated Show resolved Hide resolved

rphmeier reviewed Jul 7, 2020

View reviewed changes

roadmap/implementors-guide/src/node/availability/bitfield-signing.md Outdated Show resolved Hide resolved

coriolinus force-pushed the prgn-bitfield-signing branch from 58a5e6f to 6a062fa Compare July 20, 2020 08:26

coriolinus added 6 commits July 22, 2020 14:12

update guide to reduce confusion and TODOs

170efd3

work from previous bitfield signing effort

ed778a4

There were large merge issues with the old bitfield signing PR, so we're just copying all the work from that onto this and restarting. Much of the existing work will be discarded because we now have better tools available, but that's fine.

start rewriting bitfield signing in terms of the util module

a8d312c

implement construct_availability_bitvec

6d3081a

It's not an ideal implementation--we can make it much more concurrent-- but at least it compiles.

implement the unimplemented portions of bitfield signing

29cc1c3

get core availability concurrently, not sequentially

5807799

coriolinus force-pushed the prgn-bitfield-signing branch from 2a2c862 to 5807799 Compare July 22, 2020 12:14

coriolinus commented Jul 22, 2020

View reviewed changes

coriolinus added 4 commits July 22, 2020 14:35

use sp-std instead of std for a parachain item

00ef611

resolve type inference failure caused by multiple From impls

7fc7780

handle bitfield signing subsystem & Allmessages variant in overseer

684038f

fix more multi-From inference issues

f14ffe5

ordian reviewed Jul 22, 2020

View reviewed changes

parachain/src/primitives.rs Outdated Show resolved Hide resolved

more concisely handle overflow

89ff07b

Co-authored-by: Andronik Ordian <write@reusable.software>

coriolinus added 5 commits July 23, 2020 12:03

Merge remote-tracking branch 'origin/master' into prgn-bitfield-signing

51dbee5

Revert "resolve type inference failure caused by multiple From impls"

c1b8b54

This reverts commit 7fc7780.

Revert "fix more multi-From inference issues"

4410c87

This reverts commit f14ffe5.

impl From<i32> for ParaId

528cf9b

handle another instance of AllSubsystems

7779405

coriolinus marked this pull request as ready for review July 23, 2020 12:19

coriolinus requested review from rphmeier and drahnr July 23, 2020 12:19

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 23, 2020

drahnr reviewed Jul 23, 2020

View reviewed changes

node/bitfield-signing/src/lib.rs Show resolved Hide resolved

drahnr reviewed Jul 23, 2020

View reviewed changes

node/bitfield-signing/src/lib.rs Show resolved Hide resolved

drahnr reviewed Jul 23, 2020

View reviewed changes

node/bitfield-signing/src/lib.rs Show resolved Hide resolved

drahnr reviewed Jul 23, 2020

View reviewed changes

improve consistency when returning existing options

333d076

drahnr approved these changes Jul 23, 2020

View reviewed changes

coriolinus merged commit 8217ca6 into master Jul 23, 2020

coriolinus deleted the prgn-bitfield-signing branch July 23, 2020 14:05

ordian reviewed Jul 23, 2020

View reviewed changes

node/bitfield-signing/Cargo.toml Show resolved Hide resolved

rphmeier reviewed Jul 23, 2020

View reviewed changes

coriolinus mentioned this pull request Jul 24, 2020

Fix bitfield signing #1466

Merged

coriolinus added a commit that referenced this pull request Jul 24, 2020

Apply suggestions from #1364 code review

474c221

- use CoreState, not CoreOccupied - query for availability chunks, not the whole PoV - create a stub `fn availability_cores`

coriolinus added a commit that referenced this pull request Jul 27, 2020

Apply suggestions from #1364 code review

ed2662d

- use CoreState, not CoreOccupied - query for availability chunks, not the whole PoV - create a stub `fn availability_cores`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement bitfield signing subsystem #1364

implement bitfield signing subsystem #1364

coriolinus commented Jul 6, 2020 •

edited

Loading

coriolinus Jul 22, 2020

rphmeier Jul 23, 2020

rphmeier Jul 23, 2020

coriolinus Jul 22, 2020

coriolinus Jul 22, 2020

rphmeier Jul 23, 2020

coriolinus Jul 22, 2020

rphmeier Jul 23, 2020

coriolinus commented Jul 22, 2020

rphmeier commented Jul 22, 2020

drahnr Jul 23, 2020

drahnr left a comment

rphmeier Jul 23, 2020 •

edited

Loading

rphmeier Jul 23, 2020

rphmeier Jul 23, 2020

rphmeier Jul 23, 2020

		// REVIEW: is it safe to ignore parathreads here, or do they also figure in the availability mapping?
		if let Some(CoreOccupied::Parachain) = core {

implement bitfield signing subsystem #1364

implement bitfield signing subsystem #1364

Conversation

coriolinus commented Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coriolinus commented Jul 22, 2020

rphmeier commented Jul 22, 2020

Choose a reason for hiding this comment

drahnr left a comment

Choose a reason for hiding this comment

rphmeier Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coriolinus commented Jul 6, 2020 •

edited

Loading

rphmeier Jul 23, 2020 •

edited

Loading