Fix max outputs in tx construction, and fix static asserts

include/lws_frontend.h

@@ -29,6 +29,7 @@
 #pragma once
 
 #include <cstdint>
+#include <optional>
 #include <string>
 #include <vector>
 #include "wallet/api/wallet2_api.h" // monero/src
@@ -36,7 +37,7 @@
 namespace lwsf
 {
   std::string displayAmount(std::uint64_t amount);
-  std::uint64_t amountFromString(const std::string &amount);
+  std::optional<std::uint64_t> amountFromString(const std::string &amount);
   bool addressValid(const std::string &str, Monero::NetworkType nettype);
 
   std::vector<std::vector<std::uint8_t>> qrcode(Monero::Wallet const* wal, std::uint32_t major, std::uint32_t minor);

src/backend.cpp

@@ -576,8 +576,15 @@ namespace lwsf { namespace internal { namespace backend
       self.primary.restore_height = source.start_height;
       self.primary.scan_height = source.scanned_block_height;
 
-      self.per_byte_fee = unspents.per_byte_fee;
       self.fee_mask = unspents.fee_mask;
+      if (unspents.fees.empty())
+      {
+        self.per_byte_fee.resize(1);
+        self.per_byte_fee[0] = unspents.per_byte_fee;
+      }
+      else
+        self.per_byte_fee = unspents.fees;
+
       return out;
     }
 
@@ -787,12 +794,12 @@ namespace lwsf { namespace internal { namespace backend
     : listener(nullptr),
       client(),
       primary{},
+      per_byte_fee(),
       refresh_error(),
       lookahead_error(default_subaddr_state),
       import_error(),
       last_sync(),
       blockchain_height(0),
-      per_byte_fee(0),
       fee_mask(0),
       server_lookahead(0),
       sync(),
@@ -879,7 +886,7 @@ namespace lwsf { namespace internal { namespace backend
     {
       passed_login = false;
       blockchain_height = 0;
-      per_byte_fee = 0;
+      per_byte_fee.clear();
       fee_mask = 0;  
       server_lookahead = 0;
       import_error = {};

src/backend.h

@@ -227,12 +227,12 @@ namespace lwsf { namespace internal { namespace backend
     Monero::WalletListener* listener;
     rpc::http_client client;
     account primary;
+    std::vector<std::uint64_t> per_byte_fee; //!< by priority level
     std::error_code refresh_error; //!< Cached because `refresh(...)` is rate limited
     std::error_code lookahead_error; //!< warnings/errors of `server_lookahead` value
     std::error_code import_error; //!< Error from `import_wallet_request`
     std::chrono::steady_clock::time_point last_sync;
     std::uint64_t blockchain_height;
-    std::uint64_t per_byte_fee;
     std::uint64_t fee_mask;
     std::uint32_t server_lookahead; //!< Status of major lookahead server-side. Inclusive
     mutable boost::mutex sync;

src/error.cpp

@@ -55,26 +55,6 @@ namespace lwsf
       return "server has subaddresses disabled";
     case error::subaddr_local:
       return "lwsf (local) limits on subaddresses too small";
-    case error::tx_critical:
-      return "Tx construction had critical error";
-    case error::tx_decoys:
-      return "Tx construction failed due to invalid decoy response";
-    case error::tx_extra:
-      return "Tx construction failed to generate tx extra";
-    case error::tx_failed:
-      return "Tx construction failed (construct_tx failure)";
-    case error::tx_invalid_address:
-      return "Tx construction was given invalid address";
-    case error::tx_long_pid:
-      return "Tx construction requested with deprecated long payment id";
-    case error::tx_low_funds:
-      return  "Tx construction failed due to insufficient funds";
-    case error::tx_size_mismatch:
-      return "Tx construction requested with amount and destination mismatch";
-    case error::tx_sweep:
-      return "Tx construction sweep with multiple destinations";
-    case error::tx_two_pid:
-      return "Tx construction given two payment ids";
     case error::unexpected_userinfo:
       return "Unexpected user+pass field in URL";
 

src/error.h

@@ -45,16 +45,6 @@ namespace lwsf
     subaddr_ahead,      //!< Server limits on subaddresses affects lookahead
     subaddr_disabled,   //!< Server has subaddresses disabled
     subaddr_local,      //!< Local limits on subaddresses too small
-    tx_critical,        //!< Tx creation had critical error
-    tx_decoys,          //!< Tx creation failed due to invalid decoy response
-    tx_extra,           //!< Tx creation failed to generate tx extra
-    tx_failed,          //!< Tx creation failed (contruct_tx failure)
-    tx_invalid_address, //!< Tx creation was given invalid address
-    tx_long_pid,        //!< Tx creation with long payment_id (deprecated)
-    tx_low_funds,       //!< Tx creation failed due to insufficient funds
-    tx_size_mismatch,   //!< Tx creation mismatch on amounts and destinations
-    tx_two_pid,         //!< Tx creation given two payment ids
-    tx_sweep,           //!< Tx creation requested sweep to multiple dests
     unexpected_userinfo,//!< Unexpected user+pass provided
   };
 

src/lwsf_config.h

@@ -48,10 +48,11 @@ namespace lwsf { namespace config
   constexpr const lookahead default_minimal_lookahead{5, 15};
   constexpr const std::string_view default_primary_name{"Primary account"};
   constexpr const std::size_t initial_buffer_size = 1024 * 64; // 64 KiB
-  constexpr const std::size_t max_inputs = 512;
+  constexpr const std::size_t max_inputs_in_rpc = 512;
-  constexpr const std::size_t max_ring_size = 128;
+  constexpr const std::size_t max_outputs_in_construction = 16;
+  constexpr const std::size_t max_ring_size_in_rpc = 128;
   constexpr const std::size_t max_txes_in_rpc = 2048;
-  constexpr const std::size_t minimum_outputs = 2;
+  constexpr const std::size_t min_outputs = 2;
   constexpr const std::uint32_t mixin_default = 15;
   constexpr const std::chrono::seconds refresh_interval{30};
   constexpr const std::chrono::seconds refresh_interval_min{5};

src/pending_transaction.cpp

@@ -47,11 +47,17 @@ namespace lwsf { namespace internal
       return {};
     }
   }
-  pending_transaction::pending_transaction(std::shared_ptr<backend::wallet> wallet, expect<cryptonote::transaction> source, std::shared_ptr<backend::transaction> local)
+  pending_transaction::pending_transaction(std::shared_ptr<backend::wallet> wallet, std::string error, std::vector<std::shared_ptr<backend::transaction>> local)
-    : wallet_(std::move(wallet)), error_(source.error()), local_(std::move(local))
+    : wallet_(std::move(wallet)), local_(std::move(local)), error_(std::move(error))
   {
     if (!wallet_)
-      throw std::invalid_argument{"lwsf::internal::pending_transction cannot be given nullptr backend"};
+      throw std::invalid_argument{"lwsf::internal::pending_transaction cannot be given nullptr backend"};
+
+    for (const auto& e : local)
+    {
+      if (!e)
+        throw std::invalid_argument{"lwsf::internal::pending_transaction cannot be given nullptr backend::transaction"};
+    }
   }
 
   pending_transaction::~pending_transaction()
@@ -59,56 +65,53 @@ namespace lwsf { namespace internal
 
   int pending_transaction::status() const
   {
-    return error_ ? Status_Error : Status_Ok;
+    return error_.empty() ? Status_Ok : Status_Error;
-  }
-
-  std::string pending_transaction::errorString() const
-  {
-    if (error_)
-      return error_.message();
-    return {};
   }
 
   bool pending_transaction::commit(const std::string &filename, bool)
   {
     // saving to file not yet implemented
-    if (error_ || !filename.empty())
+    if (!error_.empty() || !filename.empty())
       return false;
 
-    if (local_)
+    for (auto e : local_)
-      error_ = wallet_->send_tx(local_->raw_bytes.clone());
-    else if (!error_)
-      error_ = error::tx_failed;
-
-    if (!error_ && local_)
     {
+      const std::error_code error = wallet_->send_tx(e->raw_bytes.clone());
+      if (error)
+      {
+        error_ = error.message();
+        return false;
+      }
       const boost::lock_guard<boost::mutex> lock{wallet_->sync};
-      auto entry = wallet_->primary.txes.try_emplace(local_->id).first;
+      auto entry = wallet_->primary.txes.try_emplace(e->id).first;
       if (!entry->second)
-        entry->second = local_;
+        entry->second = std::move(e);
     }
 
-    return !error_;
+    return true;
   }
 
   std::uint64_t pending_transaction::amount() const
   {
-    if (!local_)
+    std::uint64_t out = 0;
-      return 0;
+    for (const auto& e : local_)
-    return local_->amount;
+      out += e->amount;
+    return out;
   }
 
   std::uint64_t pending_transaction::fee() const
   {
-    if (!local_)
+    std::uint64_t out = 0;
-      return 0;
+    for (const auto& e : local_)
-    return local_->fee;
+      out += e->fee;
+    return out;
   }
 
   std::vector<std::string> pending_transaction::txid() const
   {
-    if (!local_)
+    std::vector<std::string> out;
-      return {};
+    for (const auto& e : local_)
-    return {epee::to_hex::string(epee::as_byte_span(local_->id))};
+      out.push_back(epee::to_hex::string(epee::as_byte_span(e->id)));
+    return out;
   }
 }} // lwsf // internal

src/pending_transaction.h

@@ -49,11 +49,11 @@ namespace lwsf { namespace internal
   class pending_transaction final : public Monero::PendingTransaction
   {
     const std::shared_ptr<backend::wallet> wallet_;
-    std::error_code error_;
+    const std::vector<std::shared_ptr<backend::transaction>> local_;
-    const std::shared_ptr<backend::transaction> local_;
+    std::string error_;
 
   public:
-    pending_transaction(std::shared_ptr<backend::wallet> wallet, expect<cryptonote::transaction> source, std::shared_ptr<backend::transaction> local = {}); 
+    pending_transaction(std::shared_ptr<backend::wallet> wallet, std::string error, std::vector<std::shared_ptr<backend::transaction>> local = {}); 
 
     pending_transaction(pending_transaction&&) = delete;
     pending_transaction(const pending_transaction&) = delete;
@@ -64,13 +64,13 @@ namespace lwsf { namespace internal
     pending_transaction& operator=(const pending_transaction&) = delete;
 
     virtual int status() const override final;
-    virtual std::string errorString() const override final;
+    virtual std::string errorString() const override final { return error_; }
     virtual bool commit(const std::string &filename = "", bool overwrite = false) override final;
     virtual std::uint64_t amount() const override final;
     virtual std::uint64_t dust() const override final { return 0; }
     virtual std::uint64_t fee() const override final;
     virtual std::vector<std::string> txid() const override final;
-    virtual std::uint64_t txCount() const override final { return 1; }
+    virtual std::uint64_t txCount() const override final { return local_.size(); }
     virtual std::vector<uint32_t> subaddrAccount() const override final { return {}; }
     virtual std::vector<std::set<uint32_t>> subaddrIndices() const override final { return {}; }
 

src/rpc.cpp

@@ -262,7 +262,7 @@ namespace lwsf { namespace internal { namespace rpc
 
   void read_bytes(wire::json_reader& source, random_outputs& self)
   {
-    using max_ring = wire::max_element_count<config::max_ring_size>;
+    using max_ring = wire::max_element_count<config::max_ring_size_in_rpc>;
     wire::object(source, WIRE_FIELD_ARRAY(outputs, max_ring), WIRE_FIELD(amount));
   }
 
@@ -274,7 +274,7 @@ namespace lwsf { namespace internal { namespace rpc
 
   void read_bytes(wire::json_reader& source, get_random_outs_response& self)
   {
-    using max_inputs = wire::max_element_count<config::max_inputs>;
+    using max_inputs = wire::max_element_count<config::max_inputs_in_rpc>;
     wire::object(source, WIRE_FIELD_ARRAY(amount_outs, max_inputs));
   }
 
@@ -421,8 +421,10 @@ namespace lwsf { namespace internal { namespace rpc
     using min_output_size = wire::min_element_sizeof<
       ringct, crypto::hash, crypto::hash, crypto::public_key, crypto::public_key
     >;
+    using max_fees = wire::max_element_count<8>;
     wire::object(source,
       WIRE_FIELD_ARRAY(outputs, min_output_size),
+      WIRE_FIELD_ARRAY(fees, max_fees),
       WIRE_FIELD(per_byte_fee),
       WIRE_FIELD(fee_mask)
     );

src/rpc.h

@@ -370,6 +370,7 @@ namespace lwsf { namespace internal { namespace rpc
     static constexpr const char* endpoint() noexcept { return "/get_unspent_outs"; }
 
     std::vector<output> outputs;
+    std::vector<std::uint64_t> fees;
     std::uint64_t per_byte_fee;
     std::uint64_t fee_mask;
   };

src/wallet.cpp

@@ -33,6 +33,7 @@
 #endif
 
 #include <algorithm>
+#include <boost/range/combine.hpp>
 #include <exception>
 #include <filesystem>
 #include <fstream>
@@ -63,11 +64,11 @@
 #include "wire/msgpack.h"
 
 //! Runtime-check (assertion) for tx construction
-#define LWSF_TX_ASSERT(x) \
+#define LWSF_TX_VERIFY(x) \
   do                      \
   {                       \
     if (!(x))             \
-      throw std::logic_error{"Tx construction verify (line " + std::to_string(__LINE__) + "): " + #x}; \
+      throw std::logic_error{"Tx construction assertion failed (line " + std::to_string(__LINE__) + "): " + #x}; \
   } while (0)
 
 namespace lwsf { namespace internal
@@ -88,14 +89,14 @@ namespace lwsf { namespace internal
       }
     };
 
-    std::uint64_t calculate_fee_from_weight(const std::uint64_t base_fee, const std::uint64_t weight, const std::uint64_t fee_quantization_mask)
+    std::uint64_t calculate_fee_from_weight(const std::uint64_t base_fee, const std::uint64_t weight, const std::uint64_t fee_quantization_mask) noexcept
     {
       uint64_t fee = weight * base_fee;
       fee = (fee + fee_quantization_mask - 1) / fee_quantization_mask * fee_quantization_mask;
       return fee;
     }
 
-    std::size_t estimate_rct_tx_size(const std::size_t n_inputs, const std::size_t n_outputs, const std::uint32_t mixin, const std::size_t extra_size)
+    std::size_t estimate_rct_tx_size(const std::size_t n_inputs, const std::size_t n_outputs, const std::uint32_t mixin, const std::size_t extra_size) noexcept
     {
       std::size_t size = 0;
 
@@ -155,7 +156,7 @@ namespace lwsf { namespace internal
       return size;
     }
 
-    std::uint64_t estimate_tx_weight(const std::size_t n_inputs, const std::size_t n_outputs, const std::uint32_t mixin, const std::size_t extra_size)
+    std::uint64_t estimate_tx_weight(const std::size_t n_inputs, const std::size_t n_outputs, const std::uint32_t mixin, const std::size_t extra_size) noexcept
     {
       size_t size = estimate_rct_tx_size(n_inputs, n_outputs, mixin, extra_size);
       if (/*use_rct && (bulletproof || bulletproof_plus) && */n_outputs > 2)
@@ -173,23 +174,11 @@ namespace lwsf { namespace internal
       return size;
     }
 
-    std::uint64_t estimate_fee(const std::uint64_t base_fee, const std::size_t n_inputs, const std::size_t n_outputs, const std::uint32_t mixin, const std::size_t extra_size, const std::uint64_t fee_mask)
+    std::uint64_t estimate_fee(const std::uint64_t base_fee, const std::size_t n_inputs, const std::size_t n_outputs, const std::uint32_t mixin, const std::size_t extra_size, const std::uint64_t fee_mask) noexcept
     {
       return calculate_fee_from_weight(base_fee, estimate_tx_weight(n_inputs, n_outputs, mixin, extra_size), fee_mask);
     }
 
-    class need_more_sources final : public std::exception
-    {
-      std::uint64_t fee_;
-    public:
-      explicit need_more_sources(const std::uint64_t fee) noexcept
-        : std::exception(), fee_(fee)
-      {}
-
-      std::uint64_t fee() const noexcept { return fee_; }
-      const char* what() const noexcept override { return "need_more_sources"; }
-    };
-
     struct encrypted_file
     {
       std::string cipher;        //!< Name of cipher+authentication used
@@ -423,6 +412,18 @@ namespace lwsf { namespace internal
    #error atomic_file_write not implemented for this platform
 #endif
 
+    class need_more_sources final : public std::exception
+    {
+      std::uint64_t fee_;
+    public:
+      explicit need_more_sources(const std::uint64_t fee) noexcept
+        : std::exception(), fee_(fee)
+      {}
+
+      std::uint64_t fee() const noexcept { return fee_; }
+      const char* what() const noexcept override { return "need_more_sources"; }
+    };
+
     struct unknown_exception : std::exception
     {
       unknown_exception() noexcept
@@ -434,9 +435,15 @@ namespace lwsf { namespace internal
 
     constexpr boost::chrono::nanoseconds to_boost(const std::chrono::nanoseconds source) noexcept
     {
-      static_assert(
+      // types could differ - verify they can represent all the same values
-        std::is_same<boost::chrono::nanoseconds::rep, std::chrono::nanoseconds::rep>{}
+      using boost_rep = boost::chrono::nanoseconds::rep;
-      );
+      using std_rep = std::chrono::nanoseconds::rep;
+      static_assert(std::is_integral<boost_rep>());
+      static_assert(std::is_integral<std_rep>());
+      static_assert(std::is_signed<boost_rep>());
+      static_assert(std::is_signed<std_rep>());
+      static_assert(std::numeric_limits<boost_rep>::min() == std::numeric_limits<std_rep>::min());
+      static_assert(std::numeric_limits<boost_rep>::max() == std::numeric_limits<std_rep>::max());
       return boost::chrono::nanoseconds{source.count()};
     }
 
@@ -578,7 +585,7 @@ namespace lwsf { namespace internal
       return false;
 
     encrypted_file contents{};
-    if (!wire::msgpack::from_bytes(std::move(file), contents))
+    if (wire::msgpack::from_bytes(std::move(file), contents))
       return false;
     return !contents.get_payload(password).empty();
   }
@@ -1261,17 +1268,30 @@ namespace lwsf { namespace internal
 
   Monero::PendingTransaction* wallet::createTransactionMultDest(const std::vector<std::string> &dst_addr, const std::string &payment_id,
                                                    Monero::optional<std::vector<uint64_t>> amount, uint32_t mixin_count,
-                                                   Monero::PendingTransaction::Priority priority,
+                                                   const Monero::PendingTransaction::Priority priority,
-                                                   uint32_t subaddr_account,
+                                                   const uint32_t subaddr_account,
                                                    std::set<uint32_t> subaddr_indices)
   {
+    using destinations_vector = std::vector<cryptonote::tx_destination_entry>;
+    using transfers_map = std::multiset<std::pair<std::uint64_t, std::string>>;
+    using unspent_map =
+      std::unordered_map<crypto::public_key, std::pair<std::pair<std::uint64_t, std::uint64_t>, std::shared_ptr<const backend::transaction>>>;
+    using unspent_by_amount_map =
+      std::map<std::pair<std::uint64_t, std::uint64_t>, std::pair<crypto::public_key, std::shared_ptr<const backend::transaction>>>;
+
+    // default priority not handled like standard wallet
+    static_assert(Monero::PendingTransaction::Priority_Default == 0);
+    static_assert(Monero::PendingTransaction::Priority_Medium == 2);
+
+    const bool subtract_from_dest = !amount;
+    const unsigned priority_int = priority <= 0 ? 2 : unsigned(priority) - 1;
     if (!mixin_count)
       mixin_count = mixin_;
 
     std::unique_ptr<internal::pending_transaction> out;
-    const auto tx_error = [this] (std::error_code error)
+    const auto throw_low_funds = [] ()
     {
-      return std::make_unique<internal::pending_transaction>(data_, std::move(error));
+      throw std::runtime_error{"Unlocked funds too low"};
     };
 
     try
@@ -1282,29 +1302,35 @@ namespace lwsf { namespace internal
       out = [&, this] ()
         {
           if (!amount && dst_addr.size() > 1)
-            return tx_error(error::tx_sweep);
+            throw std::invalid_argument{"Sweep requested with multiple destinations"};
           if (amount && amount->size() != dst_addr.size())
-            return tx_error(error::tx_size_mismatch);
+            throw std::invalid_argument{"Must have equal amounts and destinations"};
+          if (amount && amount->empty())
+            throw std::invalid_argument{"Zero destinations only valid with empty amounts"};
           if (!payment_id.empty())
-            return tx_error(error::tx_long_pid);
+            throw std::invalid_argument{"Long payment id was provided - deprecated"};
 
           cryptonote::network_type ctype{};
           Monero::NetworkType mtype{};
           std::uint64_t per_byte_fee = 0;
           std::uint64_t fee_mask = 0;
-          std::string change_address;
+          std::string change_address{};
           cryptonote::account_keys keys{};
           cryptonote::account_public_address change_account{};
-          std::unordered_map<crypto::public_key, std::pair<std::pair<std::uint64_t, std::uint64_t>, std::shared_ptr<const backend::transaction>>> unspent;
+          unspent_map unspent{};
           {
             data_->refresh(); // get latest outputs, block height, and fee info
 
             const boost::lock_guard<boost::mutex> lock{data_->sync};
-            per_byte_fee = data_->per_byte_fee;
             fee_mask = data_->fee_mask;
 
-            if (!per_byte_fee || !fee_mask || !data_->passed_login)
+            if (data_->per_byte_fee.empty() || !fee_mask || !data_->passed_login)
-              return tx_error(data_->refresh_error);
+              throw std::system_error{data_->refresh_error, "Tx construction"};
+
+            if (data_->per_byte_fee.size() <= priority_int)
+              per_byte_fee = data_->per_byte_fee.back();
+            else
+              per_byte_fee = data_->per_byte_fee[priority_int];
 
             mtype = data_->primary.type;
             ctype = data_->get_net_type();
@@ -1346,86 +1372,36 @@ namespace lwsf { namespace internal
             }
           }
  
-          const bool subtract_from_dest = !amount;
-          if (subtract_from_dest)
-          {
-            std::uint64_t total_unspent = 0;
-            for (const auto& tx : unspent)
-            {
-              const auto output = tx.second.second->receives.find(tx.first);
-              LWSF_TX_ASSERT(output != tx.second.second->receives.end());
-              total_unspent += output->second.amount;
-            }
-            amount = std::vector<std::uint64_t>{total_unspent};
-          }
-
-          LWSF_TX_ASSERT(dst_addr.size() <= amount->size());
-
           // By copy to catch mutations (want each invoke to be consistent)
-          const auto gather_sources_and_construct_tx = [=] (const std::uint64_t min_fee)
+          const auto gather_sources_and_construct_tx =
-            -> std::unique_ptr<internal::pending_transaction>
+            [=] (unspent_by_amount_map unspent_by_amount, destinations_vector dests, const transfers_map& transfers, const std::string& extra_nonce, const std::uint64_t min_fee)
+              -> std::shared_ptr<backend::transaction>
           {
-            std::string extra_nonce;
+            // leave 1 spot for change
-            std::vector<cryptonote::tx_destination_entry> dests;
+            LWSF_TX_VERIFY(!dests.empty());
-            std::multiset<std::pair<std::uint64_t, std::string>> transfers;
+            LWSF_TX_VERIFY(dests.size() < config::max_outputs_in_construction);
-            for (std::size_t i = 0; i < dst_addr.size(); ++i)
+            LWSF_TX_VERIFY(dests.size() == transfers.size());
-            {
-              cryptonote::address_parse_info info{};
-              if (!cryptonote::get_account_address_from_str(info, ctype, dst_addr.at(i)))
-                return tx_error(error::tx_invalid_address);
 
-              if (info.has_payment_id)
+            std::uint64_t transfer_total = 0;
-              {
+            for (const auto & e : dests)
-                if (!extra_nonce.empty())
+              transfer_total += e.amount;
-                  return tx_error(error::tx_two_pid);
-                cryptonote::set_encrypted_payment_id_to_tx_extra_nonce(extra_nonce, info.payment_id);
-              }
-
-              transfers.insert({amount->at(i), dst_addr.at(i)});
-
-              auto& dest = dests.emplace_back();
-              dest.original = dst_addr.at(i);
-              dest.addr = info.address;
-              dest.amount = amount->at(i);
-              dest.is_subaddress = info.is_subaddress;
-              dest.is_integrated = info.has_payment_id;
-            }
-
-            for (std::size_t i = dst_addr.size(); i < amount->size(); ++i)
-            {
-              // this is a "self-sweep" (consolidation)
-              auto& dest = dests.emplace_back();
-              dest.original = change_address;
-              dest.addr = change_account;
-              dest.amount = amount->at(i);
-              dest.is_subaddress = subaddr_account;
-              dest.is_integrated = false;
-            }
 
             std::vector<std::uint8_t> extra;
             if (!extra_nonce.empty() && !cryptonote::add_extra_nonce_to_tx_extra(extra, extra_nonce))
-              return tx_error(error::tx_extra);
+              throw std::runtime_error{"cryptonote:add_extra_nonce_to_tx_extra failed"};
-
-
-            // actually sorted by amount then reverse index (see comment above)
-            std::map<std::pair<std::uint64_t, std::uint64_t>, std::pair<crypto::public_key, std::shared_ptr<const backend::transaction>>> unspent_by_amount_then_index;
-            for (const auto& tx : unspent)
-              unspent_by_amount_then_index.try_emplace(tx.second.first).first->second = {tx.first, tx.second.second};
 
             bool has_change = false;
             std::vector<std::pair<crypto::public_key, std::shared_ptr<const backend::transaction>>> spending;
             {
-              std::uint64_t fee = 0;
+              std::uint64_t fee = 1;
-              std::uint64_t remaining =
+              std::uint64_t remaining = transfer_total;
-                std::accumulate(amount->begin(), amount->end(), std::uint64_t(0));
-
               while (bool(remaining + fee))
               {
-                if (unspent_by_amount_then_index.empty())
+                if (unspent_by_amount.empty())
                 {
                   if (subtract_from_dest && !remaining)
                     break;
-                  return tx_error(error::tx_low_funds);
+                  throw_low_funds();
                 }
 
                 /* Check if we can omit change address for more efficiency. We
@@ -1435,12 +1411,12 @@ namespace lwsf { namespace internal
                   fee = estimate_fee(per_byte_fee, spending.size() + 1, dests.size(), mixin_count, extra.size(), fee_mask);
                   fee = std::max(min_fee, fee);
                   const std::uint64_t needed = remaining + fee;
-                  const auto output = unspent_by_amount_then_index.lower_bound({needed, 0});
+                  const auto output = unspent_by_amount.lower_bound({needed, 0});
-                  if (output != unspent_by_amount_then_index.end() && needed == output->first.first)
+                  if (output != unspent_by_amount.end() && needed == output->first.first)
                   {
                     remaining = 0;
                     spending.emplace_back(output->second.first, output->second.second);
-                    unspent_by_amount_then_index.erase(output);
+                    unspent_by_amount.erase(output);
                     break;
                   }
                 }
@@ -1450,14 +1426,14 @@ namespace lwsf { namespace internal
                 fee = std::max(min_fee, fee);
 
                 const std::uint64_t needed = remaining + fee;
-                auto output = unspent_by_amount_then_index.lower_bound({needed, 0});
+                auto output = unspent_by_amount.lower_bound({needed, 0});
-                if (output == unspent_by_amount_then_index.end())
+                if (output == unspent_by_amount.end())
                   --output;
 
                 const std::uint64_t this_amount = output->first.first;
                 const bool complete = needed <= this_amount;
                 spending.emplace_back(output->second.first, output->second.second);
-                unspent_by_amount_then_index.erase(output);
+                unspent_by_amount.erase(output);
 
                 remaining -= this_amount;
                 if (complete)
@@ -1466,9 +1442,9 @@ namespace lwsf { namespace internal
 
               if (subtract_from_dest)
               {
-                LWSF_TX_ASSERT(dests.size() == 1);
+                LWSF_TX_VERIFY(dests.size() == 1);
                 if (dests.back().amount < fee)
-                  return tx_error(error::tx_low_funds);
+                  throw_low_funds();
                 dests.back().amount -= fee;
                 fee = 0;
               }
@@ -1485,7 +1461,7 @@ namespace lwsf { namespace internal
                 change_dest.is_integrated = false;
               }
 
-              LWSF_TX_ASSERT(config::minimum_outputs <= dests.size());
+              LWSF_TX_VERIFY(config::min_outputs <= dests.size());
 
               // verify SUM(input) = SUM(output) - fee
 
@@ -1493,14 +1469,14 @@ namespace lwsf { namespace internal
               for (const auto& source : spending)
               {
                 const auto output = source.second->receives.find(source.first);
-                LWSF_TX_ASSERT(output != source.second->receives.end());
+                LWSF_TX_VERIFY(output != source.second->receives.end());
                 remaining += output->second.amount;
               }
 
               for (const auto& dest : dests)
                 remaining -= dest.amount;
 
-              LWSF_TX_ASSERT(fee == remaining);
+              LWSF_TX_VERIFY(fee == remaining);
             }
 
             const auto rct_amount = [] (const backend::transfer_in& source)
@@ -1517,8 +1493,7 @@ namespace lwsf { namespace internal
 
               auto resp = data_->get_decoys(req);
               if (!resp)
-                return tx_error(resp.error());
+                throw std::system_error{resp.error()};
-
               decoys = std::move(*resp);
             }
 
@@ -1555,7 +1530,7 @@ namespace lwsf { namespace internal
                 const std::uint64_t amount = rct_amount(source);
                 const auto ring = std::lower_bound(decoys.begin(), decoys.end(), amount, by_amount{});
                 if (ring == decoys.end() || ring->amount != rpc::uint64_string(amount))
-                  return tx_error(error::tx_decoys);
+                  throw std::runtime_error{"Missing requested decoys"};
 
                 for (const auto& decoy : ring->outputs)
                   entry.outputs.emplace_back(std::uint64_t(decoy.global_index), rct::ctkey{decoy.public_key, decoy.rct});
@@ -1583,11 +1558,11 @@ namespace lwsf { namespace internal
             std::shuffle(sources.begin(), sources.end(), crypto::random_device{});
             for (auto& source : sources)
             {
-              LWSF_TX_ASSERT(!source.outputs.empty());
+              LWSF_TX_VERIFY(!source.outputs.empty());
 
               const std::uint64_t real_source = source.outputs.front().first;
               std::sort(source.outputs.begin(), source.outputs.end(), ring_sort);
-              LWSF_TX_ASSERT(std::unique(source.outputs.begin(), source.outputs.end(), ring_compare) == source.outputs.end());
+              LWSF_TX_VERIFY(std::unique(source.outputs.begin(), source.outputs.end(), ring_compare) == source.outputs.end());
 
               for (std::size_t i = 0; i < source.outputs.size(); ++i)
               {
@@ -1601,7 +1576,7 @@ namespace lwsf { namespace internal
 
             // By copy to catch mutations (want each invoke to be consistent)
             const auto construct_tx = [=, &dests, &sources = std::as_const(sources)] ()
-              -> std::unique_ptr<internal::pending_transaction>
+              -> std::shared_ptr<backend::transaction>
             {
               auto rsources = sources; // already shuffled above
               auto rdests = dests; // in case we need to retry fee
@@ -1614,13 +1589,15 @@ namespace lwsf { namespace internal
               crypto::secret_key tx_key;
               std::vector<crypto::secret_key> tx_keys;
               cryptonote::transaction tx;
+              LWSF_TX_VERIFY(config::min_outputs <= rdests.size());
+              LWSF_TX_VERIFY(rdests.size() <= config::max_outputs_in_construction);
               if (!cryptonote::construct_tx_and_get_tx_key(keys, subs, rsources, rdests, change_account, extra, tx, tx_key, tx_keys, true /* rct */, config, true /* view_tags */))
-                return tx_error(error::tx_failed);
+                throw std::runtime_error{"cryptonote::construct_tx_and_get_tx_key failed"};
 
               auto details = std::make_shared<backend::transaction>();
               details->raw_bytes = epee::byte_slice{cryptonote::t_serializable_object_to_blob(tx)};
               details->timestamp = std::chrono::system_clock::now();
-              details->amount = std::accumulate(amount->begin(), amount->end(), std::uint64_t(0));
+              details->amount = transfer_total;
               details->fee = get_tx_fee(tx);
 
               const std::uint64_t weight = get_transaction_weight(tx, details->raw_bytes.size());
@@ -1628,7 +1605,7 @@ namespace lwsf { namespace internal
 
               const auto get_change_index = [&] () -> std::size_t
               {
-                LWSF_TX_ASSERT(!dests.empty());
+                LWSF_TX_VERIFY(!dests.empty());
                 if (subtract_from_dest)
                   return 0;
                 if (has_change && dests.back().original == change_address)
@@ -1663,7 +1640,7 @@ namespace lwsf { namespace internal
               get_transaction_hash(tx, details->id);
               get_transaction_prefix_hash(tx, details->prefix);
 
-              LWSF_TX_ASSERT(rsources.size() == tx.vin.size());
+              LWSF_TX_VERIFY(rsources.size() == tx.vin.size());
               for (std::size_t i = 0; i < rsources.size(); ++i)
               {
                 const auto& source = rsources.at(i);
@@ -1674,16 +1651,16 @@ namespace lwsf { namespace internal
                 const auto elem = std::find_if(spending.begin(), spending.end(), [&] (const auto& e) {
                   return e.first == spend->second.output_pub;
                 });
-                LWSF_TX_ASSERT(elem != spending.end());
+                LWSF_TX_VERIFY(elem != spending.end());
 
                 const auto& base = elem->second->receives.at(spend->second.output_pub);
                 spend->second.sender = base.recipient;
                 spend->second.tx_pub = base.tx_pub;
               }
 
-              LWSF_TX_ASSERT(rdests.size() == tx.vout.size());
+              LWSF_TX_VERIFY(rdests.size() == tx.vout.size());
-              LWSF_TX_ASSERT(rdests.size() == tx.rct_signatures.ecdhInfo.size());
+              LWSF_TX_VERIFY(rdests.size() == tx.rct_signatures.ecdhInfo.size());
-              LWSF_TX_ASSERT(tx_keys.empty() || tx_keys.size() == rdests.size());
+              LWSF_TX_VERIFY(tx_keys.empty() || tx_keys.size() == rdests.size());
 
               struct get_output_pub
               {
@@ -1728,7 +1705,7 @@ namespace lwsf { namespace internal
                   details->transfers.emplace_back(dest.original, dest.amount).secret = tx_keys.at(i);
                   rtransfers.erase(is_transfer);
                 }
-                LWSF_TX_ASSERT(rtransfers.empty());
+                LWSF_TX_VERIFY(rtransfers.empty());
               }
               else
               {
@@ -1736,7 +1713,7 @@ namespace lwsf { namespace internal
                   details->transfers.emplace_back(transfer.second, transfer.first).secret = tx_key;
               }
 
-              return std::make_unique<internal::pending_transaction>(data_, std::move(tx), std::move(details));
+              return details;
             }; // end `construct_tx` lambda
 
             // only one retry should be needed
@@ -1747,28 +1724,198 @@ namespace lwsf { namespace internal
                 return pending;
               // else `dests` should've been modified for retry
             }
-            LWSF_TX_ASSERT(false); // this should be unreachable
+            LWSF_TX_VERIFY(false); // this should be unreachable
           }; // end `gather_sources_and_construct_tx` lamda
 
-          std::uint64_t min_fee = 0;
+          const auto get_unspent_by_amount = [] (const unspent_map& unspent)
-          for (unsigned attempt = 0; attempt < unspent.size(); ++attempt)
           {
-            try { return gather_sources_and_construct_tx(min_fee); }
+            // actually sorted by amount then reverse index (see comment above)
-            catch (const need_more_sources& e)
+            unspent_by_amount_map out;
+            for (const auto& tx : unspent)
+              out.try_emplace(tx.second.first).first->second = {tx.first, tx.second.second};
+            return out;
+          };
+
+          if (subtract_from_dest)
+          {
+            std::uint64_t total_unspent = 0;
+            for (const auto& tx : unspent)
             {
-              min_fee = e.fee();
+              const auto output = tx.second.second->receives.find(tx.first);
+              LWSF_TX_VERIFY(output != tx.second.second->receives.end());
+              total_unspent += output->second.amount;
             }
+            amount = std::vector<std::uint64_t>{total_unspent};
           }
 
-          return tx_error(error::tx_low_funds);
+          std::string extra_nonce{};
+          destinations_vector dests_flat{};
+          transfers_map transfers_flat{};
+
+          for (std::size_t i = 0; i < dst_addr.size(); ++i)
+          {
+            cryptonote::address_parse_info info{};
+            if (!cryptonote::get_account_address_from_str(info, ctype, dst_addr.at(i)))
+              throw std::invalid_argument{"Invalid destination address"};
+
+            if (info.has_payment_id)
+            {
+              if (!extra_nonce.empty())
+                throw std::invalid_argument{"Two payment ids provided"};
+              cryptonote::set_encrypted_payment_id_to_tx_extra_nonce(extra_nonce, info.payment_id);
+            }
+
+            transfers_flat.insert({amount->at(i), dst_addr.at(i)});
+
+            auto& dest = dests_flat.emplace_back();
+            dest.original = dst_addr.at(i);
+            dest.addr = info.address;
+            dest.amount = amount->at(i);
+            dest.is_subaddress = info.is_subaddress;
+            dest.is_integrated = info.has_payment_id;
+          }
+
+          LWSF_TX_VERIFY(bool(amount));
+          LWSF_TX_VERIFY(dst_addr.size() <= amount->size());
+          for (std::size_t i = dst_addr.size(); i < amount->size(); ++i)
+          {
+            // this is a "self-sweep" (consolidation)
+            auto& dest = dests_flat.emplace_back();
+            dest.original = change_address;
+            dest.addr = change_account;
+            dest.amount = amount->at(i);
+            dest.is_subaddress = subaddr_account;
+            dest.is_integrated = false;
+          }
+
+          std::sort(dests_flat.begin(), dests_flat.end(), [] (const auto& lhs, const auto& rhs) {
+            return lhs.amount < rhs.amount;
+          });
+
+          /* This function makes a crude attempt at breaking up destinations
+            into multiple transactions IFF exceeding output maximum. The
+            algorithm is crude, and does a best effort to minimize source/input
+            usage. Unfortunately capture by reference, just much easier. */
+          const auto get_and_update_destinations = [&] ()
+          {
+            // leave a spot for change
+            LWSF_TX_VERIFY(dests_flat.size() == transfers_flat.size());
+            if (dests_flat.size() < config::max_outputs_in_construction)
+            {
+              auto out = std::make_pair(std::move(dests_flat), std::move(transfers_flat));
+              dests_flat.clear();
+              transfers_flat.clear();
+              return out;
+            }
+
+            // split //
+
+            const std::size_t extra_size = extra_nonce.empty() ? 0 : extra_nonce.size() + 2;
+            auto unspent_by_amount = get_unspent_by_amount(unspent);
+            const std::size_t unspent_initial = unspent_by_amount.size();
+
+            std::pair<destinations_vector, transfers_map> out{};
+
+            const auto stop_condition = [&] ()
+            {
+              // leave one spot for change
+              static_assert(1 <= config::max_outputs_in_construction);
+              return dests_flat.empty() || std::get<0>(out).size() == config::max_outputs_in_construction - 1;
+            };
+            const auto update_transfers = [&] (const auto& dest)
+            {
+              auto transfer = transfers_flat.find({dest.amount, dest.original});
+              LWSF_TX_VERIFY(transfer != transfers_flat.end());
+              std::get<1>(out).insert(std::move(*transfer));
+              transfers_flat.erase(transfer);
+            };
+            const auto by_amount = [] (const auto& lhs, const std::uint64_t rhs)
+            {
+              return lhs.amount < rhs;
+            };
+
+            std::uint64_t remaining = 0;
+            while (!stop_condition())
+            {
+              // add largest amount first
+              std::get<0>(out).push_back(std::move(dests_flat.back()));
+              dests_flat.pop_back();
+              update_transfers(std::get<0>(out).back());
+
+              {
+                const std::uint64_t amount = std::get<0>(out).back().amount;
+                const std::size_t spend_size = unspent_initial - unspent_by_amount.size() + 1;
+                const std::uint64_t fee =
+                  estimate_fee(per_byte_fee, spend_size, std::get<0>(out).size() + 1, mixin_count, extra_size, fee_mask);
+                const auto source = unspent_by_amount.lower_bound({amount + fee, 0});
+                if (source == unspent_by_amount.end())
+                  return out; // the "real" tx construction mechanism needs to run
+                remaining += std::get<0>(source->first) - amount;
+                unspent_by_amount.erase(source);
+              }
+
+              const std::size_t spend_size = unspent_initial - unspent_by_amount.size();
+              while (!stop_condition())
+              {
+                const std::uint64_t fee =
+                  estimate_fee(per_byte_fee, spend_size, std::get<0>(out).size() + 1, mixin_count, extra_size, fee_mask);
+                if (remaining <= fee)
+                  break;
+
+                auto dest = std::lower_bound(dests_flat.begin(), dests_flat.end(), remaining - fee, by_amount);
+                if (dest == dests_flat.end())
+                  --dest;
+
+                std::get<0>(out).push_back(std::move(*dest));
+                dests_flat.erase(dest);
+                update_transfers(std::get<0>(out).back());
+
+                remaining -= std::get<0>(out).back().amount;
+              }
+            }
+
+            return out;
+          };
+
+          std::vector<std::shared_ptr<backend::transaction>> pending;
+          while (!dests_flat.empty())
+          {
+            std::uint64_t min_fee = 0;
+            const auto dests = get_and_update_destinations(); // `dests_flat` pruned
+            const auto unspent_by_amount = get_unspent_by_amount(unspent);
+
+            unsigned attempt = 0;
+            for (; attempt < unspent.size(); ++attempt)
+            {
+              try
+              {
+                auto tx = gather_sources_and_construct_tx(unspent_by_amount, std::get<0>(dests), std::get<1>(dests), extra_nonce, min_fee);
+                LWSF_TX_VERIFY(tx != nullptr);
+                for (const auto& spend : tx->spends)
+                  unspent.erase(spend.second.output_pub);
+                pending.push_back(std::move(tx));
+                break; // goto next tx
+              }
+              catch (const need_more_sources& e)
+              {
+                min_fee = std::max(min_fee, e.fee());
+              }
+            }
+
+            if (attempt == unspent.size())
+              break;
+          }
+
+          if (!dests_flat.empty())
+            throw_low_funds();
+          return std::make_unique<internal::pending_transaction>(data_, std::string{}, std::move(pending));
         }(); // end unnamed lambda encapsulating all tx construction
 
-      LWSF_TX_ASSERT(out != nullptr);
+      LWSF_TX_VERIFY(out != nullptr);
     }
-    catch (const std::exception & e)
+    catch (const std::exception& e)
     {
-      set_critical(e);
+      out = std::make_unique<internal::pending_transaction>(data_, e.what());
-      out = tx_error(error::tx_critical);
     }
 
     return out.release();

src/wallet_manager.cpp

@@ -393,11 +393,12 @@ namespace lwsf
     return cryptonote::print_money(amount);
   }
 
-  std::uint64_t amountFromString(const std::string &amount)
+  std::optional<std::uint64_t> amountFromString(const std::string &amount)
   {
     std::uint64_t result = 0;
-    cryptonote::parse_amount(result, amount);
+    if (cryptonote::parse_amount(result, amount))
-    return result;
+      return result;
+    return std::nullopt;
   }
 
   bool addressValid(const std::string &str, Monero::NetworkType nettype)