Policy: Refactor: inline main:GetMinRelayFee() in main:AcceptToMemoryPool() #5709
Conversation
jtimon
force-pushed
the
getminrelayfee
branch
from
5c84ca8 to
767358a
Compare
|
Needed rebase |
jtimon
force-pushed
the
getminrelayfee
branch
from
767358a to
f03b16a
Compare
jtimon
changed the title
|
@luke-jr you once said you would like GetMinRelayFee to become part of policy, I would prefer to destroy it (and then pick it up as part of a new CPolicy method) instead. |
|
I bet it will take very long until I have to rebase this because the refactored code is way too ugly for anyone to want to touch it. Hopefully by the time this gets any attention, +47 −67 while 0-functional-changes will be enough of an argument in favor of this single-commit refactor (I know, I have to s/SQUASHME:/fixup! but as said that may never happen because this may never need rebase). I would love to hear @morcos 's opinion. |
There was a problem hiding this comment.
Probably "insufficient fee" should be replaced with "bad-txns-fee-negative" to better keep track of the validation duplication.
jtimon
changed the title
|
I took a first pass at reviewing this. I think if you're going to do a refactor here, you should improve the readability of the code. Although I like this code better than the original in some ways, it feels like you over optimized it instead of clarifying it. In any case, I don't feel strongly, if you get some concept ACK's, I'm happy to test it for you. I don't think your second commit is right though, why do you think minRelayTxFee is in MoneyRange already? |
|
Well, I was hoping that by doing the stuff change by change it could become obvious for reviewers that this doesn't change anything functionally, but thanks for the testing offer (probably better to do it after some more review like you say).
You are right, I thought, ParseMoney had a moneyRange check. I should add one there or in https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L817 The other place where minRelayTxFee is initialized is in https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L67 I will fix that commit. Re optimizing: that's just a nice side effect. The main goal is to reduce the amount of code before moving it to policy. |
|
Updated fixing the second commit. Also reword "SQUASHME:" with "fixup!" in the commit descriptions. |
… sooner than that
|
Rebased |
3 participants
main::GetMinRelayFee() is only called from main::AcceptToMemoryPool().
By in-lining in we can make some code simplifications and test that no new money is being created before calling main::AreInputsStandard(), instead of doing it in https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1032 after all those free transaction special cases.
Even removing the
nFees < 0check would be safe since it is repeated in https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1073 via https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1471But moving it up seems more DoS-safe.
I've done the refactor step by step to to help with review.