Minor cleanups for AcceptToMemoryPool #11309
Merged
Conversation
This file contains hidden or 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
Combine fLimitFree and fOverrideMempoolLimit into a single boolean: bypass_limits. This is used to indicate that mempool limiting based on feerate should be bypassed. It is used when readding transactions from a reorg and then the mempool is trimmed to size after all transactions are added and they can be evaluated in the context of their descendants. No changes to behavior.
This should have always been the case, but we will correctly trim to size after a reorg which is when bypass_limits is set.
promag
reviewed
Sep 13, 2017
|
|
||
| if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, ptx, true, &fMissingInputs, &lRemovedTxn)) { | ||
| if (!AlreadyHave(inv) && | ||
| AcceptToMemoryPool(mempool, state, ptx, &fMissingInputs, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { |
Contributor
There was a problem hiding this comment.
Nit, feeling uncomfortable with these comments 😄.
Member
There was a problem hiding this comment.
@promag Why? I think that the function calls are effectively unreadable without them (though we should change ATMP to take an options object instead of a gazillion arguments instead).
Contributor
There was a problem hiding this comment.
I think he's just joking about the existence of "bypass limits" and "absurd fee" options.
Contributor
There was a problem hiding this comment.
No joking, just what @sipa said. One struct with the arguments and fill them, resembles named arguments.
Contributor
|
utACK bf64c3c |
Contributor
|
utACK bf64c3c. I also have a branch to delete plTxnReplaced from ATMP in the future as well. |
sipa
reviewed
Sep 16, 2017
|
|
||
| if (!AlreadyHave(inv) && AcceptToMemoryPool(mempool, state, ptx, true, &fMissingInputs, &lRemovedTxn)) { | ||
| if (!AlreadyHave(inv) && | ||
| AcceptToMemoryPool(mempool, state, ptx, &fMissingInputs, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { |
Member
There was a problem hiding this comment.
@promag Why? I think that the function calls are effectively unreadable without them (though we should change ATMP to take an options object instead of a gazillion arguments instead).
Member
|
utACK bf64c3c |
Member
|
utACK bf64c3c |
maflcko
pushed a commit
that referenced
this pull request
Sep 29, 2017
bf64c3c Ignore transactions added to mempool during a reorg for fee estimation purposes. (Alex Morcos) 04f78ab Do not reject based on mempool min fee when bypass_limits is set. (Alex Morcos) fd849e1 Change AcceptToMemoryPool function signature (Alex Morcos) Pull request description: First commit just removes default arguments from `AcceptToMemoryPool` and consolidates two arguments, it does not change behavior. Second commit finally fixes the fact that we're not meant to reject based on mempool min fee when adding a transaction from a disconnected block during a reorg as mentioned [here](#9602 (comment)) Third commit makes fee estimation ignore transactions added from a disconnected block during a reorg. I think this was another source of fee estimates returning estimates below 1000 sat/kB as in #11303. Tree-SHA512: 30925ca8b341915bb214f1d2590b36b7931f2e125b7660150e38ae70338f00db5aa7f1608546dddb181446924177eb7cf62ea8bd2583068acc074d6c3f86bc0c
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Mar 16, 2020
bf64c3c Ignore transactions added to mempool during a reorg for fee estimation purposes. (Alex Morcos) 04f78ab Do not reject based on mempool min fee when bypass_limits is set. (Alex Morcos) fd849e1 Change AcceptToMemoryPool function signature (Alex Morcos) Pull request description: First commit just removes default arguments from `AcceptToMemoryPool` and consolidates two arguments, it does not change behavior. Second commit finally fixes the fact that we're not meant to reject based on mempool min fee when adding a transaction from a disconnected block during a reorg as mentioned [here](bitcoin#9602 (comment)) Third commit makes fee estimation ignore transactions added from a disconnected block during a reorg. I think this was another source of fee estimates returning estimates below 1000 sat/kB as in bitcoin#11303. Tree-SHA512: 30925ca8b341915bb214f1d2590b36b7931f2e125b7660150e38ae70338f00db5aa7f1608546dddb181446924177eb7cf62ea8bd2583068acc074d6c3f86bc0c fix 11309 Signed-off-by: Pasta <[email protected]> fix & Signed-off-by: Pasta <[email protected]>
gades
pushed a commit
to cosanta/cosanta-core
that referenced
this pull request
Jun 30, 2021
bf64c3c Ignore transactions added to mempool during a reorg for fee estimation purposes. (Alex Morcos) 04f78ab Do not reject based on mempool min fee when bypass_limits is set. (Alex Morcos) fd849e1 Change AcceptToMemoryPool function signature (Alex Morcos) Pull request description: First commit just removes default arguments from `AcceptToMemoryPool` and consolidates two arguments, it does not change behavior. Second commit finally fixes the fact that we're not meant to reject based on mempool min fee when adding a transaction from a disconnected block during a reorg as mentioned [here](bitcoin#9602 (comment)) Third commit makes fee estimation ignore transactions added from a disconnected block during a reorg. I think this was another source of fee estimates returning estimates below 1000 sat/kB as in bitcoin#11303. Tree-SHA512: 30925ca8b341915bb214f1d2590b36b7931f2e125b7660150e38ae70338f00db5aa7f1608546dddb181446924177eb7cf62ea8bd2583068acc074d6c3f86bc0c fix 11309 Signed-off-by: Pasta <[email protected]> fix & Signed-off-by: Pasta <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
9 participants
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.
First commit just removes default arguments from
AcceptToMemoryPooland consolidates two arguments, it does not change behavior.Second commit finally fixes the fact that we're not meant to reject based on mempool min fee when adding a transaction from a disconnected block during a reorg as mentioned here
Third commit makes fee estimation ignore transactions added from a disconnected block during a reorg. I think this was another source of fee estimates returning estimates below 1000 sat/kB as in #11303.