Skip to content

fatxpool: added mortal transactions integration test #8887

Merged
iulianbarbu merged 36 commits into
paritytech:masterfrom
iulianbarbu:test-mortal-txs
Nov 19, 2025
Merged

fatxpool: added mortal transactions integration test #8887
iulianbarbu merged 36 commits into
paritytech:masterfrom
iulianbarbu:test-mortal-txs

Conversation

@iulianbarbu

@iulianbarbu iulianbarbu commented Jun 17, 2025

Copy link
Copy Markdown
Contributor

Description

Based on michalkucharczyk/tx-test-tool#43.

Integration

N/A

Review Notes

  • added tests with future mortal txs that are dropped due to not being included in blocks within their lifetime, but also future mortal txs that have a sufficient lifetime to be included in blocks
  • added test which uses priorities to delay mortal txs inclusion and make them invalid

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu self-assigned this Jun 17, 2025
@iulianbarbu iulianbarbu marked this pull request as draft June 17, 2025 15:51
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Comment thread substrate/client/transaction-pool/tests/integration.rs
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
iulianbarbu and others added 2 commits June 18, 2025 17:42
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Jun 19, 2025
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Comment thread substrate/client/transaction-pool/tests/zombienet/mod.rs Outdated
iulianbarbu and others added 2 commits June 24, 2025 08:46
@ggwpez ggwpez removed the R0-no-crate-publish-required The change does not require any crates to be re-published. label Jun 25, 2025
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Jul 10, 2025
@iulianbarbu iulianbarbu marked this pull request as ready for review July 10, 2025 21:22
Comment thread prdoc/pr_8887.prdoc Outdated
Comment thread prdoc/pr_8887.prdoc Outdated
@@ -0,0 +1,9 @@
title: '`fatxpool`: added mortal transactions integration test '

@iulianbarbu iulianbarbu Jul 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: a prdoc shouldn't be needed by this PR which adds just tests. However, even I pick the label for no crates should be published, check semver still fails by detecting changes in the crate (which is true, but the changes are only in tests, and they shouldn't count for crate bumps). Not sure if I am supposed to do something else, to check with release team.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the prdoc, will leave checksemver failing - at the same time it must ignore changes in tests if possible, must debug why it does.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this. We changed the code, PRdoc won't hurt us after all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We changed the tests, checksemver implies a version bump should be done. The problem I have with this is that I am required to add a prdoc for non-user facing stuff, with an associated bump.

Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Comment thread substrate/client/transaction-pool/tests/integration.rs Outdated
Co-authored-by: Sebastian Kunert <mail@skunert.dev>

@sistemd sistemd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iulianbarbu iulianbarbu added the R0-no-crate-publish-required The change does not require any crates to be re-published. label Nov 17, 2025
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu added this pull request to the merge queue Nov 18, 2025
// so we get 3750KB. In the test scenario we aim for 5 txs per block roughly (not precesily)
// so to fill a block each user tx must have around 750kb.
#[tokio::test(flavor = "multi_thread")]
#[ignore]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is quick test, maybe we should remove ignore?
Could also be applied for other quick e2e tests..

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes even more sense now when the pool is no longer actively maintained. I used to run all of these tests regularly so I knew all is good.
Now, nobody checks them, and maybe other components may cause regression in pool.

@iulianbarbu iulianbarbu Nov 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they won't work either way because of the error bellow. I think we have already some zn-sdk tests that use such binaries, but I remember they were included in a certain dedicated zn job, which might do the necessary setup. Maybe @lrubasze knows more about what's needed from a CI perspective to run zn-sdk tests that depend on certain binaries?

thread 'send_lower_priority_mortal_txs' panicked at substrate/client/transaction-pool/tests/integration.rs:222:10:
    called `Result::unwrap()` on an `Err` value: NetworkInit(Invalid network config to use provider native: Invalid configuration: 
     Missing binary polkadot, compile by running (in the polkadot-sdk repo): 
     cargo build --locked --release --features fast-runtime --bin polkadot --bin polkadot-prepare-worker --bin polkadot-execute-worker

@iulianbarbu iulianbarbu Nov 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #10348. I plan to tackle it separately from this PR. For now I would keep the #ignore attribute for the added tests just because I don't plan to investigate on this PR what's required to have e2e tests runnable in the CI (e.g. the main issue being binaries access for the current tests, for others I am not sure), or invest time in setting up other environments for longer running e2e tests. For #10348 maybe it is worth chatting more with @pepoviola - again, not part of the current PR scope. I'll follow up specifically when I'll have bandwidth.

Comment thread substrate/client/transaction-pool/tests/zombienet/mod.rs
@iulianbarbu iulianbarbu removed this pull request from the merge queue due to a manual request Nov 18, 2025
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
Signed-off-by: Iulian Barbu <iulian.barbu@parity.io>
@iulianbarbu iulianbarbu added this pull request to the merge queue Nov 18, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 18, 2025
@iulianbarbu iulianbarbu added this pull request to the merge queue Nov 19, 2025
Merged via the queue into paritytech:master with commit 38c598e Nov 19, 2025
443 of 494 checks passed
@iulianbarbu iulianbarbu deleted the test-mortal-txs branch November 19, 2025 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. T10-tests This PR/Issue is related to tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants