From 08edc0bd58a650169ce213aa2a71cf494a1a58ee Mon Sep 17 00:00:00 2001 From: Paul Shapiro Date: Tue, 28 Aug 2018 15:57:07 -0400 Subject: [PATCH] modified the interface to create transactions to remove requirement to construct and send dsts with either change or fake dst, by exposing change amount as param and uncommenting kept change addr / dsts construction - which actually solves a tx not created bug when sending to an addr (who is not oneself) with a short pid --- src/monero_transfer_utils.cpp | 76 ++++++++++++++++------------------- src/monero_transfer_utils.hpp | 9 +++-- src/serial_bridge_index.cpp | 18 +-------- test/test_all.cpp | 40 ++++-------------- 4 files changed, 49 insertions(+), 94 deletions(-) diff --git a/src/monero_transfer_utils.cpp b/src/monero_transfer_utils.cpp index 41c1b5e..df02ed6 100644 --- a/src/monero_transfer_utils.cpp +++ b/src/monero_transfer_utils.cpp @@ -304,10 +304,11 @@ void monero_transfer_utils::create_transaction( const account_keys& sender_account_keys, // this will reference a particular hw::device const uint32_t subaddr_account_idx, const std::unordered_map &subaddresses, - const vector &dsts, + uint64_t sending_amount, + uint64_t change_amount, + uint64_t fee_amount, vector &outputs, vector &mix_outs, - uint64_t fee_amount, std::vector &extra, use_fork_rules_fn_type use_fork_rules_fn, uint64_t unlock_time, // or 0 @@ -321,10 +322,6 @@ void monero_transfer_utils::create_transaction( uint32_t fake_outputs_count = fixed_mixinsize(); bool bulletproof = use_fork_rules_fn(get_bulletproof_fork(), 0); // - if (dsts.size() == 0) { - retVals.errCode = noDestinations; - return; - } if (mix_outs.size() != outputs.size() && fake_outputs_count != 0) { retVals.errCode = wrongNumberOfMixOutsProvided; return; @@ -340,14 +337,12 @@ void monero_transfer_utils::create_transaction( retVals.errCode = invalidSecretKeys; return; } - uint64_t needed_money = 0; - for (size_t i = 0; i < dsts.size(); i++) { - needed_money += dsts[i].amount; - if (needed_money > UINT64_MAX) { - retVals.errCode = outputAmountOverflow; - return; - } + uint64_t needed_money = sending_amount + change_amount + fee_amount; // TODO: is this correct? + if (needed_money > UINT64_MAX) { + retVals.errCode = outputAmountOverflow; + return; } + // uint64_t found_money = 0; std::vector sources; // TODO: log: "Selected transfers: " << outputs @@ -481,33 +476,31 @@ void monero_transfer_utils::create_transaction( } // // TODO: if this is a multisig wallet, create a list of multisig signers we can use - std::vector splitted_dsts = dsts; - cryptonote::tx_destination_entry change_dts = AUTO_VAL_INIT(change_dts); - change_dts.amount = found_money - needed_money; + std::vector splitted_dsts; + cryptonote::tx_destination_entry change_dst = AUTO_VAL_INIT(change_dst); + change_dst.amount = change_amount; // - /* This is commented because it's presently supplied by whoever is calling this function.... But there's a good argument for bringing it in, here, especially after MyMonero clients integrate with this code and soon, share an implementation of SendFunds() (the analog of create_transactions_2 + transfer_selected*) - if (change_dts.amount == 0) { - if (splitted_dsts.size() == 1) { - // If the change is 0, send it to a random address, to avoid confusing - // the sender with a 0 amount output. We send a 0 amount in order to avoid - // letting the destination be able to work out which of the inputs is the - // real one in our rings - LOG_PRINT_L2("generating dummy address for 0 change"); - cryptonote::account_base dummy; - dummy.generate(); - change_dts.addr = dummy.get_keys().m_account_address; - LOG_PRINT_L2("generated dummy address for 0 change"); - splitted_dsts.push_back(change_dts); - } - } else { - change_dts.addr = sender_account_keys.m_account_address; - splitted_dsts.push_back(change_dts); - } - */ + if (change_dst.amount == 0) { + if (splitted_dsts.size() == 1) { + // If the change is 0, send it to a random address, to avoid confusing + // the sender with a 0 amount output. We send a 0 amount in order to avoid + // letting the destination be able to work out which of the inputs is the + // real one in our rings + LOG_PRINT_L2("generating dummy address for 0 change"); + cryptonote::account_base dummy; + dummy.generate(); + change_dst.addr = dummy.get_keys().m_account_address; + LOG_PRINT_L2("generated dummy address for 0 change"); + splitted_dsts.push_back(change_dst); + } + } else { + change_dst.addr = sender_account_keys.m_account_address; + splitted_dsts.push_back(change_dst); + } // // TODO: log: "sources: " << sources if (found_money > needed_money) { - if (change_dts.amount != fee_amount) { + if (change_dst.amount != fee_amount) { retVals.errCode = resultFeeNotEqualToGiven; // aka "early fee calculation != later" return; // early } @@ -523,7 +516,7 @@ void monero_transfer_utils::create_transaction( std::vector additional_tx_keys; bool r = cryptonote::construct_tx_and_get_tx_key( sender_account_keys, subaddresses, - sources, splitted_dsts, change_dts.addr, extra, + sources, splitted_dsts, change_dst.addr, extra, tx, unlock_time, tx_key, additional_tx_keys, true, bulletproof, /*m_multisig ? &msout : */NULL @@ -555,9 +548,9 @@ void monero_transfer_utils::convenience__create_transaction( const string &sec_spendKey_string, const string &to_address_string, optional payment_id_string, - uint64_t amount, // to send + uint64_t sending_amount, + uint64_t change_amount, uint64_t fee_amount, - const std::vector &dsts, // this must include change or dummy address vector &outputs, vector &mix_outs, use_fork_rules_fn_type use_fork_rules_fn, @@ -641,10 +634,11 @@ void monero_transfer_utils::convenience__create_transaction( account_keys, subaddr_account_idx, subaddresses, - dsts, + sending_amount, + change_amount, + fee_amount, outputs, mix_outs, - fee_amount, extra, use_fork_rules_fn, unlock_time, diff --git a/src/monero_transfer_utils.hpp b/src/monero_transfer_utils.hpp index 1c9fd8a..6ca2032 100644 --- a/src/monero_transfer_utils.hpp +++ b/src/monero_transfer_utils.hpp @@ -185,10 +185,11 @@ namespace monero_transfer_utils const account_keys& sender_account_keys, // this will reference a particular hw::device const uint32_t subaddr_account_idx, // pass 0 for no subaddrs const std::unordered_map &subaddresses, - const vector &dsts, // presently, this must include change as well as, if necessary, dummy output + uint64_t sending_amount, + uint64_t change_amount, + uint64_t fee_amount, vector &outputs, vector &mix_outs, - uint64_t fee_amount, std::vector &extra, // this is not declared const b/c it may have the output tx pub key appended to it use_fork_rules_fn_type use_fork_rules_fn, uint64_t unlock_time = 0, // or 0 @@ -211,9 +212,9 @@ namespace monero_transfer_utils const string &sec_spendKey_string, const string &to_address_string, optional payment_id_string, - uint64_t amount, // to send + uint64_t sending_amount, + uint64_t change_amount, uint64_t fee_amount, - const std::vector &dsts, // this must include change or else dummy address vector &outputs, vector &mix_outs, use_fork_rules_fn_type use_fork_rules_fn, diff --git a/src/serial_bridge_index.cpp b/src/serial_bridge_index.cpp index 86d21fc..3ccdcc4 100644 --- a/src/serial_bridge_index.cpp +++ b/src/serial_bridge_index.cpp @@ -474,20 +474,6 @@ string serial_bridge::create_transaction(const string &args_string) } network_type nettype = nettype_from_string(json_root.get("nettype_string")); // - std::vector dsts; - BOOST_FOREACH(boost::property_tree::ptree::value_type &dst_desc, json_root.get_child("dsts")) - { - assert(dst_desc.first.empty()); // array elements have no names - cryptonote::tx_destination_entry de; - cryptonote::address_parse_info de_addr_info; - THROW_WALLET_EXCEPTION_IF(!cryptonote::get_account_address_from_str(de_addr_info, nettype, dst_desc.second.get("addr")), error::wallet_internal_error, "Invalid dsts.addr"); - de.addr = de_addr_info.address; - de.is_subaddress = de_addr_info.is_subaddress; - de.amount = stoull(dst_desc.second.get("amount")); - // - dsts.push_back(de); - } - // vector outputs; BOOST_FOREACH(boost::property_tree::ptree::value_type &output_desc, json_root.get_child("outputs")) { @@ -529,9 +515,9 @@ string serial_bridge::create_transaction(const string &args_string) json_root.get("sec_spendKey_string"), json_root.get("to_address_string"), json_root.get_optional("payment_id_string"), - stoull(json_root.get("amount")), // to send + stoull(json_root.get("sending_amount")), + stoull(json_root.get("change_amount")), stoull(json_root.get("fee_amount")), - dsts, outputs, mix_outs, [] (uint8_t version, int64_t early_blocks) -> bool diff --git a/test/test_all.cpp b/test/test_all.cpp index 94c8f87..b4f62d9 100644 --- a/test/test_all.cpp +++ b/test/test_all.cpp @@ -143,7 +143,7 @@ BOOST_AUTO_TEST_CASE(transfers__create) cout << "transfers__create: spend pub key: " << string_tools::pod_to_hex(from_addr_info.address.m_spend_public_key) << endl; // optional payment_id_string = string("b79f8efc81f58f67"); - uint64_t amount = 10000000000; + uint64_t sending_amount = 10000000000; uint64_t fee_amount = 2167750000; string to_address_string("43zxvpcj5Xv9SEkNXbMCG7LPQStHMpFCQCmkmR4u5nzjWwq5Xkv5VmGgYEsHXg4ja2FGRD5wMWbBVMijDTqmmVqm93wHGkg"); cryptonote::address_parse_info to_addr_info; @@ -173,12 +173,7 @@ BOOST_AUTO_TEST_CASE(transfers__create) } } // - std::vector dsts; // without change this would normally require a dummy addr with 0 amount pushed as a fake 'change' output ... that should probably be moved into the function - { // 0. actual destination address - cryptonote::tx_destination_entry de; - de.addr = to_addr_info.address; - de.amount = amount; - de.is_subaddress = to_addr_info.is_subaddress; + { // payment id if (to_addr_info.is_subaddress && payment_id_seen) { BOOST_REQUIRE_MESSAGE(false, "Illegal: Never supply a pid with a subaddress."); // TODO: is this true? return; @@ -201,14 +196,6 @@ BOOST_AUTO_TEST_CASE(transfers__create) } payment_id_seen = true; } - dsts.push_back(de); - } - { // 1. change - cryptonote::tx_destination_entry de; - de.addr = from_addr_info.address; - de.amount = 112832250000; - de.is_subaddress = from_addr_info.is_subaddress; // not - dsts.push_back(de); } // vector outputs; @@ -289,10 +276,11 @@ BOOST_AUTO_TEST_CASE(transfers__create) account_keys, subaddr_account_idx, subaddresses, - dsts, + sending_amount, + 112832250000, // change amount + fee_amount, outputs, mix_outs, - fee_amount, extra, use_fork_rules_fn, 0, // unlock_time @@ -347,24 +335,10 @@ BOOST_AUTO_TEST_CASE(bridged__transfers__create) root.put("sec_spendKey_string", "4e6d43cd03812b803c6f3206689f5fcc910005fc7e91d50d79b0776dbefcd803"); root.put("to_address_string", to_address_string); root.put("payment_id_string", "b79f8efc81f58f67"); - root.put("amount", amount_string); + root.put("sending_amount", amount_string); + root.put("change_amount", "112832250000"); root.put("fee_amount", "2167750000"); // - boost::property_tree::ptree dsts; - { // 0. actual destination address - boost::property_tree::ptree dst; - dst.put("addr", to_address_string); - dst.put("amount", amount_string); - dsts.push_back(std::make_pair("", dst)); - } - { // 1. change (otherwise we'd have to supply a dummy addr) - boost::property_tree::ptree dst; - dst.put("addr", from_address_string); - dst.put("amount", "112832250000"); - dsts.push_back(std::make_pair("", dst)); - } - root.add_child("dsts", dsts); - // boost::property_tree::ptree outputs; { boost::property_tree::ptree out;