Skip to content

Make -checkmempool=1 not fail through int32 overflow #6896

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

Merged
merged 1 commit into from
Oct 30, 2015

Conversation

sipa
Copy link
Member

@sipa sipa commented Oct 28, 2015

Fix a bug in #6776 discovered by @gmaxwell: -checkmempool=1 causes the internal 32-bit variable to overflow to zero.

@@ -360,7 +360,7 @@ class CTxMemPool
* check does nothing.
*/
void check(const CCoinsViewCache *pcoins) const;
void setSanityCheck(double dFrequency = 1.0) { nCheckFrequency = dFrequency * 4294967296.0; }
void setSanityCheck(double dFrequency = 1.0) { nCheckFrequency = dFrequency * 4294967295.0; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to cast explicitly?
std::static_cast<int>(dFrequency * 4294967295.0)?

IMHO it might have made this more obvious to reviewers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree w/ @dcousens

Additionally, raw big numbers with no explanation comment should be avoided. I know it's 2^32 but it's not immediately obvious to every code reader.

This should be uint_max-1 and getdouble() should be similar.

Copy link

Choose a reason for hiding this comment

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

@jgarzik Agreed

@dcousens
Copy link
Contributor

ACK

@laanwj
Copy link
Member

laanwj commented Oct 28, 2015

Oops.
Probably needs a range check, >1 and <0 will still silently overflow.

@dcousens
Copy link
Contributor

@laanwj I think the issue was that it would overflow to 0.
At least that was my understanding.

In any case, a range check would be sane.
Or just min(max(dFrequency, 0), 1)

@laanwj
Copy link
Member

laanwj commented Oct 28, 2015

Isn't any overflow an issue?

@instagibbs
Copy link
Member

Right now it doesn't appear overflow as currently used(post-fix), but probably a good idea to internally check.

@laanwj
Copy link
Member

laanwj commented Oct 30, 2015

Apparently a range clamp is already done in init.cpp. That's good enough, going to merge this.

@laanwj laanwj merged commit e9e6163 into bitcoin:master Oct 30, 2015
laanwj added a commit that referenced this pull request Oct 30, 2015
e9e6163 Make -checkmempool=1 not fail through int32 overflow (Pieter Wuille)
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Jun 14, 2020
2105947 Implement helper class for CTxMemPoolEntry constructor (Alex Morcos)
1cef905 Make -checkmempool=1 not fail through int32 overflow (Pieter Wuille)
0f72ff2 Support -checkmempool=N, which runs checks on average once every N transactions (Pieter Wuille)
89483d0 [Bug] Make operator() a const function in CompareTxMemPoolEntryByX (random-zebra)
a50ad77 Lower default policy limits (random-zebra)
03f7152 fix locking issue with new mempool limiting (random-zebra)
1598961 Fix stale comment in CTxMemPool::TrimToSize. (random-zebra)
98d0d68 Undo GetMinFee-requires-extra-call-to-hit-0 (random-zebra)
6ad6ee6 Add reasonable test case for mempool trimming (random-zebra)
8dcbb7e Only call TrimToSize once per reorg/blocks disconnect (random-zebra)
c20cd38 Implement on-the-fly mempool size limitation. (random-zebra)
aee2e17 Print mempool size in KB when adding txn (random-zebra)
f7c85fd Add CFeeRate += operator (random-zebra)
5bd2a00 Track (and define) ::minRelayTxFee in CTxMemPool (random-zebra)
0b50f6c Add Mempool Expire function to remove old transactions (random-zebra)
d26f5e0 Fix calling mempool directly, instead of pool, in ATMP (random-zebra)
fc5eddb Reverse the sort on the mempool's feerate index (random-zebra)
0ce1df0 [BUG] Fix CTxMemPool::check excluding zerocoins from children checks (random-zebra)
1f7bd52 Track transaction packages in CTxMemPoolEntry (random-zebra)
1fd406b TxMemPool: Change mapTx to a boost::multi_index_container (random-zebra)

Pull request description:

  built on top of
  - [x] #1645

  This PR pulls some updates from upstream in the mempool area, adding the required adjustments for legacy zerocoin txes and updating the functional test suite.

  Specifically, here we:
  - track mempool descendants (in-mempool transactions that depend on other mempool transactions)
  - turn `mapTx` into a `boost::multi_index_container` that sorts the mempool on 3 criteria:
    - transaction hash
    - fee rate
    - time in the mempool
  - Add a max size for the mempool (throwing away the cheapest txs and bumping the min relay fee, when full)
  - Implement on-the-fly mempool size limit with the flag `-maxmempool`
  - Implement `-checkmempool=N` to customize the frequency of the mempool check
  - Implement helper for `CTxMemPoolEntry` for the unit tests.

  Backports:

  - bitcoin#6654
  - bitcoin#6722 [`*`]
  - bitcoin#6889
  - bitcoin#6771
  - bitcoin#6776
  - bitcoin#6896
  - bitcoin#7020

  [`*`] excluding bitcoin@9e93640 as our default minimum tx fee rate of 10k satoshis is only 0,00003 USD at the time of writing.

ACKs for top commit:
  Fuzzbawls:
    utACK 2105947
  furszy:
    Re utACK 2105947 and merging this nice upgrade :) .

Tree-SHA512: 51a7d75bd52f7646d461252c78f0dd9d7e8b5c1c66c22944120bfe293b28f5d48135de339ebf3d8a5b4c61ca5452383ed1b10c417be06dc4a335ac645842ea14
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants