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

Rework "bucket transactions" logic (buckets are now weight limited) #2487

Merged
merged 8 commits into from
Feb 1, 2019

Conversation

antiochp
Copy link
Member

Resolves #2456.

This PR fixes 2 issues present in our current prepare_mineable_transactions implementation.
Specifically around how we "bucket" dependent transactions together.

  1. It is currently possible to produce a bucket containing two transactions that together exceed MAX_BLOCK_WEIGHT. (See Transaction aggregation in bucket_transactions is not weight aware #2456).
  2. If a tx depends on two txs in two different buckets it is added to the last dependent bucket. The subsequent sorting of buckets and filtering based on weight can produce an invalid ordering of buckets and an invalid set of final txs (if one bucket is included and one excluded).

Proposed Implementation -

  • bucket_transactions now aggregates buckets into aggregated txs (each bucket is a single aggregated tx)
  • these aggregated txs are "weight aware" and we only aggregate txs if this results ina valid aggregated tx
  • if a tx depends on multiple buckets we simply reject it (and let the subsequent block pick it up)
    • The alternative here would be to introduce logic to "merge" multiple existing buckets but this is more complex and covers a rare edge-case. It is simpler to reject the tx and include it in the next block.

@antiochp
Copy link
Member Author

I'm planning to spend some time adding some test coverage for this.
Making MAX_BLOCK_WEIGHT configurable via chain_type would allow us to do this with a manageable volume of test data...
Likely in a separate PR.

@antiochp antiochp force-pushed the tx_bucket_rework branch 2 times, most recently from f6c8642 to cdc7cd8 Compare January 30, 2019 16:34
@antiochp
Copy link
Member Author

@ignopeverell Reworked this a bit more.

Added a new Weighting::AsLimitedTransaction because miners can mine artificially small blocks based on their mineable_max_weight config.

Also added some test coverage for mining blocks at the max_block_weight limit from the txpool.

So "preparing mineable transactions" from the txpool now takes the miners max block weight config into account when bucketing and aggregating the txs.

This PR got bigger than I originally anticipated but there's no clear way of splitting this up in a meaningful way.

Thoughts?

@ignopeverell
Copy link
Contributor

Still looks good to me but looks like the new test is failing?

@antiochp
Copy link
Member Author

antiochp commented Jan 31, 2019

Some tests in the wallet cli are failing - I'm guessing its to do with the lower block weight limit in the test chain_type.

Let me try and get this fixed up before the weekend.
We had a test that was splitting change into 75 outputs and this is vastly exceeding the test block weight limit.

@antiochp
Copy link
Member Author

antiochp commented Feb 1, 2019

Final testing on usernet and things look good.
Going to merge.

@antiochp antiochp merged commit a82e2a0 into mimblewimble:master Feb 1, 2019
@antiochp antiochp deleted the tx_bucket_rework branch February 1, 2019 10:44
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.

Transaction aggregation in bucket_transactions is not weight aware
2 participants