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

Calculate default mempool capacity as 2x the ledger max block size #1468

Merged
merged 1 commit into from Jan 22, 2020

Conversation

intricate
Copy link
Contributor

Closes #1467

@intricate intricate added consensus issues related to ouroboros-consensus mempool labels Jan 15, 2020
@intricate intricate self-assigned this Jan 15, 2020
@intricate intricate requested a review from edsko January 15, 2020 14:37
Comment on lines 239 to 250
mpCap <- atomically $ do
ledger <- ledgerState <$> ChainDB.getCurrentLedger chainDB
pure (mempoolCapacity ledger)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the max block size changes in the ledger? (Will this happen?) If that happens, we'll only change the capacity when restarting the node.

cc: @edsko @dcoutts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we support a dynamically changing mempool capacity? I wasn't sure if we wanted that additional complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would indeed avoid it. But basing its static size on a dynamically changing thing might be confusing (needs a comment). The main reason I'm commenting on this is to ask whether the max block size will actually change in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

The max block size will change. I presume the question is "if using the default size (no local override), is it acceptable to require a restart before picking up a change in the default block size?".

Consider what happens when syncing. We'll start with the size from the genesis and not change it. The mainnet genesis specifies a 2Mb block size, that was later reduced down to 64k (and will probably be bumped up again after the Shelley hard fork).

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to me that if operators override the max block size, they will want to keep an eye on the max block size on the chain anyway, and requiring a restart when this happens (which will be rare) doesn't seem like a huge deal.

@mrBliss
Copy link
Contributor

mrBliss commented Jan 22, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Jan 22, 2020
1468: Calculate default mempool capacity as 2x the ledger max block size r=mrBliss a=intricate

Closes #1467 

Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 22, 2020

Timed out

@edsko
Copy link
Contributor

edsko commented Jan 22, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Jan 22, 2020
1468: Calculate default mempool capacity as 2x the ledger max block size r=edsko a=intricate

Closes #1467 

Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 22, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus mempool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calculate the default mempool capacity based on the max block size from the ledger
4 participants