Skip to content

Conversation

morcos
Copy link
Contributor

@morcos morcos commented Feb 16, 2017

@gmaxwell, this is meant to address #9761 (comment)

Should probably go in 0.14 with the rest of the grace period / importmulti changes

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

utACK 051fc42521dd5417f34ed27a727ad045a9823764

I think it should probably not be hard to add a test for this in:

def manual_test(self, node_number, use_timestamp):

Though pruning.py is kind of a pain to run.

@@ -820,7 +820,8 @@ UniValue pruneblockchain(const JSONRPCRequest& request)
throw runtime_error(
"pruneblockchain\n"
"\nArguments:\n"
"1. \"height\" (numeric, required) The block height to prune up to. May be set to a discrete height, or to a unix timestamp to prune based on block time.\n"
"1. \"height\" (numeric, required) The block height to prune up to. May be set to a discrete height, or a unix timestamp\n"
" to prune based on block time. Timestamp based pruning automatically adds a 2 hour buffer.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd maybe make the comment more literal and say timestamp pruning will only prune blocks that are at least 2 hours older than the provided timestamp (instead of that timestamp pruning automatically adds a 2 hour buffer).

@fanquake fanquake added this to the 0.14.0 milestone Feb 16, 2017
Copy link
Contributor

@jonasschnelli jonasschnelli left a comment

Choose a reason for hiding this comment

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

code review utACK 051fc42521dd5417f34ed27a727ad045a9823764

@jtimon
Copy link
Contributor

jtimon commented Feb 16, 2017

Maybe 7200 could be made a constant to be reused in #9761 ? (and the normal rescan?)
Also agree with @ryanofsky 's nit, and, yes, a test would be nice (although, yeah, prunning.py is a pita).
utACK 051fc42

@morcos
Copy link
Contributor Author

morcos commented Feb 16, 2017

ok slightly modified the help text
otherwise unchanged from ACK'ed commit

The 7200 number was added simultaneously in 3 separate PR's, but I agree it should be cleaned up to a constant in a later PR.

@morcos
Copy link
Contributor Author

morcos commented Feb 16, 2017

Actually my personal preference would have been to leave the details of the exact buffering mechanism used off the help text. It seems too dependent on the exact implementation choice, and there is nothing magical about the 2 hour number. But at this point I think we are bike shedding.

@gmaxwell
Copy link
Contributor

utACK you could also leave it out of the description as far as I care, it's an implementation detail.

@laanwj
Copy link
Member

laanwj commented Feb 17, 2017

I think it's fine to mention it in the description. The two hours could be important in edge cases e.g. determining what blocks to keep with manual pruning.

@laanwj
Copy link
Member

laanwj commented Feb 17, 2017

utACK 91fb506

@laanwj laanwj merged commit 91fb506 into bitcoin:master Feb 17, 2017
laanwj added a commit that referenced this pull request Feb 17, 2017
91fb506 Add two hour buffer to manual pruning (Alex Morcos)
@morcos
Copy link
Contributor Author

morcos commented Feb 21, 2017

This caused pruning.py to break.
@ryanofsky or I will fix the test.

codablock pushed a commit to codablock/dash that referenced this pull request Jan 23, 2018
91fb506 Add two hour buffer to manual pruning (Alex Morcos)
andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019
91fb506 Add two hour buffer to manual pruning (Alex Morcos)
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019
91fb506 Add two hour buffer to manual pruning (Alex Morcos)
@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.

7 participants