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 <[email protected]>

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.

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 <[email protected]>

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

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