test: Remove duplicate NodeContext hacks #19098

pull ryanofsky wants to merge 1 commits into bitcoin:master from ryanofsky:pr/qtx changing 7 files +58 −38
  1. ryanofsky commented at 9:02 PM on May 28, 2020: contributor

    Qt tests currently are currently using two NodeContext structs at the same time, one in interfaces::NodeImpl::m_context, and the other in BasicTestingSetup::m_node, and the tests have hacks transferring state between them.

    Fix this by getting rid of the NodeImpl::m_context struct and making it a pointer. This way a common BitcoinApplication object can be used for all qt tests, but they can still have their own testing setups.

    Non-test code is changing but non-test behavior is still the same as before.

    Motivation for this PR is to be able to remove the "std::move(test.m_node.connman)" and mempool hacks for swapping individual NodeContext members in Qt tests, because followup PR #19099 adds yet another member (wallet_client) that needs to be swapped. After this change, the whole NodeContext struct can be swapped instead of individual members, so the workarounds are less fragile and invasive.

  2. DrahtBot added the label GUI on May 28, 2020
  3. DrahtBot added the label Tests on May 28, 2020
  4. ryanofsky cross-referenced this on May 28, 2020 from issue refactor: Move wallet methods out of chain.h and node.h by ryanofsky
  5. ryanofsky cross-referenced this on May 28, 2020 from issue refactor: remove ::vpwallets and related global variables by ryanofsky
  6. in src/qt/test/apptests.cpp:66 in 09569ffe62 outdated 
      63 | @@ -63,6 +64,7 @@ void AppTests::appTests()
  64 |  #endif
  65 |  
  66 |      BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to
    MarcoFalke commented at 10:33 PM on May 28, 2020:

    Duplicate testing setups are illegal anyway. Have you seen https://github.com/bitcoin/bitcoin/pull/18923/commits/fa5f7fa24cc5ec34f3ba9616e6941123f751d8ed which is a massively smaller diff, potentially fixing the issue you ran into.

    MarcoFalke commented at 11:20 PM on May 28, 2020:

    #18923

    ryanofsky commented at 4:25 AM on May 29, 2020:

    re: #19098 (review)

    Duplicate testing setups are illegal anyway. Have you seen fa5f7fa which is a massively smaller diff, potentially fixing the issue you ran into.

    Acked other PR, and this PR overlaps a little bit with another part of it, but this PR is solving a different problem: not duplicate test setups but duplicate NodeContext (and WalletContext) instances inside a single test setup and confusion and fragility created by this. I added more motivation to PR description above.

    Also, the diff isn't very big here, it's mostly just replacing . with ->

  7. DrahtBot cross-referenced this on May 29, 2020 from issue wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101
  8. DrahtBot commented at 6:50 AM on May 29, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19619 (Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky)
    • #19471 (util: Make default arg values more specific by hebasto)
    • #19425 (refactor: Get rid of more redundant chain methods by ryanofsky)
    • #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
    • #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
    • #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
    • #18077 (net: Add NAT-PMP port forwarding support by hebasto)
    • #15454 (Remove the automatic creation and loading of the default wallet by achow101)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    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.

  9. DrahtBot cross-referenced this on May 29, 2020 from issue wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101
  10. DrahtBot cross-referenced this on May 29, 2020 from issue Reduce cs_main lock accumulation during GUI startup by jonasschnelli
  11. DrahtBot cross-referenced this on May 29, 2020 from issue wallet: Refactor the classes in wallet/db.{cpp/h} by achow101
  12. DrahtBot cross-referenced this on May 29, 2020 from issue wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off by MarcoFalke
  13. DrahtBot cross-referenced this on May 29, 2020 from issue Use shared pointers only in validation interface by bvbfan
  14. DrahtBot cross-referenced this on May 29, 2020 from issue net: Add NAT-PMP port forwarding support by hebasto
  15. DrahtBot cross-referenced this on May 29, 2020 from issue qt, refactor: Make BitcoinUnits::Unit a scoped enum by hebasto
  16. ryanofsky force-pushed on May 29, 2020
  17. ryanofsky commented at 12:07 PM on May 29, 2020: contributor

    Updated 09569ffe624ff46bb44c6b8d1dac8b3d2193c0d4 -> 8569817a4341220d734d445b5846bb01b57fa303 (pr/qtx.1 -> pr/qtx.2, compare) with fix for address book asan error https://travis-ci.org/github/bitcoin/bitcoin/jobs/692330030#L4304 Updated 8569817a4341220d734d445b5846bb01b57fa303 -> 934a8ae4b43350b5a2852ce7de7c87ace25ecfb5 (pr/qtx.2 -> pr/qtx.3, compare) fixing pedantic asan null reference error https://travis-ci.org/github/bitcoin/bitcoin/jobs/692528901#L4294 Rebased 934a8ae4b43350b5a2852ce7de7c87ace25ecfb5 -> d2c9cd790b05b50292c604d5d4b36d35b58448b7 (pr/qtx.3 -> pr/qtx.4, compare) due to conflict with #19176 Rebased d2c9cd790b05b50292c604d5d4b36d35b58448b7 -> ab1736cd92ee2ce0bdfe3c9bf23594cd2213b659 (pr/qtx.4 -> pr/qtx.5, compare) due to conflict with #19310

  18. bitcoin deleted a comment on May 29, 2020
  19. ryanofsky force-pushed on May 29, 2020
  20. tarboss commented at 8:19 AM on May 30, 2020: none

    good approach, in wallettests.cpp the function TestGui() uses the codeline: "make CWallet()" to create the CWallet. The chain parameter is from 1. Test (Apptests) in the current master branch, and not from Wallettest setup!!!!.

    I tried this (dont wanna break whole top node interface):

    test.m_node.chain = interfaces::MakeChain(test.m_node); //node.context()->connman = std::move(test.m_node.connman); //node.context()->mempool = std::move(test.m_node.mempool); cwallet = std::make_shared<CWallet>(test.m_node.chain.get(), location L"", WalletDBmock)

    btw: walletlocation is empty, but the wallettest run thru, even if u use QApplication app(argc, argv) & comment all other tests out.

  21. DrahtBot cross-referenced this on Jun 2, 2020 from issue wallettool: Add dump and createfromdump commands by achow101
  22. DrahtBot cross-referenced this on Jun 2, 2020 from issue Multiprocess bitcoin by ryanofsky
  23. DrahtBot cross-referenced this on Jun 4, 2020 from issue multiprocess: Add basic spawn and IPC support by ryanofsky
  24. DrahtBot cross-referenced this on Jun 5, 2020 from issue refactor: Error message bilingual_str consistency by laanwj
  25. ryanofsky commented at 1:38 PM on June 5, 2020: contributor

    re: tarboss #19098 (comment)

    i tried this

    It sounds like you tried this and got expected result, but let me know if I'm misunderstanding. Thanks for the feedback

  26. DrahtBot added the label Needs rebase on Jun 9, 2020
  27. ryanofsky force-pushed on Jun 10, 2020
  28. DrahtBot removed the label Needs rebase on Jun 10, 2020
  29. DrahtBot cross-referenced this on Jun 11, 2020 from issue Replace automatic bans with discouragement filter by sipa
  30. DrahtBot cross-referenced this on Jun 17, 2020 from issue wallet: BerkeleyDatabase make BerkeleyDatabase::Create, CreateMock, and CreateDummy non-static functions by achow101
  31. DrahtBot added the label Needs rebase on Jun 18, 2020
  32. ryanofsky force-pushed on Jun 19, 2020
  33. DrahtBot removed the label Needs rebase on Jun 19, 2020
  34. DrahtBot cross-referenced this on Jun 20, 2020 from issue build: Do not include server symbols in wallet by MarcoFalke
  35. DrahtBot cross-referenced this on Jun 25, 2020 from issue Remove the automatic creation and loading of the default wallet by achow101
  36. in src/qt/test/apptests.cpp:8 in ab1736cd92 outdated 
       5 | @@ -6,6 +6,7 @@
   6 |  
   7 |  #include <chainparams.h>
   8 |  #include <key.h>
   9 | +#include <interfaces/node.h>
    promag commented at 10:38 PM on July 6, 2020:

    nit, sort.

    ryanofsky commented at 5:16 PM on July 8, 2020:

    re: #19098 (review)

    nit, sort.

    Good catch, fixed

  37. promag commented at 10:41 PM on July 6, 2020: member

    Code review ACK ab1736cd92ee2ce0bdfe3c9bf23594cd2213b659.

    (don't mind the nit unless this needs a push)

  38. DrahtBot added the label Needs rebase on Jul 7, 2020
  39. ryanofsky force-pushed on Jul 8, 2020
  40. ryanofsky force-pushed on Jul 8, 2020
  41. ryanofsky force-pushed on Jul 8, 2020
  42. ryanofsky force-pushed on Jul 8, 2020
  43. ryanofsky commented at 5:18 PM on July 8, 2020: contributor

    Rebased ab1736cd92ee2ce0bdfe3c9bf23594cd2213b659 -> fa522f9b9b3ac78d6f52554ad3f468dcb6e42288 (pr/qtx.5 -> pr/qtx.6, compare) due to conflict with #19219 Updated fa522f9b9b3ac78d6f52554ad3f468dcb6e42288 -> 12135dd28ef7619b1508e98e307c0a66bf7dd4a3 (pr/qtx.6 -> pr/qtx.7, compare) with suggested include fix Rebased 12135dd28ef7619b1508e98e307c0a66bf7dd4a3 -> 2dbdb4355b873223388b3c2a737b1911bc179e43 (pr/qtx.7 -> pr/qtx.8, compare) due to conflict with #18923

  44. DrahtBot removed the label Needs rebase on Jul 8, 2020
  45. DrahtBot cross-referenced this on Jul 8, 2020 from issue util: Make default arg values more specific by hebasto
  46. DrahtBot cross-referenced this on Jul 8, 2020 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
  47. DrahtBot cross-referenced this on Jul 8, 2020 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
  48. DrahtBot added the label Needs rebase on Jul 11, 2020
  49. test: Remove duplicate NodeContext hacks
    Qt tests currently are currently using two NodeContext structs at the same
time, one in interfaces::NodeImpl::m_context, and the other in
BasicTestingSetup::m_node, and the tests have hacks transferring state between
them.

Fix this by getting rid of the NodeImpl::m_context struct and making it a
pointer. This way a common BitcoinApplication object can be used for all qt
tests, but they can still have their own testing setups.

Non-test code is changing but non-test behavior is still the same as before.

Motivation for this PR is to be able to remove the
"std::move(test.m_node.connman)" and mempool hacks for swapping individual
NodeContext members in Qt tests, because followup PR #19099 adds yet another
member (wallet_client) that needs to be swapped. After this change, the whole
NodeContext struct can be swapped instead of individual members, so the
workarounds are less fragile and invasive.
    edc316020e
  50. ryanofsky force-pushed on Jul 13, 2020
  51. DrahtBot removed the label Needs rebase on Jul 13, 2020
  52. in src/test/util/setup_common.cpp:13 in 2dbdb4355b outdated 
       9 | @@ -10,6 +10,7 @@
  10 |  #include <consensus/params.h>
  11 |  #include <consensus/validation.h>
  12 |  #include <crypto/sha256.h>
  13 | +#include <interfaces/chain.h>
    promag commented at 10:38 PM on July 16, 2020:

    nit, sort 🙊 🏃

    ryanofsky commented at 3:55 PM on July 21, 2020:

    re: #19098 (review)

    nit, sort

    Thanks, fixed

  53. in src/qt/bitcoin.h:93 in 2dbdb4355b outdated 
      89 | @@ -90,6 +90,8 @@ class BitcoinApplication: public QApplication
  90 |      /// Setup platform style
  91 |      void setupPlatformStyle();
  92 |  
  93 | +    interfaces::Node& node() { return m_node; }
    promag commented at 10:48 PM on July 16, 2020:

    Add only when needed?

    ryanofsky commented at 3:55 PM on July 21, 2020:

    re: #19098 (review)

    Add only when needed?

    Thanks, removed here. It's added in https://github.com/bitcoin-core/gui/pull/35

  54. DrahtBot cross-referenced this on Jul 20, 2020 from issue Remove mempool global by MarcoFalke
  55. ryanofsky force-pushed on Jul 21, 2020
  56. ryanofsky commented at 3:58 PM on July 21, 2020: contributor

    Thanks for review! Updated with suggestions

    Updated 2dbdb4355b873223388b3c2a737b1911bc179e43 -> edc316020e8270dafc5e31465d532baebdafd3dd (pr/qtx.8 -> pr/qtx.9, compare)

  57. DrahtBot cross-referenced this on Jul 21, 2020 from issue refactor: Get rid of more redundant chain methods by ryanofsky
  58. DrahtBot cross-referenced this on Jul 29, 2020 from issue Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky
  59. DrahtBot cross-referenced this on Jul 30, 2020 from issue Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior
  60. MarcoFalke commented at 6:39 AM on July 31, 2020: member

    crACK edc316020e8270dafc5e31465d532baebdafd3dd 🌮

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

crACK edc316020e8270dafc5e31465d532baebdafd3dd 🌮
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgNVQv/Rp1DWQ63V3hRrPG3/Uu713VE7nYlG/B4T37Jkj0PVR5vKFj0pkT45s9Y
3yDHFDhP+yTQjH5e+mCATQNs9+60D/1f6776O7CLIkiZ2Mh+K+g6Q807feo+AVTu
R7Ra56ZgXADrIvICWu7N7AOElNh/gwA+JHX13DcS4abLo13c+Xa9X5ILAB3L2uV+
Vv8LxivWpo/yF6pNifVsj6rMuvpNzjvKimYM2gu8zPKuXhRye+KqM6KkuFUgiY2B
TY/c0MGzhbxNTXhiCnYqfgNRT4s6ZO35W3CzReX72Y6g3LsjQ4sTp+pi67vkkP84
vo5OhC6XgFvX8/ZS66eyPmaOtkmxP7ehydr16NDosACfnnI7SDTGeSApICTyn00i
ynEE2kcXL/KY1wKhcGKq0J6mbXuzTPL5GGOJTkxyXeFdJCYe1PEPv0E1wvt0f7vR
AKeEjSlf7J3Wv4PT+1OdcrFVSZ19YC27z1mQ1q/UMNTZvwqLfMIm7rTcFpKjHESs
3zxlj4ev
=9d77
-----END PGP SIGNATURE-----

    Timestamp of file with hash e4953628661a432a6114bf785c7dd10e546b98aee3b7f032fe9b6cd632fd6334 -

    </details>

  61. MarcoFalke closed this on Jul 31, 2020
  62. MarcoFalke reopened this on Jul 31, 2020
  63. MarcoFalke removed the label GUI on Jul 31, 2020
  64. promag commented at 10:07 PM on August 6, 2020: member

    ACK edc316020e8270dafc5e31465d532baebdafd3dd.

  65. MarcoFalke merged this on Aug 7, 2020
  66. MarcoFalke closed this on Aug 7, 2020
  67. sidhujag referenced this in commit 1e78f38193 on Aug 7, 2020
  68. ryanofsky cross-referenced this on Aug 7, 2020 from issue Parse params directly instead of through node (partial revert #10244) by ryanofsky
  69. jasonbcox referenced this in commit c146a4640b on Oct 20, 2020
  70. ryanofsky referenced this in commit 5baa88fd38 on Dec 8, 2020
  71. bitcoin locked this on Feb 15, 2022
Contributors
ryanofskyMarcoFalkeDrahtBottarbosspromag
Labels
Tests
Linked (view graph)
#18923 wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off#19099 refactor: Move wallet methods out of chain.h and node.h#19101 refactor: remove ::vpwallets and related global variables
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:53 UTC