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: mempool fetch_highest_priority_txs #5551

Merged
merged 5 commits into from
Jun 30, 2023

Conversation

Cifko
Copy link
Contributor

@Cifko Cifko commented Jun 29, 2023

Description

Fix the fetch_highest_priority_txs.
The process of selection is as follows:
Assume that all transaction have the same weight for simplicity. A(20)->B(2) means A depends on B and A has
fee 20 and B has fee 2. A(20)->B(2)->C(14), D(12)

  1. A will be selected first, but B and C will be piggybacked on A, because overall fee_per_byte is 12, so we
    store it temporarily.
  2. We look at transaction C with fee per byte 14, it's good, nothing is better.
  3. We come back to transaction A with fee per byte 12, but now that C is already in, we recompute it's fee
    per byte to 11, and again we store it temporarily.
  4. Next we process transaction D, it's good, nothing is better.
  5. And now we proceed finally to transaction A, because there is no other possible better option.

Motivation and Context

As it was it was easy to piggyback heavy transaction with low fee_per_byte on light transaction with high fee per byte.

How Has This Been Tested?

There are two tests for zero conf, one is for the piggybacking.

What process can a PR reviewer use to test or verify this change?

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@github-actions
Copy link

github-actions bot commented Jun 29, 2023

Test Results (Integration tests)

26 tests  +26   26 ✔️ +26   13m 54s ⏱️ + 13m 54s
11 suites +11     0 💤 ±  0 
  2 files   +  2     0 ±  0 

Results for commit 2f3d5f4. ± Comparison against base commit 6c3fa50.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Jun 29, 2023
@github-actions
Copy link

github-actions bot commented Jun 29, 2023

Test Results (CI)

1 180 tests   1 180 ✔️  17m 58s ⏱️
     37 suites         0 💤
       1 files           0

Results for commit 2f3d5f4.

♻️ This comment has been updated with latest results.

SWvheerden
SWvheerden previously approved these changes Jun 29, 2023
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

UtAck

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Jun 29, 2023
@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Jun 29, 2023
Copy link
Contributor

@brianp brianp left a comment

Choose a reason for hiding this comment

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

LGTM

utAck

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Jun 30, 2023
@SWvheerden SWvheerden merged commit f7f749c into tari-project:development Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants