test: Add CreateWalletFromFile test

ryanofsky commented at 7:54 pm on April 21, 2020: member

Add unit test calling CreateWalletFromFile, which isn’t currently called from other unit tests, with some basic checks to make sure it rescans and registers for notifications correctly.

Motivation for this change was to try to write a test that would fail without the early handleNotifications call in ef8c6ca60767cac589d98ca57ee33179608ccda8 from #16426, but succeed with it:

https://github.com/bitcoin/bitcoin/blob/ef8c6ca60767cac589d98ca57ee33179608ccda8/src/wallet/wallet.cpp#L3978-L3986

However, writing a full test for the race condition that call prevents isn’t possible without the locking changes from #16426. So this PR just adds as much test coverage as is possible now.

This new test is also useful for #15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates.

DrahtBot added the label Tests on Apr 21, 2020

DrahtBot added the label Wallet on Apr 21, 2020

DrahtBot commented at 0:42 am on April 22, 2020: member

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

Conflicts

No conflicts as of last run.

in src/wallet/test/wallet_tests.cpp:762 in f9cea7231d outdated

732+        BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_tx.GetHash()), 1);
733+        BOOST_CHECK_EQUAL(wallet->mapWallet.count(mempool_tx.GetHash()), 1);
734+    }
735+
736+    // Unblock notification queue and make sure stale blockConnected and
737+    // transactionAddedToMempool events are processed

ariard commented at 1:16 am on April 23, 2020:

Transactions contained by stale blockConnected and transactionAddedToMempool should be processed twice ? May we assert that their nTimeReceived hasn’t change between duplicate processing?

ryanofsky commented at 1:48 pm on April 28, 2020:

re: #18727 (review)

Transactions contained by stale blockConnected and transactionAddedToMempool should be processed twice ? May we assert that their nTimeReceived hasn’t change between duplicate processing?

Interesting suggestion, I added these checks.

I don’t think they should ideally be processed twice, and this was my original motivation for writing #15719: to get rid of stale, out of order events. But for now they are processed twice, so this PR just gets better test coverage in place before #15719

in src/wallet/test/wallet_tests.cpp:695 in f9cea7231d outdated

665@@ -634,4 +666,80 @@ BOOST_FIXTURE_TEST_CASE(dummy_input_size_test, TestChain100Setup)
666     BOOST_CHECK_EQUAL(CalculateNestedKeyhashInputSize(true), DUMMY_NESTED_P2WPKH_INPUT_SIZE);
667 }
668 
669+//! Test CreateWalletFromFile function and its behavior handling potential race
670+//! conditions if it's called the same time an incoming transaction shows up in
671+//! the mempool or a new block.

ariard commented at 1:20 am on April 23, 2020:

I think you can just copy-paste actual new comment in d6d6632, which is originally yours (IIRC), I found it clearer

ryanofsky commented at 1:48 pm on April 28, 2020:

re: #18727 (review)

I think you can just copy-paste actual new comment in d6d6632, which is originally yours (IIRC), I found it clearer

I don’t think I understand this suggestion. This comment is describing what the test does, and I don’t think anything in d6d6632edd3465e2b577c9eeedad90b061f8a392 would be reusable here to describe a new test. I did have a slightly different comment in the alternate version of the test I posted 25651aad58b1f6e543f1ad565d821de46268e724 (branch) #16426 (review) if something about that is better

ariard commented at 1:22 am on April 23, 2020: member

Concept ACK, I can also take it on top of #16426, with your compatible version at https://github.com/bitcoin/bitcoin/commit/25651aad58b1f6e543f1ad565d821de46268e724, either

test: Add CreateWalletFromFile test

Add unit test calling CreateWalletFromFile, which isn't currently called from
other unit tests, with some basic checks to make sure it rescans and registers
for notifications correctly.

Motivation for this change was to try to write a test that would fail without
the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8
from https://github.com/bitcoin/bitcoin/pull/16426, but succeed with it:

https://github.com/bitcoin/bitcoin/blob/ef8c6ca60767cac589d98ca57ee33179608ccda8/src/wallet/wallet.cpp#L3978-L3986

However, writing a full test for the race condition that call prevents isn't
possible without the locking changes from #16426. So this PR just adds as much
test coverage as is possible now.

This new test is also useful for https://github.com/bitcoin/bitcoin/pull/15719,
since it detects the stale notifications.transactionAddedToMempool notifications
that PR eliminates.

7918c1b019

DrahtBot added the label Needs rebase on Apr 27, 2020

promag commented at 0:37 am on April 27, 2020: member

Concept ACK, obviously.

ryanofsky force-pushed on Apr 27, 2020

ryanofsky commented at 6:42 pm on April 27, 2020: member

Rebased f9cea7231dfec9c75b206899c818301401d8c0d4 -> 2142bf14fc72b8555ca81af5913e329ceda8f782 (pr/create.1 -> pr/create.2, compare) due to conflict with #16528

DrahtBot removed the label Needs rebase on Apr 27, 2020

ryanofsky force-pushed on Apr 28, 2020

ryanofsky commented at 2:18 pm on April 28, 2020: member

Updated 2142bf14fc72b8555ca81af5913e329ceda8f782 -> 66a4d4b2ec76d507b089918d2ba944024e300549 (pr/create.2 -> pr/create.3, compare) with suggestion to check nTimeReceived after stale notifications

in src/test/util/logging.h:19 in 66a4d4b2ec outdated

15@@ -16,11 +16,13 @@ class DebugLogHelper
16     const std::string m_message;
17     bool m_found{false};
18     std::list<std::function<void(const std::string&)>>::iterator m_print_connection;
19+    using MatchFn = std::function<bool(const std::string*)>;

MarcoFalke commented at 3:55 pm on April 29, 2020:

0    using MatchCb = std::function<void(const std::string&)>;

why does this need a return value? From glancing over it, it seems the same can be achieved with a callback that circumvents m_found and m_cb is set to nullptr by default.

I presume you want to make this as flexible as possible for potential future use cases?

ryanofsky commented at 5:20 pm on April 29, 2020:

re: #18727 (review)

why does this need a return value? From glancing over it, it seems the same can be achieved with a callback that circumvents m_found and m_cb is set to nullptr by default.

I added comment to clarify what this is doing. Suggestion to change return and argument types seems like it would complicate DebugLogHelper implementation instead of simplifying it, and make behavior specific to this particular test case (no ability to stop searching after a custom match and customize behavior if no match is found). I also like Fn _fn suffixes over Cb and am using them in interfaces and wallet code to be consistent with standard library naming and std::function, similar to how I like mutex more than cs to be consistent with std::mutex.

in src/test/util/logging.h:30 in 66a4d4b2ec outdated

15@@ -16,11 +16,13 @@ class DebugLogHelper
16     const std::string m_message;
17     bool m_found{false};
18     std::list<std::function<void(const std::string&)>>::iterator m_print_connection;
19+    using MatchFn = std::function<bool(const std::string*)>;
20+    MatchFn m_match;

MarcoFalke commented at 3:56 pm on April 29, 2020:

Could add a docstring here on how to use it?

ryanofsky commented at 5:21 pm on April 29, 2020:

re: #18727 (review)

Could add a docstring here on how to use it?

Yes, added this

MarcoFalke approved

MarcoFalke commented at 3:56 pm on April 29, 2020: member

ACK

ryanofsky force-pushed on Apr 29, 2020

ryanofsky commented at 6:02 pm on April 29, 2020: member

re: #18727#pullrequestreview-402808591

Thanks for the review! Without knowing exact details of your ideal DebugLogHelper API I’d prefer not to have to make guesses and implement it myself. But I’ll happily ACK a followup PR or take changes here if you want to implement them.

Updated 66a4d4b2ec76d507b089918d2ba944024e300549 -> 7918c1b019a36a8f9aa55daae422c6b6723b2a39 (pr/create.3 -> pr/create.4, compare) adding suggested MatchFn documentation.

MarcoFalke commented at 6:04 pm on April 29, 2020: member

ACK 7918c1b019a36a8f9aa55daae422c6b6723b2a39

jonatack commented at 7:04 pm on April 29, 2020: member

ACK 7918c1b019a36a8f9aa55daae422c6b6723b2a39

Thanks for adding these tests.

MarcoFalke merged this on Apr 29, 2020

MarcoFalke closed this on Apr 29, 2020

ariard commented at 9:07 am on April 30, 2020: member

@MarcoFalke, I may miss ACK timeline but you could have wait #16426 first to avoid another round of review :(

MarcoFalke commented at 11:11 am on April 30, 2020: member

@ariard Sorry :sob: , but I didn’t feel comfortable merging #16426 with the assert removed. So 16426 would have to go through another round of review anyway.

MarcoFalke commented at 11:15 am on April 30, 2020: member

It seems two other reviewers agreed with me: #16426 (comment) and this one seemed ready to merge.

ryanofsky commented at 2:02 pm on April 30, 2020: member

I though #16426 was ready to merge, but it should be pretty easy to update #16426 after this PR. All this PR does is add a test, and I posted a version of the test that works with #16426 previously: 25651aad58b1f6e543f1ad565d821de46268e724 (branch) in #16426 (review), so there should be no new code to write, and only a little bit to review

MarcoFalke commented at 2:15 pm on April 30, 2020: member

How hard is it to adapt the new tests? As you have already done the work, could you rebase 25651aa on current master+the other pull. So it can simply be cherry-picked by @ariard and we can move forward with #16426 and merge it?

ryanofsky commented at 2:19 pm on April 30, 2020: member

I’ll rebase 25651aad58b1f6e543f1ad565d821de46268e724, but it’s should also be pretty simple for antoine to just copy and paste working test code https://github.com/ryanofsky/bitcoin/blob/25651aad58b1f6e543f1ad565d821de46268e724/src/wallet/test/wallet_tests.cpp#L669-L771 into the last commit over the broken test

ariard commented at 7:09 pm on April 30, 2020: member

@MarcoFalke yes you were right, I didn’t know how to interpret your Approach ACK at first look, given your also quoted a commit. Maybe it’s better in this kind of case to not ACK at all to underlies fix matters for the reviewer. Nevermind let’s move forward :)

sidhujag referenced this in commit d7c1f85877 on May 2, 2020

luke-jr referenced this in commit 74de8d57b2 on Aug 15, 2020

Fabcien referenced this in commit a8ce4f8ed8 on Feb 9, 2021

DrahtBot locked this on Feb 15, 2022

test: Add CreateWalletFromFile test #18727

Conflicts