`BlockId` removal: `tx-pool` refactor #1678

michalkucharczyk · 2023-09-22T14:41:31Z

It changes following APIs:

trait ChainApi
-- validate_transaction
trait TransactionPool
--submit_at
--submit_one
--submit_and_watch

and some implementation details, in particular:

impl Pool
--submit_at
--resubmit_at
--submit_one
--submit_and_watch
--prune_known
--prune
--prune_tags
--resolve_block_number
--verify
--verify_one
revalidation queue

All tests are also adjusted.

This PR is part of BlockId::Number refactoring analysis (#53)
Some ground work towards: (#1202)

It changes following APIs: - trait ChainApi -- `validate_transaction` - trait `TransactionPool` --`submit_at` --`submit_one` --`submit_and_watch` and some implementation details, in particular: - impl `Pool` --`submit_at` --`resubmit_at` --`submit_one` --`submit_and_watch` --`prune_known` --`prune` --`prune_tags` --`resolve_block_number` --`verify` --`verify_one`A - revalidation queue All tests are also adjusted.

michalkucharczyk · 2023-09-22T14:45:43Z

bot fmt

command-bot · 2023-09-22T14:45:48Z

@michalkucharczyk https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3775436 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 7-2e3d2e38-25bd-41ef-aaf2-580ea4e2b3f7 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-09-22T14:46:29Z

@michalkucharczyk Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3775436 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3775436/artifacts/download.

bkchr

Good work 👍

bkchr · 2023-09-22T15:13:50Z

substrate/client/transaction-pool/src/revalidation.rs

+				sc_transaction_pool_api::error::Error::InvalidBlockId(format!("{:?}", at)).into(),
+			)
+		})
+		.expect("hash to number should work");


Ohh wait!

Move this above the let validation_results. Print a debug message and return. Should not really happen. Just one thing to check, is it fine to drop batch or are the tx still in the pool?

yeah, this shall never happen. hash to block number shall always work - I can add debug and return.

Just one thing to check, is it fine to drop batch or are the tx still in the pool?

Did not get it, could you give me more details on that?

What happens if the batch, that is passed in to this function, is dropped without it being processed and "re-added".

The transactions will stay in validated_pool, so they can be resubmitted in the next batch.
Skipping re-validation sounds reasonable, otherwise the transactions could be invalidated (if the block is not available).

Done in 7c002a4.

I also added test for this case:

polkadot-sdk/substrate/client/transaction-pool/src/revalidation.rs

Lines 445 to 450 in 17e9cb0

// revalidation shall be skipped for unknown block:

block_on(queue.revalidate_later(unknown_block, uxt_hashes));

// no revalidation shall be done

assert_eq!(api.validation_requests().len(), 4);

// number of ready shall not change

assert_eq!(pool.validated_pool().status().ready, 2);

substrate/client/transaction-pool/src/revalidation.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

skunert

Looks Good!

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

substrate/client/transaction-pool/benches/basics.rs

substrate/client/transaction-pool/src/revalidation.rs

substrate/utils/frame/rpc/system/src/lib.rs

* master: (61 commits) OpenGov in Westend and Rococo (#1177) Associated type Hasher for `QueryPreimage`, `StorePreimage` and `Bounded` (#1720) Migrate polkadot-primitives to v6 (#1543) genesis-builder: implemented for all runtimes (#1492) `BlockId` removal: `tx-pool` refactor (#1678) Bump directories from 4.0.1 to 5.0.1 (#1656) Allow debug_assertions in short-benchmarks CI job (#1711) chainHead/storage: Fix storage iteration using the query key (#1665) Implement more useful traits in `Slot` type (#1595) Make downloads in parallel and give more time to complete (#1699) Bump actions/checkout from 4.0.0 to 4.1.0 (#1688) contracts: Fix incorrect storage alias in mirgration (#1687) Fix documentation about justification and `finalized == true` requirement (#1607) tweak pallet macro (genesis_config etc) to cater for RA users as well. (#1689) Uncoupling pallet-xcm from frame-system's RuntimeCall (#1684) Bump aes-gcm from 0.10.2 to 0.10.3 (#1681) docs / Update PR template to reflect monorepo (#1674) update contributing guide and ui-tests scripts (#1668) pallet epm: add `TrimmingStatus` to the mined solution (#1659) Update HRMP pallet benchmarking to use benchmarks v2 (#1676) ...

* tsv-disabling-node-side: (69 commits) runtime-api: cleanup after v7 stabilization (#1729) Move requests-responses and polling from `ChainSync` to `SyncingEngine` (#1650) Add custom error message for `StorageNoopGuard` (#1727) Clarify docs cargo fmt add a CAVEAT comment implement disabled_validators correctly remove unnecessary hash string (#1722) OpenGov in Westend and Rococo (#1177) Associated type Hasher for `QueryPreimage`, `StorePreimage` and `Bounded` (#1720) Migrate polkadot-primitives to v6 (#1543) genesis-builder: implemented for all runtimes (#1492) `BlockId` removal: `tx-pool` refactor (#1678) Bump directories from 4.0.1 to 5.0.1 (#1656) Allow debug_assertions in short-benchmarks CI job (#1711) chainHead/storage: Fix storage iteration using the query key (#1665) Implement more useful traits in `Slot` type (#1595) Make downloads in parallel and give more time to complete (#1699) Bump actions/checkout from 4.0.0 to 4.1.0 (#1688) contracts: Fix incorrect storage alias in mirgration (#1687) ...

It changes following APIs: - trait `ChainApi` -- `validate_transaction` - trait `TransactionPool` --`submit_at` --`submit_one` --`submit_and_watch` and some implementation details, in particular: - impl `Pool` --`submit_at` --`resubmit_at` --`submit_one` --`submit_and_watch` --`prune_known` --`prune` --`prune_tags` --`resolve_block_number` --`verify` --`verify_one` - revalidation queue All tests are also adjusted. --------- Co-authored-by: command-bot <> Co-authored-by: Bastian Köcher <git@kchr.de>

michalkucharczyk requested a review from a team September 22, 2023 14:41

michalkucharczyk added the T0-node This PR/Issue is related to the topic “node”. label Sep 22, 2023

".git/.scripts/commands/fmt/fmt.sh"

b81399d

bkchr approved these changes Sep 22, 2023

View reviewed changes

bkchr requested changes Sep 22, 2023

View reviewed changes

michalkucharczyk added 8 commits September 22, 2023 17:16

fixes

9706b00

fix

79ceb1a

manual seal bug fix 🤦 + other fixes

88a7a94

fixes

3628ccf

properly initialize best_block in RevalidationWorker

e416c8d

Merge branch 'master' into mku-blockid-txpool

02ed3c3

revalidation skipped when hash-to-number fails

7c002a4

fmt fixes

17e9cb0

michalkucharczyk requested review from bkchr and a team September 26, 2023 09:12

spelling

c94032d

bkchr approved these changes Sep 26, 2023

View reviewed changes

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

michalkucharczyk requested a review from a team September 26, 2023 12:01

Update substrate/client/transaction-pool/src/revalidation.rs

c375c6d

Co-authored-by: Bastian Köcher <git@kchr.de>

skunert approved these changes Sep 26, 2023

View reviewed changes

davxy approved these changes Sep 26, 2023

View reviewed changes

michalkucharczyk added 2 commits September 27, 2023 08:05

review nits

d7917cb

Merge branch 'master' into mku-blockid-txpool

147e728

michalkucharczyk merged commit ab3a3bc into master Sep 27, 2023
107 checks passed

michalkucharczyk deleted the mku-blockid-txpool branch September 27, 2023 09:58

kiltbot mentioned this pull request Oct 19, 2023

[AUTOMATIC] Update Polkadot dependencies from 1.1.0 to 1.2.0 KILTprotocol/kilt-node#571

Closed

ahmadkaouk mentioned this pull request Nov 16, 2023

Update polkadot-sdk from v.1.1.0 to v1.3.0 moonbeam-foundation/moonbeam#2565

Closed

ukint-vs mentioned this pull request Apr 17, 2024

feat: Update substrate to v1.2.0 gear-tech/gear#3898

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`BlockId` removal: `tx-pool` refactor #1678

`BlockId` removal: `tx-pool` refactor #1678

michalkucharczyk commented Sep 22, 2023 •

edited

Loading

michalkucharczyk commented Sep 22, 2023

command-bot bot commented Sep 22, 2023 •

edited

Loading

command-bot bot commented Sep 22, 2023

bkchr left a comment

bkchr Sep 22, 2023

michalkucharczyk Sep 22, 2023

bkchr Sep 24, 2023

michalkucharczyk Sep 26, 2023

skunert left a comment

	// revalidation shall be skipped for unknown block:
	block_on(queue.revalidate_later(unknown_block, uxt_hashes));
	// no revalidation shall be done
	assert_eq!(api.validation_requests().len(), 4);
	// number of ready shall not change
	assert_eq!(pool.validated_pool().status().ready, 2);

BlockId removal: tx-pool refactor #1678

BlockId removal: tx-pool refactor #1678

Conversation

michalkucharczyk commented Sep 22, 2023 • edited Loading

michalkucharczyk commented Sep 22, 2023

command-bot bot commented Sep 22, 2023 • edited Loading

command-bot bot commented Sep 22, 2023

bkchr left a comment

Choose a reason for hiding this comment

bkchr Sep 22, 2023

Choose a reason for hiding this comment

michalkucharczyk Sep 22, 2023

Choose a reason for hiding this comment

bkchr Sep 24, 2023

Choose a reason for hiding this comment

michalkucharczyk Sep 26, 2023

Choose a reason for hiding this comment

skunert left a comment

Choose a reason for hiding this comment

`BlockId` removal: `tx-pool` refactor #1678

`BlockId` removal: `tx-pool` refactor #1678

michalkucharczyk commented Sep 22, 2023 •

edited

Loading

command-bot bot commented Sep 22, 2023 •

edited

Loading