Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ZEND-552] Remove stale transactions when a hard fork activates #594

Merged
merged 5 commits into from
Aug 25, 2023

Conversation

ptagl
Copy link
Contributor

@ptagl ptagl commented Aug 24, 2023

The RemoveStaleTransaction() function is currently called every time a block is connected/disconnected to eventually remove transactions that became invalid.

To keep the flow as efficient as possible, such function doesn't run all the checks available inside AcceptTxToMemoryPool() but only the ones that could really fail and depend on the context (like the chain height, or the state of a sidechain).

This PR includes a small fix to apply additional checks in case a hard fork activates, because the change in the set of validation rules may affect pending transactions (for instance, a shielding transaction will not be valid anymore after the activation of hard fork #11).

JackPiri and others added 4 commits August 24, 2023 15:03
Extended the cross hard fork checks also to the case of tip disconnection.
The test is creating a shielding transaction around the shielded pool deprecation hard fork, and such transaction wasn't correctly removed in the past after the fork activation.
@ptagl ptagl requested review from titusen and a-petrini August 24, 2023 13:12
Copy link
Contributor

@a-petrini a-petrini left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for this PR! I only left a minor non-blocking comment regarding the Python test.

The "listunspent()" RPC command may return outdated results if it's called immediately after the submission of a transaction, as the wallet is notified by a separate thread with some delay.
@ptagl ptagl force-pushed the gp/post_hard_fork_tx_eviction branch from 57add7c to 421686d Compare August 25, 2023 13:22
Copy link
Contributor

@titusen titusen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@ptagl ptagl merged commit b00dc2c into main Aug 25, 2023
@ptagl ptagl deleted the gp/post_hard_fork_tx_eviction branch August 25, 2023 15:10
a-petrini added a commit that referenced this pull request Aug 30, 2023
Python tests of this PR relies on changes introduced in PR #586, which is not included
in this release. This commit fixes the issue temporarily.
a-petrini added a commit that referenced this pull request Aug 30, 2023
Python tests of this PR relies on changes introduced in PR #586, which is not included
in this release. This commit fixes the issue temporarily.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants