txpool api: remove_invalid call improved

[michalkucharczyk]

Note

This PR depends on work done in #6647

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 293 to 310 in 8c8e033

	/// Reports invalid transactions to the transaction pool.
	///
	/// This function accepts an array of tuples, each containing a transaction hash and an
	/// optional error encountered during the transaction execution at a specific (also optional)
	/// block.
	///
	/// The transaction pool implementation decides which transactions to remove. Transactions
	/// dependent on invalid ones will also be removed.
	///
	/// If the tuple's error 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.
	fn report_invalid(
	&self,
	at: Option<<Self::Block as BlockT>::Hash>,
	invalid_tx_errors: &[(TxHash<Self>, Option<sp_blockchain::Error>)],
	) -> 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 impleneted 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).

fixes: #6008

[michalkucharczyk]

We could also keep old function remove_invalid and add new one, with no optional error/block parameters. Really no strong opinion here.

Maybe triggering invalid event in case of removal the transaction via RPC call is (maybe) undesirable - so maybe we should have pure remove function (which would trigger Dropped, IMO it makes more sense). Any thoughts? (This could also be done as follow-up).

[michalkucharczyk]

/cmd prdoc --bump minor --audience node_dev

[michalkucharczyk]

I'd like to force-push here to clean up commit history.
Is that fine for you (@skunert @iulianbarbu)?

[paritytech-workflow-stopper]

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12772650851
Failed job name: test-linux-stable-no-try-runtime

[skunert]

Looks good.

[michalkucharczyk]

I decided not to force push to preserve comments which could be deleted.

I added fatp_invalid.rs file, which focuses on handling widely understood invalid transactions (submit / revalidate and report).

Some tests are new, but some where just moved (and slightly updated, with no logic changes) from the original fatp.rs file.

I also reviewed these tests having in mind #5477.