Remove block file location upgrade code

benma commented at 6:24 am on February 22, 2017: none

An effort to reduce the size of AppInitMain().

The removed code upgrades the location of the block files when upgrading to 0.8. 0.8 seems to be the oldest version still in use.

dcousens commented at 6:35 am on February 22, 2017: contributor

concept ACK

benma commented at 6:50 am on February 22, 2017: none

Related: could we just also remove that part? The comment says Upgrading to 0.8, and according to https://bitnodes.21.co/nodes/, there is no Core node running older than that.

sipa commented at 7:06 am on February 22, 2017: member

@benma I believe it may be impossible or at least very nontrivial to get a 0.7 node to sync at this point, so i’d be in favor of removing that piece of logic.

benma force-pushed on Feb 22, 2017

benma renamed this:
~~Refactor: Split part of AppInitMain() into a new function~~
Remove block file location upgrade code
on Feb 22, 2017

benma commented at 7:16 am on February 22, 2017: none

Thanks @sipa. Pushed a new commit that removes the code.

laanwj commented at 7:56 am on February 22, 2017: member

Yes, let’s drop this for 0.15. utACK https://github.com/bitcoin/bitcoin/pull/9822/commits/45d7ee25f1bfaff7d65a248116e401a4a13d0608

laanwj added the label Block storage on Feb 22, 2017

benma force-pushed on Feb 22, 2017

MarcoFalke commented at 8:48 am on February 22, 2017: member

utACK fb678229429364f9a0a93add93e21fe444ae0fcb

luke-jr approved

luke-jr commented at 8:51 am on February 22, 2017: member

Wow, I forgot they were even somewhere else before! utACK

in src/init.cpp: in fb67822942 outdated

1378-        {
1379-            fReindex = true;
1380-        }
1381-    }
1382-
1383+    boost::filesystem::create_directories(GetDataDir() / "blocks");

benma commented at 8:52 am on February 22, 2017:

I’ll push a commit soon that removes this line again, which creates the directory in CDBWrapper where it belongs.

benma commented at 9:21 am on February 22, 2017:

Done.

dcousens approved

laanwj commented at 10:47 am on February 22, 2017: member

I think an unrelated commit snuck into here accidentally: https://github.com/bitcoin/bitcoin/pull/9822/commits/4cce79cc137d27900e1ceebe8e9bfc5d1befb302

Remove block file location upgrade code

An effort to reduce the size of AppInitMain().

The removed code upgrades the location of the block files when
upgrading to 0.8. 0.8 seems to be the oldest version still in use.

4b183d33f3

benma force-pushed on Feb 22, 2017

benma commented at 11:35 am on February 22, 2017: none

It was not unrelated, I needed it to cleanly remove the full block and not leave an

boost::filesystem::create_directories(GetDataDir() / "blocks");

In any case, I removed the second commit and will make a new PR for that :)

dcousens approved

benma commented at 1:48 pm on February 22, 2017: none

I am a bit confused about the CI failure. Locally, test/test_bitcoin passes, and I am almost sure that the same version (but a previous commit) passed CI tests before. Might there be a problem with the CI?

jnewbery commented at 2:08 pm on February 22, 2017: member

We should include an upgrade compatibility note in the V0.15 release notes. Something like:

Upgrade to V0.15 is supported from V0.8 and higher. To upgrade from V0.7 and lower, you must first upgrade to V0.14, and then upgrade to V0.15.

MarcoFalke commented at 2:27 pm on February 22, 2017: member

To upgrade from V0.7 and lower, you must first upgrade to V0.14, and then upgrade to V0.15.

This is not required. Doing so has the only advantage that you don’t have to download the blockchain again and that there are no duplicate block files in the datadir after upgrade.

MarcoFalke commented at 2:28 pm on February 22, 2017: member

same version (but a previous commit) passed CI tests

If the same commit passed review and travis, you might want to force push that instead, to not invalidate the results.

On Wed, Feb 22, 2017 at 3:27 PM, Marco Falke marco.falke@tum.de wrote:

To upgrade from V0.7 and lower, you must first upgrade to V0.14, and then upgrade to V0.15.

This is not required. Doing so has the only advantage that you don’t have to download the blockchain again and that there are no duplicate block files in the datadir after upgrade.

jnewbery commented at 2:35 pm on February 22, 2017: member

Doing so has the only advantage that you don’t have to download the blockchain again and that there are no duplicate block files in the datadir after upgrade.

Which is currently >100GB of duplicate data that the user has no way of removing except for going into the datadir and manually deleting. Why not just add a note to the release notes saying “don’t upgrade directly from <=0.7 to >=0.15?

benma commented at 2:37 pm on February 22, 2017: none

My take on that is that it is unlikely anyone would actually upgrade from <=0.7. It does not look like anybody is using it, and as @sipa mentioned, it would be impossible or very hard to run a <=0.7 client in the first place.

benma commented at 2:39 pm on February 22, 2017: none

If the same commit passed review and travis, you might want to force push that instead, to not invalidate the results.

In that one I accidentally had some trailing whitespace. Wouldn’t force-pushing it again re-run the CI?

Can I retrigger CI (magic comment or a UI button somewhere)?

laanwj commented at 7:32 am on February 23, 2017: member

Which is currently >100GB of duplicate data that the user has no way of removing except for going into the datadir and manually deleting. Why not just add a note to the release notes saying “don’t upgrade directly from <=0.7 to >=0.15?

For such old releases, the procedure should be: backup the wallet and nuke the rest of the data directory. The configuration options changed, the layout of the data directory changed (multiple times), there can be no assumption of compatibility besides the wallet.

It should work but yes you’d be wasting some space. Not that much, mind you, as said 0.7 can’t sync that far up the chain before getting stuck anyway.

I don’t think this needs mentioning in the release notes: Realistically, no one is using such an old version anymore. Check the node stats.

laanwj commented at 7:36 am on February 23, 2017: member

Can I retrigger CI (magic comment or a UI button somewhere)?

Sure. But we should first figure out what is broken. It doesn’t seem to work very well on the branches either (#9825, #9826) to it’s almost certainly not related to your change.

MarcoFalke commented at 1:49 pm on February 23, 2017: member

utACK 4b183d3

jnewbery commented at 2:37 pm on February 23, 2017: member

there can be no assumption of compatibility besides the wallet.

My default expectation would be compatibility between upgrades. Any non-compatibility issues should be documented in release notes.

Realistically, no one is using such an old version anymore. Check the node stats.

We shouldn’t make decisions based on what we see in node stats. We have a limited view of what happens in the network and what users are doing.

I’m actually not that concerned about this particular issue. I just think it’s good practice (and good courtesy to users) to note upgrade compatibility issues in release notes.

benma commented at 2:58 am on February 28, 2017: none

How are those things usually resolved here? I don’t think the release note is necessary, but I won’t stand in the way of it either.

laanwj commented at 11:29 am on February 28, 2017: member

Let’s leave writing a note for the release notes, if really necessary, to someone that insists on it, not going to block this pull on that.

laanwj merged this on Feb 28, 2017

laanwj closed this on Feb 28, 2017

laanwj referenced this in commit b7547fa93e on Feb 28, 2017

jnewbery commented at 4:45 pm on February 28, 2017: member

@laanwj I’m happy to write the upgrade advice for the release notes. Just let me know where I need to do that.

MarcoFalke commented at 4:54 pm on February 28, 2017: member

@jnewbery Release notes for 0.15.0 are ready for amending. You can do it anytime prior to 0.15 branch off of master.

PastaPastaPasta referenced this in commit 6bd3083639 on Dec 28, 2018

PastaPastaPasta referenced this in commit ac376fdc5a on Dec 29, 2018

PastaPastaPasta referenced this in commit 8929948aae on Dec 29, 2018

PastaPastaPasta referenced this in commit a2d6ca4a53 on Dec 31, 2018

PastaPastaPasta referenced this in commit 7a757cf8fa on Jan 2, 2019

PastaPastaPasta referenced this in commit 4972f908a4 on Jan 3, 2019

PastaPastaPasta referenced this in commit c76b697ccb on Jan 5, 2019

PastaPastaPasta referenced this in commit 2735cf5746 on Jan 7, 2019

PastaPastaPasta referenced this in commit 02a4ba18d8 on Jan 7, 2019

PastaPastaPasta referenced this in commit 8264e15cd4 on Jan 23, 2019

PastaPastaPasta referenced this in commit e9086824d2 on Jan 25, 2019

zkbot referenced this in commit 5db5bd749c on Dec 10, 2019

MarcoFalke locked this on Sep 8, 2021

Remove block file location upgrade code #9822