CandidateBackingSubsystem

[montekki]

This is going to update the guide and add the implementation in a single commit, so it

Closes #1227
Closes #1150

[coriolinus]

Have not yet looked at node/core/backing/src/lib.rs; I'll get on that in the morning.

[rphmeier]

maybe slightly more efficient to join these runtime API requests so they can be pipelined.

At some point I'd like to do a refactor where we return most of this info from a single runtime API.

[montekki]

I don't think that it's doable: it would require two &mut self refs

[rphmeier]

Yeah, this may be one of the "Bad side-effects" of Object-Oriented design in Rust. In the functional style you can get away with this more easily because the lifetime of the return type of request_validators should not depend on the lifetime of the borrow of self. The thing that is returned is an owned oneshot::Receiver so there should be no reason that we cannot .await both at the same time. However Rust's async fn can make it hard to be explicit about lifetimes.

[montekki]

If I am understanding you right it would run into the same problem unless we do this by hand without any functions at all. Otherwise there still is a single channel we do the communication on and it would require passing two mutable references into two funcions:

async fn do_this(s: &mut mpsc::Sender<()>) { s.send(()).await; }

async fn do_that(s: &mut mpsc::Sender<()>) { s.send(()).await; }

join(do_this(&mut tx), do_that(&mut tx));

Or clone the Sender and pass it by value.

Or do everything in place: send message1 and get the oneshot::Receiver, send message 2 and get the oneshot::Receiver and do a join on those two.

[rphmeier]

Or do everything in place: send message1 and get the oneshot::Receiver, send message 2 and get the oneshot::Receiver and do a join on those two.

Yeah, this is what I was thinking of. Because as written, we wait for the response on one channel before even sending the message on the other.

If async fn request_x(&mut Sender) -> impl Future<Item = Result<X, SendError>> works it might be the best way to implement that. So it's asynchronously sending the message so it can return a future that you wait on to receive the response.

[rphmeier]

import_statement also does the signature check. IMO it should be done as early as possible and only once.

[rphmeier]

Check out what I did in #1344 : We don't need to clone the entire AbridgedCandidateReceipt (which can be large, containing the commitments) just to get a CandidateDescriptor. There is a to_descriptor function that clones only the necessary pieces, which are all small.

[rphmeier]

As far as I can see issued_validity is not updated here as well.

It might be easier to just track all candidates we have issued statements for and what kind of statement we have issued.

The current implementation of the code causes us to attempt revalidation every time we see a new Seconded message, even if we have already validated that candidate or even seconded it ourselves. So just tracking all candidates we have already issued statement on (regardless of what type of statement that is) can already be used to prevent issuing conflicting statements.

[rphmeier]

It also doesn't seem like the data is made available anywhere. Probably Valid should be expanded with outputs of validation in order to do that.

[rphmeier]

I'm bikeshedding, but I don't feel this name captures the fact that this does kick off validation work, if necessary.

[rphmeier]

The main reason I bring it up is that it might make sense to refactor this into two functions: one for importing external statements and one for kicking off validation work.

Actually, we already have a function for checking signature and importing the statement. It's called self.import_statement. The inner table returns an Option<Summary> that includes both the digest and the group ID.

So a simple kick_off_validation_work(Summary) might be enough, because the table also has a get_candidate function. Then we can kick off validation work for any candidate in our validation group. That would make most of this function redundant.

[rphmeier]

This seems to be requesting validation regardless of our group membership. We only need to be validating candidates from paras that are assigned to the group we are part of. We should not be validating or issuing statements about candidates from every para

[coriolinus]

Suggested change

	for p in proposed.into_iter() {
	let TableAttestedCandidate { candidate, validity_votes, .. } = p;
	for TableAttestedCandidate { candidate, validity_votes, .. } in proposed.into_iter() {

[rphmeier]

That line might be too long but I agree with it in spirit.

[rphmeier]

This seems to let a validator slide if we have already reported them for a difference offence. Slashing should be more eager than that

[rphmeier]

ATM we configured these as 100% slashes but we will probably scale that back somewhat and then the difference between multiple offences will be meaningful

[rphmeier]

@montekki file as a follow-on issue?

[montekki]

yes right now we are a bit limited with the implementation of the statement-table which holds a

	detected_misbehavior: HashMap<C::AuthorityId, MisbehaviorFor<C>>,

and what we get from it is this API:

	/// Access all witnessed misbehavior.
	pub fn get_misbehavior(&self)
		-> &HashMap<C::AuthorityId, MisbehaviorFor<C>>

so it looks like currently there is no way to get multiple offences out of it if I am not mistaken. I will file an issue for that.

[rphmeier]

I am realizing now that AvailabilityStore should have a StorePoV as well, which might encapsulate this logic. It doesn't seem to truly belong in this module.

[rphmeier]

#1240 (comment) Description of follow-up work that should come with a refactor of this code

[rphmeier]

clone should not be necessary with to_descriptor. At least the version in master takes &self

[montekki]

Conversion to_descriptor is not the reason.
clone is happening because table.get_candidate returns an Option<&Candidate> and we have to clone it to pass it by value into the validation.

[rphmeier]

It might be better to call this issued_statement because we are recording even Invalid statements here.

[rphmeier]

A sanity check here that the candidate is within our assignment group would be helpful too, I think.

[rphmeier]

LGTM modulo nits