Produce block always on updated transaction pool state #5227

NikVolf · 2020-03-11T16:35:16Z

This mostly fixes pipeline problems we experience during block production (#5211, partially #5139, but the latter may require small follow-up) and makes so we always produce block with updated transaction pool state.

The idea is that pending set iterator returned only when transaction pool has processed previous block update.

This will:

avoid validating transactions that are included in previous block
avoid marking invalid transactions that made it to the previous block and reimported
avoid sending invalid rpc updates associated with above behaviour
generally make block import-production more correct

This might induce small blocking before starting block production, but generally should increase speed, since we won't validate something that is known to be invalid.

bkchr

Can we get a test?

NikVolf · 2020-03-12T09:01:35Z

Can we get a test?

There are multiple tests altered and unaltered to show this should work

I'll add dedicated one to specify behaviour though, yes

tomusdrw

Looks good! Although I'd prefer to proceed with (empty) block production even though the transaction pool is not ready yet. Unless I can be proven that returning an error from block production will cause immediate restart or is just simply more desirable.

tomusdrw · 2020-03-12T10:16:47Z

client/basic-authorship/src/basic_authorship.rs

-		let pending_iterator = self.transaction_pool.ready();
+		let pending_iterator = match executor::block_on(future::select(
+			self.transaction_pool.ready_at(Some(self.parent_number)),
+			futures_timer::Delay::new(deadline - (self.now)()),


That might not give us enough time to produce anything. I think it should be half of the remaining time to at least produce an empty block.

I somewhat agree

But also there is an argument that we must just fail here instead of trying to recover from some invalid system state (transaction pool does not receive new block notifications or cannot handle them fast for some reason). Timeout is therefore here (for now) just for avoiding thread hanging forever, not for some logic after it.

Updated to warning & continue

client/basic-authorship/src/basic_authorship.rs

client/transaction-pool/src/lib.rs

Co-Authored-By: Tomasz Drwięga <[email protected]>

NikVolf · 2020-03-12T12:32:00Z

Can we get a test?

@bkchr added!

client/consensus/manual-seal/src/lib.rs

client/consensus/manual-seal/Cargo.toml

client/transaction-pool/src/lib.rs

primitives/transaction-pool/src/pool.rs

client/transaction-pool/src/lib.rs

client/basic-authorship/src/basic_authorship.rs

bkchr · 2020-03-17T12:18:47Z

client/basic-authorship/src/basic_authorship.rs

-		let tx_remaining = 0;
-		let block = propose_block(&client, 1, 2, tx_remaining);
+		// now let's make sure that we can still make some progress
+		let block = propose_block(&client, 1, 2, 5);


Why did we had before 0 remaining and now 5?

There were 7 initially, and 2 was included in previous block
(see deleted comment above that outlines that 0 was an incorrect observation caused by the bug this pr aims to fix among other things)

client/transaction-pool/src/lib.rs

Co-Authored-By: Bastian Köcher <[email protected]>

* make sure return ready iterator once state is updated * update sc_basic_authorship tests * update node tests * fix manual seal * actually fix service test * add tests * Update client/basic-authorship/src/basic_authorship.rs Co-Authored-By: Tomasz Drwięga <[email protected]> * helper function * review suggestions * warning and continue * add debug log * use futures::chennel::oneshot * use declaration bound * no option for updated_at * no allocation * ready_at / ready * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> * Update client/transaction-pool/src/lib.rs Co-Authored-By: Bastian Köcher <[email protected]> Co-authored-by: Tomasz Drwięga <[email protected]> Co-authored-by: Bastian Köcher <[email protected]>

NikVolf added 2 commits March 11, 2020 18:48

make sure return ready iterator once state is updated

550602e

update sc_basic_authorship tests

2b881eb

NikVolf added A3-in_progress Pull request is in progress. No review needed at this stage. B1-apinoteworthy labels Mar 11, 2020

NikVolf requested a review from tomusdrw as a code owner March 11, 2020 16:35

update node tests

341a2fb

NikVolf force-pushed the nv-txpool-poll branch from efa9ff4 to 341a2fb Compare March 11, 2020 17:47

fix manual seal

c831275

bkchr reviewed Mar 11, 2020

View reviewed changes

actually fix service test

7136bd3

NikVolf force-pushed the nv-txpool-poll branch from b0d0bcf to 7136bd3 Compare March 12, 2020 08:41

NikVolf added this to the 2.0 milestone Mar 12, 2020

tomusdrw reviewed Mar 12, 2020

View reviewed changes

NikVolf and others added 4 commits March 12, 2020 14:37

add tests

31ea82e

Update client/basic-authorship/src/basic_authorship.rs

c97e9d6

Co-Authored-By: Tomasz Drwięga <[email protected]>

helper function

532351d

review suggestions

4b3bd80

NikVolf added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 12, 2020

NikVolf added 3 commits March 12, 2020 16:34

warning and continue

bbf0b0a

add debug log

b08b6e5

use futures::chennel::oneshot

00c15b0

gavofyork requested review from bkchr and tomusdrw March 15, 2020 19:22

bkchr suggested changes Mar 15, 2020

View reviewed changes

NikVolf added 3 commits March 16, 2020 10:16

use declaration bound

3b01817

no option for updated_at

e7e5e76

no allocation

b109316

ready_at / ready

8c53a14

CodeSandwich mentioned this pull request Mar 16, 2020

Update substrate to latest master radicle-dev/radicle-registry#238

Merged

Merge remote-tracking branch 'origin/master' into nv-txpool-poll

f941891

bkchr approved these changes Mar 17, 2020

View reviewed changes

gnunicorn added the A5-grumble label Mar 17, 2020

NikVolf and others added 2 commits March 17, 2020 07:57

Update client/transaction-pool/src/lib.rs

f5a5e0d

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/transaction-pool/src/lib.rs

335ba7d

Co-Authored-By: Bastian Köcher <[email protected]>

NikVolf added A8-looksgood and removed A0-please_review Pull request needs code review. A5-grumble labels Mar 17, 2020

NikVolf and others added 4 commits March 17, 2020 07:59

Update client/transaction-pool/src/lib.rs

7bb7eff

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/transaction-pool/src/lib.rs

a7c65be

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/transaction-pool/src/lib.rs

24be207

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/transaction-pool/src/lib.rs

65375b2

Co-Authored-By: Bastian Köcher <[email protected]>

gnunicorn added A4-awaitingci and removed A5-grumble labels Mar 17, 2020

gnunicorn merged commit abd00b1 into master Mar 17, 2020

gnunicorn deleted the nv-txpool-poll branch March 17, 2020 15:24

tomusdrw mentioned this pull request Mar 23, 2020

Transaction are marked Invalid, but still execute completely. #5355

Closed

Produce block always on updated transaction pool state #5227

Produce block always on updated transaction pool state #5227

Uh oh!

Conversation

NikVolf commented Mar 11, 2020

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

NikVolf commented Mar 12, 2020

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

NikVolf Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

NikVolf Mar 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NikVolf commented Mar 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkchr Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

NikVolf Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants