Trivial: Fix warning introduced by #7053 by casting to uint64

jtimon commented at 2:12 PM on November 27, 2015: contributor

No description provided.

jtimon referenced this in commit 2e29e7e247 on Nov 27, 2015

laanwj commented at 2:16 PM on November 27, 2015: member

This is inconsistent too:

src/chainparams.h:    int64_t PruneAfterHeight() const { return nPruneAfterHeight; }
src/chainparams.h:    uint64_t nPruneAfterHeight;

laanwj added the label Refactoring on Nov 27, 2015

jtimon commented at 2:17 PM on November 27, 2015: contributor

I don't even really know why this parameter has to be in chainparams...Seems completely orthogonal to the chain used.

jtimon force-pushed on Nov 27, 2015

jtimon commented at 2:22 PM on November 27, 2015: contributor

Solved nit.

jtimon commented at 2:38 PM on November 27, 2015: contributor

What about this last commit? Maybe making it a configurable option or something?

paveljanik commented at 4:18 PM on November 27, 2015: contributor

Your last commit makes it 100000 in all networks. Do we want it?

jtimon commented at 5:36 PM on November 27, 2015: contributor

Do we don't? As said another option is making it configurable, but it really doesn't make sense to me that this is a per-chain thing.

paveljanik commented at 6:35 PM on November 27, 2015: contributor

I think making it 100000 for all networks is OK.

ACK (warning is gone, nGlobalPruneAfterHeight can be made configurable in the future if needed).

in src/main.cpp:None in 7992d97fa4 outdated

  53 | @@ -54,6 +54,7 @@ using namespace std;
  54 |  
  55 |  CCriticalSection cs_main;
  56 |  
  57 | +uint64_t nGlobalPruneAfterHeight = 100000;

MarcoFalke commented at 6:31 PM on November 28, 2015:

Please fix doc of https://github.com/jtimon/bitcoin/blob/7992d97fa4e92351f9f4fa155384ea2181b28f56/src/main.h#L218

paveljanik commented at 9:57 AM on November 29, 2015: contributor

I'd remove the removal now to get this integrated...

jtimon commented at 10:46 AM on November 29, 2015: contributor

Yes, I can leave it at the first commit for now and leave the second for another PR, but I was curious to see how much code it would be and what people would think about it.

paveljanik commented at 10:53 AM on November 29, 2015: contributor

I think others did not read it at all now (because of Trivial etc.).

sdaftuar commented at 11:14 AM on November 29, 2015: member

The nPruneAfterHeight is different for each chain so that the pruning code can be tested. In particular, changing the regtest value to be 100000 will break the pruning.py rpc test.

MarcoFalke commented at 11:19 AM on November 29, 2015: member

When restoring the initial commit, please still fix the doc of https://github.com/jtimon/bitcoin/blob/7992d97fa4e92351f9f4fa155384ea2181b28f56/src/main.h#L218 which says "10" for regtest.

jtimon commented at 11:27 AM on November 29, 2015: contributor

@MarcoFalke I will correct the doc, if you have a complete text replacement I can use that. @sdaftuar Thanks for the clarification but it doesn't seem like the only possible way to test something (ie there's many other things that are testable and configurable without being in CChainParams. I mean, if the RPC need a lower value I guess it's simpler to make it directly configurable than to adapt the tests.

MarcoFalke commented at 11:33 AM on November 29, 2015: member

<  * Pruning cannot take place until the longest chain is at least a certain length (100000 on mainnet, 1000 on testnet, 10 on regtest).
---
>  * Pruning cannot take place until the longest chain is at least a certain length (100000 on mainnet, 1000 on testnet, 1000 on regtest).

Trivial: Fix warning introduced by #7053 by casting to uint64_t cb491e7788

jtimon force-pushed on Nov 29, 2015

jtimon commented at 8:21 PM on November 29, 2015: contributor

I have kept only the most trivial things (including @MarcoFalke 's nit) and left the rest in https://github.com/bitcoin/bitcoin/compare/master...jtimon:chainparams-prune-out for now.

paveljanik commented at 8:33 PM on November 29, 2015: contributor

ACK

MarcoFalke commented at 9:45 PM on November 29, 2015: member

utACK cb491e7

laanwj commented at 9:17 AM on November 30, 2015: member

I mean, if the RPC need a lower value I guess it's simpler to make it directly configurable than to adapt the tests.

I agree. If that is the rationale, the RPC test could just pass in a hidden -pruneafterheight option, that seems to be better than making it dependent on the chain.

But yes let's not do it in this pull which proclaims to be a 'trivial' integer signedness fix.

laanwj merged this on Nov 30, 2015

laanwj closed this on Nov 30, 2015

laanwj referenced this in commit 74b5ce24c6 on Nov 30, 2015

MarcoFalke locked this on Sep 8, 2021

Trivial: Fix warning introduced by #7053 by casting to uint64_t #7116