Add block announce validator. #3346

twittner · 2019-08-08T15:31:04Z

Adds a BlockAnnounceValidator trait which is used to check BlockAnnouncements. It also adds an associated data field to BlockAnnounce which is used for things like candidate messages.

core/consensus/common/src/block_validation.rs

arkpar · 2019-08-09T09:50:50Z

Not sure it is such a great idea to bundle extra data with block announcement. Does the protocol guarantee that announcement for the same block will always come with the same extra data?
To me it looks like the extra data should go into a separate message in the protocol extension.

rphmeier · 2019-08-09T11:25:02Z

@arkpar

To me it looks like the extra data should go into a separate message in the protocol extension.

To be honest, that is a decent approach as well. Perhaps we should rather have a method to use a custom block announcement message rather than bundle the extra data.

The desired use-case is for Cumulus: before a block is included on the relay chain but after voting on it has begun, the block should be synced by peers only when bundled with a submission attestation.

We would need two changes:

A way for substrate-network to ask some user code if it would like to use its own message for the announcement.
A way for user code to tell substrate-network there has been a block announcement.

arkpar · 2019-08-09T12:15:41Z

How is it different from e.g block justifications, that come not as part of the announcement, but as block data?

rphmeier · 2019-08-09T13:28:15Z

Justifications are meant to prove finality and be permanently valid. It's exactly what you pinpointed - that this is for a use-case where the justification-like thing is temporary and doesn't need to be persisted, at least not for long.

We want this data to come along with the announcement, because it is crucial for our decision as to whether to download the head or not. Having to do an extra round-trip for all announced blocks would be pretty ridiculous.

bkchr · 2019-09-03T08:26:24Z

So, I personally would go with an extra field for the block announcement message and move the default validation completely out of the sync code. I could imagine that people want to bring in their own validation of the block announcement, for whatever reasons.

But yeah, for the sake of bringing Cumulus to a point that it works, we can also go the way of adding an extra message for our own block announcements.

@arkpar @rphmeier which way to we want to go?

arkpar · 2019-09-03T11:37:42Z

So, I personally would go with an extra field for the block announcement message and move the default validation completely out of the sync code.

I'm fine with this

rphmeier · 2019-09-03T12:02:43Z

@twittner OK - looks like the current approach of the PR with slight modification to move default validation out of the sync code will work.

twittner · 2019-09-03T12:49:13Z

It would be helpful to know what you consider "default validation" from here:

substrate/core/network/src/protocol/sync.rs

Lines 893 to 1002 in d2e7d66

    
           pub fn on_block_announce(&mut self, who: PeerId, hash: B::Hash, header: &B::Header) -> OnBlockAnnounce<B> { 
        
           	let number = *header.number(); 
        
           	debug!(target: "sync", "Received block announcement with number {:?}", number); 
        
           	if number.is_zero() { 
        
           		warn!(target: "sync", "Ignored genesis block (#0) announcement from {}: {}", who, hash); 
        
           		return OnBlockAnnounce::Nothing 
        
           	} 
        
           	let parent_status = self.block_status(header.parent_hash()).ok().unwrap_or(BlockStatus::Unknown); 
        
           	let known_parent = parent_status != BlockStatus::Unknown; 
        
           	let ancient_parent = parent_status == BlockStatus::InChainPruned; 
        
           	let known = self.is_known(&hash); 
        
           	let peer = if let Some(peer) = self.peers.get_mut(&who) { 
        
           		peer 
        
           	} else { 
        
           		error!(target: "sync", "Called on_block_announce with a bad peer ID"); 
        
           		return OnBlockAnnounce::Nothing 
        
           	}; 
        
           	while peer.recently_announced.len() >= ANNOUNCE_HISTORY_SIZE { 
        
           		peer.recently_announced.pop_front(); 
        
           	} 
        
           	peer.recently_announced.push_back(hash.clone()); 
        
           	if number > peer.best_number { 
        
           		// update their best block 
        
           		peer.best_number = number; 
        
           		peer.best_hash = hash; 
        
           	} 
        
           	if let PeerSyncState::AncestorSearch(_, _) = peer.state { 
        
           		return OnBlockAnnounce::Nothing 
        
           	} 
        
           	// We assume that the announced block is the latest they have seen, and so our common number 
        
           	// is either one further ahead or it's the one they just announced, if we know about it. 
        
           	if known { 
        
           		peer.common_number = number 
        
           	} else if header.parent_hash() == &self.best_queued_hash || known_parent { 
        
           		peer.common_number = number - One::one(); 
        
           	} 
        
           	self.is_idle = false; 
        
           	// known block case 
        
           	if known || self.is_already_downloading(&hash) { 
        
           		trace!(target: "sync", "Known block announce from {}: {}", who, hash); 
        
           		return OnBlockAnnounce::Nothing 
        
           	} 
        
           	// stale block case 
        
           	let requires_additional_data = !self.role.is_light(); 
        
           	if number <= self.best_queued_number { 
        
           		if !(known_parent || self.is_already_downloading(header.parent_hash())) { 
        
           			let block_status = self.client.block_status(&BlockId::Number(*header.number())) 
        
           				.unwrap_or(BlockStatus::Unknown); 
        
           			if block_status == BlockStatus::InChainPruned { 
        
           				trace!( 
        
           					target: "sync", 
        
           					"Ignored unknown ancient block announced from {}: {} {:?}", who, hash, header 
        
           				); 
        
           				return OnBlockAnnounce::Nothing 
        
           			} 
        
           			trace!( 
        
           				target: "sync", 
        
           				"Considering new unknown stale block announced from {}: {} {:?}", who, hash, header 
        
           			); 
        
           			if let Some(request) = self.download_unknown_stale(&who, &hash) { 
        
           				if requires_additional_data { 
        
           					return OnBlockAnnounce::Request(who, request) 
        
           				} else { 
        
           					return OnBlockAnnounce::ImportHeader 
        
           				} 
        
           			} else { 
        
           				return OnBlockAnnounce::Nothing 
        
           			} 
        
           		} else { 
        
           			if ancient_parent { 
        
           				trace!(target: "sync", "Ignored ancient stale block announced from {}: {} {:?}", who, hash, header); 
        
           				return OnBlockAnnounce::Nothing 
        
           			} 
        
           			if let Some(request) = self.download_stale(&who, &hash) { 
        
           				if requires_additional_data { 
        
           					return OnBlockAnnounce::Request(who, request) 
        
           				} else { 
        
           					return OnBlockAnnounce::ImportHeader 
        
           				} 
        
           			} else { 
        
           				return OnBlockAnnounce::Nothing 
        
           			} 
        
           		} 
        
           	} 
        
           	if ancient_parent { 
        
           		trace!(target: "sync", "Ignored ancient block announced from {}: {} {:?}", who, hash, header); 
        
           		return OnBlockAnnounce::Nothing 
        
           	} 
        
           	trace!(target: "sync", "Considering new block announced from {}: {} {:?}", who, hash, header); 
        
           	let (range, request) = match self.select_new_blocks(who.clone()) { 
        
           		Some((range, request)) => (range, request), 
        
           		None => return OnBlockAnnounce::Nothing 
        
           	}; 
        
           	let is_required_data_available = !requires_additional_data 
        
           		&& range.end - range.start == One::one() 
        
           		&& range.start == *header.number(); 
        
           	if !is_required_data_available { 
        
           		return OnBlockAnnounce::Request(who, request) 
        
           	} 
        
           	OnBlockAnnounce::ImportHeader 
        
           }

? Most of it relates to the state of peers which would mean to replicate a lot of the peer state handling and also to keep it up to date w.r.t. what is currently being downloaded from where etc. I am not sure it is a good idea to replicate this which begs the question what is left to validate?

bkchr · 2019-09-03T12:58:32Z

The peer management does not seems to be used after line 933? As the updating of the peer information is not validating, it should stay in the sync layer.

twittner · 2019-09-03T13:26:32Z

The peer management does not seems to be used after line 933?

It is, e.g.

is_already_downloading (line 941),
download_unknown_stale (lines 955, 969),
select_new_blocks (line 988).

bkchr · 2019-09-04T10:43:50Z

Can we not make this available to the announce validator using a trait?

The validator would probably return the following:

enum ValidationResult {
    Request(Request),
    Nothing,
}

Then in the on_block_announce we convert this into the OnBlockAnnounce by doing:

match res {
    ValidationResult::Request(req) => if requires_additional_data {
						return OnBlockAnnounce::Request(who, request)
					} else {
						return OnBlockAnnounce::ImportHeader
					}
     ValidationResult::Nothing => OnBlockAnnounce::Nothing
}

twittner · 2019-09-05T10:25:40Z

Could you be more specific? Do you mean we should run on_block_announce in sync.rs as it is today and within this method collect some data (what exactly?) which we then pass to the validator which in turn constructs a substrate_network::protocol::message::BlockRequest that on_block_announce then sends over the network? If so I still do not understand what validation logic should be moved to the validator. Also, is this coupling not a bit tight?

twittner · 2019-09-09T13:17:53Z

@bkchr?

rphmeier · 2019-09-09T14:34:32Z

The goal of the PR is to give us the option to do additional, customized validation on whether a block announcement is valid based on possibly time-sensitive circumstances. We'd do so by bundling a bit of optional extra data with the announcement to be checked by some user code, along with the announcement.

Most of the checks that the on_block_announce function is doing are technically announcement validation, but are the kind of checks that you would always want to do. I don't think it's worthwhile to abstract to the point where these are extractable:

Checking if the peer has recently announced the block
Checking if the block's parent is ancient
Checking what protocol state the peer is in
Checking whether we know the block already or are downloading it

With that in mind I'd suggest that "default" verification could be an empty or nearly-empty trait implementation - this trait implementation could live outside of the sync code, although I'd say it's reasonable for it to live in the network crate.

I assume that Basti is referring to replacing these lines

		if !is_required_data_available {
			return OnBlockAnnounce::Request(who, request)
		}

with

match res {
    ValidationResult::Request(req) => if requires_additional_data {
        return OnBlockAnnounce::Request(who, request)
    } else {
	return OnBlockAnnounce::ImportHeader
    },
    ValidationResult::Nothing => OnBlockAnnounce::Nothing
}

I'm not 100% sure what the implications of having the request come from the announce-validator are. @bkchr, it doesn't seem to be a problem to me if we have the request generated by self.select_new_blocks as is the case now, and only give the announce-validator the option to say whether to request in general.

rphmeier · 2019-09-09T14:45:51Z

Then there's also the other side of the coin to discuss: how the block announcement should actually be bundled with this extra-data.

I'm not particularly opinionated on this, but ideally the two methods can fall within the same trait to prevent user-protocol logic from becoming spread out.

bkchr · 2019-09-10T06:12:59Z

Yeah, sorry for the delay!

I think what @rphmeier writes, sounds reasonable. Regarding the extra-data, as this is probably just some opaque field, we could probably check this in the same function we are giving the general "yes/no" to the block request.

twittner · 2019-09-10T15:29:51Z

Ok, turning it into a non-WIP PR now. Please review when time permits it. Regarding

Then there's also the other side of the coin to discuss: how the block announcement should actually be bundled with this extra-data.

This is not addressed in this PR. The various places where interfaces require associated data use empty vectors for now.

Demi-Marie

Nits

Demi-Marie · 2019-09-11T13:48:51Z

core/network/src/test/sync.rs

@@ -499,7 +499,7 @@ fn light_peer_imports_header_from_announce() {
 	let mut runtime = current_thread::Runtime::new().unwrap();

 	fn import_with_announce(net: &mut TestNet, runtime: &mut current_thread::Runtime, hash: H256) {
-		net.peer(0).announce_block(hash);
+		net.peer(0).announce_block(hash, Vec::new()); // TODO


Needs an issue filed.

Demi-Marie · 2019-09-11T20:05:38Z

core/client/src/client.rs

@@ -1015,6 +1002,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 				is_new_best,
 				storage_changes,
 				retracted,
+				associated_data: Vec::new(), // TODO


Any TODOs or FIXMEs need to contain links to open issues.

kianenigma · 2019-09-12T07:49:41Z

core/client/src/client.rs

-		&self,
-		notify_import: ImportSummary<Block>,
-	) -> error::Result<()> {
+	fn notify_imported(&self, notify_import: ImportSummary<Block>) -> error::Result<()> {
 		if let Some(storage_changes) = notify_import.storage_changes {
 			// TODO [ToDr] How to handle re-orgs? Should we re-emit all storage changes?


@tomusdrw solved or needs an issue?

It seems that we are notifying about all blocks (not only the new best ones), so I think we don't really need any special handling here. CC @jacogr what would you expect in case of re-orgs and storage changes subscription?

If there is a re-org, would expect the subscriptions to pass through the most-recent-actual-state values.

Effectively the lasted values from the subscription and state_getStorage should match - both should reflect the current chain state.

core/network/src/protocol.rs

kianenigma · 2019-09-12T07:53:57Z

core/network/src/protocol/sync.rs

 use crate::{
 	config::{Roles, BoxFinalityProofRequestBuilder},
-	message::{self, generic::FinalityProofRequest, BlockAttributes, BlockRequest, BlockResponse, FinalityProofResponse},
+	message::{
+		BlockAnnounce,


better to wrap when the line limit exceeds.

consistency with the rest of substrate -- though this (afaik) is not strictly in our style guide. Usually what I do (and I think it is sort of the implicit styleguide):

use crate::{ top_level_stuff_1, top_level_stuff_2, top_level_stuff_3, top_level_stuff_wrap_if_needed, inner_module_1::{stuff, bar, Bazinga}, inner_module_2::{foo, kaboom, blah}, }

basically break once per sub-module but other than that only when line-width is reached.

bkchr

Some nitpicks, in general looking good.

core/network/src/protocol.rs

core/network/src/test/sync.rs

bkchr · 2019-09-12T12:06:02Z

core/network/src/protocol/sync.rs

 use crate::{
 	config::{Roles, BoxFinalityProofRequestBuilder},
-	message::{self, generic::FinalityProofRequest, BlockAttributes, BlockRequest, BlockResponse, FinalityProofResponse},
+	message::{
+		BlockAnnounce,


bkchr · 2019-09-12T12:12:07Z

core/consensus/common/src/block_validation.rs

+#[derive(Debug, PartialEq, Eq)]
+pub enum Validation {
+	/// Valid block announcement.
+	Success,


Why not call them Valid and Invalid directly? And the enum ValidationResult or something similar?

Result has a special meaning in Rust - as a nitpick, we shouldn't call anything except for variants of the Result<T, E> enum Result.

Typical Rust style would have this be enum Valid { Valid, NotValid } or enum Valid { Yes, No }.

I think naming the type "Valid" can be misleading because "valid" is already biased towards "yes". Saying something valid is not valid feels a little inconsistent to me, so at a minimum I would like to see a neutral name. I agree that the "Result" suffix should better be avoided, especially since the type name will often be embedded in another Result. I would rather read Result<Validation, ...> instead of Result<ValidationResult, ...>.

bkchr · 2019-09-12T12:12:55Z

core/client/src/client.rs

@@ -201,6 +186,8 @@ pub struct BlockImportNotification<Block: BlockT> {
 	pub is_new_best: bool,
 	/// List of retracted blocks ordered by block number.
 	pub retracted: Vec<Block::Hash>,
+	/// Any data associated with this block import.
+	pub associated_data: Vec<u8>,


Why do we need the data as part of the import notification?

Hmm, now I understand, but I don't think that this is required. In Cumulus we will have to delay sending the notification anyway and will fetch the associated data from a different source. So, yeah, just remove it.

If we remove this, how do we send block announcements with associated data to network peers?

https://github.com/paritytech/substrate/pull/3346/files#diff-16b4c34d50bdfcbf732c09c7be1fdcf9R684 here do we send the block announcement. The import notification is just used to know that we need to send a block announcement. But as I already said, we will send the block announcements differently in Cumulus.

Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

bkchr

Just one last master merge.

@arkpar do we need to update the protocol version? I assume yes?

twittner · 2019-09-19T14:25:29Z

Just one last master merge.

Merging was actually quite involved. I think #3602 has introduced some changes in the vicinity. Maybe @andresilva could check that things still make sense?

arkpar · 2019-09-19T14:43:35Z

Indeed the protocol should be backwards compatible. It should not send any additional data to nodes running older version. Furthermore message encoding for the current version should match standard SCALE encoding.

rphmeier

Looks good to me! Thanks Toralf.

edit: logic/API is fine, Arkady's point about backwards compatibility would be the last thing to address

arkpar · 2019-09-19T14:48:01Z

core/network/src/protocol/message.rs

@@ -284,16 +286,21 @@ pub mod generic {
 			if let Some(state) = &self.state {
 				state.encode_to(dest);
 			}
+			if !self.data.is_empty() {


This will interfere with encoding any additional fields. I.e. this new protocol version should always encode data when communicating with the same protocol version and do not encode when communicating with an older version. I'd suggest using Option<Vec<u8>> in BlockAnnounce for that. Later we can remove backwards compatibility and Option without changing the actual encoding for the current version.

I would normally have called encode_to unconditionally. Wrapping data in an Option is not bulletproof either, as one can easily just put it in a Some. Also, changing the type ripples through the whole system which is not great. As I mentioned elsewhere, we should reconsider our encoding, otherwise minor changes such as this will continue to hurt us.

Sure, we should get proper versioning support at some point, but this is out of scope of this PR I believe.

bkchr

2 last nitpicks, then it is good to be merged.

core/network/src/protocol/sync.rs

bkchr · 2019-09-23T11:08:54Z

core/network/src/protocol/message.rs

@@ -273,6 +273,8 @@ pub mod generic {
 		pub header: H,
 		/// Block state. TODO: Remove `Option` and custom encoding when v4 becomes common.
 		pub state: Option<BlockState>,
+		/// Data associated with this block announcement, e.g. a candidate message.


The Option should be removed here as well, when 4 becomes common.

bkchr · 2019-09-23T11:10:15Z

core/network/src/protocol/message.rs

 		}
 	}

 	impl<H: Decode> Decode for BlockAnnounce<H> {
 		fn decode<I: Input>(input: &mut I) -> Result<Self, codec::Error> {
 			let header = H::decode(input)?;
 			let state = BlockState::decode(input).ok();
+			let data = Vec::decode(input).ok();


I'm not happy with this. Conceptually it should be Option<(BlockState, Vec<u8>)>. As both should be present with version 4. But yeah, for the sake of merging this and don't over-complicating it more, we probably can go with this.

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

rphmeier reviewed Aug 8, 2019

View reviewed changes

core/consensus/common/src/block_validation.rs Show resolved Hide resolved

rphmeier reviewed Aug 8, 2019

View reviewed changes

core/consensus/common/src/block_validation.rs Show resolved Hide resolved

gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Aug 21, 2019

twittner added 3 commits September 10, 2019 12:04

Add BlockAnnounceValidator trait.

88f0b53

Add associated data to block announcement.

877419c

Make tests compile.

dabb6d8

twittner force-pushed the block-announce-validator branch from a5e2946 to dabb6d8 Compare September 10, 2019 14:35

twittner added 2 commits September 10, 2019 17:06

Move validator into sync.rs.

89f26a5

Smaller changes.

f05b2d1

twittner marked this pull request as ready for review September 10, 2019 15:29

twittner requested review from andresilva and Demi-Marie as code owners September 10, 2019 15:29

twittner requested a review from rphmeier September 10, 2019 15:30

twittner requested a review from bkchr September 10, 2019 15:30

Demi-Marie reviewed Sep 11, 2019

View reviewed changes

kianenigma reviewed Sep 12, 2019

View reviewed changes

core/network/src/protocol.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 12, 2019

View reviewed changes

bkchr reviewed Sep 12, 2019

View reviewed changes

twittner and others added 6 commits September 16, 2019 10:24

Update core/network/src/protocol.rs

ce11dd9

Co-Authored-By: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

Update core/network/src/protocol.rs

5d279bf

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update core/network/src/test/sync.rs

8becf1f

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Formatting.

a5d2802

Remove assoc. data from BlockImportNotification.

87d20bc

Merge branch 'master' into block-announce-validator

4a2fd48

bkchr approved these changes Sep 19, 2019

View reviewed changes

rphmeier approved these changes Sep 19, 2019

View reviewed changes

arkpar reviewed Sep 19, 2019

View reviewed changes

Use Option<Vec<u8>> for associated data.

acc2b11

bkchr approved these changes Sep 23, 2019

View reviewed changes

twittner and others added 2 commits September 23, 2019 13:12

Update core/network/src/protocol/sync.rs

4adb484

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Fix type error.

ca2aa07

bkchr merged commit 7a7d641 into paritytech:master Sep 24, 2019

twittner deleted the block-announce-validator branch September 24, 2019 09:16

Add block announce validator. #3346

Add block announce validator. #3346

Conversation

twittner commented Aug 8, 2019

arkpar commented Aug 9, 2019

rphmeier commented Aug 9, 2019

arkpar commented Aug 9, 2019

rphmeier commented Aug 9, 2019 • edited Loading

bkchr commented Sep 3, 2019

arkpar commented Sep 3, 2019

rphmeier commented Sep 3, 2019

twittner commented Sep 3, 2019

bkchr commented Sep 3, 2019

twittner commented Sep 3, 2019

bkchr commented Sep 4, 2019

twittner commented Sep 5, 2019

twittner commented Sep 9, 2019

rphmeier commented Sep 9, 2019 • edited Loading

rphmeier commented Sep 9, 2019

bkchr commented Sep 10, 2019

twittner commented Sep 10, 2019

Demi-Marie left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

twittner Sep 16, 2019 • 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

bkchr left a comment

Choose a reason for hiding this comment

twittner commented Sep 19, 2019

arkpar commented Sep 19, 2019

rphmeier left a comment • 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

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier commented Aug 9, 2019 •

edited

Loading

rphmeier commented Sep 9, 2019 •

edited

Loading

kianenigma Sep 16, 2019 •

edited

Loading

rphmeier Sep 12, 2019 •

edited

Loading

twittner Sep 16, 2019 •

edited

Loading

rphmeier left a comment •

edited

Loading