test: Remove duplicate NodeContext hacks #19098

ryanofsky · 2020-05-28T21:02:58Z

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.

maflcko · 2020-05-28T22:33:43Z

src/qt/test/apptests.cpp

@@ -63,6 +64,7 @@ void AppTests::appTests()
 #endif

    BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to


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

re: #19098 (comment)

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 ->

DrahtBot · 2020-05-29T06:50:04Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

ryanofsky

Updated 09569ff -> 8569817 (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 8569817 -> 934a8ae (pr/qtx.2 -> pr/qtx.3, compare) fixing pedantic asan null reference error https://travis-ci.org/github/bitcoin/bitcoin/jobs/692528901#L4294
Rebased 934a8ae -> d2c9cd7 (pr/qtx.3 -> pr/qtx.4, compare) due to conflict with #19176
Rebased d2c9cd7 -> ab1736c (pr/qtx.4 -> pr/qtx.5, compare) due to conflict with #19310

ryanofsky · 2020-05-29T04:25:50Z

src/qt/test/apptests.cpp

@@ -63,6 +64,7 @@ void AppTests::appTests()
 #endif

    BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to


re: #19098 (comment)

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 ->

tarboss · 2020-05-30T08:19:36Z

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(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.

ryanofsky · 2020-06-05T13:38:20Z

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

ryanofsky

Rebased ab1736c -> fa522f9 (pr/qtx.5 -> pr/qtx.6, compare) due to conflict with #19219
Updated fa522f9 -> 12135dd (pr/qtx.6 -> pr/qtx.7, compare) with suggested include fix
Rebased 12135dd -> 2dbdb43 (pr/qtx.7 -> pr/qtx.8, compare) due to conflict with #18923

src/qt/test/apptests.cpp

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 bitcoin#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.

promag · 2020-07-16T22:38:20Z

src/test/util/setup_common.cpp

@@ -10,6 +10,7 @@
 #include <consensus/params.h>
 #include <consensus/validation.h>
 #include <crypto/sha256.h>
+#include <interfaces/chain.h>


nit, sort 🙊 🏃

re: #19098 (comment)

nit, sort

Thanks, fixed

promag · 2020-07-16T22:48:25Z

src/qt/bitcoin.h

@@ -90,6 +90,8 @@ class BitcoinApplication: public QApplication
    /// Setup platform style
    void setupPlatformStyle();

+    interfaces::Node& node() { return m_node; }


Add only when needed?

re: #19098 (comment)

Add only when needed?

Thanks, removed here. It's added in bitcoin-core/gui#35

ryanofsky

Thanks for review! Updated with suggestions

Updated 2dbdb43 -> edc3160 (pr/qtx.8 -> pr/qtx.9, compare)

ryanofsky · 2020-07-21T15:55:12Z

src/qt/bitcoin.h

@@ -90,6 +90,8 @@ class BitcoinApplication: public QApplication
    /// Setup platform style
    void setupPlatformStyle();

+    interfaces::Node& node() { return m_node; }


re: #19098 (comment)

Add only when needed?

Thanks, removed here. It's added in bitcoin-core/gui#35

ryanofsky · 2020-07-21T15:55:20Z

src/test/util/setup_common.cpp

@@ -10,6 +10,7 @@
 #include <consensus/params.h>
 #include <consensus/validation.h>
 #include <crypto/sha256.h>
+#include <interfaces/chain.h>


re: #19098 (comment)

nit, sort

Thanks, fixed

maflcko · 2020-07-31T06:39:01Z

crACK edc3160 🌮

Show signature and timestamp

Signature:

Timestamp of file with hash e4953628661a432a6114bf785c7dd10e546b98aee3b7f032fe9b6cd632fd6334 -

promag

ACK edc3160.

edc3160 test: Remove duplicate NodeContext hacks (Russell Yanofsky) Pull request description: 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 bitcoin#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. ACKs for top commit: MarcoFalke: crACK edc3160 🌮 promag: ACK edc3160. Tree-SHA512: c1650e4127f43a4020304ca7c13b5d9122fb5723aacd8fa1cf855d03c6052fcfb7685810aa2a5ef708561015f0022fecaacbad479295104ca45d2c17579466a4

Summary: ``` 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. ``` Backport of core [[bitcoin/bitcoin#19098 | PR19098]]. Depends on D7992. Test Plan: ninja all check-all ./contrib/teamcity/build-configurations.py build-ubsan Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7993

These calls are no longer needed after edc3160 from bitcoin#19098 which started instantiating BasicTestingSetup.m_node.chain Patch from MarcoFalke <falke.marco@gmail.com> in bitcoin#19425 (comment) Co-authored-by: MarcoFalke <falke.marco@gmail.com>

DrahtBot added GUI Tests labels May 28, 2020

This was referenced May 28, 2020

refactor: Move wallet methods out of chain.h and node.h #19099

Merged

refactor: remove ::vpwallets and related global variables #19101

Merged

maflcko reviewed May 28, 2020

View reviewed changes

DrahtBot mentioned this pull request May 29, 2020

wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102

Merged

ryanofsky force-pushed the pr/qtx branch from 09569ff to 8569817 Compare May 29, 2020 12:04

ryanofsky commented May 29, 2020

View reviewed changes

bitcoin deleted a comment from vahidjalaliii May 29, 2020

ryanofsky force-pushed the pr/qtx branch from 8569817 to 934a8ae Compare May 29, 2020 17:33

This was referenced Jun 2, 2020

wallettool: Add dump and createfromdump commands #19137

Merged

Multiprocess bitcoin #10102

Draft

multiprocess: Add basic spawn and IPC support #19160

Merged

refactor: Error message bilingual_str consistency #19176

Merged

DrahtBot added the Needs rebase label Jun 9, 2020

ryanofsky force-pushed the pr/qtx branch from 934a8ae to d2c9cd7 Compare June 10, 2020 18:25

DrahtBot removed the Needs rebase label Jun 10, 2020

This was referenced Jun 11, 2020

Replace automatic bans with discouragement filter #19219

Merged

wallet: BerkeleyDatabase make BerkeleyDatabase::Create, CreateMock, and CreateDummy non-static functions #19310

Merged

DrahtBot added the Needs rebase label Jun 18, 2020

ryanofsky force-pushed the pr/qtx branch 3 times, most recently from fa522f9 to 12135dd Compare July 8, 2020 17:18

ryanofsky commented Jul 8, 2020

View reviewed changes

src/qt/test/apptests.cpp Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label Jul 8, 2020

This was referenced Jul 8, 2020

util: Make default arg values more specific #19471

Closed

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

DrahtBot added the Needs rebase label Jul 11, 2020

ryanofsky force-pushed the pr/qtx branch from 12135dd to 2dbdb43 Compare July 13, 2020 20:03

DrahtBot removed the Needs rebase label Jul 13, 2020

promag reviewed Jul 16, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 20, 2020

Remove mempool global #19556

Merged

ryanofsky force-pushed the pr/qtx branch from 2dbdb43 to edc3160 Compare July 21, 2020 15:58

ryanofsky commented Jul 21, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 21, 2020

refactor: Get rid of more redundant chain methods #19425

Merged

This was referenced Jul 29, 2020

Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp #19619

Merged

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766

Merged

maflcko closed this Jul 31, 2020

maflcko reopened this Jul 31, 2020

maflcko removed the GUI label Jul 31, 2020

promag reviewed Aug 6, 2020

View reviewed changes

maflcko merged commit 4b705b1 into bitcoin:master Aug 7, 2020

ryanofsky mentioned this pull request Aug 7, 2020

Parse params directly instead of through node (partial revert #10244) bitcoin-core/gui#35

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		@@ -63,6 +64,7 @@ void AppTests::appTests()
		#endif

		BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to

test: Remove duplicate NodeContext hacks #19098

test: Remove duplicate NodeContext hacks #19098

Uh oh!

Conversation

ryanofsky commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarboss commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jun 5, 2020

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 31, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented May 28, 2020 •

edited

Loading

DrahtBot commented May 29, 2020 •

edited

Loading

ryanofsky left a comment •

edited

Loading

tarboss commented May 30, 2020 •

edited

Loading

ryanofsky left a comment •

edited

Loading