Skip to content

Fix a bug if the min fee is 0 for FeeFilterRounder#9288

Merged
laanwj merged 2 commits intobitcoin:masterfrom
morcos:fixFFRbug
Jan 4, 2017
Merged

Fix a bug if the min fee is 0 for FeeFilterRounder#9288
laanwj merged 2 commits intobitcoin:masterfrom
morcos:fixFFRbug

Conversation

@morcos
Copy link
Contributor

@morcos morcos commented Dec 6, 2016

Thanks @dcousens and @gmaxwell

@gmaxwell
Copy link
Contributor

gmaxwell commented Dec 6, 2016

ACK. This appears sensible and also fixes the bug.

@dcousens
Copy link
Contributor

dcousens commented Dec 6, 2016 •
edited
Loading

ACK 0e78469, does it need to be put in place at https://github.com/bitcoin/bitcoin/blob/master/src/policy/fees.cpp#L304 also?

Probably not, since that is bounded by MIN_FEERATE...

Actually, probably is worth it since if someone changed MIN_FEERATE to 0, it would dead lock again.

@laanwj
Copy link
Member

laanwj commented Dec 6, 2016

utACK 0e78469

@maflcko
Copy link
Member

maflcko commented Dec 6, 2016

if someone changed MIN_FEERATE to 0

I think we can't protect against developers randomly changing internal constants.

utACK 0e78469

@sipa
Copy link
Member

sipa commented Dec 6, 2016

@dcousens One way to deal with such constraints that some parts of the code have, is by adding a static_assert(MIN_FEERATE > 0, "Min feerate must be nonzero); in the place where it is used.

@dcousens
Copy link
Contributor

dcousens commented Dec 6, 2016 •
edited
Loading

@sipa sounds fine to me

@laanwj
Copy link
Member

laanwj commented Dec 8, 2016

Yes, static assertions would be the way to go then.

@laanwj laanwj mentioned this pull request Dec 8, 2016
Merged
@morcos
Copy link
Contributor Author

morcos commented Dec 8, 2016

I added the static_assert, although I'm not sure it matters since the value used is actually max of MIN_FEERATE and minRelayTxFee which can't be 0. But it doesn't hurt.

I noticed another "bug" if a minRelayTxFee of 1 satoshi is used in the mempool min fee decay.
Wouldn't mind someone else verifying the fix makes sense though..

@gmaxwell
Copy link
Contributor

gmaxwell commented Dec 12, 2016

re-ACK.

@laanwj laanwj mentioned this pull request Dec 14, 2016
Closed
@laanwj
Copy link
Member

laanwj commented Jan 4, 2017

utACK f8d43b8

@laanwj laanwj merged commit f8d43b8 into bitcoin:master Jan 4, 2017
laanwj added a commit that referenced this pull request Jan 4, 2017
@laanwj
Merge #9288: Fix a bug if the min fee is 0 for FeeFilterRounder
1ce7ede 
f8d43b8 Avoid rollingMinimumFeeRate never being able to decay below half (Alex Morcos)
eab8e1b fix a bug if the min fee is 0 for FeeFilterRounder (Alex Morcos)
codablock pushed a commit to codablock/dash that referenced this pull request Jan 18, 2018
@laanwj @codablock
Merge bitcoin#9288: Fix a bug if the min fee is 0 for FeeFilterRounder
bdd151f 
f8d43b8 Avoid rollingMinimumFeeRate never being able to decay below half (Alex Morcos)
eab8e1b fix a bug if the min fee is 0 for FeeFilterRounder (Alex Morcos)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
@laanwj @andvgal
Merge bitcoin#9288: Fix a bug if the min fee is 0 for FeeFilterRounder
eb7947e 
f8d43b8 Avoid rollingMinimumFeeRate never being able to decay below half (Alex Morcos)
eab8e1b fix a bug if the min fee is 0 for FeeFilterRounder (Alex Morcos)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 26, 2019
@laanwj @CryptoCentric
Merge bitcoin#9288: Fix a bug if the min fee is 0 for FeeFilterRounder
502d1ca 
f8d43b8 Avoid rollingMinimumFeeRate never being able to decay below half (Alex Morcos)
eab8e1b fix a bug if the min fee is 0 for FeeFilterRounder (Alex Morcos)
@random-zebra random-zebra mentioned this pull request Feb 5, 2021
Merged
Fuzzbawls added a commit to PIVX-Project/PIVX that referenced this pull request Feb 14, 2021
@Fuzzbawls
Merge #2189: [Bug][Refactoring] Miscellaneous fixes
71ed429 
84b4185 [Tests] raise minimum fee in feature_blockindexstats (random-zebra)
39d8a20 [Cleanup] Remove OldSetKeyFromPassphrase/OldEncrypt/OldDecrypt (random-zebra)
613e1da [BUG] Fix sizeof in paymentservertests (random-zebra)
0f3e961 [Cleanup] Remove useless call (random-zebra)
b1ca5e0 Remove unused fsbridge::freopen (practicalswift)
b3a1d84 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)
ecfbcfd [Cleanup] Remove unused function AddInvalidSpendsToMap (random-zebra)
15b018c [Refactor] Use InsecureRand in the unit tests (random-zebra)
81fd84c [Refactoring] Replace risky call to rand() with GetRandInt() (random-zebra)
8ba1978 [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)
b6cd719 Remove unreachable code (g_rpcSignals.PostCommand) (practicalswift)
e4f9ab3 [BUG] Memory leak after new CNode / ConnectNode (random-zebra)
536122b Avoid rollingMinimumFeeRate never being able to decay below half (Alex Morcos)
13cad19 fix a bug if the min fee is 0 for FeeFilterRounder (Alex Morcos)
aa832d8 [Refactoring] Dereference before/after null check (random-zebra)
4630b7d [Trivial] Pass big parameters by reference, not value (random-zebra)
aa7bc7f [Refactoring] Remove unneeded CScript::IsNormalPaymentScript (random-zebra)
6cf3c8f [Trivial] Unitialized scalar fields and variables (random-zebra)

Pull request description:

  This is a collection of small fixes for the following issues/defects (discovered with the coverity tool):

  - Big parameters passed by value
  - calls to rand() function
  - Dereference before/after null-pointer check
  - Pointer to local variable out of scope
  - Resource leak with call to ConnectNode
  - Non-floating point result - unintended integer division (bitcoin#9288)
  - String not-null-terminated (bitcoin#11193)
  - Structurally dead code (bitcoin#9575)
  - Unchecked boolean return value of functions (GetOp, GetTransaction, TxOutToPublicCoin)
  - Unitialized pointers and scalar fields
  - Illegal memory access (bitcoin#13159)
  - Useless call to pubkey.IsCompressed()
  - Wrong `sizeof` argument (bitcoin#11024 + revert #494)

ACKs for top commit:
  furszy:
    Looking good, ACK 84b4185
  Fuzzbawls:
    ACK 84b4185

Tree-SHA512: 4c920a1858ccde65795c090e4becc47c50c0a78db7ab11935b4d3e2bea3f7f1f8ca3b48ce633d8112673092c35ae7cd05c444ed2b16e283a305c765874e55c1c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

TX fees and policy

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@morcos @gmaxwell @dcousens @laanwj @maflcko @sipa @fanquake