fix finding wrong change bug (#23)

src/carrot_impl/account.cpp

@@ -120,6 +120,10 @@ std::unordered_map<crypto::public_key, cryptonote::subaddress_index> carrot_and_
     return res;
 }
 //----------------------------------------------------------------------------------------------------------------------
+std::unordered_map<crypto::public_key, subaddress_index_extended>& carrot_and_legacy_account::get_subaddress_map_ref() {
+    return subaddress_map;
+}
+//----------------------------------------------------------------------------------------------------------------------
 void carrot_and_legacy_account::opening_for_subaddress(const subaddress_index_extended &subaddress_index,
     crypto::secret_key &address_privkey_g_out,
     crypto::secret_key &address_privkey_t_out,
@@ -283,7 +287,7 @@ void carrot_and_legacy_account::generate_subaddress_map()
         {
             for (const AddressDeriveType derive_type : derive_types)
             {
-                const subaddress_index_extended subaddr_index{{major_index, minor_index}, derive_type};
+                const subaddress_index_extended subaddr_index{{major_index, minor_index}, derive_type, false};
                 const CarrotDestinationV1 addr = subaddress(subaddr_index);
                 subaddress_map.insert({addr.address_spend_pubkey, subaddr_index});
             }
@@ -339,7 +343,7 @@ void carrot_and_legacy_account::set_carrot_keys(const AddressDeriveType default_
 void carrot_and_legacy_account::insert_subaddresses(const std::unordered_map<crypto::public_key, subaddress_index_extended>& subaddress_map_cn)
 {
     for (const auto &p : subaddress_map_cn)
-        subaddress_map.insert({p.first, {{p.second.index.major, p.second.index.minor}, p.second.derive_type}});
+        subaddress_map.insert({p.first, {{p.second.index.major, p.second.index.minor}, p.second.derive_type, p.second.is_return_spend_key}});
 }
 
 //----------------------------------------------------------------------------------------------------------------------

src/carrot_impl/account.h

@@ -50,7 +50,6 @@ namespace carrot
     view_balance_secret_ram_borrowed_device s_view_balance_dev;
     generate_address_secret_ram_borrowed_device s_generate_address_dev;
 
-    std::unordered_map<crypto::public_key, subaddress_index_extended> subaddress_map;
     AddressDeriveType default_derive_type;
 
     carrot_and_legacy_account(): k_view_incoming_dev(get_keys().k_view_incoming),
@@ -70,6 +69,7 @@ namespace carrot
     CarrotDestinationV1 subaddress(const subaddress_index_extended &subaddress_index) const;
 
     std::unordered_map<crypto::public_key, cryptonote::subaddress_index> get_subaddress_map_cn() const;
+    std::unordered_map<crypto::public_key, subaddress_index_extended>& get_subaddress_map_ref();
     
     // brief: opening_for_subaddress - return (k^g_a, k^t_a) for j s.t. K^j_s = (k^g_a * G + k^t_a * T)
     void opening_for_subaddress(const subaddress_index_extended &subaddress_index,
@@ -110,5 +110,8 @@ namespace carrot
     void insert_subaddresses(const std::unordered_map<crypto::public_key, subaddress_index_extended>& subaddress_map);
     
     AddressDeriveType resolve_derive_type(const AddressDeriveType derive_type) const;
+
+    private:
+        std::unordered_map<crypto::public_key, subaddress_index_extended> subaddress_map;
   };
 }

src/carrot_impl/subaddress_index.h

@@ -77,10 +77,11 @@ struct subaddress_index_extended
 {
     subaddress_index index;
     AddressDeriveType derive_type;
+    bool is_return_spend_key;
 };
 static inline bool operator==(const subaddress_index_extended &a, const subaddress_index_extended &b)
 {
-    return a.index == b.index && a.derive_type == b.derive_type;
+    return a.index == b.index && a.derive_type == b.derive_type && a.is_return_spend_key == b.is_return_spend_key;
 }
 static inline bool operator!=(const subaddress_index_extended &a, const subaddress_index_extended &b)
 {

src/wallet/scanning_tools.cpp

@@ -241,7 +241,7 @@ static std::optional<enote_view_incoming_scan_info_t> view_incoming_scan_pre_car
     // HERE BE DRAGONS!!!
     // SRCG: whilst the following code will work, it'd be better being moved to the TX_BUILDER code
     // add the entry to our subaddress map in case it's a change payment (false positives won't hurt us)
-    account.subaddress_map.insert({enote.onetime_address, subaddr_index});
+    account.get_subaddress_map_ref().insert({enote.onetime_address, subaddr_index});
     // LAND AHOY!!!
     
     return enote_view_incoming_scan_info_t{
@@ -360,8 +360,8 @@ static std::optional<enote_view_incoming_scan_info_t> view_incoming_scan_carrot_
         dummy_enote_type))
     return std::nullopt;
 
-    const auto subaddr_it = account.subaddress_map.find(res.address_spend_pubkey);
+    const auto subaddr_it = account.get_subaddress_map_ref().find(res.address_spend_pubkey);
-    CHECK_AND_ASSERT_MES(subaddr_it != account.subaddress_map.cend(),
+    CHECK_AND_ASSERT_MES(subaddr_it != account.get_subaddress_map_ref().cend(),
         std::nullopt,
         "view_incoming_scan_carrot_enote: carrot enote scanned successfully, "
         "but the recovered address spend pubkey was not found in the subaddress map");
@@ -826,7 +826,7 @@ std::vector<std::optional<enote_view_incoming_scan_info_t>> view_incoming_scan_t
     // 3. do view-incoming scan for each output enotes
     hw::device &hwdev = hw::get_device("default");
     carrot::carrot_and_legacy_account dummy_account;
-    dummy_account.subaddress_map[address.m_spend_public_key] = {{}};
+    dummy_account.get_subaddress_map_ref()[address.m_spend_public_key] = {{}};
     for (size_t local_output_index = 0; local_output_index < n_outputs; ++local_output_index)
     {
         auto &enote_scan_info = res[local_output_index];

src/wallet/tx_builder.cpp

@@ -120,7 +120,7 @@ static bool build_payment_proposals(std::vector<carrot::CarrotPaymentProposalV1>
                 .amount = tx_dest_entry.amount,
                 .enote_type = carrot::CarrotEnoteType::PAYMENT
             },
-            .subaddr_index = {subaddr_index, carrot::AddressDeriveType::PreCarrot} // @TODO: handle carrot
+            .subaddr_index = {subaddr_index, carrot::AddressDeriveType::Carrot, false},
         });
     }
     else // not *known* self-send address
@@ -180,17 +180,13 @@ static crypto::public_key find_change_address_spend_pubkey(
     CHECK_AND_ASSERT_THROW_MES(change_it != subaddress_map.cend(),
         "find_change_address_spend_pubkey: missing change address (index "
         << subaddr_account << ",0) in subaddress map");
-    // HERE BE DRAGONS!!!
+
-    // SRCG: Disabling the following check is necessary to allow return_payments to work...
-    //       ...but if we can find an alternative to using the subaddress_map, we should! 
-    /*
     const auto change_it_2 = std::find_if(std::next(change_it), subaddress_map.cend(),
         [subaddr_account](const auto &p) { return p.second.major == subaddr_account && p.second.minor == 0; });
     CHECK_AND_ASSERT_THROW_MES(change_it_2 == subaddress_map.cend(),
         "find_change_address_spend_pubkey: provided subaddress map is malformed!!! At least two spend pubkeys map to "
         "index " << subaddr_account << ",0 in the subaddress map!");
-    */
+
-    // LAND AHOY!!!
     return change_it->first;
 }
 //-------------------------------------------------------------------------------------------------------------------
@@ -200,21 +196,27 @@ static crypto::public_key find_change_address_spend_pubkey(
     const std::uint32_t subaddr_account)
 {
     const auto change_it = std::find_if(subaddress_map.cbegin(), subaddress_map.cend(),
-        [subaddr_account](const auto &p) { return p.second.index.major == subaddr_account && p.second.index.minor == 0; });
+        [subaddr_account](const auto &p) { 
+            return  p.second.index.major == subaddr_account &&
+                    p.second.index.minor == 0 &&
+                    p.second.derive_type == carrot::AddressDeriveType::Carrot &&
+                    p.second.is_return_spend_key == false;
+        });
     CHECK_AND_ASSERT_THROW_MES(change_it != subaddress_map.cend(),
         "find_change_address_spend_pubkey: missing change address (index "
         << subaddr_account << ",0) in subaddress map");
-    // HERE BE DRAGONS!!!
+    
-    // SRCG: Disabling the following check is necessary to allow return_payments to work...
-    //       ...but if we can find an alternative to using the subaddress_map, we should! 
-    /*
     const auto change_it_2 = std::find_if(std::next(change_it), subaddress_map.cend(),
-        [subaddr_account](const auto &p) { return p.second.major == subaddr_account && p.second.minor == 0; });
+        [subaddr_account](const auto &p) { 
+            return  p.second.index.major == subaddr_account &&
+                    p.second.index.minor == 0 &&
+                    p.second.derive_type == carrot::AddressDeriveType::Carrot && 
+                    p.second.is_return_spend_key == false;
+        });
     CHECK_AND_ASSERT_THROW_MES(change_it_2 == subaddress_map.cend(),
         "find_change_address_spend_pubkey: provided subaddress map is malformed!!! At least two spend pubkeys map to "
         "index " << subaddr_account << ",0 in the subaddress map!");
-    */
+
-    // LAND AHOY!!!
     return change_it->first;
 }
 //-------------------------------------------------------------------------------------------------------------------
@@ -445,13 +447,12 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
 {
     wallet2::transfer_container unused_transfers;
     w.get_transfers(unused_transfers);
-    const auto subaddress_map = w.get_subaddress_map_ref();
 
     std::vector<carrot::CarrotTransactionProposalV1> tx_proposals;
     tx_proposals.reserve(dsts.size() / (carrot::CARROT_MAX_TX_OUTPUTS - 1) + 1);
 
     const crypto::public_key change_address_spend_pubkey
-      = find_change_address_spend_pubkey(w.get_account().subaddress_map, subaddr_account);
+      = find_change_address_spend_pubkey(w.get_account().get_subaddress_map_ref(), subaddr_account);
 
     while (!dsts.empty())
     {
@@ -468,7 +469,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
             const bool is_selfsend = build_payment_proposals(normal_payment_proposals,
                 selfsend_payment_proposals,
                 dst,
-                subaddress_map);
+                w.get_account().get_subaddress_map_cn());
             if (subtract_fee_from_outputs.count(dsts.size() - 1))
             {
                 if (is_selfsend)
@@ -503,7 +504,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
             tx_type,
             std::move(select_inputs),
             change_address_spend_pubkey,
-            {{subaddr_account, 0}, carrot::AddressDeriveType::PreCarrot}, //! @TODO: handle Carrot keys
+            {{subaddr_account, 0}, carrot::AddressDeriveType::Carrot, false},
             subtractable_normal_payment_proposals,
             subtractable_selfsend_payment_proposals,
             tx_proposal);
@@ -615,7 +616,7 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     }
 
     const crypto::public_key change_address_spend_pubkey
-      = find_change_address_spend_pubkey(w.get_account().subaddress_map, subaddr_account);
+      = find_change_address_spend_pubkey(w.get_account().get_subaddress_map_ref(), subaddr_account);
 
     // get 1 payment proposal corresponding to (address, is_subaddres)
     std::vector<carrot::CarrotPaymentProposalV1> normal_payment_proposals;

src/wallet/wallet2.cpp

@@ -2582,7 +2582,8 @@ void wallet2::process_new_scanned_transaction(
     const cryptonote::subaddress_index subaddr_index_cn{
         enote_scan_info->subaddr_index->index.major, enote_scan_info->subaddr_index->index.minor};
 
-    // Only count >0 amount outs
+    // we have to save even the 0 amount enotes since we need to save their one time addresses
+    // for some protocol txs.
     //if (enote_scan_info->amount > 0)
     {
       tx_money_got_in_outs[subaddr_index_cn][enote_scan_info->asset_type] += extra_received_money;
@@ -2847,7 +2848,7 @@ void wallet2::process_new_scanned_transaction(
         } else {
           derive_type = carrot::AddressDeriveType::PreCarrot;
         }
-        const carrot::subaddress_index_extended subaddr_ext = {i->first.major, i->first.minor, derive_type};
+        const carrot::subaddress_index_extended subaddr_ext = {i->first.major, i->first.minor, derive_type, true};
         m_account.insert_subaddresses({{onetime_address, subaddr_ext}});
         // save to m_subaddresses as well, so that we can populate account subaddress map
         // when we open the wallet first time.
@@ -6587,7 +6588,8 @@ void wallet2::load(const std::string& wallet_, const epee::wipeable_string& pass
     // if we have subaddresses, we need to insert them into the account
     for (const auto &subaddress : m_subaddresses)
       m_account.insert_subaddresses(
-        {{subaddress.first, {{subaddress.second.major, subaddress.second.minor}, carrot::AddressDeriveType::PreCarrot}}}
+        // TODO: we assume none of these subaddresses are return tx subaddresses
+        {{subaddress.first, {{subaddress.second.major, subaddress.second.minor}, carrot::AddressDeriveType::PreCarrot, false}}}
       );
   }