Skip to content
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

utils and libraries: Make 'blocksdir' always net specific #14409

Merged
merged 2 commits into from Jan 16, 2019

Conversation

Projects
None yet
8 participants
@hebasto
Copy link
Member

commented Oct 5, 2018

The blocks directory is net specific by definition.

Also this prevents the side effect of calling GetBlocksDir(false) in the non-mainnet environment.
Currently a new node creates an unused blocks\ directory in the root of the data directory when -testnet or -regtest is specified.

Refs:

@hebasto hebasto changed the title utils and libraries: Make blockdir always net specific utils and libraries: Make 'blocksdir' always net specific Oct 5, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #13746 (-masterdatadir for datadir bootstrapping by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@ken2812221

This comment has been minimized.

Copy link
Member

commented Oct 6, 2018

Concept ACK. This seem to require a release note.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

Needs tests before merge

@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Concept ACK, so this only affects the case where the user has explicitly specified a directory with the -blocksdir argument right?

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2018

@meshcollider

Thank you for your review.

Concept ACK, so this only affects the case where the user has explicitly specified a directory with the -blocksdir argument right?

This PR works regardless of the -blocksdir option.

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

Does this risk breaking any existing configurations?

@meshcollider

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

@sipa that was my concern, the actual behaviour looks unchanged but I think now if you specify a base directory with -blocksdir it will error if the child directory doesn't exist, previously it would just create it? Which could be annoying for users...

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2018

@meshcollider @sipa

... I think now if you specify a base directory with -blocksdir it will error if the child directory doesn't exist, previously it would just create it?

Test of this PR:

$ rm -rf /home/hebasto/.bitcoin/
$ /home/hebasto/github/bitcoin/src/bitcoind
2018-10-13T09:27:14Z Bitcoin Core version v0.17.99.0-c1a60d4a1 (release build)
...
2018-10-13T09:27:30Z Shutdown: done
$ find /home/hebasto/.bitcoin/ > ~/pr14409-without-blocksdir

$ rm -rf /home/hebasto/.bitcoin/
$ /home/hebasto/github/bitcoin/src/bitcoind -blocksdir=/home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR
2018-10-13T09:33:59Z Bitcoin Core version v0.17.99.0-c1a60d4a1 (release build)
2018-10-13T09:33:59Z InitParameterInteraction: parameter interaction: -whitelistforcerelay=1 -> setting -whitelistrelay=1
2018-10-13T09:33:59Z Error: Specified blocks directory "/home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR" does not exist.
Error: Specified blocks directory "/home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR" does not exist
$ mkdir -p /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR
$ /home/hebasto/github/bitcoin/src/bitcoind -blocksdir=/home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR
2018-10-13T09:36:37Z Bitcoin Core version v0.17.99.0-c1a60d4a1 (release build)
...
2018-10-13T09:37:00Z Shutdown: done
$ find /home/hebasto/.bitcoin/ > ~/pr14409-with-blocksdir

$ diff ~/pr14409-without-blocksdir ~/pr14409-with-blocksdir 
3a4,8
> /home/hebasto/.bitcoin/TEST_DIR
> /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR
> /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR/blocks
> /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR/blocks/rev00000.dat
> /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR/blocks/blk00000.dat
12d16
< /home/hebasto/.bitcoin/blocks/rev00000.dat
18d21
< /home/hebasto/.bitcoin/blocks/blk00000.dat

Test of master be99270:

$ rm -rf /home/hebasto/.bitcoin/
$ /home/hebasto/github/bitcoin/src/bitcoind
2018-10-13T10:19:33Z Bitcoin Core version v0.17.99.0-be992701b (release build)
...
2018-10-13T10:19:51Z Shutdown: done
$ find /home/hebasto/.bitcoin/ > ~/master-be992701b-without-blocksdir

$ rm -rf /home/hebasto/.bitcoin/
$ /home/hebasto/github/bitcoin/src/bitcoind -blocksdir=/home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR
2018-10-13T10:22:30Z Bitcoin Core version v0.17.99.0-be992701b (release build)
2018-10-13T10:22:30Z InitParameterInteraction: parameter interaction: -whitelistforcerelay=1 -> setting -whitelistrelay=1
2018-10-13T10:22:30Z Error: Specified blocks directory "/home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR" does not exist.
Error: Specified blocks directory "/home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR" does not exist.
$ mkdir -p /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR
$ /home/hebasto/github/bitcoin/src/bitcoind -blocksdir=/home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR
2018-10-13T10:23:18Z Bitcoin Core version v0.17.99.0-be992701b (release build)
...
2018-10-13T10:23:35Z Shutdown: done
$ find /home/hebasto/.bitcoin/ > ~/master-be992701b-with-blocksdir

$ diff ~/master-be992701b-without-blocksdir ~/master-be992701b-with-blocksdir 
3a4,8
> /home/hebasto/.bitcoin/TEST_DIR
> /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR
> /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR/blocks
> /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR/blocks/rev00000.dat
> /home/hebasto/.bitcoin/TEST_DIR/TEST_BLOCKS_DIR/blocks/blk00000.dat
12d16
< /home/hebasto/.bitcoin/blocks/rev00000.dat
18d21
< /home/hebasto/.bitcoin/blocks/blk00000.dat

The behaviour is unchanged. If you specify a base directory with -blocksdir and if the child directory doesn't exist it will an error both on master and on this PR.

Make blockdir always net specific
The blocks directory is net specific by definition.

Also this prevents the side effect of calling GetBlocksDir(false) in the
non-mainnet environment.

@hebasto hebasto force-pushed the hebasto:20181005-blockspath-always-netspecific branch to c3f1821 Nov 5, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2018

Rebased.

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

This absolutely needs testing in a functional test.

Also to make it clear what behavior is failing right now, and which will pass after this.

Improve blocksdir functional test.
A new node should not create an unused `blocks` directory in the root of
the data directory when `-testnet` or `-regtest` is specified.
@hebasto

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

@laanwj Thank you for your review.

This absolutely needs testing in a functional test.
Also to make it clear what behavior is failing right now, and which will pass after this.

The existed feature_blocksdir.py functional test has been patched. Currently this patched test fails on master.

@laanwj

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

utACK e4a0c35

1 similar comment
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

utACK e4a0c35

@laanwj laanwj merged commit e4a0c35 into bitcoin:master Jan 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 16, 2019

Merge #14409: utils and libraries: Make 'blocksdir' always net specific
e4a0c35 Improve blocksdir functional test. (Hennadii Stepanov)
c3f1821 Make blockdir always net specific (Hennadii Stepanov)

Pull request description:

  The blocks directory is net specific by definition.

  Also this prevents the side effect of calling `GetBlocksDir(false)` in the non-mainnet environment.
  Currently a new node creates an unused `blocks\` directory in the root of the data directory when `-testnet` or `-regtest` is specified.

  Refs:
  - #12653
  - #12653 (comment) by @laanwj
  - #14595 (comment)

Tree-SHA512: c9957a68a4a200ebd2010823a56db7e61563afedcb7c9828e86b13f3af2990e07854b622c1f3374756f94574acb3ea32de7d2a399eef6c0623f0e11265155627

@hebasto hebasto deleted the hebasto:20181005-blockspath-always-netspecific branch Jan 16, 2019

@MarcoFalke MarcoFalke added this to the 0.17.2 milestone Feb 9, 2019

hebasto added a commit to hebasto/bitcoin that referenced this pull request Feb 22, 2019

Make blockdir always net specific
The blocks directory is net specific by definition.

Also this prevents the side effect of calling GetBlocksDir(false) in the
non-mainnet environment.

Github-Pull: bitcoin#14409
Rebased-From: c3f1821

hebasto added a commit to hebasto/bitcoin that referenced this pull request Feb 22, 2019

Improve blocksdir functional test.
A new node should not create an unused `blocks` directory in the root of
the data directory when `-testnet` or `-regtest` is specified.

Github-Pull: bitcoin#14409
Rebased-From: e4a0c35
@fanquake

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Being backported in #15445.

LitecoinZ added a commit to litecoinz-core/devrepo that referenced this pull request Mar 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.