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

[2.x.x] Take age of txs into consideration when bucketing and sorting txs in txpool #2878

Merged

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Jun 7, 2019

Turns out we were not taking tx "age" into consideration when bucketing and sorting txs in the txpool.
Which means that at block capacity it is basically a lottery whether your tx will get included in the next block...

This PR fixes this.

We maintain txs in the pool in insertion order so we do not need to track "age" explicitly.
Bucket creation follows tx insertion order so we just need to take bucket creation order into consideration when sorting the buckets.


Existing behavior -

  • aggregate txs in pool if bucket fee_to_weight is not reduced
  • sort buckets by fee_to_weight descending
  • aggregate buckets up to max tx size

Revised behavior -

  • aggregate txs in pool if bucket fee_to_weight is not reduced
  • sort buckets by fee_to_weight descending and age (oldest first)
  • aggregate buckets up to max tx size

Our txpool bucketing and sorting logic still picks txs that maximize the overall block fee. But now also prioritizes txs that have been in the txpool the longest.

@antiochp antiochp added this to the 2.x.x milestone Jun 7, 2019
Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

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

Code looks good to me.
One thing I do not understand is why an old but low fee transaction should be prioritised over a new high fee transaction. It seems to me that the curent behaviour is we expect: the more you pay the more you have a chance to get into the next block regardless of how old is your transaction.

@antiochp
Copy link
Member Author

One thing I do not understand is why an old but low fee transaction should be prioritised over a new high fee transaction.

It won't be. We will always prioritize by fee_by_weight first.
But if we have two txs with the same fee_by_weight then we should prioritize the oldest one first (and we don't currently do this).

@quentinlesceller
Copy link
Member

Okay in that case it makes perfect sense to add this. LGTM 👍

@antiochp antiochp changed the base branch from master to milestone/2.x.x June 27, 2019 16:20
@antiochp antiochp changed the title Take age of txs into consideration when bucketing and sorting txs in txpool [2.x.x] Take age of txs into consideration when bucketing and sorting txs in txpool Jun 27, 2019
@antiochp antiochp force-pushed the bucket_txs_sort_fee_and_age branch from cece5b1 to 5c38ca1 Compare June 27, 2019 16:21
@antiochp antiochp force-pushed the bucket_txs_sort_fee_and_age branch from 5c38ca1 to e8f9572 Compare June 27, 2019 16:27
@antiochp antiochp merged commit 8277516 into mimblewimble:milestone/2.x.x Jun 27, 2019
@antiochp antiochp deleted the bucket_txs_sort_fee_and_age branch June 27, 2019 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants