Add settings.json prune-prev, proxy-prev, onion-prev settings

[ryanofsky]

With #602, if proxy and pruning settings are disabled in the GUI and the GUI is restarted, proxy and prune values are not stored anywhere. So if these settings are enabled in the future, default values will be shown, not previous values.

This PR stores previous values so they will preserved across restarts. I'm not sure I like this behavior because showing default values seems simpler and safer to me. Previous values may just have been set temporarily and may have never actually worked, and it adds some code complexity to store them.

This PR is one way of resolving #596. Other solutions are possible and could be implemented as alternatives.

[Rspigler]

Concept ACK, since users can reset settings. Will test soon

[DrahtBot]

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

Conflicts

No conflicts as of last run.

[Rspigler]

tACK c2051b3

Works just as described, I think it's really smooth. I think this is a good solution, but open to hearing more opinions obviously.

[Rspigler]

make check error:

FAIL: qt/test/test_bitcoin-qt

test-suite.log:

FAIL: qt/test/test_bitcoin-qt

********* Start testing of AppTests *********
Config: Using QtTest library 5.15.2, Qt 5.15.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.2.1 20210110), debian 11
PASS : AppTests::initTestCase()
QINFO : AppTests::appTests() Backing up GUI settings to "/tmp/test_common_Bitcoin Core/81926fa71dbc85c864c7dcdacb11a038f304fc445d63880f3579c457bffc9b87/regtest/guisettings.ini.bak"
QDEBUG : AppTests::appTests() requestInitialize : Requesting initialize
QDEBUG : AppTests::appTests() Running initialization in thread
QDEBUG : AppTests::appTests() initializeResult : Initialization result: true
QINFO : AppTests::appTests() Platform customization: "other"
QWARN : AppTests::appTests() This plugin does not support propagateSizeHints()
QWARN : AppTests::appTests() This plugin does not support propagateSizeHints()
QWARN : AppTests::appTests() This plugin does not support raise()
QWARN : AppTests::appTests() This plugin does not support raise()
QWARN : AppTests::appTests() This plugin does not support grabbing the keyboard
QWARN : AppTests::appTests() This plugin does not support propagateSizeHints()
QDEBUG : AppTests::appTests() requestShutdown : Requesting shutdown
QDEBUG : AppTests::appTests() Running Shutdown in thread
QDEBUG : AppTests::appTests() Shutdown finished
PASS : AppTests::appTests()
PASS : AppTests::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 554ms
********* Finished testing of AppTests *********
********* Start testing of OptionTests *********
Config: Using QtTest library 5.15.2, Qt 5.15.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.2.1 20210110), debian 11
PASS : OptionTests::initTestCase()
FAIL! : OptionTests::migrateSettings() Compared strings are not the same
Actual (std::string(std::istreambuf_iterator(file), std::istreambuf_iterator()).c_str()) : {
"dbcache": "600",
"listen": false,
"onion": "onion:234",
"onion-prev": "",
"par": "12",
"proxy": "proxy:123",
"proxy-prev": "",
"prune": "2861",
"prune-prev": "0"
}

Expected ("{\n" " "dbcache": "600",\n" " "listen": false,\n" " "onion": "onion:234",\n" " "par": "12",\n" " "proxy": "proxy:123",\n" " "prune": "2861"\n" "}\n"): {
"dbcache": "600",
"listen": false,
"onion": "onion:234",
"par": "12",
"proxy": "proxy:123",
"prune": "2861"
}

Loc: [qt/test/optiontests.cpp(64)]
PASS : OptionTests::integerGetArgBug()
PASS : OptionTests::parametersInteraction()
PASS : OptionTests::cleanupTestCase()
Totals: 4 passed, 1 failed, 0 skipped, 0 blacklisted, 207ms
********* Finished testing of OptionTests *********
********* Start testing of URITests *********
Config: Using QtTest library 5.15.2, Qt 5.15.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.2.1 20210110), debian 11
PASS : URITests::initTestCase()
PASS : URITests::uriTests()
PASS : URITests::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 0ms
********* Finished testing of URITests *********
********* Start testing of RPCNestedTests *********
Config: Using QtTest library 5.15.2, Qt 5.15.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.2.1 20210110), debian 11
PASS : RPCNestedTests::initTestCase()
PASS : RPCNestedTests::rpcNestedTests()
PASS : RPCNestedTests::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 10ms
********* Finished testing of RPCNestedTests *********
********* Start testing of WalletTests *********
Config: Using QtTest library 5.15.2, Qt 5.15.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.2.1 20210110), debian 11
PASS : WalletTests::initTestCase()
QWARN : WalletTests::walletTests() This plugin does not support propagateSizeHints()
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: 44c8b7988d51de5198858b0c8cfa504d3a8de71acda23506c45b8dc47e84373e status= 0"
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: ec76e4ab5ae7066ba51a42c5e09a79adaf9708a4a3cdbd02a28d378179f3024e status= 1"
QDEBUG : WalletTests::walletTests() "NotifyAddressBookChanged: mfWxJ45yp2SFn7UciZyNpvDKrzbhyfKrY8 isMine=0 purpose=send status=0"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: 44c8b7988d51de5198858b0c8cfa504d3a8de71acda23506c45b8dc47e84373e 0"
QDEBUG : WalletTests::walletTests() " inModel=0 Index=20-20 showTransaction=1 derivedStatus=0"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: ec76e4ab5ae7066ba51a42c5e09a79adaf9708a4a3cdbd02a28d378179f3024e 1"
QDEBUG : WalletTests::walletTests() " inModel=1 Index=27-28 showTransaction=1 derivedStatus=1"
QWARN : WalletTests::walletTests() This plugin does not support propagateSizeHints()
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: 8f3bc8d976966056ec5597ee811f35df382dd48cb7bae427802e2849bd72bc74 status= 0"
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: 20bb379fd28f090730ac2d207b92c6e95c69dfa48a8118577edb20e068c9a65d status= 1"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: 8f3bc8d976966056ec5597ee811f35df382dd48cb7bae427802e2849bd72bc74 0"
QDEBUG : WalletTests::walletTests() " inModel=0 Index=43-43 showTransaction=1 derivedStatus=0"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: 20bb379fd28f090730ac2d207b92c6e95c69dfa48a8118577edb20e068c9a65d 1"
QDEBUG : WalletTests::walletTests() " inModel=1 Index=34-35 showTransaction=1 derivedStatus=1"
QWARN : WalletTests::walletTests() This plugin does not support propagateSizeHints()
QWARN : WalletTests::walletTests() This plugin does not support propagateSizeHints()
QWARN : WalletTests::walletTests() This plugin does not support propagateSizeHints()
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: c396007f84cb9643a1030708dcc0b9ad2a03bda49a87762f29a1b48d639a153a status= 0"
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: 20bb379fd28f090730ac2d207b92c6e95c69dfa48a8118577edb20e068c9a65d status= 1"
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: 8f3bc8d976966056ec5597ee811f35df382dd48cb7bae427802e2849bd72bc74 status= 1"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: 8f3bc8d976966056ec5597ee811f35df382dd48cb7bae427802e2849bd72bc74 1"
QDEBUG : WalletTests::walletTests() " inModel=1 Index=43-44 showTransaction=1 derivedStatus=1"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: c396007f84cb9643a1030708dcc0b9ad2a03bda49a87762f29a1b48d639a153a 0"
QDEBUG : WalletTests::walletTests() " inModel=0 Index=19-19 showTransaction=1 derivedStatus=0"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: 20bb379fd28f090730ac2d207b92c6e95c69dfa48a8118577edb20e068c9a65d 1"
QDEBUG : WalletTests::walletTests() " inModel=1 Index=35-36 showTransaction=1 derivedStatus=1"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: 8f3bc8d976966056ec5597ee811f35df382dd48cb7bae427802e2849bd72bc74 1"
QDEBUG : WalletTests::walletTests() " inModel=1 Index=44-45 showTransaction=1 derivedStatus=1"
QWARN : WalletTests::walletTests() This plugin does not support propagateSizeHints()
QDEBUG : WalletTests::walletTests() "NotifyAddressBookChanged: bcrt1q6r5w50cgdqknd6dpxyxk8legnkxy5scygqem4t TEST_LABEL_1 isMine=1 purpose=receive status=0"
QWARN : WalletTests::walletTests() This plugin does not support propagateSizeHints()
PASS : WalletTests::walletTests()
PASS : WalletTests::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 707ms
********* Finished testing of WalletTests *********
********* Start testing of AddressBookTests *********
Config: Using QtTest library 5.15.2, Qt 5.15.2 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 10.2.1 20210110), debian 11
PASS : AddressBookTests::initTestCase()
QWARN : AddressBookTests::addressBookTests() This plugin does not support propagateSizeHints()
QWARN : AddressBookTests::addressBookTests() This plugin does not support propagateSizeHints()
QDEBUG : AddressBookTests::addressBookTests() "NotifyAddressBookChanged: bcrt1q6m4jcjapjw9zrfv0a60f5qfkupfv4ayu4l0w2h new isMine=0 purpose=send status=0"
PASS : AddressBookTests::addressBookTests()
PASS : AddressBookTests::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 603ms
********* Finished testing of AddressBookTests *********

Failed tests: 1

~InitExecutor : Stopping thread
~InitExecutor : Stopped thread
FAIL qt/test/test_bitcoin-qt (exit status: 1)

Functional and util tests pass.

[ryanofsky]

make check error:

Yes this happens on cirrus and locally too because actually enabling proxy/prune/onion settings sets the -prev settings to "" instead of removing them from the file. I could just update optionstest to reflect this, but I think it would be a little nicer to delete the -prev settings when the real settings are active, so will work on this when I update the PR.

[hebasto]

This is based on #15936 + #600 + #601 + #602.

Time to undraft and rebase?
:)

Maybe also clean up the PR description a bit?

[ryanofsky]

Time to undraft and rebase? :)

Done!

Rebased c2051b3 -> 9d3127b (pr/qtkeep.3 -> pr/qtkeep.4, compare) after base PR #602 merge

[ryanofsky]

I think it would be a little nicer to delete the -prev settings when the real settings are active, so will work on this when I update the PR.

This was also implemented in the last push

[hebasto]

ACK 9d3127b, tested on Ubuntu 22.04.

Although, #596 (comment).

[hebasto]

cc @vasild @Sjors @jarolrod

[ryanofsky]

re: #596 (comment)

While reviewing #603, I've made an opinion that

Other approaches like storing unused prune and proxy settings in QSettings

looks nicer, as all those settings are GUI-specific.

If others agree, I can probably change the PR pretty easily to do this. Just replace new std::string suffix parameters with bool inactive parameters, pass true instead of "-prev", and false instead of "". When inactive is true, store the setting to QSettings with "-prev" suffix. When inactive is false, store the setting normally to settings.json.

[vasild]

Makes sense to me.

nit: maybe use "active" instead of "inactive" to avoid double negation "inactive=false" aka "is not non-active".

[vasild]

ACK 9d3127b

Would be nice to have this in 24.0 to avoid changing user behavior from 23->24 (and to restore it in 24->25).

Would be happy to re-review if changed like discussed in: #603 (comment)

[hebasto]

Would be happy to re-review if changed like discussed in: #603 (comment)

So would I :)

[ryanofsky]

Would be happy to re-review if changed like discussed in: #603 (comment)

So would I :)

I started working on this, but I think I changed my mind about this alternate approach and now prefer the current approach.

I don't think storing enabled settings in settings.json but storing disabled settings in QSettings is a good idea, because the idea of storing disabled settings is not really inherently GUI specific. Storing disabled settings could be useful for the RPC interface as well. Actually one of the original reasons for introducing settings.json was to allow runtime configuration via RPC, and I still think an interface like:

bitcoin-cli settings set prune 3G
bitcoin-cli settings disable prune
bitcoin-cli settings enable prune
bitcoin-cli settings set proxy 127.0.0.1:9050
bitcoin-cli settings disable proxy
bitcoin-cli settings enable proxy

could be a good idea, and if implemented should use same disabled settings storage that the GUI uses, making settings.json a more appropriate place than QSettings.

In light of this it would probably be better to move disabled settings logic down into the common or util layers instead of being in the GUI layer. But this could happen in a followup.

Would be curious if this makes sense to others. Even if we never add another interface that enables & disables settings, it just seem plausible that there could be other interfaces that use these values, so it makes sense to store them alongside the normal settings, not someplace GUI-specific.

[ryanofsky]

Just to summarize discussion above, I think that as long we believe it is useful to store disabled prune and proxy values, then settings.json is a better place to store them than QSettings, so this PR is implementing the right approach, and is ready for review/testing/merge.

If we think it's ok to reset disabled settings to default instead of saving them, then that is the current behavior, and this PR could just be closed. I'm beginning to think more that it is good to store disabled settings, so this PR seems like a good idea, but resetting them also seems reasonable, so I don't have a strong opinion.

[jarolrod]

tACK 9d3127b

This is useful for us over at https://github.com/bitcoin-core/gui-qml