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

bucket_transactions is now fee_to_weight aware #2758

Merged
merged 3 commits into from
Apr 30, 2019

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Apr 16, 2019

Reworked bucket_transactions to make it fee_to_weight aware.
We also return the vec of underlying txs from bucket_transactions. We used to return the aggregated bucket txs.

bucket_transactions now performs the following logic -

  • Buckets consist of a vec of txs and track the aggregate fee_to_weight and a "depth".
  • Bucket depth is incremented if a bucket depends on an earlier bucket.
    • buckets only ever have dependencies on buckets with higher fee_to_weight
  • We aggregate (cut-through) dependent transactions within a bucket unless adding a tx
    would reduce the aggregate fee_to_weight, in which case we start a new bucket (and increment the depth).
  • We then sort the buckets by depth (ascending) and fee_to_weight (descending) to preserve dependency ordering and maximize both cut-through and overall fees.

The vec of txs returned by bucket_transactions satisfies the following -

  • No tx is dependent on any subsequent tx in the vec
  • Txs that can be cut-through are adjacent (as long as fee_to_weight is not adversely impacted)
  • Cut-through takes max tx weight into account

This prevents low fee 0-conf txs from "piggy-backing" off of higher fee txs in the pool.
While enabling low fee 0-conf txs stuck in the pool to be spent via high fee 0-conf txs (CPFP style).

The pool logic will now attempt to maximize cut-through as long as overall fees are not adversely impacted.

A nice side-effect of all this is related to tx eviction (see #2706). The txs at the end of the vec of txs returned by bucket_transactions are good candidates for eviction.
They are likely to have low fee_to_weight and can be safely evicted without affecting dependent txs and without impacting cut-through.

@antiochp antiochp force-pushed the bucket_txs_fee_check branch from d341e13 to e37acd0 Compare April 17, 2019 10:54
@antiochp antiochp marked this pull request as ready for review April 17, 2019 10:54
@antiochp
Copy link
Member Author

antiochp commented Apr 17, 2019

I guess the complication here is we end up building buckets with dependencies between them (one bucket depends on a prior bucket).
Which is fine as long as we then sort them in a consistent way (by descending fee_to_weight) but this is not necessarily clear in the code and the dependency between buckets is implicit.

Maybe we're fine with this assumption that the buckets will be sorted consistently.

Crazy idea: buckets are not simply single aggregated txs but a vec of aggregated txs - each bucket is a vec of txs ordered by dependency (later ones dependent on earlier one).
And we do not reorder the txs in a bucket, preserving dependency.

So we aggregates txs in a bucket as long as this does not reduce the fee_to_weight of the aggregate tx, otherwise we append a new aggregate tx to the end of our bucket.

Then we are free to reorder buckets as we see fit (no dependency between buckets).
But we do not reorder txs within a bucket (txs are dependent on earlier txs in a bucket).

  • To prepare mineable txs we sort the buckets and pick some aggregate txs.
  • To evict txs we sort the buckets and evict later txs from cheaper buckets.

Does that work?


Alternatively: Maybe the simplest solution is for bucket_transactions() to handle the sorting internally (then we can keep it all consistent).
And clients promise not to resort them (as sorting might break dependencies between buckets).

@quentinlesceller
Copy link
Member

The first idea bother me on the fact that making buckets possibly dependant of each other can be tricky for a lot of things.
For the second method, let's say you have this transaction pool with the following buckets (each bucket is Vec<Vec< Transaction >>):

Bucket A <<Tx1>>
Bucket B <<Tx2;Tx2,1>;<Tx2,2>> where Tx2,1 and Tx2,2 are dependant on Tx2 but not dependant on each other.
Bucket C <Tx3>

Let's say you must take 2 transactions to build a block.
You order the transactions by fee over weight and let's say fee_weight(Tx3)>fee_weight(Tx2,2). So you end up taking Tx1 and <Tx2;Tx2,1>. So basically you flatten the buckets in a list of transactions and again order them by fee over weight and take the first X txs. I think that could work.

To evict txs we sort the buckets and evict later txs from cheaper buckets.

This I think we need to the same as above (flatten the buckets and take the last one) as example fee_weight(Tx3)>fee_weight(Tx2,2).
Overall I don't directly see any flaws in this.

Alternatively: Maybe the simplest solution is for bucket_transactions() to handle the sorting internally (then we can keep it all consistent).

What do you mean?

@antiochp antiochp force-pushed the bucket_txs_fee_check branch from e37acd0 to 26e522e Compare April 17, 2019 16:32
@antiochp
Copy link
Member Author

antiochp commented Apr 17, 2019

@quentinlesceller Take another look.

Reworked this so bucket_transactions returns a vec of the underlying txs (and not the aggregate bucket txs).
These are sorted such that dependencies flow right to left (no tx is dependent on txs later in the vec).

Buckets of txs can be cut-through.
We start a new bucket if a tx would lower the aggregate fee_to_weight of an existing bucket.
We sort the buckets by aggregate fee_to_weight (while preserving dependencies between buckets via "depth").
Then we flatten the buckets and return the txs.

If you want to evict a single tx you can use the last one in the vec.
No other tx depends on it.
It has low fee_to_weight.
It is unlikely to participate in any cut-through.

If you want to evict n txs, just take the last n from the vec.

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.

Looks good to me. This will drastically simplify the eviction of transaction while keeping a simple data structure.
I would be even more comfortable if @ignopeverell could review as well so we are 99% sure that we did not miss anything.

pool/src/pool.rs Outdated Show resolved Hide resolved
// Otherwise put it in its own bucket at the end.
// Note we increment the depth here to track the dependency.
tx_buckets
.push(Bucket::new_with_depth(entry.tx.clone(), bucket.depth + 1));
}
} else {
// Aggregation failed so discard this new tx.
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity out frequently this happens and how?

Copy link
Member Author

Choose a reason for hiding this comment

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

The aggregation failure?

Basically the only way this would fail is if the aggregated tx ended up being too big to fit in a block. Say we attempted two huge transactions that in aggregate was larger than our block weight limit. This should happen only rarely.

The only other way this could happen is if the pool got itself into a bad state somehow with a double spend or something similar in there.
This should never happen in practice as we check for consistency at various stages of the tx pool lifecycle.

@antiochp antiochp requested a review from ignopeverell April 17, 2019 19:42
@ignopeverell
Copy link
Contributor

Sorry for being so late to the party. So here we'd prioritize a transaction with no dependent ("child") transaction over a potentially much higher paying one with one dependent that lowers its parent's fee/weight ratio?

@antiochp
Copy link
Member Author

antiochp commented Apr 25, 2019

So here we'd prioritize a transaction with no dependent ("child") transaction over a potentially much higher paying one with one dependent that lowers its parent's fee/weight ratio?

Not quite no. We would bucket these up into 3 buckets.

  1. tx A with no dependents
  2. the high fee tx B
  3. tx C (lower fee), dependent on B

i.e. B and C would not be bucketed together because C would lower the bucket fee/weight ratio. But they would both still be included in (separate) buckets.

We would then sort the buckets by fee/weight ratio (while maintaining the ordering constraint between B and C).

So in your example B then A then C.

Does this makes sense?

tl;dr Bucket dependent txs together if no negative effect on fee/weight ratios.
Then sort them to maximize fee/weight ratios (while maintaining dependency ordering constraints).


Another example, say we have the following -

  • A <- B <- C are all dependent txs
  • A and C have high fee/weight ratios
  • B has a low fee/weight ratio

In this example we would bucket them as -

  • [A], [B, C] (B would not be bucketed with A as it lowers fee/weight ratio)

We would then sort them as [A], [B, C] (B depends on A).
This constraint would be maintained even if [B, C] had a significantly higher fee/weight ratio.
In a situation with a block at capacity, [A] may be included, while [B, C] would remain in the pool for the next block. In which case [B, C] would presumably be prioritized next block due to fee/weight ratio of the bucket.
We would sacrifice some cut-through to maximize fee/weight of the block, if the block was at capacity.
But it is also possible that both [A] and [B, C] are included in the block (even as separate buckets, they still get cut-through at block level) if [B, C] fee/weight ratio was high.

The separate buckets only affects the ordering and the filtering, not necessarily the final cut-through for the block.

@antiochp
Copy link
Member Author

antiochp commented Apr 25, 2019

Said another way -

It is possible to bump a tx up the priority list by sending a high-fee dependent tx.
It is not possible to bump a tx down the priority list by sending a low-fee dependent tx.

Edit: So by definition -

  • buckets only ever depend on prior buckets (never later buckets)
  • buckets later in the priority list have lower fee/weight ratio than anything they depend on

i.e. Buckets never depend on buckets with lower fee/weight ratios.

So we don't need the additional depth sort key - we can simply sort by fee/weight ratio, and be confident this does not violate any tx dependencies across buckets.

Need to think this through a bit more but I'm pretty sure we can simplify the sorting logic here.

@antiochp antiochp force-pushed the bucket_txs_fee_check branch from 4422a71 to a22df73 Compare April 25, 2019 13:06
@antiochp antiochp added this to the 1.1.0 milestone Apr 25, 2019
@ignopeverell
Copy link
Contributor

Thanks a lot for the detailed explanation, all makes sense now. Very nice!

@antiochp antiochp merged commit b2b96f3 into mimblewimble:master Apr 30, 2019
@antiochp antiochp deleted the bucket_txs_fee_check branch April 30, 2019 11:14
@antiochp antiochp added the release notes To be included in release notes (of relevant milestone). label Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release notes To be included in release notes (of relevant milestone).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants