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

ryanofsky · 2021-06-04T23:10:51Z

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 · 2021-06-04T23:26:00Z

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 def672f is dropped for now to make the PR smaller and test-only

DrahtBot · 2021-06-05T05:26:08Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Make bech32m the default for RPC, opt-in for GUI #22260 by Sjors
Remove gArgs from wallet.h and wallet.cpp #22183 by kiminuo
refactor: Clean up new wallet spend, receive files added #21207 #22100 by ryanofsky
refactor: Make CWalletTx sync state type-safe #21206 by ryanofsky
refactor: remove ::vpwallets and related global variables #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.

achow101

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:

diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index 8c7794eb47..41ee5f3ea0 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -26,6 +26,8 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
     int change_pos = -1;
     bilingual_str error;
     CCoinControl coin_control;
+    coin_control.m_feerate.emplace(10000);
+    coin_control.fOverrideFeeRate = true;
     FeeCalculation fee_calc;
     BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
     BOOST_CHECK_EQUAL(tx->vout.size(), 1);
@@ -35,14 +37,26 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
     // Check subtract from recipient transaction slightly less than coinbase
     // amount also does not create change output and pays extra dust amount to
     // recipient instead of miner
-    const CAmount dust_amount = 123;
-    const CAmount expected_fee = fee;
+    CAmount dust_amount = 123;
+    CAmount expected_fee = fee;
     recipient.nAmount -= dust_amount;
     BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
     BOOST_CHECK_EQUAL(tx->vout.size(), 1);
     BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount - fee + dust_amount);
     BOOST_CHECK_EQUAL(fee, expected_fee);
     tx.reset();
+
+    // The dust amount is greater than the fee paid for this transaction.
+    // The entire dust amount should end up as fees, rather than the excess fee paid to the recipients.
+    dust_amount = 1350;
+    expected_fee = dust_amount;
+    recipient.nAmount = 50 * COIN - dust_amount;
+    BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
+    BOOST_CHECK_EQUAL(tx->vout.size(), 1);
+    BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount - fee + dust_amount);
+    BOOST_CHECK_EQUAL(fee, expected_fee);
+    BOOST_CHECK_LE(tx->vout[0].nValue, recipient.nAmount);
+    tx.reset();
 }
 
 BOOST_AUTO_TEST_SUITE_END()

achow101 · 2021-06-07T23:50:32Z

src/wallet/test/spend_tests.cpp

+    CAmount fee;
+    int change_pos = -1;
+    bilingual_str error;
+    CCoinControl coin_control;


In e459d01 "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.

Suggested change

CCoinControl coin_control;

CCoinControl coin_control;

coin_control.m_feerate.emplace(10000);

coin_control.fOverrideFeeRate = true;

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.

ryanofsky

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

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

ryanofsky · 2021-06-08T03:24:19Z

src/wallet/test/spend_tests.cpp

+    CAmount fee;
+    int change_pos = -1;
+    bilingual_str error;
+    CCoinControl coin_control;


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 · 2021-06-08T18:22:02Z

ACK 972e87c

ryanofsky · 2021-06-10T12:26:43Z

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

jonatack · 2021-06-10T12:50:45Z

Concept ACK, good to see test coverage here.

ryanofsky · 2021-06-10T14:56:09Z

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

achow101 · 2021-06-10T18:26:46Z

re-ACK 6a50482

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.

Behavior might have recently changed in bitcoin#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>

ryanofsky · 2021-06-12T18:46:42Z

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

jonatack

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

a few minor edits while reviewing, feel free to pick/choose/ignore (not sure about some of the includes)

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 45a02f59a1..b3a8d18446 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -764,8 +764,7 @@ bool CWallet::CreateTransactionInternal(
     // 1. The change output would be dust
     // 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)
     CAmount change_amount = change_position->nValue;
-    if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change)
-    {
+    if (IsDust(*change_position, coin_selection_params.m_discard_feerate) || change_amount <= coin_selection_params.m_cost_of_change) {
         nChangePosInOut = -1;
         change_amount = 0;
         txNew.vout.erase(change_position);
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index a202dd9146..66e7de4273 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -22,7 +22,7 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
     // be uneconomical to add and spend the output), and make sure it pays the
     // leftover input amount which would have been change to the recipient
     // instead of the miner.
-    auto check_tx = [&](CAmount leftover_input_amount) {
+    auto check_tx = [&wallet](CAmount leftover_input_amount) {
         CRecipient recipient{GetScriptForRawPubKey({}), 50 * COIN - leftover_input_amount, true /* subtract fee */};
         CTransactionRef tx;
         CAmount fee;
@@ -41,7 +41,7 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
 
     // Send full input amount to recipient, check that only nonzero fee is
     // subtracted (to_reduce == fee).
-    const CAmount fee = check_tx(0);
+    const CAmount fee{check_tx(0)};
 
     // Send slightly less than full input amount to recipient, check leftover
     // input amount is paid to recipient not the miner (to_reduce == fee - 123)
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
index 097abf9310..f156dbc71d 100644
--- a/src/wallet/test/util.cpp
+++ b/src/wallet/test/util.cpp
@@ -5,12 +5,16 @@
 #include <wallet/test/util.h>
 
 #include <chain.h>
+#include <interfaces/chain.h>
+#include <key.h>
 #include <test/util/setup_common.h>
 #include <wallet/wallet.h>
 #include <wallet/walletdb.h>
 
 #include <boost/test/unit_test.hpp>
 
+#include <memory>
+
 std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key)
 {
     auto wallet = std::make_unique<CWallet>(&chain, "", CreateMockWalletDatabase());
diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h
index 288c111571..a9d7ef70f0 100644
--- a/src/wallet/test/util.h
+++ b/src/wallet/test/util.h
@@ -5,6 +5,13 @@
 #ifndef BITCOIN_WALLET_TEST_UTIL_H
 #define BITCOIN_WALLET_TEST_UTIL_H
 
+#include <chain.h>
+#include <interfaces/chain.h>
+#include <key.h>
+#include <test/util/setup_common.h>
+#include <wallet/wallet.h>
+#include <wallet/walletdb.h>
+
 #include <memory>
 
 class CChain;
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 9e7f1f3ed7..75a08b6f74 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -20,8 +20,8 @@
 #include <util/translation.h>
 #include <validation.h>
 #include <wallet/coincontrol.h>
-#include <wallet/test/wallet_test_fixture.h>
 #include <wallet/test/util.h>
+#include <wallet/test/wallet_test_fixture.h>
 
 #include <boost/test/unit_test.hpp>
 #include <univalue.h>

ryanofsky · 2021-07-14T18:19:30Z

Updated 0cc10f8 -> def672f (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)

jonatack

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

jonatack · 2021-07-14T18:42:52Z

src/wallet/spend.cpp

    }

    // Reduce output values for subtractFeeFromAmount
    if (coin_selection_params.m_subtract_fee_outputs) {
-        CAmount to_reduce = fee_needed + change_amount - change_and_fee;
+        CAmount to_reduce = nFeeRet + change_amount - change_and_fee;


def672f (feel free to ignore)

Suggested change

CAmount to_reduce = nFeeRet + change_amount - change_and_fee;

const CAmount to_reduce{nFeeRet + change_amount - change_and_fee};

S3RK

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.

S3RK · 2021-07-15T07:08:39Z

src/wallet/test/spend_tests.cpp

+        FeeCalculation fee_calc;
+        BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
+        BOOST_CHECK_EQUAL(tx->vout.size(), 1);
+        BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount + leftover_input_amount - fee);


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

re: #22155 (comment)

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.

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.

S3RK · 2021-07-15T07:20:43Z

src/wallet/test/util.cpp

+
+#include <memory>
+
+std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key)


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.

re: #22155 (comment)

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.

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.

re: #22155 (comment)

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)

S3RK · 2021-07-20T07:36:15Z

src/Makefile.test.include

@@ -170,6 +170,8 @@ endif


 BITCOIN_TEST_SUITE += \
+  wallet/test/util.cpp \
+  wallet/test/util.h \


Why not reuse wallet/test/wallet_test_fixture.*?

It already contains WalletTestingSetup that we use anyway.

re: #22155 (comment)

Why not reuse wallet/test/wallet_test_fixture.*?

Responded in detail about the fixtures below.

S3RK · 2021-07-20T07:51:09Z

src/wallet/test/spend_tests.cpp

+
+BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup)
+
+BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)


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

BOOST_FIXTURE_TEST_SUITE(spend_tests, TestChain100Setup) BOOST_AUTO_TEST_CASE(SubtractFee)

or

BOOST_AUTO_TEST_SUITE(spend_tests) BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)

re: #22155 (comment)

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.

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

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.

ryanofsky

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 def672f -> fe6dc76 (pr/subfee.7 -> pr/subfee.8, compare) dropping the final commit, so the PR is easier to review and test-only

ryanofsky · 2021-07-20T11:22:07Z

src/Makefile.test.include

@@ -170,6 +170,8 @@ endif


 BITCOIN_TEST_SUITE += \
+  wallet/test/util.cpp \
+  wallet/test/util.h \


re: #22155 (comment)

Why not reuse wallet/test/wallet_test_fixture.*?

Responded in detail about the fixtures below.

ryanofsky · 2021-07-20T11:28:02Z

src/wallet/test/spend_tests.cpp

+
+BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup)
+
+BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)


re: #22155 (comment)

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.

ryanofsky · 2021-07-20T11:40:33Z

src/wallet/test/spend_tests.cpp

+        FeeCalculation fee_calc;
+        BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
+        BOOST_CHECK_EQUAL(tx->vout.size(), 1);
+        BOOST_CHECK_EQUAL(tx->vout[0].nValue, recipient.nAmount + leftover_input_amount - fee);


re: #22155 (comment)

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.

ryanofsky · 2021-07-20T11:51:13Z

src/wallet/test/util.cpp

+
+#include <memory>
+
+std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key)


re: #22155 (comment)

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.

achow101 · 2021-07-21T19:02:14Z

ACK fe6dc76

glozow

ACK fe6dc76

Could also test this for multiple subtract fee recipients?

promag

Code review ACK fe6dc76.

promag · 2021-07-23T13:48:55Z

src/wallet/test/spend_tests.cpp

+        coin_control.fOverrideFeeRate = true;
+        FeeCalculation fee_calc;
+        BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, change_pos, error, coin_control, fee_calc));
+        BOOST_CHECK_EQUAL(tx->vout.size(), 1);


fe6dc76

nit, also check change_pos == -1.

…pient b…

ryanofsky · 2021-07-29T16:01:32Z

😱 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 · 2021-07-29T16:42:27Z

Good suggestions for followup

S3RK switching wallet & spend test default fixtures to minimal basic setup wallet test: Add test for subtract fee from recipient behavior #22155 (comment)
glozow extending coverage for multiple recipients wallet test: Add test for subtract fee from recipient behavior #22155 (review)
promag checking returned changed_pos wallet test: Add test for subtract fee from recipient behavior #22155 (comment)

DrahtBot added Build system Wallet labels Jun 4, 2021

fanquake added Tests and removed Build system labels Jun 5, 2021

achow101 reviewed Jun 8, 2021

View reviewed changes

ryanofsky force-pushed the pr/subfee branch 2 times, most recently from 4f7c483 to 972e87c Compare June 8, 2021 13:43

ryanofsky commented Jun 8, 2021

View reviewed changes

DrahtBot added the Needs rebase label Jun 9, 2021

ryanofsky force-pushed the pr/subfee branch from 972e87c to 8f6113f Compare June 10, 2021 12:25

DrahtBot removed the Needs rebase label Jun 10, 2021

ryanofsky force-pushed the pr/subfee branch from 8f6113f to 6a50482 Compare June 10, 2021 14:56

DrahtBot added the Needs rebase label Jun 12, 2021

ryanofsky and others added 2 commits June 12, 2021 11:22

wallet test refactor: add CreateSyncedWallet function

2565478

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.

ryanofsky force-pushed the pr/subfee branch from 6a50482 to 0cc10f8 Compare June 12, 2021 18:44

DrahtBot removed the Needs rebase label Jun 12, 2021

DrahtBot mentioned this pull request Jun 16, 2021

Make bech32m the default for RPC, opt-in for GUI #22260

Closed

ryanofsky mentioned this pull request Jul 14, 2021

wallet: Decide which coin selection solution to use based on waste metric #22009

Merged

jonatack reviewed Jul 14, 2021

View reviewed changes

ryanofsky force-pushed the pr/subfee branch from 0cc10f8 to def672f Compare July 14, 2021 18:18

jonatack reviewed Jul 14, 2021

View reviewed changes

This was referenced Jul 15, 2021

refactor: Clean up new wallet spend, receive files added #21207 #22100

Merged

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

Merged

S3RK reviewed Jul 20, 2021

View reviewed changes

ryanofsky force-pushed the pr/subfee branch from def672f to 0cc10f8 Compare July 20, 2021 18:09

ryanofsky commented Jul 20, 2021

View reviewed changes

ryanofsky force-pushed the pr/subfee branch from 0cc10f8 to fe6dc76 Compare July 20, 2021 18:25

glozow reviewed Jul 23, 2021

View reviewed changes

promag reviewed Jul 23, 2021

View reviewed changes

maflcko merged commit 7075a52 into bitcoin:master Jul 27, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 28, 2021

Merge bitcoin#22155: wallet test: Add test for subtract fee from reci…

de78e5d

…pient b…

ryanofsky mentioned this pull request Feb 28, 2022

Do not use LocalTestingSetup in getarg_tests test file. #24375

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

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


		#include <memory>

		std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cchain, const CKey& key)


		BOOST_FIXTURE_TEST_SUITE(spend_tests, WalletTestingSetup)

		BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)

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

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

Uh oh!

Conversation

ryanofsky commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

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

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 8, 2021

Uh oh!

ryanofsky commented Jun 10, 2021

Uh oh!

jonatack commented Jun 10, 2021

Uh oh!

ryanofsky commented Jun 10, 2021

Uh oh!

achow101 commented Jun 10, 2021

Uh oh!

ryanofsky commented Jun 12, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jul 14, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

ryanofsky commented Jun 4, 2021 •

edited

Loading

ryanofsky commented Jun 4, 2021 •

edited

Loading

DrahtBot commented Jun 5, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading