Skip to content

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Nov 27, 2015

No description provided.

jtimon referenced this pull request in jtimon/bitcoin Nov 27, 2015
1) Chainparams: Explicit CChainParams arg for main:

-AcceptBlock
-AcceptBlockHeader
-ActivateBestChain
-ConnectTip
-InitBlockIndex
-LoadExternalBlockFile
-VerifyDB parametric constructor

2) Also pickup more Params()\. in main.cpp

3) Pass nPruneAfterHeight explicitly to new FindFilesToPrune() in main.cpp
@laanwj
Copy link
Member

laanwj commented Nov 27, 2015

This is inconsistent too:

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

@jtimon
Copy link
Contributor Author

jtimon commented Nov 27, 2015

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

@jtimon
Copy link
Contributor Author

jtimon commented Nov 27, 2015

Solved nit.

@jtimon
Copy link
Contributor Author

jtimon commented Nov 27, 2015

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

@paveljanik
Copy link
Contributor

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

@jtimon
Copy link
Contributor Author

jtimon commented Nov 27, 2015

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
Copy link
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).

@@ -54,6 +54,7 @@ using namespace std;

CCriticalSection cs_main;

uint64_t nGlobalPruneAfterHeight = 100000;
Copy link
Member

Choose a reason for hiding this comment

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

@paveljanik
Copy link
Contributor

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

@jtimon
Copy link
Contributor Author

jtimon commented Nov 29, 2015

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
Copy link
Contributor

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

@sdaftuar
Copy link
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.

@maflcko
Copy link
Member

maflcko commented Nov 29, 2015

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
Copy link
Contributor Author

jtimon commented Nov 29, 2015

@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.

@maflcko
Copy link
Member

maflcko commented Nov 29, 2015

<  * 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).

@jtimon
Copy link
Contributor Author

jtimon commented Nov 29, 2015

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

@paveljanik
Copy link
Contributor

ACK

@maflcko
Copy link
Member

maflcko commented Nov 29, 2015

utACK cb491e7

@laanwj
Copy link
Member

laanwj commented Nov 30, 2015

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 laanwj merged commit cb491e7 into bitcoin:master Nov 30, 2015
laanwj added a commit that referenced this pull request Nov 30, 2015
cb491e7 Trivial: Fix warning introduced by #7053 by casting to uint64_t (Jorge Timón)
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants