Lean BEEFY to Full BEEFY - don't skip (older) mandatory blocks and import justifications

[acatangiu]

The first step from Lean->Full BEEFY is to have the worker enforce uninterrupted line of BEEFY finalized mandatory blocks.

There is one mandatory block per session (the first block in the session). As such, votes processing and votes generation now enforces that all mandatory blocks are finalized in strict monotonically increasing sequence and no block N will be worked on if there is any GRANDPA finalized but BEEFY non-final mandatory block M, where M < N.

Implementation details:

• Plugged into the BlockImport queue and now forwarding all valid justifications to the voter.
• Introduced 'VoterOracle' to separate the voting decisions logic, and track new/pending sessions.
• New sessions get queued up with the worker operating either:
  1. up-to-date - all mandatory blocks leading up to current GRANDPA finalized: queue has ONE element, the 'current session' where mandatory_done == true,
  2. lagging behind GRANDPA: queue has [1, N] elements, where all mandatory_done == false. In this state, everytime a session gets its mandatory block BEEFY finalized, the session is popped off the queue, eventually getting to operating mode 1. up-to-date.
• Justifications and votes get triaged and those that fall withing the VoterOracle allowed window get processed, the others get dropped if stale, or buffered for later processing (when they reach the window).
• Worker general code was also updated to fall in one of two roles:
  1. react to external events and change internal 'state',
  2. generate events/votes based on internal 'state'.

Fixes #11788
Fixes #11789

---

Future PR will implement SYNC mechanism for on-demand justifications.

---

polkadot companion: paritytech/polkadot#5796

[acatangiu]

Actually hold off on reviewing this for a couple of days while I confirm this approach still works when plugging in block import (and later sync protocol).

L.E. done

[acatangiu]

Actually hold off on reviewing this for a couple of days while I confirm this approach still works when plugging in block import (and later sync protocol).

Plugged in block import now, please review.

[svyatonik]

I haven't finished yet - just couple of dumb questions :)

[svyatonik]

The inner here would be (or include) grandpa block import, right? If so, then iiuc in its current implementation (here, here and here) it won't pass BEEFY justification to the client => it won't be stored in the database. Am I missing something, or it isn't a part of this PR?

[svyatonik]

Another q: if justification is supposed to be saved in this call, then what if it is invalid? Since it is checked after this line, it may be invalid, but it still be saved to the db during this call, right?

[acatangiu]

Yes, inner here will be (or in the future include) grandpa block import.

it won't pass BEEFY justification to the client => it won't be stored in the database

I think you're right, I didn't realize the client will need it as well for storing in the db.
I cloned the beefy just before passing it down the line exactly because I saw that grandpa can take/consume all of them, so the BeefyWorker does get the justification, but the client backend+db might not.

Let's see what @andresilva thinks here before I start hacking the grandpa code.

if justification is supposed to be saved in this call, then what if it is invalid?

Also good catch! I guess as long as we filter before BeefyWorker it still works, but filtering out invalid justif early avoids us caching and forwarding invalid justifications. I will fix this.

[acatangiu]

filtering out invalid justif early avoids us caching and forwarding invalid justifications. I will fix this.

done

[acatangiu]

because we don't want to tie BEEFY with GRANDPA logic:

1. the BEEFY justification is removed from list going downstream,
2. once all inner finish successfully, justification is manually appended to backend if it is valid.

[svyatonik]

Dumb q: is it really rejecting such blocks, or just ignore BEEFY justifications? I'm asking because inner.import_block() has been (successfully) called already and here we just returning error here. Sorry - many things have changed in block import since I've last checked it, so maybe there's some transactional mechanism here, but I haven't found it.

[svyatonik]

Also (I believe you've said about that yesterday) - you're going to implement mandatory justifications requests in follow-up PRs, right? There's still needs_justification field in the import result - it won't be used in BEEFY pipeline, right?

[acatangiu]

I thought it actually rejected the block, even if inner components have accepted/imported it. I'll look into it.

you're going to implement mandatory justifications requests in follow-up PRs, right? There's still needs_justification field in the import result - it won't be used in BEEFY pipeline, right?

Right, we'll have our own on-demand justifications protocol, so we won't be using needs_justification (or any of the client-sync code I believe). I believe @andresilva has more details here.

[acatangiu]

So the block is still imported, only an error log and event are emitted:

substrate/client/consensus/slots/src/lib.rs

Lines 384 to 397 in 4a19d40

	Err(err) => {
	warn!(
	target: logging_target,
	"Error with block built on {:?}: {}", parent_hash, err,
	);
	telemetry!(
	telemetry;
	CONSENSUS_WARN;
	"slots.err_with_block_built_on";
	"hash" => ?parent_hash,
	"err" => ?err,
	);
	},

I'll remove the incorrect comment in beefy block import.

[acatangiu]

Reworked the block import logic now to:

• check validity early
• remove BEEFY justification before passing into inner, and manually add it to backend if all layers successful
• pass justif to worker only if all inner block imports are successful
• removed the incorrect comment around block not being imported in case of bad justification

I tried to split work in self-sufficient commits for easier review.

[svyatonik]

Everything looks sane to me, great job! I'll give it another (last) look tomorrow :)

[svyatonik]

Is this .hash() call really required? Can we reuse hash from above here, or is it different? I wonder if following scenario may happen: (1) you have best finalized header = 100 and hash of finalized block 50 = Hash50 (2) then we import alternate header 50 here with hash AlternateHash50 and then when you call client.hash(50) here, it returns Hash50, so we'll try to append justification for AlternateHash50 to Hash50.

[acatangiu]

This check is to protect against exactly that:
number and hash are the block number and hash of the currently importing block that came with current justification.

Right before actually importing justification we double-check that this block was in fact finalized (by GRANDPA or whatever finality gadget is underneath - we don't want to import any BEEFY justification for non-final blocks):

1. number <= status.finalized_number verifies that this block could be in the canonical fork/chain,
2. while Some(hash) == self.client.hash(number) builds on top of that and verifies this hash is part of the best fork (which is also finalized as checked in 1.)

(1) you have best finalized header = 100 and hash of finalized block 50 = Hash50 (2) then we import alternate header 50 here with hash AlternateHash50 and then when you call client.hash(50) here, it returns Hash50, so we'll try to append justification for AlternateHash50 to Hash50.

say we import both AlternateHash50 and Hash50:
only Hash50 == self.client.hash(50) when 50 <= status.finalized_number, so we drop any justification for AlternateHash50 (and log error - it can only happen if authorities are double voting) and only import one from Hash50.

In theory this should not be possible (BEEFY justifications should only be possible for finalized blocks, never for alternates), but this check is here as an extra layer of defence.

[svyatonik]

LGTM!

[andresilva]

looks good to me. great work @acatangiu

[andresilva]

I think we can avoid using the client here if we do instead self.backend.blockchain().info() (I didn't check though).

[acatangiu]

Nice! This way I could completely remove the need for Client: HeaderBackend generic from the block import object 👍

[acatangiu]

bot merge