carrot_impl: @j-berman dusty sweep review

Co-authored-by: j-berman <justinberman@protonmail.com>

Author: jeffro256

src/carrot_impl/input_selection.inl

@@ -63,8 +63,9 @@ std::pair<std::size_t, boost::multiprecision::uint128_t> get_input_count_for_max
 
     std::size_t input_count = 0;
     boost::multiprecision::uint128_t cumulative_input_sum = 0;
-    boost::multiprecision::uint128_t best_cumulative_input_sum = 0;
     std::size_t best_input_count = 0;
+    boost::multiprecision::uint128_t best_cumulative_input_sum = 0;
+    boost::multiprecision::uint128_t best_net_input_sum = 0;
     // for all valid input counts (or existing amount counts, whichever is fewer)...
     for (auto top_amount_it = top_amounts.crbegin(); top_amount_it != top_amounts.crend(); ++top_amount_it)
     {
@@ -82,12 +83,14 @@ std::pair<std::size_t, boost::multiprecision::uint128_t> get_input_count_for_max
         // if the input amount total is greater than fee for that number of inputs...
         if (cumulative_input_sum > current_fee)
         {
-            // and if that input amount total is better than any other total observed yet...
-            if (cumulative_input_sum > best_cumulative_input_sum)
+            const boost::multiprecision::uint128_t net_input_sum = cumulative_input_sum - current_fee;
+            // and if that input amount total minus its fees is better than any other total observed yet...
+            if (net_input_sum > best_net_input_sum)
             {
                 // set that as best.
-                best_cumulative_input_sum = cumulative_input_sum;
                 best_input_count = input_count;
+                best_cumulative_input_sum = cumulative_input_sum;
+                best_net_input_sum = net_input_sum;
             }
         }
     }

src/carrot_impl/multi_tx_proposal_utils.cpp

@@ -168,6 +168,7 @@ void make_multiple_carrot_transaction_proposals_sweep(
 
     tx_proposals_out.reserve((selected_inputs.size() + FCMP_PLUS_PLUS_MAX_INPUTS - 1) / FCMP_PLUS_PLUS_MAX_INPUTS);
 
+    // callback for calling `get_input_count_for_max_usable_money()` on some slice of inputs starting at `window_offset`
     const auto get_input_count_for_max_usable_money_in_window = 
         [&selected_inputs, &fee_by_input_count](const std::size_t window_offset)
     {
@@ -188,17 +189,24 @@ void make_multiple_carrot_transaction_proposals_sweep(
                 fee_by_input_count).first;
     };
 
+    // To try to maximize the total amount of money sent in a sweep, we first order the amounts in
+    // ascending order. Then from low to high, we slide a "window" over a slice of contiguous
+    // amounts and call `get_input_count_for_max_usable_money_in_window` over that window. That call
+    // will tell how many of the inputs in that window are dusty. If any are dusty, we should keep
+    // sliding the window until we can't. Once that window stops, we use that window for the inputs
+    // for the current transaction. Repeat until no inputs are left or only dusty inputs are left.
+
     // while some selection of inputs of the highest amounts at some input count yields a net positive output sum...
     while (get_input_count_for_max_usable_money_in_window(0))
     {
-        // slide a window in ascending amount order until some inputs in that window yield a net positive output sum..
-        std::size_t window_offset = selected_inputs.size()
-            - std::min<std::size_t>(CARROT_MAX_TX_INPUTS, selected_inputs.size());
+        // slide a window in ascending amount order until all inputs in that window yield a net positive output sum...
+        const std::size_t max_window_size = std::min<std::size_t>(CARROT_MAX_TX_INPUTS, selected_inputs.size());
+        std::size_t window_offset = selected_inputs.size() - max_window_size;
         std::size_t n_tx_inputs = 0;
         do
         {
             n_tx_inputs = get_input_count_for_max_usable_money_in_window(window_offset);
-            if (0 == window_offset || 0 != n_tx_inputs)
+            if (0 == window_offset || max_window_size == n_tx_inputs)
                 break;
             --window_offset;
         }

tests/unit_tests/carrot_impl.cpp

@@ -1177,6 +1177,34 @@ TEST(carrot_impl, multi_account_transfer_over_transaction_16)
     subtest_multi_account_transfer_over_transaction(tx_proposal);
 }
 //----------------------------------------------------------------------------------------------------------------------
+TEST(carrot_impl, get_input_count_for_max_usable_money_1)
+{
+    const std::vector<rct::xmr_amount> amounts{17, 7, 11, 8, 9};
+
+    const std::map<std::size_t, rct::xmr_amount> fee_by_input_count{
+        {1, 10},
+        {2, 20},
+        {3, 30},
+        {4, 40},
+        {5, 50},
+        {6, 60},
+        {7, 70},
+        {8, 80}
+    };
+
+    const auto input_count_for_max_usable_money = carrot::get_input_count_for_max_usable_money(
+        amounts.cbegin(),
+        amounts.cend(),
+        fee_by_input_count.size(),
+        fee_by_input_count);
+
+    const std::pair<std::size_t, boost::multiprecision::uint128_t> expected_input_count_for_max_usable_money{
+        2, 17 + 11
+    };
+
+    EXPECT_EQ(expected_input_count_for_max_usable_money, input_count_for_max_usable_money);
+}
+//----------------------------------------------------------------------------------------------------------------------
 TEST(carrot_impl, make_single_transfer_input_selector_not_enough_money_1)
 {
     // no input candidates, should throw `not_enough_money`
@@ -1603,6 +1631,13 @@ TEST(carrot_impl, make_multiple_carrot_transaction_proposals_sweep_1)
             ASSERT_EQ(1, tx_proposal.selfsend_payment_proposals.size());
             ASSERT_TRUE(tx_proposal.extra.empty());
 
+            const carrot::CarrotPaymentProposalVerifiableSelfSendV1 &selfsend_payment_proposal
+                = tx_proposal.selfsend_payment_proposals.at(0);
+            ASSERT_EQ(0, selfsend_payment_proposal.proposal.amount);
+            ASSERT_EQ(0, selfsend_payment_proposal.subaddr_index.index.major);
+            ASSERT_EQ(0, selfsend_payment_proposal.subaddr_index.index.minor);
+            ASSERT_EQ(carrot::AddressDeriveType::Auto, selfsend_payment_proposal.subaddr_index.derive_type);
+
             carrot::CarrotPaymentProposalV1 modified_normal_payment_proposal = normal_payment_proposal;
             modified_normal_payment_proposal.amount = tx_proposal.normal_payment_proposals.at(0).amount;
             ASSERT_EQ(modified_normal_payment_proposal, tx_proposal.normal_payment_proposals.at(0));
@@ -1622,4 +1657,4 @@ TEST(carrot_impl, make_multiple_carrot_transaction_proposals_sweep_1)
         EXPECT_EQ(n_input_enote_amounts, seen_ota.size());
     }
 }
-//----------------------------------------------------------------------------------------------------------------------
+//----------------------------------------------------------------------------------------------------------------------