test: Add remaining scenarios of 0 waste, in wallet waste

rajarshimaitra commented at 10:27 am on September 10, 2021: contributor

As per the review club discussion on #22009 , it was observed that there were other two fee scenarios in which selection waste could be zero.

These are:

(LTF - Fee) == Change Cost
(LTF - Fee) == Excess

Even though these are obvious by the definition of waste metric, adding tests for them can be helpful in explaining its behavior to new readers of the code base, along with pinning the behavior for future.

This PR adds those two cases to waste calculation unit test.

Also let me know if I am missing more scenarios.

fanquake added the label Tests on Sep 10, 2021

rajarshimaitra force-pushed on Sep 10, 2021

theStack commented at 10:37 am on September 10, 2021: member

Concept ACK

Xekyo commented at 2:28 pm on September 10, 2021: member

Concept ACK

These are:

* (Fee - LTF) == Change Cost

* (Fee - LTF) == Excess

Strictly speaking it should be:

(Fee - LTF) + Change Cost = 0
⇒ (Fee - LTF) == -1×Change Cost
(Fee - LTF) + Excess = 0 ⇒ (Fee - LTF) == -1×Excess

achow101 commented at 4:42 pm on September 10, 2021: member

ACK c976ee0a0ca00fe8a7de5c806560da35c2f48729

in src/wallet/test/coinselector_tests.cpp:734 in c976ee0a0c outdated

730     add_coin(2 * COIN, 2, selection, fee, fee);
731     const CAmount exact_target = in_amt - 2 * fee;
732     BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, 0, exact_target));
733+    selection.clear();
734 
735+    // 0 Waste when (fee - long term fee) == cost of change, and no excess

Xekyo commented at 5:04 pm on September 10, 2021:

0    // 0 Waste when (long term fee - fee) == cost of change, and no excess

As it is currently written the two aren’t equal but negatives of each other.

in src/wallet/test/coinselector_tests.cpp:741 in c976ee0a0c outdated

737+    add_coin(1 * COIN, 1, selection, fee, fee + fee_diff);
738+    add_coin(2 * COIN, 2, selection, fee, fee + fee_diff);
739+    BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, new_change_cost, target));
740+    selection.clear();
741+
742+    // 0 Waste when (fee - long term fee) == excess, no change cost

Xekyo commented at 5:06 pm on September 10, 2021:

0    // 0 Waste when (long term fee - fee) == excess, no change cost

Same here.

Xekyo commented at 5:06 pm on September 10, 2021: member

Left a nit on the comment explaining the test

rajarshimaitra force-pushed on Sep 12, 2021

rajarshimaitra commented at 1:56 pm on September 12, 2021: contributor

Thanks @Xekyo for the review and agreed. It should be reversed.

Updated everywhere..

in src/wallet/test/coinselector_tests.cpp:745 in 0ce33016eb outdated

741+
742+    // 0 Waste when (long term fee - fee) == excess, no change cost
743+    const CAmount new_target = in_amt - fee * 2 - fee_diff * 2;
744+    add_coin(1 * COIN, 1, selection, fee, fee + fee_diff);
745+    add_coin(2 * COIN, 2, selection, fee, fee + fee_diff);
746+    BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, 0, new_target));

jonatack commented at 6:43 pm on September 12, 2021:

minor suggestions: s/0 Waste/No waste/, braced initialization for type safety, named args

 0@@ -724,25 +724,25 @@ BOOST_AUTO_TEST_CASE(waste_test)
 1     BOOST_CHECK_LT(waste_nochange2, waste_nochange1);
 2     selection.clear();
 3 
 4-    // 0 Waste when fee == long term fee, no change, and no excess
 5+    // No waste when fee == long term fee, no change, and no excess
 6     add_coin(1 * COIN, 1, selection, fee, fee);
 7     add_coin(2 * COIN, 2, selection, fee, fee);
 8-    const CAmount exact_target = in_amt - 2 * fee;
 9-    BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, 0, exact_target));
10+    const CAmount exact_target{in_amt - 2 * fee};
11+    BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, /* change_cost */ 0, exact_target));
12     selection.clear();
13 
14-    // 0 Waste when (long term fee - fee) == cost of change, and no excess
15-    const CAmount new_change_cost = fee_diff * 2;
16+    // No waste when (long term fee - fee) == cost of change, and no excess
17+    const CAmount new_change_cost{fee_diff * 2};
18     add_coin(1 * COIN, 1, selection, fee, fee + fee_diff);
19     add_coin(2 * COIN, 2, selection, fee, fee + fee_diff);
20     BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, new_change_cost, target));
21     selection.clear();
22 
23-    // 0 Waste when (long term fee - fee) == excess, no change cost
24-    const CAmount new_target = in_amt - fee * 2 - fee_diff * 2;
25+    // No waste when (long term fee - fee) == excess, no change cost
26+    const CAmount new_target{in_amt - fee * 2 - fee_diff * 2};
27     add_coin(1 * COIN, 1, selection, fee, fee + fee_diff);
28     add_coin(2 * COIN, 2, selection, fee, fee + fee_diff);
29-    BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, 0, new_target));
30+    BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, /* change_cost */ 0, new_target));

jonatack commented at 6:45 pm on September 12, 2021: member

ACK 0ce33016eb905f5ac3709ed95ae02db0f97885a2

Perhaps update the commit message and pull description with @Xekyo’s calculations in #22938 (comment). Edit: never mind, it seems fine now IIUC.

Xekyo commented at 1:59 pm on September 16, 2021: member

ACK 0ce33016eb905f5ac3709ed95ae02db0f97885a2

rajarshimaitra force-pushed on Sep 23, 2021

rajarshimaitra commented at 10:12 am on September 23, 2021: contributor

Thanks @jonatack for the review. Those were good suggestions.

Updated the comment description. Updated commit message. Updated with braced initialization.

Xekyo commented at 8:20 pm on September 23, 2021: member

Re-ack https://github.com/bitcoin/bitcoin/commit/a2a9c23e261ad88c0133b407c6e1b4c54f90a860

in src/wallet/test/coinselector_tests.cpp:730 in a2a9c23e26 outdated

728+    // No Waste when fee == long_term_fee, no change, and no excess
729     add_coin(1 * COIN, 1, selection, fee, fee);
730     add_coin(2 * COIN, 2, selection, fee, fee);
731-    const CAmount exact_target = in_amt - 2 * fee;
732-    BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, 0, exact_target));
733+    const CAmount exact_target{in_amt - 2 * fee};

jonatack commented at 8:32 pm on September 23, 2021:

only because your other calculations are in this order

0    const CAmount exact_target{in_amt - fee * 2};

in src/wallet/test/coinselector_tests.cpp:741 in a2a9c23e26 outdated

739+    add_coin(1 * COIN, 1, selection, fee, fee + fee_diff);
740+    add_coin(2 * COIN, 2, selection, fee, fee + fee_diff);
741+    BOOST_CHECK_EQUAL(0, GetSelectionWaste(selection, new_change_cost, target));
742+    selection.clear();
743+
744+    // 0 Waste when (fee - long_term_fee) == (-excess), no change cost

jonatack commented at 8:33 pm on September 23, 2021:

0    // No Waste when (fee - long_term_fee) == (-excess), no change cost

jonatack commented at 8:35 pm on September 23, 2021: member

Code review re-ACK a2a9c23e261ad88c0133b407c6e1b4c54f90a860 per git diff 0ce3301 a2a9c23

Sub-atomic consistency nits, but definitely ignore unless you have to rebase or something (and maybe even then).

test: Add remaining scenarios of 0 waste

There are two more cases where waste can be 0, when:
 - (Fee - LTF) == -Change Cost
 - (Fee - LTF) == -Excess

Adding these two conditions explicitly in the unit test will help
pin the behavior, also demonstrate waste calculation scenarios to new
readers.

efcaefc7b5

rajarshimaitra force-pushed on Sep 24, 2021

rajarshimaitra commented at 10:08 am on September 24, 2021: contributor

@jonatack Sorry for my silly overlooks.. Updated now only..

jonatack commented at 3:31 pm on September 24, 2021: member

Tested re-ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be

achow101 commented at 5:52 pm on September 24, 2021: member

ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be

fanquake requested review from meshcollider on Sep 28, 2021

meshcollider commented at 7:18 am on September 28, 2021: contributor

ACK efcaefc7b5ffe0495e7c809032342ee5ca4841be

meshcollider merged this on Sep 28, 2021

meshcollider closed this on Sep 28, 2021

DrahtBot locked this on Oct 30, 2022

test: Add remaining scenarios of 0 waste, in wallet waste_test #22938