Finalized block event triggers tx maintanance

[michalkucharczyk]

tx-pool is driven by ChainEvents: NewBestBlock (NBB) and Finalized (F) which can be delivered in arbitrary order.

Existing implementation of tx-pool, handles correctly the typical sequence of events: [NBB(a), F(a)].

Maintenance of tx-pool is performed in two steps:

1. enactment/retracment of blocks, and moving associated transaction between internal pools (enactment phase), notifying Ready/InBlock/Retracted events
2. sending finalization event for transactions in finalized blocks

Existing implementation does not handle properly [F(a), NBB(a)] sequence of events (which was a case for instant-seal consensus): enactment phase was not executed when F was reported before NBB event.

For F events coming first both phases of maintenance should be executed. For NBB event coming after F the enactment phase should not be executed.

This PR:

• moves the enactment phase implementation into single function (handle_enactment) which is called for both NBB and F events.
• introduces the enactment_helper which decides if enactment phase of maintenance should be executed for incoming event.

Please note that this PR ignores tree_route provided within event, and uses tree_route function to compute retractred/enacted blocks.

Consider the following chain:

  B1-C1-D1-E1
 /
A
 \
  B2-C2-D2-E2

The following list presents some scenarios and expected behavior:

• NBB(C1), F(C1) -> false (enactment phase was already performed in NBB(C1))
• F(C1), NBB(C1) -> false (enactmant phase was already performed in F(C1))
• F(E1), F(B1) -> false (enactmant phase was already performed in F(E1))
• F(C1), NBB(D2) -> false (enactment phase was already performed in F(C1), we should not retract finalized block)
• F(C1), F(C2) -> false (enactment phase was already performed in F(C1), we should not retract finalized block)
• F(C1), F(C2), NBB(C1) -> false (enactmant phase was already performed in F(C1))
• NBB(C1), NBB(C2) -> true (switching fork is OK)

---

Closes #10279

[andresilva]

While reviewing this PR I was trying to come up with a way to simplify this. I also think there is another bug with the original implementation, i.e. when we retract blocks by finalizing some other fork we don't re-add the retracted transactions to the pool (since we only run that logic on NewBestBlock events). Let's try to break down the problem:

• We want to keep track of some "canon" block that represents our internal status of tx pool "enactment"
• This block can be updated either by processing NewBestBlock events or Finalized events and it should work properly under "inconsistent" ordering of events
• Whenever we update this "canon" block we can retract blocks (in comparison to our previous "canon"), enact new blocks and also finalize some blocks
• For the retracted blocks we want to extract the txs in them and re-add them to the pool
• For enacted blocks we want to extract the txs in them and remove them from the pool
• For finalized blocks we want to notify this to tx pool listeners

I think the overall logic should be:

• Compute new "canon" block on new event
• Compute enacted, retracted and finalized in relation to new event
• Do all the actions in "maintain" regardless of event type but instead using the extracted values above

To compute "canon" block and the remaining necessary data we do as follows (roughly the stuff that's being done in update_and_check_if_new_enactment_is_valid now):

• When we receive a NewBestBlock event we calculate the tree route between our "canon" block and the block in the event. If there are no enacted blocks then we can ignore the event (we can ignore retracted blocks since they must have been processed in an earlier event). If there are enacted blocks then we update our "canon" block to this block and we return the given enacted, retracted from the tree route we computed and no finalized blocks
• When we receive a Finalized event we calculate the tree route between our "canon" block and the block in the event, we then return the enacted and retracted from this route, and we return the finalized blocks from the tree route that comes from the event. We update our "canon" block to the finalized block from the event

Let me know what you think about this, it's very possible my logic is faulty.

[andresilva]

After talking to @michalkucharczyk there is something wrong in my suggestion above which is that my solution treats "conflicting" NBB events as being out-of-order. We need to make the assumption that Finalized and NewBestBlock events can arrive in any order, but within NBB events they are logically ordered (otherwise we can't distinguish whether NBB(d), NBB(b) was out-of-order or whether we actually re-orged to b). After re-reviewing and using my brain a bit I think the logic in update_and_check_if_new_enactment_is_valid is correct.

[bkchr]

Some last nitpicks, otherwise it looks good 👍

[bkchr]

Thank you 👍

[bkchr]

DQ: Why do you changed this?

[michalkucharczyk]

I explained it in: https://github.com/paritytech/substrate/pull/12305/files#r982204082

Please cross-check if my reasoning is correct.

[michalkucharczyk]

Speculating - this maybe related to the isbestblock bug in the testapi implementation. (this)

[andresilva]

Just pushed a commit to address some small nits and avoid another review loop. Thanks for the PR @michalkucharczyk 👍

[andresilva]

bot merge

[paritytech-processbot]

Waiting for commit status.

[shunjizhan]

Thanks for the fix! And I have a question 🙋‍♂️

*Before* the fix, for [F(a), NBB(a)] sequence of events, enactment phase was not executed, so InBlock event is not triggered. However, does it still trigger Finalized event?

More particularly, if I have a tx like this with instal-seal node, will the console.log get executed?

  // triggers [F(a), NBB(a)] sequence
  extrinsic.signAndSend(sender, (result) => {
    if (result.status.isInBlock) {
      // this will not happen since  enactment phase was not executed
    } else if (result.status.isFinalized) {
      console.log('will this happen???')
    }
  });

[michalkucharczyk]

Before the fix only Finalized event was triggered.
After the fix both events shall be triggered for discussed sequence, see this test:

substrate/client/transaction-pool/tests/pool.rs

Lines 1080 to 1093 in 2eb8ad2

	let event =
	ChainEvent::Finalized { hash: header.clone().hash(), tree_route: Arc::from(vec![]) };
	block_on(pool.maintain(event));
	block_on(pool.maintain(block_event(header.clone())));
	assert_eq!(pool.status().ready, 0);
	{
	let mut stream = futures::executor::block_on_stream(watcher);
	assert_eq!(stream.next(), Some(TransactionStatus::Ready));
	assert_eq!(stream.next(), Some(TransactionStatus::InBlock((header.clone().hash(), 0))));
	assert_eq!(stream.next(), Some(TransactionStatus::Finalized((header.hash(), 0))));
	assert_eq!(stream.next(), None);
	}