Skip to content

Remove block file location upgrade code#9822

Merged
laanwj merged 1 commit intobitcoin:masterfrom
benma:appinitmain
Feb 28, 2017
Merged

Remove block file location upgrade code#9822
laanwj merged 1 commit intobitcoin:masterfrom
benma:appinitmain

Conversation

@benma
Copy link

@benma benma commented Feb 22, 2017

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

dcousens commented Feb 22, 2017

concept ACK

@benma
Copy link
Author

benma commented Feb 22, 2017

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

sipa commented Feb 22, 2017

@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 benma changed the title Refactor: Split part of AppInitMain() into a new function Remove block file location upgrade code Feb 22, 2017
@benma
Copy link
Author

benma commented Feb 22, 2017

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

@laanwj
Copy link
Member

laanwj commented Feb 22, 2017

Yes, let's drop this for 0.15. utACK 45d7ee2

@maflcko
Copy link
Member

maflcko commented Feb 22, 2017

utACK fb678229429364f9a0a93add93e21fe444ae0fcb

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

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

src/init.cpp Outdated
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@laanwj
Copy link
Member

laanwj commented Feb 22, 2017

I think an unrelated commit snuck into here accidentally: 4cce79c

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

benma commented Feb 22, 2017

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 :)

@benma
Copy link
Author

benma commented Feb 22, 2017

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

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.

@maflcko
Copy link
Member

maflcko commented Feb 22, 2017 via email

@maflcko
Copy link
Member

maflcko commented Feb 22, 2017 via email

@jnewbery
Copy link
Contributor

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

benma commented Feb 22, 2017

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

benma commented Feb 22, 2017

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

laanwj commented Feb 23, 2017

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

laanwj commented Feb 23, 2017

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.

@maflcko
Copy link
Member

maflcko commented Feb 23, 2017

utACK 4b183d3

@jnewbery
Copy link
Contributor

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

benma commented Feb 28, 2017

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

laanwj commented Feb 28, 2017

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 laanwj merged commit 4b183d3 into bitcoin:master Feb 28, 2017
laanwj added a commit that referenced this pull request Feb 28, 2017
4b183d3 Remove block file location upgrade code (Marko Bencun)

Tree-SHA512: fac1fce95341e0df645c08c7e794195b22b54df08826aa8728f2f97aede1e42f724f8133781b97f836d4a392d044d08c846bce471a6b478582014f8be501a712
@jnewbery
Copy link
Contributor

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

@maflcko
Copy link
Member

maflcko commented Feb 28, 2017

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

@jnewbery jnewbery mentioned this pull request Jul 31, 2017
12 tasks
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 23, 2019
4b183d3 Remove block file location upgrade code (Marko Bencun)

Tree-SHA512: fac1fce95341e0df645c08c7e794195b22b54df08826aa8728f2f97aede1e42f724f8133781b97f836d4a392d044d08c846bce471a6b478582014f8be501a712
zkbot added a commit to zcash/zcash that referenced this pull request Dec 10, 2019
@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