-
Notifications
You must be signed in to change notification settings - Fork 37.8k
Trivial: Fix warning introduced by #7053 by casting to uint64_t #7116
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
Conversation
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
This is inconsistent too:
|
I don't even really know why this parameter has to be in chainparams...Seems completely orthogonal to the chain used. |
Solved nit. |
What about this last commit? Maybe making it a configurable option or something? |
Your last commit makes it 100000 in all networks. Do we want it? |
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. |
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the removal now to get this integrated... |
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. |
I think others did not read it at all now (because of Trivial etc.). |
The |
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. |
@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. |
< * 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). |
I have kept only the most trivial things (including @MarcoFalke 's nit) and left the rest in master...jtimon:chainparams-prune-out for now. |
ACK |
utACK cb491e7 |
I agree. If that is the rationale, the RPC test could just pass in a hidden But yes let's not do it in this pull which proclaims to be a 'trivial' integer signedness fix. |
No description provided.