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

rpc: Don't FlushStateToDisk when pruneblockchain(0) #9524

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@MarcoFalke
Member

MarcoFalke commented Jan 12, 2017

Currently pruneblockchain(0) will call StateFlushToDisk. This might not be required/wanted, as the internal code appears to be using 0 to indicate "legacy" pruning.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 12, 2017

Member

Currently pruneblockchain(0) will call StateFlushToDisk. This might not be required/wanted.

If it doesn't cause an error or corruption, I'd prefer not to add a special case for it. There is no reason for calling it with 0 so if there is a slight performance overhead when that's done, that's too bad.

Member

laanwj commented Jan 12, 2017

Currently pruneblockchain(0) will call StateFlushToDisk. This might not be required/wanted.

If it doesn't cause an error or corruption, I'd prefer not to add a special case for it. There is no reason for calling it with 0 so if there is a slight performance overhead when that's done, that's too bad.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 12, 2017

Member

The thing is that it clashes with the default value of FlushStateToDisk(nManualPruneHeight=0), which is used to indicate "legacy" pruning. It might cause unwanted bugs if someone refactors the code in the future.

Member

MarcoFalke commented Jan 12, 2017

The thing is that it clashes with the default value of FlushStateToDisk(nManualPruneHeight=0), which is used to indicate "legacy" pruning. It might cause unwanted bugs if someone refactors the code in the future.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 12, 2017

Member

Okay, true. On one hand this is adding belt and suspenders it's just that I prefer the underlying code to be robust to cases like this instead of catching potential errors on the RPC interface. But yes you have a point too, I don't know...

Member

laanwj commented Jan 12, 2017

Okay, true. On one hand this is adding belt and suspenders it's just that I prefer the underlying code to be robust to cases like this instead of catching potential errors on the RPC interface. But yes you have a point too, I don't know...

@MarcoFalke MarcoFalke changed the title from qa/rpc: Fix pruneblockchain edge cases to rpc: Don't FlushStatToDisk when pruneblockchain(0) Jan 27, 2017

@MarcoFalke MarcoFalke changed the title from rpc: Don't FlushStatToDisk when pruneblockchain(0) to rpc: Don't FlushStateToDisk when pruneblockchain(0) Jan 28, 2017

@MarcoFalke MarcoFalke removed the Tests label Jan 28, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 17, 2017

Member

The thing is that it clashes with the default value of FlushStateToDisk(nManualPruneHeight=0), which is used to indicate "legacy" pruning. It might cause unwanted bugs if someone refactors the code in the future.

It seems that in that case there will also be automatic problems. On IRC yesterday there was a discussion about this, and there are more paths through which it will get into FlushStateToDisk with nManualPruneHeight=0 when manual pruning is enabled. This change would cover only one of them.

I wrongly assumed that fCheckForPruning would always be false when pruning manual. But it can be set, in which case FlushStateToDisk will be called from block processing.

For 0.15 or later it would be nice to refactor the pruning code. fCheckForPruning would be better as an argument of the appropriate functions instead of a global flag (to have a better grasp on when it's set and not set), and the pruning mode OFF|MANUAL|AUTO would be better represented as an enum.

Member

laanwj commented Feb 17, 2017

The thing is that it clashes with the default value of FlushStateToDisk(nManualPruneHeight=0), which is used to indicate "legacy" pruning. It might cause unwanted bugs if someone refactors the code in the future.

It seems that in that case there will also be automatic problems. On IRC yesterday there was a discussion about this, and there are more paths through which it will get into FlushStateToDisk with nManualPruneHeight=0 when manual pruning is enabled. This change would cover only one of them.

I wrongly assumed that fCheckForPruning would always be false when pruning manual. But it can be set, in which case FlushStateToDisk will be called from block processing.

For 0.15 or later it would be nice to refactor the pruning code. fCheckForPruning would be better as an argument of the appropriate functions instead of a global flag (to have a better grasp on when it's set and not set), and the pruning mode OFF|MANUAL|AUTO would be better represented as an enum.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 19, 2017

Member

Ok, I still stand by what I said above, the code in question should be refactored as there are various paths where this could theoretically go wrong, catching this at the RPC call site will only solve one entry point, and for the right might hide eventual problems further.

E.g. let's say we added a test that calls pruneblockchain(0) and verifies that it does nothing would no longer test anything useful.

Member

laanwj commented Apr 19, 2017

Ok, I still stand by what I said above, the code in question should be refactored as there are various paths where this could theoretically go wrong, catching this at the RPC call site will only solve one entry point, and for the right might hide eventual problems further.

E.g. let's say we added a test that calls pruneblockchain(0) and verifies that it does nothing would no longer test anything useful.

@laanwj laanwj closed this Apr 19, 2017

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1701-qaPruning branch Apr 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment