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

pull ryanofsky wants to merge 1 commits into bitcoin-core:master from ryanofsky:pr/qtkeep changing 2 files +77 −66
  1. ryanofsky commented at 6:40 pm on May 20, 2022: contributor

    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.

  2. ryanofsky cross-referenced this on May 20, 2022 from issue Unify bitcoin-qt and bitcoind persistent settings by ryanofsky
  3. Rspigler commented at 11:21 pm on May 20, 2022: contributor
    Concept ACK, since users can reset settings. Will test soon
  4. DrahtBot commented at 0:19 am on May 21, 2022: contributor

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

    Conflicts

    No conflicts as of last run.

  5. DrahtBot cross-referenced this on May 21, 2022 from issue Enable users to configure their monospace font specifically by luke-jr
  6. DrahtBot cross-referenced this on May 21, 2022 from issue Do not require restart if overridden option is modified by hebasto
  7. DrahtBot cross-referenced this on May 21, 2022 from issue Peers window: Show direction in a new column, with clearer icon by luke-jr
  8. Rspigler cross-referenced this on May 21, 2022 from issue interfaces: Expose settings.json methods to GUI by ryanofsky
  9. Rspigler commented at 6:38 am on May 21, 2022: contributor

    tACK c2051b35f4bbb195e4dc1211ca4f520158c601b7

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

  10. Rspigler commented at 4:52 am on May 22, 2022: contributor

    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.

  11. DrahtBot added the label Needs rebase on May 22, 2022
  12. ryanofsky commented at 6:24 pm on May 25, 2022: contributor

    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.

  13. hebasto commented at 1:02 pm on June 12, 2022: member

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

    Time to undraft and rebase? :)

    Maybe also clean up the PR description a bit?

  14. Add settings.json prune-prev, proxy-prev, onion-prev settings 
    This provides a way for the GUI settings dialog box to retain previous pruning
and proxy settings when they are disabled, as requested by vasild:

https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-937685759
https://github.com/bitcoin/bitcoin/pull/15936#discussion_r850568749
https://github.com/bitcoin/bitcoin/pull/15936#discussion_r852998379

Importantly, while this PR changes the settings.json format, it changes it in a
fully backwards compatible way, so previous versious of bitcoind and bitcoin-qt
will correctly interpret prune, proxy, and onion settins written by new
versions of bitcoin-qt.
    9d3127b11e
  15. ryanofsky force-pushed on Sep 21, 2022
  16. ryanofsky marked this as ready for review on Sep 21, 2022
  17. ryanofsky commented at 4:19 pm on September 21, 2022: contributor

    Time to undraft and rebase? :)

    Done!

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

  18. DrahtBot removed the label Needs rebase on Sep 21, 2022
  19. ryanofsky commented at 2:54 pm on September 27, 2022: contributor

    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

  20. ryanofsky cross-referenced this on Oct 24, 2022 from issue RFC: Is it OK to reset unused prune and proxy settings to default values after restarting? by ryanofsky
  21. hebasto approved
  22. hebasto commented at 3:33 pm on October 26, 2022: member

    ACK 9d3127b11e34131409dab7c08bde5b444d90b2cb, tested on Ubuntu 22.04.

    Although, #596 (comment).

  23. hebasto commented at 3:34 pm on October 26, 2022: member
    cc @vasild @Sjors @jarolrod
  24. ryanofsky commented at 3:44 pm on October 26, 2022: contributor

    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.

  25. hebasto added the label UX on Oct 26, 2022
  26. vasild commented at 8:55 am on November 2, 2022: contributor

    Makes sense to me.

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

  27. vasild approved
  28. vasild commented at 8:58 am on November 15, 2022: contributor

    ACK 9d3127b11e34131409dab7c08bde5b444d90b2cb

    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)

  29. hebasto commented at 9:10 am on November 15, 2022: member

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

    So would I :)

  30. ryanofsky commented at 5:50 pm on November 15, 2022: contributor

    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:

    0bitcoin-cli settings set prune 3G
1bitcoin-cli settings disable prune
2bitcoin-cli settings enable prune
3bitcoin-cli settings set proxy 127.0.0.1:9050
4bitcoin-cli settings disable proxy
5bitcoin-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.

  31. ryanofsky commented at 8:24 pm on February 13, 2023: contributor

    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.

  32. jarolrod commented at 8:56 pm on February 13, 2023: member

    tACK 9d3127b11e34131409dab7c08bde5b444d90b2cb

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

  33. hebasto merged this on Feb 15, 2023
  34. hebasto closed this on Feb 15, 2023
  35. sidhujag referenced this in commit 8fb40f4117 on Feb 15, 2023
  36. jarolrod cross-referenced this on Feb 17, 2023 from issue Sync with the main repo by jarolrod
  37. hebasto referenced this in commit 37c4d376b5 on Feb 17, 2023
  38. bitcoin-core locked this on Feb 15, 2024


ryanofsky Rspigler DrahtBot hebasto vasild jarolrod

Labels
UX

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-23 02:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me