Allow changing the behavior for imported blocks by cecton · Pull Request #5236 · paritytech/substrate

cecton · 2020-03-12T12:55:37Z

This change is needed for cumulus to be able to implement paritytech/cumulus#7

The goal is to allow the user to override the default behavior of substrate when a new block is imported. By default the original behavior is kept unchanged.

The changes are:

Add an option to disable the default block announce
Add an on_block_imported method on NetworkService that will allow the user to call the method manually

polkadot companion: paritytech/polkadot#955

parity-cla-bot · 2020-03-12T12:55:40Z

It looks like @cecton signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Commit: 475df46 Parent branch: origin/master Forked at: 41bb219

Commit: 67bf1ac Parent branch: origin/master Forked at: 41bb219

NikVolf

Add an on_block_imported method on NetworkService that will allow the user to call the method manually

I think we need example (test) how this on_block_imported can be used, this will also ensure that api is actually consumable

bkchr · 2020-03-12T14:32:15Z

client/network/src/service.rs

 	}
+
+	/// You must call this when a new block is imported by the client.
+	pub fn on_block_imported(&self, header: B::Header, data: Vec<u8>, is_best: bool) {


This should be announce_block

It's not just about announcing the block. It also updates the state of sync so that it knows what's in the database.

There is already a method announce block there:

substrate/client/network/src/service.rs

Lines 552 to 558 in 61c5d4b

/// Make sure an important block is propagated to peers.

///

/// In chain-based consensus, we often need to make sure non-best forks are

/// at least temporarily synced. This function forces such an announcement.

pub fn announce_block(&self, hash: B::Hash, data: Vec<u8>) {

let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::AnnounceBlock(hash, data));

}

~~Yeah I know. For whatever we decide, default_announce_block should be named accordingly.~~

(ignore)

There is already a method announce block there:

substrate/client/network/src/service.rs

Lines 552 to 558 in 61c5d4b

/// Make sure an important block is propagated to peers.

///

/// In chain-based consensus, we often need to make sure non-best forks are

/// at least temporarily synced. This function forces such an announcement.

pub fn announce_block(&self, hash: B::Hash, data: Vec<u8>) {

let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::AnnounceBlock(hash, data));

}

I just realized this. This makes the whole story here even more confusing. We just want to announce a block later, but that would not update sync and would break everything... If we continue with this new method here, I would propose to remove this announce_block function.

@tomaka any ideas?

Removing that function without replacing the functionality exactly will break GRANDPA.

tomaka · 2020-03-12T14:39:34Z

I don't think there's a better way to document on_block_imported than "you must call this when a block is now in the database, otherwise things will break".

That, however, makes it a bit worrisome to make it possible to disable the fact that it's being called automatically.

cecton · 2020-03-12T15:00:03Z

I don't think there's a better way to document on_block_imported than "you must call this when a block is now in the database, otherwise things will break".

Maybe it could be something like: "This method is normally called by substrate unless default_announce_block is set on false in the Configuration, in which case you must call this when a block is now in the database, otherwise things will break"

cecton · 2020-03-12T15:06:27Z

I'm starting to wonder if it wouldn't be better to make a Configuration parameter "override_on_block_imported" that would be an Option<Fn()> that would be called if provided. And if not provided, fallbacking to the default behavior.

cecton · 2020-03-12T15:13:54Z

Maybe a good example is better to explain my idea:

	let mut imported_blocks_stream = client.import_notification_stream().fuse();
	let mut finality_notification_stream = client.finality_notification_stream().fuse();

	futures::future::poll_fn(move |cx| {
		let before_polling = Instant::now();

		// We poll `imported_blocks_stream`.
		while let Poll::Ready(Some(notification)) = Pin::new(&mut imported_blocks_stream).poll_next(cx) {
			let maybe_import =
				if let Some(override_func) = maybe_override {
					override_func(notification)
				} else {
					Some(notification)
				};
			if let Some(notification) = maybe_import {
				network.on_block_imported(notification.header, Vec::new(), notification.is_new_best);
			}
		}

(Note that there is no need to expose the internal on_block_imported anymore)

tomaka · 2020-03-12T15:54:48Z

If the reason for this PR is to be able to announce a block later, then IMO this is a very wrong approach and we should instead configure the network to not announce a block by default when on_block_import is called, rather than not calling the method at all.

As mentioned, there is already a function that lets you announce a block.

This reverts commit ba360ca.

cecton · 2020-03-13T06:43:57Z

client/network/src/protocol.rs

+		// blocks are announced by default
+		if !self.config.default_block_announce {
+			return;
+		}
+


@tomaka something like this?

Forked at: 41bb219 Parent branch: origin/master

rphmeier · 2020-03-14T20:27:38Z

client/network/src/protocol.rs

 	/// Maximum number of peers to ask the same blocks in parallel.
 	pub max_parallel_downloads: u32,
+	/// Use default block announcement
+	pub default_announce_block: bool,


I agree with Pierre that this feels very hacky and fragile. The documentation is not good either.

rphmeier · 2020-03-14T20:32:00Z

As mentioned, there is already a function that lets you announce a block.

Right. We should definitely be using that, but come up with a cleaner way to disable the normal "announce new best" logic.

Usually this would be done via a trait.

Could something like this fit?

trait ImportedBlockAnnounceFilter {
    // Whether to announce a block that has just been imported to peers.
    fn announce_block(&self, some_block_info) -> bool;
}

struct AnnounceNewBestBlocks;
impl ImportedBlockAnnounceFilter for AnnounceNewBestBlocks {
    // ...
}

struct AnnounceNothing;
impl ImportBlockAnnounceFilter for AnnounceNothing {
    // ...
}

I want to revisit the higher-level context that is driving this PR as well, so we can make a more informed decision.

I'd imagine for Cumulus we will want more customization here anyway. At the moment, we assume a couple of things in Cumulus that will not hold true for all or most cases once we start refining APIs:

Marginal cost of producing a new block is ~0 (i.e. equivocations are cheap!)
Collator set is permissionless (i.e. the collator set is unbounded)

In confluence with each other, this leads to a situation where we only want to broadcast blocks which have been seconded by a validator. However, if we remove assumption 1 (e.g. a PoW parachain), we could just as easily announce blocks that fulfill a PoW requirement. If we alternatively remove assumption 2 (e.g. a PoS parachain), we could announce blocks that are signed by the correct slot author. It's only in the case where 1&2 are both assumed (most minimal case - pushing collator selection entirely to the emergent market) that we ever need this behavior. And it's much better IMO to be explicit about which behaviors are activated in the network service at any given time.

tomaka · 2020-03-16T13:12:05Z

Giving more thoughts, to me the best solution would simply be to never announce blocks automatically. In other words, not even give option to disable it like this PR does, but always disable it.

Instead have this code here also call announce_block.

As an alternative we could also change the signature of on_block_import to have a boolean also_announce.

This is the solution that has the less surprise factor to me. You know exactly what happens.

This adds a bit more burden to the sc_service crate, but to me this is exactly what sc_service is supposed to do: synchronize the various components of Substrate. No trait will ever be as flexible as having straight-up Rust code in sc_service explicitly call announce_block.

bkchr · 2020-03-16T13:27:32Z

Sounds good to me!

Forked at: 41bb219 Parent branch: origin/master

cecton · 2020-03-17T13:49:31Z

@tomaka I think I understand theoretically but not how it translates in practice 😅 can you check this?

tomaka · 2020-03-19T11:35:18Z

client/service/src/lib.rs

+			network.on_block_imported(notification.header, Vec::new(), notification.is_new_best, false);
+			network.service().announce_block(notification.hash, Vec::new());


Well, you can pass true here.
The entire point of passing an extra boolean parameter is that you don't have to call announce_block separately.

Suggested change

network.on_block_imported(notification.header, Vec::new(), notification.is_new_best, false);

network.service().announce_block(notification.hash, Vec::new());

network.on_block_imported(notification.header, Vec::new(), notification.is_new_best, true);

Although maybe not having a boolean is better?
If you're reading the code of sc_service trying to figure out when blocks are announced, this little extra true is easy to miss.

enum Announce { Yes, No }

I finally get it 😅 calling the code explicitly is more visible than the boolean

rphmeier · 2020-03-24T17:54:52Z

client/network/src/protocol.rs

-	/// the network.
-	pub fn on_block_imported(&mut self, header: &B::Header, data: Vec<u8>, is_best: bool, also_announce: bool) {
+	/// Call this when a block has been imported in the import queue
+	pub fn on_block_imported(&mut self, header: &B::Header, is_best: bool) {


TBH, seems that this function now does nothing unless the new block is best. Maybe it should be fn on_best_block_imported(&mut self, header: &B::Header)

But then we would need to reintroduce it if later we want to do something for non-best blocks.

I quite dislike that function to be honest, but its API seems to be "please call me when a block has been added to the client", and adding an "only if it is the new best block" clause seems even weirder and more error-prone to me.

Forked at: 41bb219 Parent branch: origin/master

Commit: 4da1756 Parent branch: origin/master Forked at: 41bb219

Commit: ac73bdc Parent branch: origin/master Forked at: 41bb219

Commit: c436863 Parent branch: origin/master Forked at: 41bb219

Commit: bfb6d4d Parent branch: origin/master Forked at: 41bb219

tomaka

Nits

client/service/src/lib.rs

Commit: 4b65f17 Parent branch: origin/master Forked at: 41bb219

t

cecton · 2020-03-30T14:40:41Z

Add an on_block_imported method on NetworkService that will allow the user to call the method manually

I think we need example (test) how this on_block_imported can be used, this will also ensure that api is actually consumable

@NikVolf I thought about it and I have no idea how this can be tested or how to give a meaningful example at this point because I don't fully understand the product. I'm open to suggestions but I propose to merge as it for now because it's technically tested (tests fail if the blocks are not announced right after being imported). When the feature in cumulus will be fully developed (this might take time) maybe I will find some idea.

bkchr · 2020-03-30T16:58:13Z

Please merge master.

bkchr · 2020-03-30T16:59:11Z

And also requires a polkadot pr.

cecton · 2020-03-31T09:01:03Z

@bkchr done. Could you click the merge button please?

* Adapt code to API changes * Update sp-io

cecton added 2 commits March 12, 2020 06:13

Added option to disable default block announce

c31d416

Added on_block_imported on NetworkService

ba360ca

cecton requested review from bkchr, mattrutherford, rphmeier and tomaka March 12, 2020 12:55

cecton self-assigned this Mar 12, 2020

cecton added 2 commits March 12, 2020 13:56

Update from parent 'origin/master' (no conflict)

f52a4c9

Commit: 475df46 Parent branch: origin/master Forked at: 41bb219

Update from parent 'origin/master' (conflicts)

61c5d4b

Commit: 67bf1ac Parent branch: origin/master Forked at: 41bb219

NikVolf reviewed Mar 12, 2020

View reviewed changes

bkchr previously requested changes Mar 12, 2020

View reviewed changes

cecton added 2 commits March 13, 2020 07:36

Revert "Added on_block_imported on NetworkService"

c1a3ede

This reverts commit ba360ca.

Do not announce block if set to not announce block

f76a080

cecton commented Mar 13, 2020

View reviewed changes

cecton added 3 commits March 13, 2020 07:44

Revert fix

de2027b

Moving default announce block to NetworkConfig

9137f9e

WIP

7911167

Forked at: 41bb219 Parent branch: origin/master

gavofyork added the A0-please_review Pull request needs code review. label Mar 14, 2020

rphmeier reviewed Mar 14, 2020

View reviewed changes

WIP

ee66173

Forked at: 41bb219 Parent branch: origin/master

tomaka approved these changes Mar 19, 2020

View reviewed changes

Removing boolean in favor of explicit call

2f111f0

tomaka approved these changes Mar 23, 2020

View reviewed changes

Fixing tests

30d493f

rphmeier reviewed Mar 24, 2020

View reviewed changes

cecton added 10 commits March 27, 2020 15:00

WIP

be04bda

Forked at: 41bb219 Parent branch: origin/master

WIP

8f63872

Forked at: 41bb219 Parent branch: origin/master

increase spec_version

6404281

Update from parent 'origin/master' (no conflict)

779215f

Commit: 4da1756 Parent branch: origin/master Forked at: 41bb219

Update from parent 'origin/master' (conflicts)

66d038d

Commit: ac73bdc Parent branch: origin/master Forked at: 41bb219

increase spec_version

4a073a4

Update from parent 'origin/master' (no conflict)

19ea9f9

Commit: c436863 Parent branch: origin/master Forked at: 41bb219

Update from parent 'origin/master' (conflicts)

875f4a3

Commit: bfb6d4d Parent branch: origin/master Forked at: 41bb219

Fixed test

7aa6ee8

Fixing test

6a616d5

tomaka approved these changes Mar 30, 2020

View reviewed changes

client/service/src/lib.rs Outdated Show resolved Hide resolved

client/service/src/lib.rs Outdated Show resolved Hide resolved

cecton added 2 commits March 30, 2020 13:06

Update from parent 'origin/master' (no conflict)

54403aa

Commit: 4b65f17 Parent branch: origin/master Forked at: 41bb219

Renamed should_announce_imported_blocks to announce_imported_blocks

c61ac54

Updated assert_cmd

96e6379

bkchr approved these changes Mar 30, 2020

View reviewed changes

bkchr added the A7-needspolkadotpr label Mar 30, 2020

cecton mentioned this pull request Mar 31, 2020

Companion PR for https://github.com/paritytech/substrate/pull/5236 paritytech/polkadot#955

Merged

bkchr merged commit fa71ce9 into master Mar 31, 2020

bkchr deleted the cecton-disable-default-announce branch March 31, 2020 09:02

bkchr pushed a commit to paritytech/polkadot that referenced this pull request Mar 31, 2020

Companion PR for paritytech/substrate#5236 (#955)

238016d

* Adapt code to API changes * Update sp-io

	/// Make sure an important block is propagated to peers.
	///
	/// In chain-based consensus, we often need to make sure non-best forks are
	/// at least temporarily synced. This function forces such an announcement.
	pub fn announce_block(&self, hash: B::Hash, data: Vec<u8>) {
	let _ = self.to_worker.unbounded_send(ServiceToWorkerMsg::AnnounceBlock(hash, data));
	}

		network.on_block_imported(notification.header, Vec::new(), notification.is_new_best, false);
		network.service().announce_block(notification.hash, Vec::new());

Conversation

cecton commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Mar 12, 2020

Uh oh!

NikVolf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomaka commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cecton commented Mar 12, 2020

Uh oh!

cecton commented Mar 12, 2020

Uh oh!

cecton commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomaka commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomaka commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkchr commented Mar 16, 2020

Uh oh!

cecton commented Mar 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomaka Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cecton commented Mar 30, 2020

Uh oh!

bkchr commented Mar 30, 2020

Uh oh!

bkchr commented Mar 30, 2020

Uh oh!

cecton commented Mar 31, 2020

Uh oh!

Reviewers

cecton commented Mar 12, 2020 •

edited

Loading

bkchr Mar 12, 2020 •

edited

Loading

tomaka commented Mar 12, 2020 •

edited

Loading

cecton commented Mar 12, 2020 •

edited

Loading

tomaka commented Mar 12, 2020 •

edited

Loading

rphmeier commented Mar 14, 2020 •

edited

Loading

tomaka commented Mar 16, 2020 •

edited

Loading

tomaka Mar 24, 2020 •

edited

Loading