wallet test: Add test for subtract fee from recipient behavior

ryanofsky commented at 11:10 pm on June 4, 2021: member

This adds test coverage for wallet subtract from recipient behavior without changing it. Behavior seems to have changed recently in a minor way in #17331 without being noticed.

ryanofsky commented at 11:26 pm on June 4, 2021: member

I’m happy to drop the last commit if it’s not wanted. Main goal of this PR is to add test coverage, which is what the first two commits do. The code cleanup in the third commit could be done in another PR or replaced with a different approach.

UPDATE: Final commit def672f8706e13f4153ff9aca3fb9e1becddae26 is dropped for now to make the PR smaller and test-only

DrahtBot added the label Build system on Jun 4, 2021

DrahtBot added the label Wallet on Jun 4, 2021

fanquake removed the label Build system on Jun 5, 2021

fanquake added the label Tests on Jun 5, 2021

DrahtBot commented at 5:26 am on June 5, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22260 by Sjors
#22183 by kiminuo
#22100 by ryanofsky
#21206 by ryanofsky
#19101 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.

in src/wallet/test/spend_tests.cpp:28 in e459d013d7 outdated

23+    CRecipient recipient{GetScriptForRawPubKey({}), 50 * COIN, true /* subtract fee */};
24+    CTransactionRef tx;
25+    CAmount fee;
26+    int change_pos = -1;
27+    bilingual_str error;
28+    CCoinControl coin_control;

achow101 commented at 11:50 pm on June 7, 2021:

In e459d013d7f2e92d51e52fdf3dd7ccdd3c57db68 “wallet test: Add test for subtract fee from recipient behavior”

Since fee estimation is not available in tests, the feerate that is used by this test is 0, which makes it not particularly meaningful. But we can set a feerate in the CCoinControl and force it to be used, e.g.

0    CCoinControl coin_control;
1    coin_control.m_feerate.emplace(10000);
2    coin_control.fOverrideFeeRate = true;

ryanofsky commented at 3:24 am on June 8, 2021:

Since fee estimation is not available in tests, the feerate that is used by this test is 0, which makes it not particularly meaningful. But we can set a feerate in the CCoinControl and force it to be used, e.g.

Thank you, great catch! I had assumed a nonzero minimum relay fee would be used. I think the tests still did cover relevant to_reduce == 0 and to_reduce < 0 cases, but having a nonzero fee is obviously more realistic.

achow101 commented at 0:07 am on June 8, 2021: member

Concept ACK

I don’t agree with the behavior in the case where the dust change is more than fees, but since this doesn’t change the current behavior, I guess it’s fine.

Here is a test for that edge case:

 0diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
 1index 8c7794eb47..41ee5f3ea0 100644
 2--- a/src/wallet/test/spend_tests.cpp
 3+++ b/src/wallet/test/spend_tests.cpp
 4@@ -26,6 +26,8 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
 5     int change_pos = -1;
 6     bilingual_str error;
 7     CCoinControl coin_control;
 8+    coin_control.m_feerate.emplace(10000);
 9+    coin_control.fOverrideFeeRate = true;
10     FeeCalculation fee_calc;
11     BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
12     BOOST_CHECK_EQUAL(tx->vout.size(), 1);
13@@ -35,14 +37,26 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
14     // Check subtract from recipient transaction slightly less than coinbase
15     // amount also does not create change output and pays extra dust amount to
16     // recipient instead of miner
17-    const CAmount dust_amount = 123;
18-    const CAmount expected_fee = fee;
19+    CAmount dust_amount = 123;
20+    CAmount expected_fee = fee;
21     recipient.nAmount -= dust_amount;
22     BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
23     BOOST_CHECK_EQUAL(tx->vout.size(), 1);
24     BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount - fee + dust_amount);
25     BOOST_CHECK_EQUAL(fee, expected_fee);
26     tx.reset();
27+
28+    // The dust amount is greater than the fee paid for this transaction.
29+    // The entire dust amount should end up as fees, rather than the excess fee paid to the recipients.
30+    dust_amount = 1350;
31+    expected_fee = dust_amount;
32+    recipient.nAmount = 50 * COIN - dust_amount;
33+    BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
34+    BOOST_CHECK_EQUAL(tx->vout.size(), 1);
35+    BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount - fee + dust_amount);
36+    BOOST_CHECK_EQUAL(fee, expected_fee);
37+    BOOST_CHECK_LE(tx->vout[0].nValue, recipient.nAmount);
38+    tx.reset();
39 }
40 
41 BOOST_AUTO_TEST_SUITE_END()

ryanofsky force-pushed on Jun 8, 2021

ryanofsky commented at 1:47 pm on June 8, 2021: member

Thanks for review! I incorporated all your test suggestions and new cases with a few additional tweaks.

Updated 73bd26cb3df75c1e5f8d2686e4d8a5a32f1409f2 -> 972e87c08268879576b9287b5998739bef1c4cc1 (pr/subfee.2 -> pr/subfee.3, compare) with suggested test improvements

achow101 commented at 6:22 pm on June 8, 2021: member

ACK 972e87c08268879576b9287b5998739bef1c4cc1

DrahtBot added the label Needs rebase on Jun 9, 2021

ryanofsky force-pushed on Jun 10, 2021

ryanofsky commented at 12:26 pm on June 10, 2021: member

Rebased 972e87c08268879576b9287b5998739bef1c4cc1 -> 8f6113f51de8b04ae39be897b2dedf98c3e8a54b (pr/subfee.3 -> pr/subfee.4, compare) due to conflict with #22008

jonatack commented at 12:50 pm on June 10, 2021: member

Concept ACK, good to see test coverage here.

DrahtBot removed the label Needs rebase on Jun 10, 2021

ryanofsky force-pushed on Jun 10, 2021

ryanofsky commented at 2:56 pm on June 10, 2021: member

Updated 8f6113f51de8b04ae39be897b2dedf98c3e8a54b -> 6a50482691964c782eecea573abeff0e2169dfcb (pr/subfee.4 -> pr/subfee.5, compare) to fix appveyor link error https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/39546773#L32

achow101 commented at 6:26 pm on June 10, 2021: member

re-ACK 6a50482691964c782eecea573abeff0e2169dfcb

DrahtBot added the label Needs rebase on Jun 12, 2021

wallet test refactor: add CreateSyncedWallet function

No change in behavior. This just moves some code from the ListCoins test
setup to a reusable util function, so it can be reused in a new test in
the next commit.

2565478c81

wallet test: Add test for subtract fee from recipient behavior

Behavior might have recently changed in #17331 (it is not clear) but not
noticed because there is no test coverage.

This adds test coverage for current subtract from recipient behavior
without changing it.

Co-authored-by: Andrew Chow <achow101-github@achow101.com>

fe6dc76b7c

ryanofsky force-pushed on Jun 12, 2021

ryanofsky commented at 6:46 pm on June 12, 2021: member

Rebased 6a50482691964c782eecea573abeff0e2169dfcb -> 0cc10f8377495fb60156d116307848674e4be38e (pr/subfee.5 -> pr/subfee.6, compare) due to conflict with #21866

DrahtBot removed the label Needs rebase on Jun 12, 2021

jonatack commented at 4:09 pm on July 14, 2021: member

ACK 0cc10f8377495fb60156d116307848674e4be38e verified the new test also passes without the last commit

 0diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
 1index 45a02f59a1..b3a8d18446 100644
 2--- a/src/wallet/spend.cpp
 3+++ b/src/wallet/spend.cpp
 4@@ -764,8 +764,7 @@ bool CWallet::CreateTransactionInternal(
 5     // 1. The change output would be dust
 6     // 2. The change is within the (almost) exact match window, i.e. it is less than or equal to the cost of the change output (cost_of_change)
 7     CAmount change_amount = change_position->nValue;
 8-    if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change)
 9-    {
10+    if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change) {
11         nChangePosInOut = -1;
12         change_amount = 0;
13         txNew.vout.erase(change_position);
14diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
15index a202dd9146..66e7de4273 100644
16--- a/src/wallet/test/spend_tests.cpp
17+++ b/src/wallet/test/spend_tests.cpp
18@@ -22,7 +22,7 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
19     // be uneconomical to add and spend the output), and make sure it pays the
20     // leftover input amount which would have been change to the recipient
21     // instead of the miner.
22-    auto check_tx = [&](CAmount leftover_input_amount) {
23+    auto check_tx = [&wallet](CAmount leftover_input_amount) {
24         CRecipient recipient{GetScriptForRawPubKey({}), 50 * COIN - leftover_input_amount, true /* subtract fee */};
25         CTransactionRef tx;
26         CAmount fee;
27@@ -41,7 +41,7 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
28 
29     // Send full input amount to recipient, check that only nonzero fee is
30     // subtracted (to_reduce == fee).
31-    const CAmount fee = check_tx(0);
32+    const CAmount fee{check_tx(0)};
33 
34     // Send slightly less than full input amount to recipient, check leftover
35     // input amount is paid to recipient not the miner (to_reduce == fee - 123)
36diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
37index 097abf9310..f156dbc71d 100644
38--- a/src/wallet/test/util.cpp
39+++ b/src/wallet/test/util.cpp
40@@ -5,12 +5,16 @@
41 #include <wallet/test/util.h>
42 
43 #include <chain.h>
44+#include <interfaces/chain.h>
45+#include <key.h>
46 #include <test/util/setup_common.h>
47 #include <wallet/wallet.h>
48 #include <wallet/walletdb.h>
49 
50 #include <boost/test/unit_test.hpp>
51 
52+#include <memory>
53+
54 std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key)
55 {
56     auto wallet = std::make_unique<CWallet>(&chain, "", CreateMockWalletDatabase());
57diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h
58index 288c111571..a9d7ef70f0 100644
59--- a/src/wallet/test/util.h
60+++ b/src/wallet/test/util.h
61@@ -5,6 +5,13 @@
62 #ifndef BITCOIN_WALLET_TEST_UTIL_H
63 #define BITCOIN_WALLET_TEST_UTIL_H
64 
65+#include <chain.h>
66+#include <interfaces/chain.h>
67+#include <key.h>
68+#include <test/util/setup_common.h>
69+#include <wallet/wallet.h>
70+#include <wallet/walletdb.h>
71+
72 #include <memory>
73 
74 class CChain;
75diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
76index 9e7f1f3ed7..75a08b6f74 100644
77--- a/src/wallet/test/wallet_tests.cpp
78+++ b/src/wallet/test/wallet_tests.cpp
79@@ -20,8 +20,8 @@
80 #include <util/translation.h>
81 #include <validation.h>
82 #include <wallet/coincontrol.h>
83-#include <wallet/test/wallet_test_fixture.h>
84 #include <wallet/test/util.h>
85+#include <wallet/test/wallet_test_fixture.h>
86 
87 #include <boost/test/unit_test.hpp>
88 #include <univalue.h>

ryanofsky force-pushed on Jul 14, 2021

ryanofsky commented at 6:19 pm on July 14, 2021: member

Updated 0cc10f8377495fb60156d116307848674e4be38e -> def672f8706e13f4153ff9aca3fb9e1becddae26 (pr/subfee.6 -> pr/subfee.7, compare) with minor updates suggested by jonatack (thanks!) and some tweaks (keeping a wallet line unchanged and preferring forward declarations over includes)

in src/wallet/spend.cpp:781 in def672f870 outdated

783     }
784 
785     // Reduce output values for subtractFeeFromAmount
786     if (coin_selection_params.m_subtract_fee_outputs) {
787-        CAmount to_reduce = fee_needed + change_amount - change_and_fee;
788+        CAmount to_reduce = nFeeRet + change_amount - change_and_fee;

jonatack commented at 6:42 pm on July 14, 2021:

def672f870 (feel free to ignore)

0        const CAmount to_reduce{nFeeRet + change_amount - change_and_fee};

jonatack commented at 6:50 pm on July 14, 2021: member

ACK def672f8706e13f4153ff9aca3fb9e1becddae26 per git diff 0cc10f8 def672f, retested after rebase to current master, reverified the added test is green before and after the last commit

in src/wallet/test/spend_tests.cpp:37 in fe6dc76b7c

32+        coin_control.m_feerate.emplace(10000);
33+        coin_control.fOverrideFeeRate = true;
34+        FeeCalculation fee_calc;
35+        BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
36+        BOOST_CHECK_EQUAL(tx->vout.size(), 1);
37+        BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount + leftover_input_amount - fee);

S3RK commented at 7:08 am on July 15, 2021:

Isn’t it always just 50 * COIN - fee I believe it’d be slightly easier to understand

ryanofsky commented at 11:40 am on July 20, 2021:

re: #22155 (review)

Isn’t it always just 50 * COIN - fee I believe it’d be slightly easier to understand

This is surprising to me, and I’m not sure how the thinking goes. I think recipient.nAmount + leftover_input_amount - fee captures the idea that when the subtract-from-recipient option is used, the recipient will receive the amount originally requested (recipient.nAmount) plus any dust amount (leftover_input_amount) that is too small to refund to the sender, minus the fee (fee). I’m not sure what 50 * COIN - fee communicates since it is involving the block reward which is a just a coincidental part of the test setup. It seems like this would be checking for the right result incidentally instead of explicitly, and it could also be more difficult to extend or maintain because it is repeating hardcoded setup values which aren’t related to the behavior being verified.

S3RK commented at 6:49 am on July 21, 2021:

The thinking is that the recipient always gets the whole UTXO - fee. On the first look it seemed to me that the value received would be different since leftover_input_amount is different for different invocations. It took me some time to figure out that the leftover_input_amount variable cancels out.

I understand your thinking as well and I don’t know which one is better.

in src/wallet/test/util.cpp:17 in 2565478c81 outdated

12+
13+#include <boost/test/unit_test.hpp>
14+
15+#include <memory>
16+
17+std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key)

S3RK commented at 7:20 am on July 15, 2021:

Any reasons to extract this from ListCoinsTestingSetup instead of using the fixture directly?

It looks like currently fixtures are preferred method of sharing setup/teardown between unit tests. ListCoinsTestingSetup already exists and does exactly what we need.

ryanofsky commented at 11:51 am on July 20, 2021:

re: #22155 (review)

Any reasons to extract this from ListCoinsTestingSetup instead of using the fixture directly?

It looks like currently fixtures are preferred method of sharing setup/teardown between unit tests. ListCoinsTestingSetup already exists and does exactly what we need.

Fixtures are overused in general right now, causing tests to do unnecessary work, and they make setup code that could be reused in other contexts (benchmarks & fuzz tests, future test & simulation code) harder to understand and less reusable. The current hierarchy of test fixtures that inherit from each other is also confusing and inflexible.

The suggestion here also seems a little vague, because I’m not sure what form or members you’d want a generic fixture to have. ListCoinsTestingSetup has more functionality than the new test requires, and less functionality than new tests may require. Would the new fixture only support tests with one synced wallet, or with multiple wallets? Would it require all the wallets to be created before each test starts, or could the tests create the wallets as needed?

The simplicity a CreateSyncedWallet util function not tied to a class hierarchy makes more sense to me than a test fixture that is more complicated with more usage restrictions.

S3RK commented at 7:49 am on July 21, 2021:

Fixtures are overused in general right now, causing tests to do unnecessary work, and they make setup code that could be reused in other contexts (benchmarks & fuzz tests, future test & simulation code) harder to understand and less reusable.

In this specific case the fixture doesn’t cause unnecessary work. CreateSyncedWallet function is used in only two places now: 1) in ListCoinsTestingSetup and 2) in the new test that uses TestChain100Setup fixture anyway.

In general I agree that fixtures are harder to reuse in other contexts but without having a real use case I’m not sure that CreateSyncedWallet will actually meet the requirements.

The current hierarchy of test fixtures that inherit from each other is also confusing and inflexible.

But you are using fixtures anyway right now. So why then split some part of the setup in fixture and some part in a standalone method. Using two separate mechanism to do the test setup is even more confusing.

Also there is value in maintaining uniformity in how the test setup is done. Fixtures are already used extensively in the code base. There are 72 test suites that use fixtures vs 24 that don’t. According to my spot check the ones that don’t use fixtures also don’t use standalone setup function as a way to reuse setup code between tests.

The suggestion here also seems a little vague, because I’m not sure what form or members you’d want a generic fixture to have. ListCoinsTestingSetup has more functionality than the new test requires, and less functionality than new tests may require. Would the new fixture only support tests with one synced wallet, or with multiple wallets? Would it require all the wallets to be created before each test starts, or could the tests create the wallets as needed?

My suggestion is to not introduce a second mechanism to do test setups and just reuse existing fixture (probably renaming it to SyncedWalletTestingSetup) because it has exactly the right setup.

The new SubtractFee test has the setup exactly equivalent to the setup of ListCoinsTestingSetup. ListCoinsTestingSetup has also AddTx method which is used exactly one time and could easily be inlined or extracted.

ryanofsky commented at 4:36 pm on July 29, 2021:

re: #22155 (review)

My suggestion is to not introduce a second mechanism to do test setups and just reuse existing fixture (probably renaming it to SyncedWalletTestingSetup) because it has exactly the right setup.

Yes, I see what you’re saying here. If you can use fixtures to help with test setup, and you can also use normal functions to also help with test setup, then setup code might be less discoverable, and someone who is trying to write a new test and only looking at the fixtures might miss a function that could be useful. I would not deny that this is a drawback. I just think there are some advantages on the other side. (Functions that don’t maintain state can be easier to understand than fixtures that are stateful. Functions can be more flexible than fixtures and in this case let you create many synced wallets instead of one. Functions can be shared across frameworks including boost framework, qt test framework, fuzzing framework, benchmark framework, and other future frameworks, instead of limited to the boost framework. And this change adds a place for more functions to go and more fixture code to be detached even if is not moving right away)

in src/Makefile.test.include:175 in 2565478c81 outdated

169@@ -170,6 +170,8 @@ endif
170 
171 
172 BITCOIN_TEST_SUITE += \
173+  wallet/test/util.cpp \
174+  wallet/test/util.h \

S3RK commented at 7:36 am on July 20, 2021:

Why not reuse wallet/test/wallet_test_fixture.*?

It already contains WalletTestingSetup that we use anyway.

ryanofsky commented at 11:22 am on July 20, 2021:

re: #22155 (review)

Why not reuse wallet/test/wallet_test_fixture.*?

Responded in detail about the fixtures below.

in src/wallet/test/spend_tests.cpp:15 in fe6dc76b7c

10+
11+#include <boost/test/unit_test.hpp>
12+
13+BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup)
14+
15+BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)

S3RK commented at 7:51 am on July 20, 2021:

No need to override fixture declared for test suite. You can do

0BOOST_FIXTURE_TEST_SUITE(spend_tests, TestChain100Setup)
1BOOST_AUTO_TEST_CASE(SubtractFee)

or

0BOOST_AUTO_TEST_SUITE(spend_tests)
1BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)

ryanofsky commented at 11:28 am on July 20, 2021:

re: #22155 (review)

No need to override fixture declared for test suite. You can do […]

I know you can do these things, but don’t see what makes them better or meaningfully different. What I think is nice about the current approach is that it makes spend_tests setup consistent with wallet_tests setup, and more efficient by default because the wallet test fixture does less work than the chain100 fixture.

S3RK commented at 7:53 am on July 21, 2021:

It’s better because it avoids unnecessary information. WalletTestingSetup is never used in the test suite.

There is no point in setting TEST_SUITE fixture to WalletTestingSetup when the only test case in the test suite overrides the fixture and uses TestChain100Setup. It’s just unused information in the test that could potentially confuse readers.

Same goes for wallet_tests. Every test case there defines there own fixture explicitly and don’t inherit it from the test suite. I have no idea why it’s like that

ryanofsky commented at 4:13 pm on July 29, 2021:

I understand the motivation now and yes I agree. WalletTestingSetup can be overkill because even if you are testing wallet code, you are likely testing smaller functions and classes that don’t need the full setup. It’s nicer in general to use simpler fixtures for efficiency and easier comprehension, and would be good to see use of fixtures cleaned up and simplified in future PRs. Consistency between spend_tests and wallet_tests here may at least help with that.

S3RK commented at 7:54 am on July 20, 2021: member

Reviewed first two commits. Great to see more coverage for coin selection!

I’m not sure what was the reason to extract a part of the ListCoinsTestingSetup fixture in a standalone method instead of just using the fixture itself. One can move ListCoinsTestingSetup to wallet/test/wallet_test_fixture.* that seems like a good fit and is included anyway.

ryanofsky force-pushed on Jul 20, 2021

ryanofsky commented at 6:11 pm on July 20, 2021: member

Reviewed first two commits. Great to see more coverage for coin selection!

Thanks for review! I just dropped the third (last) commit to make this a test-only change and avoid discouraging an ACK. I’ll make a PR from it later if it’s still useful after other coin selection updates.

I’m not sure what was the reason to extract a part of the ListCoinsTestingSetup fixture in a standalone method instead of just using the fixture itself. One can move ListCoinsTestingSetup to wallet/test/wallet_test_fixture.* that seems like a good fit and is included anyway.

Thanks, responded in detail about the fixtures below.

Updated def672f8706e13f4153ff9aca3fb9e1becddae26 -> fe6dc76b7c9c5405f37464a3b19fcf82aaf22861 (pr/subfee.7 -> pr/subfee.8, compare) dropping the final commit, so the PR is easier to review and test-only

ryanofsky force-pushed on Jul 20, 2021

achow101 commented at 7:02 pm on July 21, 2021: member

ACK fe6dc76b7c9c5405f37464a3b19fcf82aaf22861

glozow commented at 1:25 pm on July 23, 2021: member

ACK fe6dc76b7c9c5405f37464a3b19fcf82aaf22861

Could also test this for multiple subtract fee recipients?

in src/wallet/test/spend_tests.cpp:36 in fe6dc76b7c

31+        CCoinControl coin_control;
32+        coin_control.m_feerate.emplace(10000);
33+        coin_control.fOverrideFeeRate = true;
34+        FeeCalculation fee_calc;
35+        BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
36+        BOOST_CHECK_EQUAL(tx->vout.size(), 1);

promag commented at 1:48 pm on July 23, 2021:

fe6dc76b7c9c5405f37464a3b19fcf82aaf22861

nit, also check change_pos == -1.

promag commented at 1:56 pm on July 23, 2021: member

Code review ACK fe6dc76b7c9c5405f37464a3b19fcf82aaf22861.

MarcoFalke merged this on Jul 27, 2021

MarcoFalke closed this on Jul 27, 2021

sidhujag referenced this in commit de78e5d219 on Jul 28, 2021

ryanofsky commented at 4:01 pm on July 29, 2021: member

:scream: Apparently I missed a review club about this https://bitcoincore.reviews/22155. Sorry I missed but relieved it didn’t find any terrible problems!

ryanofsky commented at 4:42 pm on July 29, 2021: member

Good suggestions for followup

S3RK switching wallet & spend test default fixtures to minimal basic setup #22155 (review)
glozow extending coverage for multiple recipients #22155#pullrequestreview-713724079
promag checking returned changed_pos #22155 (review)

DrahtBot locked this on Aug 16, 2022

wallet test: Add test for subtract fee from recipient behavior #22155

Conflicts