Call init's parameter interaction before we create the UI options model #6780

Merged
merged 4 commits into from Nov 27, 2015

Conversation

Projects
None yet
7 participants
@jonasschnelli
Member

jonasschnelli commented Oct 8, 2015

Currently optionsModel(QT) parameters interaction overwrites/dominates initial and depending parameter settings. This PR would factor out init's parameter interaction and call it before AppInit2().

Fixes #6773

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 8, 2015

Member

Force pushed a copy'n'paste fix: s/bool InitParameterInteraction()/void InitParameterInteraction().
Credits to @MarcoFalke on IRC.

Member

jonasschnelli commented Oct 8, 2015

Force pushed a copy'n'paste fix: s/bool InitParameterInteraction()/void InitParameterInteraction().
Credits to @MarcoFalke on IRC.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 8, 2015

Member

Concept ACK.

Member

MarcoFalke commented Oct 8, 2015

Concept ACK.

@laanwj laanwj added the GUI label Oct 8, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 8, 2015

Member

Concept ACK

Member

laanwj commented Oct 8, 2015

Concept ACK

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Oct 8, 2015

Contributor

ACK

Contributor

paveljanik commented Oct 8, 2015

ACK

@laanwj

View changes

src/qt/bitcoin.cpp
@@ -381,7 +398,7 @@ void BitcoinApplication::startThread()
if(coreThread)
return;
coreThread = new QThread(this);
- BitcoinCore *executor = new BitcoinCore();
+ executor = new BitcoinCore();
executor->moveToThread(coreThread);

This comment has been minimized.

@laanwj

laanwj Oct 30, 2015

Member

It's not allowed to use this object [from this thread] after moving it to another thread.
I'd suggest calling InitParameterInteraction() directly from BitcoinApplication::parameterSetup() and not bothering with the stuff in BitcoinCore.

@laanwj

laanwj Oct 30, 2015

Member

It's not allowed to use this object [from this thread] after moving it to another thread.
I'd suggest calling InitParameterInteraction() directly from BitcoinApplication::parameterSetup() and not bothering with the stuff in BitcoinCore.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 13, 2015

Member

Rebased and fixed the pointer access after moveToThread by following @laanwj advice.

Member

jonasschnelli commented Nov 13, 2015

Rebased and fixed the pointer access after moveToThread by following @laanwj advice.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 17, 2015

Member

Sorry, blocksonly mode caused a need for a small change here, as it needs to get moved. @jonasschnelli mind rebasing?

What about that prune/txindex "interaction" (fails to run), should it move too?

Member

gmaxwell commented Nov 17, 2015

Sorry, blocksonly mode caused a need for a small change here, as it needs to get moved. @jonasschnelli mind rebasing?

What about that prune/txindex "interaction" (fails to run), should it move too?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 17, 2015

Member

Sorry, blocksonly mode caused a need for a small change here, as it needs to get moved. @jonasschnelli mind rebasing?

Just rebased and moved the -blocksonly interaction to the new function.

What about that prune/txindex "interaction" (fails to run), should it move too?

My idea was to only move "rules" to the new function. Things that evaluate parameters through GetBoolArg or mapArgs, etc. and depending on its state, optional change another startup variable through SoftSetBoolArg, etc.

Pruning and TxIndex are checks that lead to a InitError() (startup termination) and it would require to move the InitError() context to the ParameterInteraction() function.

Member

jonasschnelli commented Nov 17, 2015

Sorry, blocksonly mode caused a need for a small change here, as it needs to get moved. @jonasschnelli mind rebasing?

Just rebased and moved the -blocksonly interaction to the new function.

What about that prune/txindex "interaction" (fails to run), should it move too?

My idea was to only move "rules" to the new function. Things that evaluate parameters through GetBoolArg or mapArgs, etc. and depending on its state, optional change another startup variable through SoftSetBoolArg, etc.

Pruning and TxIndex are checks that lead to a InitError() (startup termination) and it would require to move the InitError() context to the ParameterInteraction() function.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 17, 2015

Member

utACK 51627cc

Member

MarcoFalke commented Nov 17, 2015

utACK 51627cc

@jonasschnelli jonasschnelli added this to the 0.12.0 milestone Nov 18, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 21, 2015

Member

Would be nice if someone finds time to test this. Binaries: https://bitcoin.jonasschnelli.ch/pulls/6780/
This fixes a ugly bug (#6773) which would be nice to have fixed in 0.12.

Member

jonasschnelli commented Nov 21, 2015

Would be nice if someone finds time to test this. Binaries: https://bitcoin.jonasschnelli.ch/pulls/6780/
This fixes a ugly bug (#6773) which would be nice to have fixed in 0.12.

@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Nov 21, 2015

Member

Will do.

On Saturday, November 21, 2015, Jonas Schnelli notifications@github.com
wrote:

Would be nice if someone finds time to test this. Binaries:
https://bitcoin.jonasschnelli.ch/pulls/6780/
This fixes a ugly bug (#6773
#6773) which would be nice to
have fixed in 0.12.


Reply to this email directly or view it on GitHub
#6780 (comment).

Member

fanquake commented Nov 21, 2015

Will do.

On Saturday, November 21, 2015, Jonas Schnelli notifications@github.com
wrote:

Would be nice if someone finds time to test this. Binaries:
https://bitcoin.jonasschnelli.ch/pulls/6780/
This fixes a ugly bug (#6773
#6773) which would be nice to
have fixed in 0.12.


Reply to this email directly or view it on GitHub
#6780 (comment).

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 21, 2015

Member

@jonasschnelli

  • master:
$ src/bitcoind -regtest -connect=127.0.0.1 -server -printtoconsole |grep parameter 
2015-11-21 17:52:56 AppInit2: parameter interaction: -connect set -> setting -dnsseed=0
2015-11-21 17:52:56 AppInit2: parameter interaction: -connect set -> setting -listen=0
2015-11-21 17:52:56 AppInit2: parameter interaction: -listen=0 -> setting -upnp=0
2015-11-21 17:52:56 AppInit2: parameter interaction: -listen=0 -> setting -discover=0
2015-11-21 17:52:56 AppInit2: parameter interaction: -listen=0 -> setting -listenonion=0

$ src/qt/bitcoin-qt -regtest -connect=127.0.0.1 -server -printtoconsole |grep parameter 
2015-11-21 17:53:34 AppInit2: parameter interaction: -connect set -> setting -dnsseed=0
  • 51627cc
$ ~/bitcoin* -regtest -connect=127.0.0.1 -debug=1 -printtoconsole |grep parameter 
Member

MarcoFalke commented Nov 21, 2015

@jonasschnelli

  • master:
$ src/bitcoind -regtest -connect=127.0.0.1 -server -printtoconsole |grep parameter 
2015-11-21 17:52:56 AppInit2: parameter interaction: -connect set -> setting -dnsseed=0
2015-11-21 17:52:56 AppInit2: parameter interaction: -connect set -> setting -listen=0
2015-11-21 17:52:56 AppInit2: parameter interaction: -listen=0 -> setting -upnp=0
2015-11-21 17:52:56 AppInit2: parameter interaction: -listen=0 -> setting -discover=0
2015-11-21 17:52:56 AppInit2: parameter interaction: -listen=0 -> setting -listenonion=0

$ src/qt/bitcoin-qt -regtest -connect=127.0.0.1 -server -printtoconsole |grep parameter 
2015-11-21 17:53:34 AppInit2: parameter interaction: -connect set -> setting -dnsseed=0
  • 51627cc
$ ~/bitcoin* -regtest -connect=127.0.0.1 -debug=1 -printtoconsole |grep parameter 
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 24, 2015

Contributor

concept ACK / utACK

Contributor

dcousens commented Nov 24, 2015

concept ACK / utACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 26, 2015

Member

I'd like to take this into 0.12 because it fixes a slightly security-critical issue (#6773). Some tested ACKs would be nice (another 5 days left until feature freeze).

Member

jonasschnelli commented Nov 26, 2015

I'd like to take this into 0.12 because it fixes a slightly security-critical issue (#6773). Some tested ACKs would be nice (another 5 days left until feature freeze).

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 26, 2015

Member

@jonasschnelli I am happy to test but the debug.log issue seems not yet resolved? Am I missing something?

Member

MarcoFalke commented Nov 26, 2015

@jonasschnelli I am happy to test but the debug.log issue seems not yet resolved? Am I missing something?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 26, 2015

Member

utACK. Agree it needs to be in 0.12

Haven't tested any specific cases - I've been holding off on this because the parameter processing is kind of fragile and I haven't properly reasoned it through yet that this doesn't have effect on the precedence of options specified in e.g. configuration files, command line, QSettings.

Member

laanwj commented Nov 26, 2015

utACK. Agree it needs to be in 0.12

Haven't tested any specific cases - I've been holding off on this because the parameter processing is kind of fragile and I haven't properly reasoned it through yet that this doesn't have effect on the precedence of options specified in e.g. configuration files, command line, QSettings.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 26, 2015

Member

Thanks @MarcoFalke for pointing out the logging issue. Added another commit that moves the logging initialization.

For reviewers: The critical part are the first 100 lines in AppInit2() because the logging init and the parameter interactions are now before AppInit2().

Member

jonasschnelli commented Nov 26, 2015

Thanks @MarcoFalke for pointing out the logging issue. Added another commit that moves the logging initialization.

For reviewers: The critical part are the first 100 lines in AppInit2() because the logging init and the parameter interactions are now before AppInit2().

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 27, 2015

Member

Sanity tests pass now:

$ src/bitcoind -regtest -connect=127.0.0.1 -server -printtoconsole |egrep "(parameter|Bitcoin)"
2015-11-27 07:39:08 Bitcoin version v0.11.99.0-296c42e (2015-11-26 14:03:27 +0100)
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -connect set -> setting -dnsseed=0
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -connect set -> setting -listen=0
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -listen=0 -> setting -upnp=0
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -listen=0 -> setting -discover=0
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0

$ src/qt/bitcoin-qt -regtest -connect=127.0.0.1 -server -printtoconsole |egrep "(parameter|Bitcoin)"
2015-11-27 07:53:02 Bitcoin version v0.11.99.0-296c42e (2015-11-26 14:03:27 +0100)
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -connect set -> setting -dnsseed=0
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -connect set -> setting -listen=0
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -listen=0 -> setting -upnp=0
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -listen=0 -> setting -discover=0
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0
Member

MarcoFalke commented Nov 27, 2015

Sanity tests pass now:

$ src/bitcoind -regtest -connect=127.0.0.1 -server -printtoconsole |egrep "(parameter|Bitcoin)"
2015-11-27 07:39:08 Bitcoin version v0.11.99.0-296c42e (2015-11-26 14:03:27 +0100)
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -connect set -> setting -dnsseed=0
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -connect set -> setting -listen=0
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -listen=0 -> setting -upnp=0
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -listen=0 -> setting -discover=0
2015-11-27 07:39:08 InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0

$ src/qt/bitcoin-qt -regtest -connect=127.0.0.1 -server -printtoconsole |egrep "(parameter|Bitcoin)"
2015-11-27 07:53:02 Bitcoin version v0.11.99.0-296c42e (2015-11-26 14:03:27 +0100)
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -connect set -> setting -dnsseed=0
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -connect set -> setting -listen=0
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -listen=0 -> setting -upnp=0
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -listen=0 -> setting -discover=0
2015-11-27 07:53:02 InitParameterInteraction: parameter interaction: -listen=0 -> setting -listenonion=0
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 27, 2015

Member

utACK 296c42e

Member

MarcoFalke commented Nov 27, 2015

utACK 296c42e

@MarcoFalke

View changes

src/init.cpp
- LogPrintf("%s: parameter interaction: -blocksonly=1 -> setting -walletbroadcast=0\n", __func__);
-#endif
- }
-

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 27, 2015

Member

@jonasschnelli Trivial white space rebase conflict due to 9a3e1a5. (Looks like you can ignore changes made in master)

@MarcoFalke

MarcoFalke Nov 27, 2015

Member

@jonasschnelli Trivial white space rebase conflict due to 9a3e1a5. (Looks like you can ignore changes made in master)

jonasschnelli added some commits Oct 8, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 27, 2015

Member

Thanks @MarcoFalke: rebased!

Member

jonasschnelli commented Nov 27, 2015

Thanks @MarcoFalke: rebased!

@laanwj laanwj merged commit a46f87f into bitcoin:master Nov 27, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Nov 27, 2015

Merge pull request #6780
a46f87f Initialize logging before we do parameter interaction (Jonas Schnelli)
df66147 Move -blocksonly parameter interaction to the new ParameterInteraction() function (Jonas Schnelli)
68354e7 [QT] Call inits parameter interaction before we create the options model (Jonas Schnelli)
411b05a Refactor parameter interaction, call it before AppInit2() (Jonas Schnelli)

zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018

Auto merge of #2390 - str4d:2132-mapargs-prep, r=<try>
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

Part of #2074 and #2132.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment