carrot_impl: fix sweep_all to create multiple txs with n_outputs *each*

src/carrot_impl/tx_proposal_utils.cpp

@@ -30,6 +30,7 @@
 #include "tx_proposal_utils.h"
 
 //local headers
+#include "carrot_core/exceptions.h"
 #include "carrot_core/output_set_finalization.h"
 #include "cryptonote_basic/cryptonote_format_utils.h"
 #include "misc_log_ex.h"
@@ -169,7 +170,7 @@ void make_carrot_transaction_proposal_v1(const std::vector<CarrotPaymentProposal
 
     // calculate the concrete fee for this transaction for each possible valid input count
     std::map<size_t, rct::xmr_amount> fee_per_input_count;
-    for (size_t num_ins = 1; num_ins <= CARROT_MAX_TX_INPUTS; ++num_ins)
+    for (size_t num_ins = CARROT_MIN_TX_INPUTS; num_ins <= CARROT_MAX_TX_INPUTS; ++num_ins)
     {
         const uint64_t tx_weight = get_fcmppp_tx_weight(num_ins, num_outs, tx_extra_size);
         CHECK_AND_ASSERT_THROW_MES(tx_weight != std::numeric_limits<uint64_t>::max(),
@@ -197,8 +198,14 @@ void make_carrot_transaction_proposal_v1(const std::vector<CarrotPaymentProposal
         selected_inputs);
 
     // get fee given the number of selected inputs
-    // note: this will fail if input selection returned a bad number of inputs
+    const std::size_t n_inputs = selected_inputs.size();
-    tx_proposal_out.fee = fee_per_input_count.at(selected_inputs.size());
+    CARROT_CHECK_AND_THROW(n_inputs >= CARROT_MIN_TX_INPUTS,
+        too_few_inputs, "input selection returned too few inputs: " << n_inputs);
+    CARROT_CHECK_AND_THROW(n_inputs <= CARROT_MAX_TX_OUTPUTS,
+        too_few_inputs, "input selection returned too many inputs: " << n_inputs);
+    CARROT_CHECK_AND_THROW(fee_per_input_count.count(n_inputs),
+        carrot_logic_error, "BUG: fee_per_input_count populated with holes, missing: " << n_inputs);
+    tx_proposal_out.fee = fee_per_input_count.at(n_inputs);
 
     // calculate input amount sum
     boost::multiprecision::uint128_t input_amount_sum = 0;

src/wallet/tx_builder.cpp

@@ -420,16 +420,16 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const std::vector<crypto::key_image> &input_key_images,
     const cryptonote::account_public_address &address,
     const bool is_subaddress,
-    const size_t n_dests,
+    const size_t n_dests_per_tx,
     const rct::xmr_amount fee_per_weight,
     const std::vector<uint8_t> &extra,
     const std::uint64_t top_block_index)
 {
     const size_t n_inputs = input_key_images.size();
-    CHECK_AND_ASSERT_THROW_MES(n_inputs,
+    CARROT_CHECK_AND_THROW(n_inputs, carrot::too_few_inputs, "no key images provided");
-        __func__ << ": no key images provided");
+    CARROT_CHECK_AND_THROW(n_dests_per_tx, carrot::too_few_outputs, "sweep must have at least one destination");
-    CHECK_AND_ASSERT_THROW_MES(n_dests,
+    CARROT_CHECK_AND_THROW(n_dests_per_tx <= FCMP_PLUS_PLUS_MAX_OUTPUTS,
-        __func__ << ": n_dests is zero");
+        carrot::too_many_outputs, "too many sweep destinations per transaction");
 
     // Check that the key image is usable and isn't spent, collect amounts, and get subaddress account index
     std::vector<rct::xmr_amount> input_amounts;
@@ -457,86 +457,51 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
         = find_change_address_spend_pubkey(subaddress_map, subaddr_account);
 
     // get 1 payment proposal corresponding to (address, is_subaddres)
-    std::vector<carrot::CarrotPaymentProposalV1> normal_payment_proposal;
+    std::vector<carrot::CarrotPaymentProposalV1> normal_payment_proposals;
-    std::vector<carrot::CarrotPaymentProposalVerifiableSelfSendV1> selfsend_payment_proposal;
+    std::vector<carrot::CarrotPaymentProposalVerifiableSelfSendV1> selfsend_payment_proposals;
-    const bool is_selfsend_dest = build_payment_proposals(normal_payment_proposal,
+    for (size_t i = 0; i < n_dests_per_tx; ++i)
-        selfsend_payment_proposal,
-        cryptonote::tx_destination_entry(/*amount=*/0, address, is_subaddress),
-        subaddress_map);
-    CHECK_AND_ASSERT_THROW_MES((is_selfsend_dest && selfsend_payment_proposal.size() == 1)
-            || (!is_selfsend_dest && normal_payment_proposal.size() == 1),
-        __func__ << ": BUG in build_payment_proposals: incorrect count for payment proposal lists");
-
-    // in/out/tx count calculations
-    const size_t max_dsts_per_tx = FCMP_PLUS_PLUS_MAX_OUTPUTS - (size_t(!is_selfsend_dest));
-    const size_t min_n_dests = div_ceil<size_t>(n_inputs, FCMP_PLUS_PLUS_MAX_INPUTS);
-    const size_t max_n_dests = n_inputs * max_dsts_per_tx;
-    CHECK_AND_ASSERT_THROW_MES(n_dests >= min_n_dests,
-        __func__ << ": not enough destinations (" << n_dests << ") for number of inputs to be spent ("
-        << n_inputs << ")");
-    CHECK_AND_ASSERT_THROW_MES(n_dests <= max_n_dests,
-        __func__ << ": too many destinations (" << n_dests << ") for number of inputs to be spent ("
-        << n_inputs << ")");
-
-    const size_t n_txs = std::max(div_ceil(n_dests, max_dsts_per_tx), min_n_dests);
-    CHECK_AND_ASSERT_THROW_MES(n_txs, __func__ << ": BUG: calculated target num of txs to be 0");
-
-    struct sweep_tx_outlay_t
     {
-        std::vector<carrot::CarrotSelectedInput> selected_inputs;
+        const bool is_selfsend_dest = build_payment_proposals(normal_payment_proposals,
-        size_t n_tx_dests;
+            selfsend_payment_proposals,
-    };
+            cryptonote::tx_destination_entry(/*amount=*/0, address, is_subaddress),
-
+            subaddress_map);
-    // build list of sweep_tx_outlay_t's
+        CHECK_AND_ASSERT_THROW_MES((is_selfsend_dest && selfsend_payment_proposals.size() == i+1)
-    std::vector<sweep_tx_outlay_t> tx_outlays(n_txs);
+                || (!is_selfsend_dest && normal_payment_proposals.size() == i+1),
-    size_t input_idx = 0;
+            __func__ << ": BUG in build_payment_proposals: incorrect count for payment proposal lists");
-    for (size_t tx_idx = 0; tx_idx < tx_outlays.size(); ++tx_idx)
-    {
-        sweep_tx_outlay_t &tx_outlay = tx_outlays[tx_idx];
-
-        const size_t n_remaining_inputs = n_inputs - input_idx;
-        const size_t n_tx_inputs = std::min(div_ceil(n_inputs, n_txs), n_remaining_inputs);
-        const size_t n_tx_dests = n_dests / n_txs + ((tx_idx < (n_dests % n_txs)) ? 1 : 0);
-
-        const size_t max_input_idx = input_idx + n_tx_inputs;
-        tx_outlay.selected_inputs.reserve(n_tx_inputs);
-        for (; input_idx < max_input_idx; ++input_idx)
-            tx_outlay.selected_inputs.push_back({input_amounts.at(input_idx), input_key_images.at(input_idx)});
-
-        tx_outlay.n_tx_dests = n_tx_dests;
     }
+    CARROT_CHECK_AND_THROW(normal_payment_proposals.size() < FCMP_PLUS_PLUS_MAX_OUTPUTS,
+        carrot::too_many_outputs, "too many *outgoing* sweep destinations per tx, we also need 1 self-send output");
 
-    //! @TODO: sanity check tx_outlays
+    // make `n_txs` tx proposals with `n_output` payment proposals each
-
+    const size_t n_txs = div_ceil<size_t>(n_inputs, FCMP_PLUS_PLUS_MAX_INPUTS);
-    // convert sweep outlays into transaction proposals
+    std::vector<carrot::CarrotTransactionProposalV1> tx_proposals(n_txs);
-    std::vector<carrot::CarrotTransactionProposalV1> tx_proposals;
+    size_t ki_idx = 0;
-    tx_proposals.reserve(tx_outlays.size());
+    for (carrot::CarrotTransactionProposalV1 &tx_proposal : tx_proposals)
-    for (sweep_tx_outlay_t &sweep_outlay : tx_outlays)
     {
-        std::vector<carrot::CarrotPaymentProposalV1> tx_normal_payment_proposals;
-        std::vector<carrot::CarrotPaymentProposalVerifiableSelfSendV1> tx_selfsend_payment_proposals;
-        if (is_selfsend_dest)
-            tx_selfsend_payment_proposals.resize(sweep_outlay.n_tx_dests, selfsend_payment_proposal.at(0));
-        else
-            tx_normal_payment_proposals.resize(sweep_outlay.n_tx_dests, normal_payment_proposal.at(0));
-
         // if a 2-selfsend, 2-out tx, flip one of the enote types to get unique derivations
-        if (tx_selfsend_payment_proposals.size() == 2)
+        if (selfsend_payment_proposals.size() == 2)
-            tx_selfsend_payment_proposals.back().proposal.enote_type = carrot::CarrotEnoteType::CHANGE;
+            selfsend_payment_proposals.back().proposal.enote_type = carrot::CarrotEnoteType::CHANGE;
 
-        carrot::CarrotTransactionProposalV1 tx_proposal;
+        // collect inputs for this tx
-        carrot::make_carrot_transaction_proposal_v1_sweep(tx_normal_payment_proposals,
+        const size_t ki_idx_end = std::min<size_t>(n_inputs, ki_idx + FCMP_PLUS_PLUS_MAX_INPUTS);
-            tx_selfsend_payment_proposals,
+        std::vector<carrot::CarrotSelectedInput> selected_inputs;
+        selected_inputs.reserve(n_inputs - ki_idx_end);
+        for (; ki_idx < ki_idx_end; ++ki_idx)
+            selected_inputs.push_back({input_amounts.at(ki_idx), input_key_images.at(ki_idx)});
+
+        carrot::make_carrot_transaction_proposal_v1_sweep(normal_payment_proposals,
+            selfsend_payment_proposals,
             fee_per_weight,
             extra,
-            std::move(sweep_outlay.selected_inputs),
+            std::move(selected_inputs),
             change_address_spend_pubkey,
             {{subaddr_account, 0}, carrot::AddressDeriveType::PreCarrot}, //! @TODO: handle Carrot keys
             tx_proposal);
-
-        tx_proposals.push_back(std::move(tx_proposal));
     }
 
+    CARROT_CHECK_AND_THROW(ki_idx == input_key_images.size(),
+        carrot::carrot_logic_error, "BUG: sweep_all did not consume the correct num of key images while iterating");
+
     return tx_proposals;
 }
 //-------------------------------------------------------------------------------------------------------------------
@@ -545,7 +510,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const std::vector<crypto::key_image> &input_key_images,
     const cryptonote::account_public_address &address,
     const bool is_subaddress,
-    const size_t n_dests,
+    const size_t n_dests_per_tx,
     const std::uint32_t priority,
     const std::vector<uint8_t> &extra)
 {
@@ -565,7 +530,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
         input_key_images,
         address,
         is_subaddress,
-        n_dests,
+        n_dests_per_tx,
         fee_per_weight,
         extra,
         top_block_index);
@@ -577,7 +542,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const rct::xmr_amount only_below,
     const cryptonote::account_public_address &address,
     const bool is_subaddress,
-    const size_t n_dests,
+    const size_t n_dests_per_tx,
     const rct::xmr_amount fee_per_weight,
     const std::vector<uint8_t> &extra,
     const std::uint32_t subaddr_account,
@@ -618,7 +583,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
         input_key_images,
         address,
         is_subaddress,
-        n_dests,
+        n_dests_per_tx,
         fee_per_weight,
         extra,
         top_block_index);
@@ -629,7 +594,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const rct::xmr_amount only_below,
     const cryptonote::account_public_address &address,
     const bool is_subaddress,
-    const size_t n_dests,
+    const size_t n_dests_per_tx,
     const std::uint32_t priority,
     const std::vector<uint8_t> &extra,
     const std::uint32_t subaddr_account,
@@ -651,7 +616,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
         only_below,
         address,
         is_subaddress,
-        n_dests,
+        n_dests_per_tx,
         fee_per_weight,
         extra,
         subaddr_account,

src/wallet/tx_builder.h

@@ -83,7 +83,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const std::vector<crypto::key_image> &input_key_images,
     const cryptonote::account_public_address &address,
     const bool is_subaddress,
-    const size_t n_dests,
+    const size_t n_dests_per_tx,
     const rct::xmr_amount fee_per_weight,
     const std::vector<uint8_t> &extra,
     const std::uint64_t top_block_index);
@@ -92,7 +92,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const std::vector<crypto::key_image> &input_key_images,
     const cryptonote::account_public_address &address,
     const bool is_subaddress,
-    const size_t n_dests,
+    const size_t n_dests_per_tx,
     const std::uint32_t priority,
     const std::vector<uint8_t> &extra);
 
@@ -102,7 +102,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const rct::xmr_amount only_below,
     const cryptonote::account_public_address &address,
     const bool is_subaddress,
-    const size_t n_dests,
+    const size_t n_dests_per_tx,
     const rct::xmr_amount fee_per_weight,
     const std::vector<uint8_t> &extra,
     const std::uint32_t subaddr_account,
@@ -113,7 +113,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const rct::xmr_amount only_below,
     const cryptonote::account_public_address &address,
     const bool is_subaddress,
-    const size_t n_dests,
+    const size_t n_dests_per_tx,
     const std::uint32_t priority,
     const std::vector<uint8_t> &extra,
     const std::uint32_t subaddr_account,

tests/unit_tests/wallet_tx_builder.cpp

@@ -296,7 +296,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_1)
         {transfers.front().m_key_image},
         bob.get_keys().m_account_address,
         /*is_subaddress=*/false,
-        /*n_dests=*/1,
+        /*n_dests_per_tx=*/1,
         /*fee_per_weight=*/1,
         /*extra=*/{},
         transfers.front().m_block_height + CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE);
@@ -330,7 +330,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_2)
         {transfers.front().m_key_image},
         bob.get_keys().m_account_address,
         /*is_subaddress=*/false,
-        /*n_dests=*/FCMP_PLUS_PLUS_MAX_OUTPUTS - 1,
+        /*n_dests_per_tx=*/FCMP_PLUS_PLUS_MAX_OUTPUTS - 1,
         /*fee_per_weight=*/1,
         /*extra=*/{},
         transfers.front().m_block_height + CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE);
@@ -372,7 +372,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_3)
         {transfers.front().m_key_image},
         alice.get_keys().m_account_address,
         /*is_subaddress=*/false,
-        /*n_dests=*/FCMP_PLUS_PLUS_MAX_OUTPUTS,
+        /*n_dests_per_tx=*/FCMP_PLUS_PLUS_MAX_OUTPUTS,
         /*fee_per_weight=*/1,
         /*extra=*/{},
         transfers.front().m_block_height + CRYPTONOTE_DEFAULT_TX_SPENDABLE_AGE);
@@ -438,7 +438,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_4)
     ASSERT_EQ(n_selected_transfers, selected_key_images.size());
     ASSERT_EQ(n_selected_transfers, amounts_by_ki.size());
 
-    const size_t n_dests = 4;
+    const size_t n_dests_per_tx = 4;
 
     // make tx proposals
     const std::vector<carrot::CarrotTransactionProposalV1> tx_proposals = tools::wallet::make_carrot_transaction_proposals_wallet2_sweep(
@@ -447,7 +447,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_4)
         selected_key_images,
         bob.get_keys().m_account_address,
         /*is_subaddress=*/false,
-        /*n_dests=*/n_dests,
+        /*n_dests_per_tx=*/n_dests_per_tx,
         /*fee_per_weight=*/1,
         /*extra=*/{},
         top_block_index);
@@ -455,11 +455,10 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_4)
 
     std::set<crypto::key_image> actual_seen_kis;
     size_t n_actual_inputs = 0;
-    size_t n_actual_dests = 0;
     for (const carrot::CarrotTransactionProposalV1 &tx_proposal : tx_proposals)
     {
         ASSERT_LE(tx_proposal.key_images_sorted.size(), FCMP_PLUS_PLUS_MAX_INPUTS);
-        ASSERT_EQ(1, tx_proposal.normal_payment_proposals.size());
+        ASSERT_EQ(n_dests_per_tx, tx_proposal.normal_payment_proposals.size());
         ASSERT_EQ(1, tx_proposal.selfsend_payment_proposals.size());
         ASSERT_EQ(0, tx_proposal.selfsend_payment_proposals.at(0).proposal.amount);
         EXPECT_EQ(0, tx_proposal.extra.size());
@@ -472,15 +471,15 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_4)
             actual_seen_kis.insert(ki);
             tx_inputs_amount += amounts_by_ki.at(ki);
         }
-        const rct::xmr_amount tx_outputs_amount = tx_proposal.fee + tx_proposal.normal_payment_proposals.at(0).amount;
+        rct::xmr_amount tx_outputs_amount = tx_proposal.fee;
+        for (const carrot::CarrotPaymentProposalV1 &normal_payment_proposal : tx_proposal.normal_payment_proposals)
+            tx_outputs_amount += normal_payment_proposal.amount;
         ASSERT_EQ(tx_inputs_amount, tx_outputs_amount);
 
         n_actual_inputs += tx_proposal.key_images_sorted.size();
-        n_actual_dests += tx_proposal.normal_payment_proposals.size();
     }
 
     EXPECT_EQ(n_selected_transfers, n_actual_inputs);
-    EXPECT_EQ(n_dests, n_actual_dests);
 }
 //----------------------------------------------------------------------------------------------------------------------
 TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_5)
@@ -520,7 +519,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_5)
     ASSERT_EQ(n_selected_transfers, selected_key_images.size());
     ASSERT_EQ(n_selected_transfers, amounts_by_ki.size());
 
-    const size_t n_dests = 8;
+    const size_t n_dests_per_tx = 8;
 
     // make tx proposals
     const std::vector<carrot::CarrotTransactionProposalV1> tx_proposals = tools::wallet::make_carrot_transaction_proposals_wallet2_sweep(
@@ -529,7 +528,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_5)
         selected_key_images,
         alice.get_keys().m_account_address,
         /*is_subaddress=*/false,
-        /*n_dests=*/n_dests,
+        /*n_dests_per_tx=*/n_dests_per_tx,
         /*fee_per_weight=*/1,
         /*extra=*/{},
         top_block_index);
@@ -537,13 +536,15 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_5)
 
     std::set<crypto::key_image> actual_seen_kis;
     size_t n_actual_inputs = 0;
-    size_t n_actual_dests = 0;
     for (const carrot::CarrotTransactionProposalV1 &tx_proposal : tx_proposals)
     {
         ASSERT_LE(tx_proposal.key_images_sorted.size(), FCMP_PLUS_PLUS_MAX_INPUTS);
-        ASSERT_EQ(1, tx_proposal.normal_payment_proposals.size());
+        ASSERT_EQ(n_dests_per_tx == 1 ? 1 : 0, tx_proposal.normal_payment_proposals.size());
-        ASSERT_EQ(1, tx_proposal.selfsend_payment_proposals.size());
+        ASSERT_EQ(n_dests_per_tx, tx_proposal.selfsend_payment_proposals.size());
-        ASSERT_EQ(0, tx_proposal.normal_payment_proposals.at(0).amount);
+        if (!tx_proposal.normal_payment_proposals.empty())
+        {
+            ASSERT_EQ(0, tx_proposal.normal_payment_proposals.at(0).amount);
+        }
         EXPECT_EQ(0, tx_proposal.extra.size());
 
         rct::xmr_amount tx_inputs_amount = 0;
@@ -554,15 +555,15 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_5)
             actual_seen_kis.insert(ki);
             tx_inputs_amount += amounts_by_ki.at(ki);
         }
-        const rct::xmr_amount tx_outputs_amount = tx_proposal.fee + tx_proposal.selfsend_payment_proposals.at(0).proposal.amount;
+        rct::xmr_amount tx_outputs_amount = tx_proposal.fee;
+        for (const carrot::CarrotPaymentProposalVerifiableSelfSendV1 &selfsend_payment_proposal : tx_proposal.selfsend_payment_proposals)
+            tx_outputs_amount += selfsend_payment_proposal.proposal.amount;
         ASSERT_EQ(tx_inputs_amount, tx_outputs_amount);
 
         n_actual_inputs += tx_proposal.key_images_sorted.size();
-        n_actual_dests += tx_proposal.selfsend_payment_proposals.size();
     }
 
     EXPECT_EQ(n_selected_transfers, n_actual_inputs);
-    EXPECT_EQ(n_dests, n_actual_dests);
 }
 //----------------------------------------------------------------------------------------------------------------------
 TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_6)
@@ -602,7 +603,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_6)
     ASSERT_EQ(n_selected_transfers, selected_key_images.size());
     ASSERT_EQ(n_selected_transfers, amounts_by_ki.size());
 
-    const size_t n_dests = 2;
+    const size_t n_dests_per_tx = 2;
 
     // make tx proposals
     const std::vector<carrot::CarrotTransactionProposalV1> tx_proposals = tools::wallet::make_carrot_transaction_proposals_wallet2_sweep(
@@ -611,7 +612,7 @@ TEST(wallet_tx_builder, make_carrot_transaction_proposals_wallet2_sweep_6)
         selected_key_images,
         alice.get_keys().m_account_address,
         /*is_subaddress=*/false,
-        /*n_dests=*/n_dests,
+        /*n_dests_per_tx=*/n_dests_per_tx,
         /*fee_per_weight=*/1,
         /*extra=*/{},
         top_block_index);