`txpool api`: `remove_invalid` call improved #6661

michalkucharczyk · 2024-11-26T18:17:51Z

Description

Currently the transaction which is reported as invalid by a block builder (or removed_invalid by other components) is silently skipped.

This PR improves this behavior. The transaction pool report_invalid function now accepts optional error associated with every reported transaction, and also the optional block hash which provides hints how reported transaction shall be handled. The following API change is proposed:

polkadot-sdk/substrate/client/transaction-pool/api/src/lib.rs

Lines 297 to 318 in 8be5ef3

    
           	/// Reports invalid transactions to the transaction pool. 
        
           	/// 
        
           	/// This function takes a map where the key is a transaction hash and the value is an 
        
           	/// optional error encountered during the transaction execution, possibly within a specific 
        
           	/// block. 
        
           	/// 
        
           	/// The transaction pool implementation decides which transactions to remove. Transactions 
        
           	/// removed from the pool will be notified with `TransactionStatus::Invalid` event (if 
        
           	/// `submit_and_watch` was used for submission). 
        
           	/// 
        
           	/// If the error associated to transaction is `None`, the transaction will be forcibly removed 
        
           	/// from the pool. 
        
           	/// 
        
           	/// The optional `at` parameter provides additional context regarding the block where the error 
        
           	/// occurred. 
        
           	/// 
        
           	/// Function returns the transactions actually removed from the pool. 
        
           	fn report_invalid( 
        
           		&self, 
        
           		at: Option<<Self::Block as BlockT>::Hash>, 
        
           		invalid_tx_errors: TxInvalidityReportMap<TxHash<Self>>, 
        
           	) -> Vec<Arc<Self::InPoolTransaction>>;

Depending on error, the transaction pool can decide if transaction shall be removed from the view only or entirely from the pool. Invalid event will be dispatched if required.

Notes for reviewers

Actual logic of removing invalid txs is implented in ViewStore::report_invalid. Method's doc explains the flow.
This PR changes HashMap to IndexMap in revalidation logic. This is to preserve the original order of transactions (mainly for purposes of unit tests).
This PR solves the problem mentioned in: fatxpool: add tests for rejecting invalid transactions #5477 (comment) (which can now be resolved). The invalid transactions found during mempool revalidation are now also removed from the view_store. No dangling invalid transaction shall be left in the pool. (bfec262)
The support for dropping invalid transactions reported from the views was also added. This should never happen, but if for any case all views will report invalid transcation (which previously was valid) the transaction will be dropped from the pool (48214a3).

fixes: #6008, #5477

substrate/client/transaction-pool/api/src/lib.rs

…nvalid

substrate/client/transaction-pool/api/src/lib.rs

…nvalid

When view report transaction as invalid it shall be treated as dropped. This commit adds the support for this.

Mempool revalidation now removes the invalid transactions from the view_store. As a result handling `transactions_invalidated` in multi-view-listener does not require handling the case when transaction is still dangling in some view. (It is guaranteed that invalid transaction was removed from the view_store and can be safely notified as Invalid to external listner).

paritytech-workflow-stopper · 2025-02-06T08:04:04Z

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13174136857
Failed job name: cargo-clippy

skunert

Left some comments, otherwise looks good

substrate/client/transaction-pool/src/fork_aware_txpool/multi_view_listener.rs

substrate/client/transaction-pool/src/fork_aware_txpool/metrics.rs

substrate/client/transaction-pool/src/fork_aware_txpool/dropped_watcher.rs

iulianbarbu

LGTM!

substrate/client/transaction-pool/api/src/lib.rs

substrate/client/transaction-pool/src/fork_aware_txpool/metrics.rs

substrate/client/transaction-pool/src/fork_aware_txpool/multi_view_listener.rs

…nvalid

michalkucharczyk added 3 commits November 26, 2024 19:12

txpool: API update: remove_invalid -> report_invalid

04af09b

other modules updated

bf2dd1f

basic-authorship: updated

8c8e033

michalkucharczyk commented Nov 26, 2024

View reviewed changes

substrate/client/transaction-pool/api/src/lib.rs Show resolved Hide resolved

michalkucharczyk changed the title ~~txpool api: remove invalid call improved~~ txpool api: remove invalid call improved Nov 26, 2024

Cargo.lock updated

18ceac8

michalkucharczyk changed the title ~~txpool api: remove invalid call improved~~ txpool api: remove_invalid call improved Nov 27, 2024

michalkucharczyk added 23 commits November 28, 2024 09:21

sketch

deb5c98

dropped_watcher: new fns are public

018f9d4

dropped_watcher: improvement

4aa24d3

fatp: handling higher prio with full mempool

9fe88a6

graph: fold_ready improved

54ae11c

graph: make some staff public

a6882eb

tests added

8d3dffe

type removed

70fd186

improvements

d3a1a7b

make use of your brain

4246dac

fatp: pending actions now support removals

c203d72

validated_pool: SubmitOutcome

edb1257

view/view_store: SubmitOutcome

f778176

mempool: update_transaction stub

a72b3f9

fatp: SubmitOutcome

c411bb4

fatp: todo added

7b461bf

single-state txpool: SubmitOutcome integration

8765d2c

tests: SubmitOutcome fixes

e8ccd44

mempool: sizes fix

6cca272

dropping transaction - size limit is properly obeyed now

3b17a16

merge / rebase fixes

4f767e5

mempool: prio is now locked option

6ba133e

tests added + dead code cleanup

46fa1fd

michalkucharczyk requested a review from skunert January 17, 2025 17:10

michalkucharczyk added 3 commits January 17, 2025 18:10

Merge remote-tracking branch 'origin/master' into mku-txpool-remove-i…

1f7280e

…nvalid

Merge remote-tracking branch 'origin/master' into mku-txpool-remove-i…

4d23a6f

…nvalid

fix

562f30d

bkchr reviewed Feb 3, 2025

View reviewed changes

substrate/client/transaction-pool/api/src/lib.rs Outdated Show resolved Hide resolved

michalkucharczyk added 10 commits February 3, 2025 17:14

report_invalid: sp_blockchain::Error -> TransactionValidityError

8d63728

Merge remote-tracking branch 'origin/master' into mku-txpool-remove-i…

4911864

…nvalid

merge fix

44d55f3

logging fixed - tracing

8d7334e

Dropped(invalid) supported

48214a3

When view report transaction as invalid it shall be treated as dropped. This commit adds the support for this.

view_store::report_invalid does not trigger event

95c8ff2

logging

37fb2e9

formatting

50a4539

mempool: remove_transaction -> remove_transactions

5b70ad9

michalkucharczyk linked an issue Feb 5, 2025 that may be closed by this pull request

fatxpool: add tests for rejecting invalid transactions #5477

Open

clippy

0e5fe6d

michalkucharczyk added 2 commits February 6, 2025 10:22

clippy again

5e39d3f

Merge branch 'master' into mku-txpool-remove-invalid

0910ead

skunert approved these changes Feb 11, 2025

View reviewed changes

michalkucharczyk added 2 commits February 11, 2025 16:09

review comments

6d7ccff

fmt

d30b6c0

iulianbarbu approved these changes Feb 12, 2025

View reviewed changes

substrate/client/transaction-pool/api/src/lib.rs Show resolved Hide resolved

substrate/client/transaction-pool/src/fork_aware_txpool/metrics.rs Show resolved Hide resolved

substrate/client/transaction-pool/src/fork_aware_txpool/multi_view_listener.rs Show resolved Hide resolved

michalkucharczyk added 6 commits February 13, 2025 09:45

Merge branch 'master' into mku-txpool-remove-invalid

cc03547

report_invalid: [tuple] -> TxInvalidityReportMap

31c0b60

do not drop future/stale event with single view

9932bf6

Merge remote-tracking branch 'origin/master' into mku-txpool-remove-i…

aa88716

…nvalid

view: unused function removed

6b41d93

Cargo.lock

8be5ef3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`txpool api`: `remove_invalid` call improved #6661

`txpool api`: `remove_invalid` call improved #6661

michalkucharczyk commented Nov 26, 2024 •

edited

Loading

paritytech-workflow-stopper bot commented Feb 6, 2025

skunert left a comment

iulianbarbu left a comment

	/// Reports invalid transactions to the transaction pool.
	///
	/// This function takes a map where the key is a transaction hash and the value is an
	/// optional error encountered during the transaction execution, possibly within a specific
	/// block.
	///
	/// The transaction pool implementation decides which transactions to remove. Transactions
	/// removed from the pool will be notified with `TransactionStatus::Invalid` event (if
	/// `submit_and_watch` was used for submission).
	///
	/// If the error associated to transaction is `None`, the transaction will be forcibly removed
	/// from the pool.
	///
	/// The optional `at` parameter provides additional context regarding the block where the error
	/// occurred.
	///
	/// Function returns the transactions actually removed from the pool.
	fn report_invalid(
	&self,
	at: Option<<Self::Block as BlockT>::Hash>,
	invalid_tx_errors: TxInvalidityReportMap<TxHash<Self>>,
	) -> Vec<Arc<Self::InPoolTransaction>>;

txpool api: remove_invalid call improved #6661

Are you sure you want to change the base?

txpool api: remove_invalid call improved #6661

Conversation

michalkucharczyk commented Nov 26, 2024 • edited Loading

Description

Notes for reviewers

paritytech-workflow-stopper bot commented Feb 6, 2025

skunert left a comment

Choose a reason for hiding this comment

iulianbarbu left a comment

Choose a reason for hiding this comment

`txpool api`: `remove_invalid` call improved #6661

`txpool api`: `remove_invalid` call improved #6661

michalkucharczyk commented Nov 26, 2024 •

edited

Loading