test: Add CreateWalletFromFile test #18727

ryanofsky · 2020-04-21T19:54:23Z

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 ef8c6ca from #16426, but succeed with it:

bitcoin/src/wallet/wallet.cpp

Lines 3978 to 3986 in ef8c6ca

    
           // Register wallet with validationinterface. It's done before rescan to avoid 
        
           // missing block connections between end of rescan and validation subscribing. 
        
           // Because of wallet lock being hold, block connection notifications are going to 
        
           // be pending on the validation-side until lock release. It's likely to have 
        
           // block processing duplicata (if rescan block range overlaps with notification one) 
        
           // but we guarantee at least than wallet state is correct after notifications delivery. 
        
           // This is temporary until rescan and notifications delivery are unified under same 
        
           // interface. 
        
           walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance);

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 · 2020-04-22T00:42:21Z

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

Conflicts

No conflicts as of last run.

ariard

Concept ACK, I can also take it on top of #16426, with your compatible version at 25651aa, either

ariard · 2020-04-23T01:16:53Z

src/wallet/test/wallet_tests.cpp

+    }
+
+    // Unblock notification queue and make sure stale blockConnected and
+    // transactionAddedToMempool events are processed


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

re: #18727 (comment)

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

ariard · 2020-04-23T01:20:50Z

src/wallet/test/wallet_tests.cpp

@@ -634,4 +666,80 @@ BOOST_FIXTURE_TEST_CASE(dummy_input_size_test, TestChain100Setup)
    BOOST_CHECK_EQUAL(CalculateNestedKeyhashInputSize(true), DUMMY_NESTED_P2WPKH_INPUT_SIZE);
 }

+//! Test CreateWalletFromFile function and its behavior handling potential race
+//! conditions if it's called the same time an incoming transaction shows up in
+//! the mempool or a new block.


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

re: #18727 (comment)

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 d6d6632 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 25651aa (branch) #16426 (comment) if something about that is better

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 ef8c6ca from bitcoin#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 bitcoin#16426. So this PR just adds as much test coverage as is possible now. This new test is also useful for bitcoin#15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates.

promag

Concept ACK, obviously.

ryanofsky · 2020-04-27T18:42:17Z

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

ryanofsky

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

ryanofsky · 2020-04-28T13:48:00Z

src/wallet/test/wallet_tests.cpp

+    }
+
+    // Unblock notification queue and make sure stale blockConnected and
+    // transactionAddedToMempool events are processed


re: #18727 (comment)

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

ryanofsky · 2020-04-28T13:48:14Z

src/wallet/test/wallet_tests.cpp

@@ -634,4 +666,80 @@ BOOST_FIXTURE_TEST_CASE(dummy_input_size_test, TestChain100Setup)
    BOOST_CHECK_EQUAL(CalculateNestedKeyhashInputSize(true), DUMMY_NESTED_P2WPKH_INPUT_SIZE);
 }

+//! Test CreateWalletFromFile function and its behavior handling potential race
+//! conditions if it's called the same time an incoming transaction shows up in
+//! the mempool or a new block.


re: #18727 (comment)

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 d6d6632 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 25651aa (branch) #16426 (comment) if something about that is better

maflcko

ACK

maflcko · 2020-04-29T15:55:51Z

src/test/util/logging.h

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


Suggested change

using MatchFn = std::function<bool(const std::string*)>;

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?

re: #18727 (comment)

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.

src/test/util/logging.h

ryanofsky

re: #18727 (review)

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 66a4d4b -> 7918c1b (pr/create.3 -> pr/create.4, compare) adding suggested MatchFn documentation.

ryanofsky · 2020-04-29T17:20:59Z

src/test/util/logging.h

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


re: #18727 (comment)

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.

src/test/util/logging.h

maflcko · 2020-04-29T18:04:27Z

ACK 7918c1b

jonatack

ACK 7918c1b

Thanks for adding these tests.

ariard · 2020-04-30T09:07:51Z

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

maflcko · 2020-04-30T11:11:48Z

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

maflcko · 2020-04-30T11:15:06Z

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

ryanofsky · 2020-04-30T14:02:18Z

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: 25651aa (branch) in #16426 (comment), so there should be no new code to write, and only a little bit to review

maflcko · 2020-04-30T14:15:28Z

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 · 2020-04-30T14:19:51Z

I'll rebase 25651aa, 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 · 2020-04-30T19:09:12Z

@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 :)

7918c1b test: Add CreateWalletFromFile test (Russell Yanofsky) Pull request description: 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 ef8c6ca from bitcoin#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 bitcoin#16426. So this PR just adds as much test coverage as is possible now. This new test is also useful for bitcoin#15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates. ACKs for top commit: MarcoFalke: ACK 7918c1b jonatack: ACK 7918c1b Tree-SHA512: 44035aee698ecb722c6039d061d8fac2011e9da0b314e4aff19be1d610b53cacff99016b34d6b84669bb3b61041b2318d9d8e3363658f087802ae4aa36ca17b8

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 ef8c6ca from bitcoin#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 bitcoin#16426. So this PR just adds as much test coverage as is possible now. This new test is also useful for bitcoin#15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates. Github-Pull: bitcoin#18727 Rebased-From: 7918c1b

Summary: 7918c1b019a36a8f9aa55daae422c6b6723b2a39 test: Add CreateWalletFromFile test (Russell Yanofsky) Pull request description: 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 bitcoin/bitcoin#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 bitcoin/bitcoin#15719, since it detects the stale notifications.transactionAddedToMempool notifications that PR eliminates. --- Backport of Core [[bitcoin/bitcoin#18727 | PR18727]] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9066

ryanofsky mentioned this pull request Apr 21, 2020

Reverse cs_main, cs_wallet lock order and reduce cs_main locking #16426

Merged

DrahtBot added Tests Wallet labels Apr 21, 2020

DrahtBot mentioned this pull request Apr 22, 2020

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

ariard reviewed Apr 23, 2020

View reviewed changes

This was referenced Apr 23, 2020

External signer support - Wallet Box edition #16546

Merged

UI external signer support (e.g. hardware wallet) #16549

Closed

DrahtBot added the Needs rebase label Apr 27, 2020

promag reviewed Apr 27, 2020

View reviewed changes

ryanofsky force-pushed the pr/create branch from f9cea72 to 2142bf1 Compare April 27, 2020 18:34

DrahtBot removed the Needs rebase label Apr 27, 2020

DrahtBot mentioned this pull request Apr 28, 2020

Use shared pointers only in validation interface #18354

Closed

ryanofsky force-pushed the pr/create branch from 2142bf1 to 66a4d4b Compare April 28, 2020 14:15

ryanofsky commented Apr 28, 2020

View reviewed changes

maflcko approved these changes Apr 29, 2020

View reviewed changes

ryanofsky force-pushed the pr/create branch from 66a4d4b to 7918c1b Compare April 29, 2020 18:00

ryanofsky commented Apr 29, 2020

View reviewed changes

jonatack reviewed Apr 29, 2020

View reviewed changes

maflcko merged commit 0f204dd into bitcoin:master Apr 29, 2020

ryanofsky mentioned this pull request Apr 30, 2020

Wallet passive startup #15719

Closed

hebasto mentioned this pull request May 1, 2020

gui: Bilingual GUI error messages #16224

Merged

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

	// Register wallet with validationinterface. It's done before rescan to avoid
	// missing block connections between end of rescan and validation subscribing.
	// Because of wallet lock being hold, block connection notifications are going to
	// be pending on the validation-side until lock release. It's likely to have
	// block processing duplicata (if rescan block range overlaps with notification one)
	// but we guarantee at least than wallet state is correct after notifications delivery.
	// This is temporary until rescan and notifications delivery are unified under same
	// interface.
	walletInstance->m_chain_notifications_handler = walletInstance->chain().handleNotifications(walletInstance);

	using MatchFn = std::function<bool(const std::string*)>;
	using MatchCb = std::function<void(const std::string&)>;

test: Add CreateWalletFromFile test #18727

test: Add CreateWalletFromFile test #18727

Uh oh!

Conversation

ryanofsky commented Apr 21, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Apr 27, 2020

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

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Apr 29, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

ariard commented Apr 30, 2020

Uh oh!

maflcko commented Apr 30, 2020

Uh oh!

maflcko commented Apr 30, 2020

Uh oh!

ryanofsky commented Apr 30, 2020

Uh oh!

maflcko commented Apr 30, 2020

Uh oh!

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

Uh oh!

Uh oh!

ariard commented Apr 30, 2020

Uh oh!

Uh oh!

DrahtBot commented Apr 22, 2020 •

edited

Loading

ryanofsky commented Apr 30, 2020 •

edited

Loading