Block initial broadcast once verified

[rakanalh]

Resolves paritytech/polkadot-sdk#65

This should trigger pre-import prior to executing the block, but after all consensus checks and validations have been made on the received block.
As discussed with @gnunicorn, converting BlockImportNotification should serve the purpose of being able to know whether the notification is about a pre-import block or for an imported block, which opens the possibility to have other components react to a pre-imported block if that serves a specific functionality.
Currently, the network layer would react to such a notification but calling announce_block which would distribute the block info to connected peers.

[gnunicorn]

LGTM

[rakanalh]

Testing shows that the network layer rejects this because the block is unknown through the chain.

substrate/client/network/src/protocol.rs

Lines 1090 to 1100 in 3c9b031

	let header = match self.chain.header(BlockId::Hash(hash)) {
	Ok(Some(header)) => header,
	Ok(None) => {
	warn!("Trying to announce unknown block: {}", hash);
	return;
	}
	Err(e) => {
	warn!("Error reading block header {}: {:?}", hash, e);
	return;
	}
	};

Since this comes through the NetworkService's call to announce_block... and is sent to the NetworkWorker through ServiceToWorkerMsg::AnnounceBlock msg, my current solution to this problem is either attach a bool which indicates that this is a preimport to skip this step. Thoughts? @gnunicorn, @tomaka.

[andresilva]

Regardless of the current design details, with the current implementation I am not entirely sure how this is supposed to work in practice. Let's look at an example of what will happen:

• Node1 starts importing block A
• It does the initial verification checks
• It pre-announces the block
• It begins importing the block (for Polkadot we can assume this will take at most 2s but it could be more for other Substrate-based chains)
• In the meantime Node1 gets a request for block A, which it can't serve since it's not imported yet
• Node2 which requested block A from Node1 will reduce its reputation since it announced something that it couldn't serve (eventually disconnecting from it when the reputation gets too low).

[gnunicorn]

pulling @rphmeier into this conversation, especially regarding the comment by @andresilva . Rob, thoughts?

[andresilva]

Currently the sync code always answers block requests by looking at what's on the database. I think for this feature to work the sync code will also need to be able to serve blocks that have been pre-imported but are still being imported, it will need to somehow manage a cache of what's in the preimport -> import stage. I don't have any concrete design suggestions for how to do this.

[rphmeier]

Yes, I agree with André - we need the sync code to be able to serve pre-verified blocks before they exist in the DB. And this overlay should be pruned after the blocks are in the DB or once verification fails.

@bkchr We'd like the checks that are done within import_block of GRANDPA and BABE to be done before broadcast. When I had a brief look at these checks, especially those in GRANDPA, it seemed safer to have them run. The main expense that we are trying to avoid is executing the block in the runtime and committing trie changes to RocksDB, AFAIK. This is difficult to do without a notification stream as the BlockImport traits are composed and ultimately form a synchronous wrapper around the client.

I see two ways forward if the extra notification stream in the client is to be avoided. I reserve judgement on whether it should be and defer to @gnunicorn and other maintainers on that decision. I do think that the addition of an extra notification stream is ugly.

1. Investigate the attack surface of doing only header-based checks and if acceptable, build the broadcast into import queue.
2. Re-design the BlockImport trait and interface to better support this use-case.

One way of redesigning might be to make BlockImport an async trait, where we introduce yet another layer of the onion that broadcasts pre-import notifications:

sketch:

struct BroadcastPreImportNotificationsBlockImport(Vec<Sender>, inner: BlockImport);

async fn import_block(...) {
    let notification = make_notification(...);
    for sender in senders { sender.send(notification).await } // no unbounded channel
    inner.import_block(...)
}

and this would be instantiated at the service level and hooked up to the sync protocol.

There are other, cleaner ways to accomplish the same thing but would require digging out even more internals

[bkchr]

@bkchr We'd like the checks that are done within import_block of GRANDPA and BABE to be done before broadcast. When I had a brief look at these checks, especially those in GRANDPA, it seemed safer to have them run. The main expense that we are trying to avoid is executing the block in the runtime and committing trie changes to RocksDB, AFAIK. This is difficult to do without a notification stream as the BlockImport traits are composed and ultimately form a synchronous wrapper around the client.

Yeah, I did read and understood what you said. For me it is more how this communication is done here. We already have a communication between the import queue and sync. Sync instructs the import queue to import blocks and it gets notified by the task about the results. This is complelty independent from the rest of the system.

I think it should stay this way. Adding some new notification type deep into substrate, is just some hack. We already have the problem that no one really knows when such a notification is send and it also changes from time to time.

This import queue is currently implemented relative ugly, but could be improved, especially for what we want here. In ethereum we apparently only checked that the header was correct. In the end we should be able to do here the same. Even if all checks in the block import in grandpa and babe are correct, executing the block could still fail. Maybe we require some way in the sync protocol to invalidate a block announcement or something similar.

[rphmeier]

In ethereum we apparently only checked that the header was correct. In the end we should be able to do here the same

Yes, however, it'll require some reorganization of GRANDPA code. Worth nothing that in Ethereum we only had PoW which has no on-disk metadata except the DAG which is just a function of block height. GRANDPA & BABE require a lot more on-disk metadata which we'd like to be written to the DB atomically with the trie changes.

Of course, these problems are all solvable, and we could easily move all these checks to the header verification in such a way that the pending DB updates are supplied to the BlockImport logic to be passed to the client without doing checks more than once per block. it just requires someone to roll up their sleeves, get a shovel, and dig ... @rakanalh ? :)

[rakanalh]

@tomaka, @gnunicorn and I discussed recent changes and we'd like your opinion on this.

Following @bkchr's opinion on keeping the logic for announcing pre-import blocks isolated in the networking layer... the current implementation (still WIP) would differentiate between announcing pre-imported blocks from imported blocks but we end up sending the same message over the network. The differentiation happens internally since when announcing to peers, we would end up keeping the pre-imported headers locally so that any requests for a specific block (or header) would be answered from the list of pre-imported blocks maintained by the sync service. So for peers, it's going to be transparent whether the announcement is for a pre-imported block or an imported one.... that follows the suggestion made by @andresilva.

Any toughts on this approach?

[rakanalh]

Currently the sync code always answers block requests by looking at what's on the database. I think for this feature to work the sync code will also need to be able to serve blocks that have been pre-imported but are still being imported, it will need to somehow manage a cache of what's in the preimport -> import stage. I don't have any concrete design suggestions for how to do this.

I have hit a roadblock regarding this. Apparently when some node (A) requests a block which was announced by Node (B), the block is not going to be registered in the backend which means that Node (A) will receive a response with 0 blocks. Therefore, disconnecting Node (B) and rejecting any further connections.

The problem with trying to resolve this issue is that the BlockRequestHandler only knows the client and can only serve blocks which are in the chain backend. However, knowledge about pre-imported blocks can exist only in the Protocol/ChainSync layers.

The only immediate idea i have right now is to pull out the BlockRequestHandler logic which fetches the block(s) from the backend and somehow make it pass through the ChainSync instead. Which leaves us with:
BlockImportHandler::handle_request -> ChainSync::block_by_id -> Either from client.header(block_id) or ChainSync::header(block_id).

Not sure how much effort that would take because the instance of ChainSync is burried under NetworkWorker/Protocol/ChainSync. The alternative is to pass down a channel to ChainSync to handle requests made by the BlockRequestHandler.

Any other ideas?

Edit: Found a solution and the PR is now ready for review

[bkchr]

What is the status of this pr?

A lot of stuff is not documented, there is not one test added.

I added quite a lot of comments.

I expect at least one test added to sc-network-test that uses some sort of "forgetting" backend to ensure that we propagate the blocks when we only have preimported them.

[bkchr]

If we don't announce this as best block, the other peers will not request this block. And no, the solution is not to pass true here.

You need to check the BlockImportParams::ForkChoice to check if the block is a new best block.

[rakanalh]

The announcement for an imported block on the networking layer looks at:

let is_best = self.chain.info().best_hash == header.hash();

To figure out whether a block is best or not. At pre-import, how can BlockImportParams::ForkChoice be helpful if we haven't imported the block yet?