knapsack_solver_test exceeds 21M BTC #35449

In resuming my experimentation around making CAmount a safer type, but ran into the unit test case knapsack_solver_test in coinselector_tests.cpp exceeding 21M BTC. This can be observed on master as of 654a5223af532e965998bb1d6988be421fba186f by applying:

--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -505,6 +505,7 @@ BOOST_AUTO_TEST_CASE(knapsack_solver_test)
         available_coins.Clear();
         for (int j = 0; j < 20; j++)
             add_coin(available_coins, *wallet, 50000 * COIN);
+        assert(available_coins.GetTotalAmount() <= MAX_MONEY);
 
         const auto result19 = KnapsackSolver(KnapsackGroupOutputs(available_coins, *wallet, filter_confirmed), 500000 * COIN, CENT);
         BOOST_CHECK(result19);

There is an outer loop with 100 iterations, so 100x20x50'000 BTC = 100'000'000 BTC.

I'm sure there is an easy fix. Just don't want to be resetting too much state to make the tests not cover some case they should be, so hoping this issue will help uncover a good solution.

cc @murchandamus

CENT instead of COIN and done?

CENT instead of COIN and done?

I think COIN is pretty essential to the case we are testing for, given the comment:

https://github.com/bitcoin/bitcoin/blob/654a5223af532e965998bb1d6988be421fba186f/src/wallet/test/coinselector_tests.cpp#L503-L507

hehe, I only read the lines you shared in the description. Just went deeper.. the issue isn't the test case; it clears the available coins right before start, so available coins shouldn't be compounding across loop iterations. The problem is inside CoinsResult::Clear:

void CoinsResult::Clear() {
    coins.clear();
}

We are missing to clear the other two cached fields total_amount and total_effective_amount. That should fix the issue.

void CoinsResult::Clear() {
     coins.clear();
+    total_amount = 0;
+    total_effective_amount = std::nullopt;
 }

Thanks @furszy, I'm cooking up a PR with a commit in the direction you suggested! Should be ready for review together with some other minor changes by tomorrow.

The correct solution is of course to remove the Knapsack coinselection algorithm. ;)

Jokes aside, thanks for surfacing why the amount compounded across loops, @furszy, I was about to check why it did when I saw your comment.

PR: https://github.com/bitcoin/bitcoin/pull/35456