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

Fix orphan pool for long pending tx issues #4199

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

chenyukang
Copy link
Collaborator

@chenyukang chenyukang commented Oct 20, 2023

What problem does this PR solve?

Project Godwoken found that some tx will stay in pending for a long time. After analyzing the log(at most 4 hours), we found tx may unexpectedly stay in orphan pool for the scenario transaction chain. For example, tx1 -> tx2, after tx1 is submitted tx2 stayed in pending for a long time, actually tx2 wasn't submitted into txpool.

Problem Summary:

What is changed and how it works?

Fix the issues in process_orphan_tx which may forget some txs in orphan pool. There are 3 issues this PR targeted to resolve:

    1. find_by_previous was only returning one tx by searching with input(because the tx in orphan pool may have conflicts), which may cause issue when there are multiple child tx for one parent tx, process_orphan_tx may can not process all of them.
    1. The evict time interval was 2 * max_block_interval, which is 96 seconds, this is too short for the delay of networking, and we didn't send Reject to filter, which may make the node don't broadcast them later.
    1. For the conflicted tx scenario, since we can not resolve all inputs in orphan pool, we can not add Replace-by-fee checking rules for them, so I change the code to use HashMap<OutPoint, HashSet<ProposalShortId>>, so we don't handle the conflict in orphan pool.

What's Changed:

Related changes

  • PR to update owner/repo:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • Performance regression
  • Breaking backward compatibility

Release note

Title Only: Include only the PR title in the release note.

@chenyukang chenyukang requested a review from a team as a code owner October 20, 2023 15:00
@chenyukang chenyukang requested review from doitian and removed request for a team October 20, 2023 15:00
@chenyukang chenyukang force-pushed the debug-orphan-issues branch 6 times, most recently from 2f1f131 to 89531c3 Compare October 23, 2023 05:38
@chenyukang chenyukang changed the title [WIP] Fix txpool orphan issue Fix txpool orphan issue Oct 23, 2023
@doitian
Copy link
Member

doitian commented Oct 24, 2023

Please choose the release note for the PR.

@chenyukang chenyukang changed the title Fix txpool orphan issue Fix orphan pool for long pending tx issues Oct 24, 2023
@chenyukang chenyukang force-pushed the debug-orphan-issues branch 2 times, most recently from 664898a to fb33a74 Compare October 25, 2023 05:58
pub(crate) const ORPHAN_TX_EXPIRE_TIME: u64 = 2 * 48; // double block interval

/// 100 max block interval
pub(crate) const ORPHAN_TX_EXPIRE_TIME: u64 = 100 * MAX_BLOCK_INTERVAL;
Copy link
Member

Choose a reason for hiding this comment

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

bitcoin will expire orphan tx after 20 mintues: https://github.com/bitcoin/bitcoin/blob/2a349f9ea5de9f0157cd117289996e8640473a6d/src/txorphanage.cpp#L14 which equals 2 block interval, so we use the 2 * 48 in our code. we may need to double check the impaction of increasing it to 100 block interval.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

one issue with increasing it to 100 block interval is we may store more orphan tx in the pool, but we still have 100 max orphan numbers limit, so it should be ok.

on the other hand, the previous 96 seconds is too short in practice, we have observed this kind of case, so let's keep this change.

tx-pool/src/component/orphan.rs Outdated Show resolved Hide resolved
@eval-exec eval-exec added the p:should-have Priority: important but not necessary for delivery in the current delivery timebox label Oct 26, 2023
@chenyukang chenyukang added this pull request to the merge queue Oct 27, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 27, 2023
@chenyukang chenyukang added this pull request to the merge queue Oct 27, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 27, 2023
@chenyukang chenyukang added this pull request to the merge queue Oct 27, 2023
Merged via the queue into nervosnetwork:develop with commit 4cbb826 Oct 27, 2023
33 checks passed
@doitian doitian mentioned this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p:should-have Priority: important but not necessary for delivery in the current delivery timebox
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants