-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Change ZIP 401 mempool limiting to use conventional fee and new constants #6564
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
daira
added
I-dos
Problems and improvements with respect to Denial-of-Service.
F-tx-fees
Feature: Transaction fees
A-mempool
Area: Mempool
labels
Apr 16, 2023
nuttycom
previously approved these changes
Apr 16, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 060a4673ae167f98ed62f3c8a5b17f728d90aa6d
…s#565. fixes zcash#6518 In a ZIP sync meeting we decided that: * The minimum cost should be changed to 10000, in order to avoid penalizing Orchard-using transactions too much relative to other transactions. * `low_fee_penalty` should be changed to 40000. This preserves the property that a transaction paying less than the ZIP 317 conventional fee is deprioritized relative to a min-cost, conventional-fee transaction by a factor of 5, as in the original design. * The recommended default for `mempooltxcostlimit` should remain at 80000000. Rationale: 80000000 was chosen so that the worst-case size of the mempool would be equal to the worst-case size of 40 blocks, which is the current default transaction expiry delta. That reasoning still holds even with the above changes. * `eviction_memory_entries` remains at 40000. It could have been lowered given that there will now be at most 80000000/10000 = 8000 transactions "in-flight", but it doesn't need to be because the rationale that "40000 [RecentlyEvicted queue] entries can be stored in ~1.6 MB, which is small compared to other node memory usage" still holds. Signed-off-by: Daira Emma Hopwood <daira@jacaranda.org>
str4d
approved these changes
Apr 17, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Apply the ZIP 401
low_fee_penalty
to transactions paying below the ZIP 317 conventional fee, rather than to transactions paying below the previous ZIP 313 default fee of 1000 zatoshis.Change ZIP 401 mempool limiting to use constants decided in ZIP 401: Increase the minimum eviction cost to avoid penalizing Orchard zips#565
In a ZIP sync meeting with @daira, @arya2, @teor2345, and @sellout (@nuttycom was also present but had to leave before these decisions were made), we decided that:
low_fee_penalty
should be changed to 40000. This preserves the property that a transaction paying less than the ZIP 317 conventional fee is deprioritized relative to a min-cost, conventional-fee transaction by a factor of 5, as in the original design.mempooltxcostlimit
should remain at 80000000. Rationale: 80000000 was chosen so that the worst-case size of the mempool would be equal to the worst-case size of 40 blocks, which is the current default transaction expiry delta. That reasoning still holds even with the above changes.eviction_memory_entries
remains at 40000. It could have been lowered given that there will now be at most 80000000/10000 = 8000 transactions "in-flight", but it doesn't need to be because the rationale that "40000 [RecentlyEvicted queue] entries can be stored in ~1.6 MB, which is small compared to other node memory usage" still holds.fixes #6518