wallet: add pending_tx sanity checker

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

src/cryptonote_core/cryptonote_tx_utils.cpp

@@ -51,41 +51,30 @@ using namespace epee;
 using namespace crypto;
 
 
-namespace
-{
-//---------------------------------------------------------------
-/**
- * @brief check if can re-derive change address from device / keys
- * @param change_addr address to attempt to re-derive
- * @param subaddresses subaddress map
- * @param keys account keys of sender
- * @return subaddress index of `change_addr` if in the subaddress map and re-derives from device, otherwise nullopt
- */
-std::optional<cryptonote::subaddress_index> sanity_check_change_address(
-  const cryptonote::account_public_address& change_addr,
-  const std::unordered_map<crypto::public_key, cryptonote::subaddress_index>& subaddresses,
-  const cryptonote::account_keys &keys
-)
-{
-  // guess/find subaddress index of `change_addr`, works for main addresses if `subaddresses` is empty
-  cryptonote::subaddress_index subaddr_index{}; // (0, 0) by default
-  const auto subaddr_it = subaddresses.find(change_addr.m_spend_public_key);
-  if (subaddr_it != subaddresses.cend())
-    subaddr_index = subaddr_it->second;
-
-  // if device does not return same address given index, then fail
-  hw::device &hwdev = keys.get_device();
-  const auto recomputed_addr = hwdev.get_subaddress(keys, subaddr_index);
-  if (change_addr != recomputed_addr)
-    return std::nullopt;
-
-  return {subaddr_index};
-}
-//---------------------------------------------------------------
-} //anonymous namespace
 
 namespace cryptonote
 {
+  //---------------------------------------------------------------
+  std::optional<cryptonote::subaddress_index> sanity_check_change_address(
+    const cryptonote::account_public_address& change_addr,
+    const std::unordered_map<crypto::public_key, cryptonote::subaddress_index>& subaddresses,
+    const cryptonote::account_keys &keys
+  )
+  {
+    // guess/find subaddress index of `change_addr`, works for main addresses if `subaddresses` is empty
+    cryptonote::subaddress_index subaddr_index{}; // (0, 0) by default
+    const auto subaddr_it = subaddresses.find(change_addr.m_spend_public_key);
+    if (subaddr_it != subaddresses.cend())
+      subaddr_index = subaddr_it->second;
+
+    // if device does not return same address given index, then fail
+    hw::device &hwdev = keys.get_device();
+    const auto recomputed_addr = hwdev.get_subaddress(keys, subaddr_index);
+    if (change_addr != recomputed_addr)
+      return std::nullopt;
+
+    return {subaddr_index};
+  }
   //---------------------------------------------------------------
   void classify_addresses(const std::vector<tx_destination_entry> &destinations, const boost::optional<cryptonote::account_public_address>& change_addr, size_t &num_stdaddresses, size_t &num_subaddresses, account_public_address &single_dest_subaddress)
   {

src/cryptonote_core/cryptonote_tx_utils.h

@@ -107,6 +107,15 @@ namespace cryptonote
     END_SERIALIZE()
   };
 
+  inline bool operator==(const tx_destination_entry &a, const tx_destination_entry &b)
+  {
+    return a.original == b.original
+      && a.amount == b.amount
+      && a.addr == b.addr
+      && a.is_subaddress == b.is_subaddress
+      && a.is_integrated == b.is_integrated;
+  }
+
   //---------------------------------------------------------------
 
   struct tx_block_template_backlog_entry
@@ -118,6 +127,19 @@ namespace cryptonote
   WIRE_DECLARE_OBJECT(tx_block_template_backlog_entry);
 
   //---------------------------------------------------------------
+  /**
+   * @brief check if can re-derive change address from device / keys
+   * @param change_addr address to attempt to re-derive
+   * @param subaddresses subaddress map
+   * @param keys account keys of sender
+   * @return subaddress index of `change_addr` if in the subaddress map and re-derives from device, otherwise nullopt
+   */
+  std::optional<cryptonote::subaddress_index> sanity_check_change_address(
+    const cryptonote::account_public_address& change_addr,
+    const std::unordered_map<crypto::public_key, cryptonote::subaddress_index>& subaddresses,
+    const cryptonote::account_keys &keys
+  );
+  void classify_addresses(const std::vector<tx_destination_entry> &destinations, const boost::optional<cryptonote::account_public_address>& change_addr, size_t &num_stdaddresses, size_t &num_subaddresses, account_public_address &single_dest_subaddress);
   crypto::public_key get_destination_view_key_pub(const std::vector<tx_destination_entry> &destinations, const boost::optional<cryptonote::account_public_address>& change_addr);
   bool construct_tx(const account_keys& sender_account_keys, std::vector<tx_source_entry> &sources, const std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx);
   bool construct_tx_with_tx_key(const account_keys& sender_account_keys, const std::unordered_map<crypto::public_key, subaddress_index>& subaddresses, std::vector<tx_source_entry>& sources, std::vector<tx_destination_entry>& destinations, const boost::optional<cryptonote::account_public_address>& change_addr, const std::vector<uint8_t> &extra, transaction& tx, const crypto::secret_key &tx_key, const std::vector<crypto::secret_key> &additional_tx_keys, bool rct = false, const rct::RCTConfig &rct_config = { rct::RangeProofBorromean, 0 }, bool shuffle_outs = true, bool use_view_tags = false);

src/wallet/CMakeLists.txt

@@ -33,6 +33,7 @@ set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
 set(wallet_sources
   wallet2.cpp
   wallet_args.cpp
+  pending_tx_validation.cpp
   ringdb.cpp
   node_rpc_proxy.cpp
   message_store.cpp

src/wallet/api/wallet.cpp

@@ -2515,6 +2515,9 @@ void WalletImpl::pendingTxPostProcess(PendingTransactionImpl * pending)
   tools::wallet2::signed_tx_set exported_txs;
   std::vector<cryptonote::address_parse_info> dsts_info;
 
+  // NOTE: We expect `cold_sign_tx` to validate `pending->m_pending_tx` with `sanity_check_pending_tx`.
+  // It is not possible to pre-validate here because the pending tx may be 'half-formed' at this point (e.g.
+  // trezor makes a tx proposal with no key images and the cold wallet has to supply those).
   m_wallet->cold_sign_tx(pending->m_pending_tx, exported_txs, dsts_info, pending->m_tx_device_aux);
   pending->m_key_images = exported_txs.key_images;
   pending->m_pending_tx = exported_txs.ptx;

src/wallet/pending_tx_validation.cpp

@@ -0,0 +1,770 @@
+// Copyright (c) 2025-2026, The Monero Project
+//
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without modification, are
+// permitted provided that the following conditions are met:
+//
+// 1. Redistributions of source code must retain the above copyright notice, this list of
+//    conditions and the following disclaimer.
+//
+// 2. Redistributions in binary form must reproduce the above copyright notice, this list
+//    of conditions and the following disclaimer in the documentation and/or other
+//    materials provided with the distribution.
+//
+// 3. Neither the name of the copyright holder nor the names of its contributors may be
+//    used to endorse or promote products derived from this software without specific
+//    prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY
+// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+//paired header
+#include "pending_tx_validation.h"
+
+//local headers
+#include "misc_log_ex.h"
+#include "common/apply_permutation.h"
+#include "crypto/crypto.h"
+#include "crypto/generators.h"
+#include "cryptonote_basic/account.h"
+#include "cryptonote_basic/cryptonote_basic_impl.h"
+#include "cryptonote_basic/cryptonote_format_utils.h"
+#include "cryptonote_basic/subaddress_index.h"
+#include "cryptonote_core/cryptonote_tx_utils.h"
+#include "cryptonote_config.h"
+#include "device/device_default.hpp"
+#include "ringct/rctOps.h"
+#include "ringct/rctTypes.h"
+#include "string_tools.h"
+#include "wallet2.h"
+#include "wallet2_basic/wallet2_types.h"
+
+//third party headers
+#include <boost/algorithm/string/join.hpp>
+#include <boost/multiprecision/cpp_int.hpp>
+
+//standard headers
+#include <algorithm>
+#include <cstdint>
+#include <string>
+#include <vector>
+
+#undef MONERO_DEFAULT_LOG_CATEGORY
+#define MONERO_DEFAULT_LOG_CATEGORY "wallet.tx_builder"
+
+namespace tools
+{
+namespace wallet
+{
+//-------------------------------------------------------------------------------------------------------------------
+static void validate_tx_outs(
+    const wallet2::pending_tx &ptx,
+    const std::vector<cryptonote::txout_to_tagged_key> &ext_outputs,
+    const crypto::secret_key &k_view,
+    const cryptonote::tx_destination_entry &change,
+    const std::vector<cryptonote::tx_destination_entry> &dests,
+    const crypto::secret_key &tx_key,
+    const std::vector<crypto::secret_key> &additional_tx_keys,
+    crypto::public_key &tx_pubkey_out,
+    std::vector<crypto::public_key> &tx_additional_pubkeys_out)
+{
+    tx_pubkey_out = crypto::public_key{};
+    tx_additional_pubkeys_out.clear();
+    CHECK_AND_ASSERT_THROW_MES(ptx.tx.vout.size() == dests.size(),
+        "validate_tx_outs: dests size mismatch");
+    CHECK_AND_ASSERT_THROW_MES(ext_outputs.size() == dests.size(),
+        "validate_tx_outs: ext_outputs size mismatch");
+    CHECK_AND_ASSERT_THROW_MES(ptx.tx.version == 1 || ptx.tx.rct_signatures.ecdhInfo.size() == dests.size(),
+        "validate_tx_outs: ecdhInfo size mismatch");
+    CHECK_AND_ASSERT_THROW_MES(ptx.tx.version == 1 || ptx.tx.rct_signatures.outPk.size() == dests.size(),
+        "validate_tx_outs: outPk size mismatch");
+
+    // Check if multiple keys are expected
+    // Note: this is the same method used in `construct_tx_and_get_tx_key()`, so we should expect the same results.
+    size_t num_stdaddresses = 0;
+    size_t num_subaddresses = 0;
+    cryptonote::account_public_address single_dest_subaddress;
+    cryptonote::classify_addresses(dests, change.addr, num_stdaddresses, num_subaddresses, single_dest_subaddress);
+    const bool need_additional_txkeys = num_subaddresses > 0 && (num_stdaddresses > 0 || num_subaddresses > 1);
+
+    const size_t expect_num_additional = need_additional_txkeys ? dests.size() : 0;
+    CHECK_AND_ASSERT_THROW_MES(additional_tx_keys.size() == expect_num_additional,
+        "validate_tx_outs: additional tx keys invalid size");
+
+    // if this is a single-destination transfer to a subaddress, we set the tx pubkey to R=s*D
+    if (num_stdaddresses == 0 && num_subaddresses == 1)
+    {
+        tx_pubkey_out = rct::rct2pk(rct::scalarmultKey(
+                rct::pk2rct(single_dest_subaddress.m_spend_public_key),
+                rct::sk2rct(tx_key)
+            ));
+    }
+    else
+    {
+        tx_pubkey_out = rct::rct2pk(rct::scalarmultBase(rct::sk2rct(tx_key)));
+    }
+
+    // It's assumed here generate_output_ephemeral_keys only needs the private view key
+    cryptonote::account_keys sender_keys;
+    sender_keys.m_view_secret_key = k_view;
+
+    std::vector<rct::key> amount_keys;
+    for (size_t i = 0; i < dests.size(); ++i)
+    {
+        const auto &dest = dests.at(i);
+
+        crypto::public_key repro_onetime_addr{};
+        crypto::view_tag repro_view_tag{};
+        const bool r = hw::core::device_default().generate_output_ephemeral_keys(
+                ptx.tx.version,
+                sender_keys,
+                tx_pubkey_out,
+                tx_key,
+                dest,
+                change.addr,
+                i,
+                need_additional_txkeys,
+                additional_tx_keys,
+                tx_additional_pubkeys_out,
+                amount_keys,
+                repro_onetime_addr,
+                ptx.construction_data.use_view_tags,
+                repro_view_tag
+            );
+        CHECK_AND_ASSERT_THROW_MES(r, "validate_tx_outs: failed to generate output ephemeral key");
+
+        // - Ko
+        CHECK_AND_ASSERT_THROW_MES(repro_onetime_addr == ext_outputs.at(i).key,
+            "validate_tx_outs: failed reproducing onetime address");
+
+        // - view tag
+        CHECK_AND_ASSERT_THROW_MES(repro_view_tag == ext_outputs.at(i).view_tag,
+            "validate_tx_outs: failed reproducing view tag");
+
+        if (ptx.tx.version == 1)
+        {
+            // - cleartext amount
+            CHECK_AND_ASSERT_THROW_MES(ptx.tx.vout.at(i).amount == dest.amount,
+                "validate_tx_outs: v1 tx output amount doesn't match destination amount");
+
+            continue;
+        }
+
+        // - encoded amount
+        rct::ecdhTuple amnt_data{};
+        memcpy(amnt_data.amount.bytes, &dest.amount, sizeof(dest.amount));
+        rct::ecdhDecode(amnt_data, amount_keys.at(i), true); //v2, decode gives mask
+        CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.ecdhInfo.at(i).amount == amnt_data.amount,
+            "validate_tx_outs: failed reproducing encoded amount");
+
+        // - C
+        const rct::key repro_C = rct::commit(dest.amount, amnt_data.mask);
+        CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.outPk.at(i).mask == repro_C,
+            "validate_tx_outs: failed reproducing amount commitment");
+    }
+}
+//-------------------------------------------------------------------------------------------------------------------
+static void reconstruct_payment_id(const std::string &extracted_payment_id,
+    const std::optional<crypto::hash8> &parsed_to_check,
+    // const std::optional<crypto::hash8> &pre_decrypted_to_check,
+    const cryptonote::account_public_address &change_addr,
+    const std::vector<cryptonote::tx_destination_entry> &dests,
+    // Will be std::nullopt if tx_key is redacted.
+    const std::optional<crypto::secret_key> tx_key,
+    const std::vector<cryptonote::tx_extra_field> &tx_extra_fields)
+{
+    // Get the destination that will be able to read the payment id in the final tx.
+    // Returns non-null if there is exactly one destination (may or may not be the change addr).
+    const crypto::public_key view_key_pub = cryptonote::get_destination_view_key_pub(dests, change_addr);
+    CHECK_AND_ASSERT_THROW_MES(view_key_pub != crypto::null_pkey,
+        "reconstruct_payment_id: encrypted payment IDs must only be in txs with one destination");
+
+    // Extract the expected encrypted payment id.
+    cryptonote::tx_extra_nonce extra_nonce;
+    crypto::hash8 encrypted_payment_id8 = crypto::null_hash8;
+    CHECK_AND_ASSERT_THROW_MES(cryptonote::find_tx_extra_field_by_type(tx_extra_fields, extra_nonce),
+        "reconstruct_payment_id: expected payment id is missing");
+    CHECK_AND_ASSERT_THROW_MES(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce(
+            extra_nonce.nonce,
+            encrypted_payment_id8
+        ),
+        "reconstruct_payment_id: expected payment id is missing");
+
+    // Parse the payment ID string extracted from the address.
+    crypto::hash8 to_encrypt_payment_id;
+    CHECK_AND_ASSERT_THROW_MES(tools::wallet2::parse_short_payment_id(extracted_payment_id, to_encrypt_payment_id),
+        "reconstruct_payment_id: failed parsing short payment id from integrated address");
+    if (parsed_to_check)
+    {
+        CHECK_AND_ASSERT_THROW_MES(*parsed_to_check == to_encrypt_payment_id,
+            "reconstruct_payment_id: did not extract the same parsed payment id");
+    }
+    // if (pre_decrypted_to_check)
+    // {
+    //     CHECK_AND_ASSERT_THROW_MES(*pre_decrypted_to_check == to_encrypt_payment_id,
+    //         "reconstruct_payment_id: did not extract the same pre-decrypted payment id");
+    // }
+
+    if (tx_key)
+    {
+        // Encrypt the address's payment ID.
+        CHECK_AND_ASSERT_THROW_MES(hw::core::device_default().encrypt_payment_id(to_encrypt_payment_id,
+                view_key_pub, *tx_key),
+            "reconstruct_payment_id: failed encrypting payment id");
+
+        // Check equivalence.
+        CHECK_AND_ASSERT_THROW_MES(to_encrypt_payment_id == encrypted_payment_id8,
+            "reconstruct_payment_id: failed encrypting payment id");
+    }
+}
+//-------------------------------------------------------------------------------------------------------------------
+void sanity_check_pending_tx(const wallet2::pending_tx &ptx,
+    const cryptonote::network_type nettype,
+    const cryptonote::account_keys &account_keys,
+    const std::unordered_map<crypto::public_key, cryptonote::subaddress_index> &subaddresses,
+    const std::vector<wallet2_basic::transfer_details> &transfers,
+    const bool redacted,
+    const std::optional<std::function<const crypto::key_image(const size_t)>> &transfer_ki_resolver,
+    const bool allow_read_only)
+{
+    const auto &construct = ptx.construction_data;
+    const bool transfer_ki_accessible = transfer_ki_resolver.has_value();
+
+    // Extract outputs
+    std::vector<cryptonote::txout_to_tagged_key> ext_outputs;
+    const bool all_are_txout_to_tagged_key = std::all_of(ptx.tx.vout.begin(), ptx.tx.vout.end(),
+        [&](const cryptonote::tx_out& s_e) -> bool
+        {
+            CHECKED_GET_SPECIFIC_VARIANT(s_e.target, const cryptonote::txout_to_tagged_key, out, false);
+            ext_outputs.push_back(out);
+            return true;
+        }
+    );
+    CHECK_AND_ASSERT_THROW_MES(all_are_txout_to_tagged_key,
+        "sanity_check_pending_tx: all outputs should have view tags");
+    CHECK_AND_ASSERT_THROW_MES(construct.use_view_tags,
+        "sanity_check_pending_tx: must be view-tag-enabled (hardfork >= 15)");
+    CHECK_AND_ASSERT_THROW_MES(construct.splitted_dsts.size() == ptx.tx.vout.size(),
+        "sanity_check_pending_tx: destination vecs are inconsistent");
+    CHECK_AND_ASSERT_THROW_MES(ptx.tx.unlock_time == 0 && construct.unlock_time == 0,
+        "sanity_check_pending_tx: unlock_time must be zero");
+
+    // Extra
+    std::vector<cryptonote::tx_extra_field> tx_extra_fields;
+    CHECK_AND_ASSERT_THROW_MES(cryptonote::parse_tx_extra(ptx.tx.extra, tx_extra_fields),
+        "sanity_check_pending_tx: ptx.tx.extra extraction failure");
+    // NOTE: Due to upstream chaos, we are unable to reliably validate `construct.extra`.
+    // if (!cleartext_payment_id)
+    // {
+    //     CHECK_AND_ASSERT_THROW_MES(construct.extra == ptx.tx.extra,
+    //         "sanity_check_pending_tx: tx_extra mismatch");
+    // }
+    // else
+    // {
+    //     std::vector<uint8_t> extra_clone_unencrypted = construct.extra;
+    //     std::vector<uint8_t> extra_clone_encrypted = ptx.tx.extra;
+    //     cryptonote::remove_field_from_tx_extra(extra_clone_unencrypted, typeid(cryptonote::tx_extra_nonce));
+    //     cryptonote::remove_field_from_tx_extra(extra_clone_encrypted, typeid(cryptonote::tx_extra_nonce));
+    //     CHECK_AND_ASSERT_THROW_MES(extra_clone_unencrypted == extra_clone_encrypted,
+    //         "sanity_check_pending_tx: tx_extra mismatch w/ payment id removal");
+
+    //     // The decrypted payment ID will be checked manually later.
+    // }
+
+    if (!redacted)
+    {
+        crypto::public_key reconstruct_pubkey;
+        std::vector<crypto::public_key> reconstruct_additional_pubkeys;
+
+        validate_tx_outs(
+            ptx,
+            ext_outputs,
+            account_keys.m_view_secret_key,
+            construct.change_dts,
+            construct.splitted_dsts,
+            ptx.tx_key,
+            ptx.additional_tx_keys,
+            reconstruct_pubkey,
+            reconstruct_additional_pubkeys
+        );
+
+        cryptonote::tx_extra_pub_key tx_extra_pub_key;
+        CHECK_AND_ASSERT_THROW_MES(cryptonote::find_tx_extra_field_by_type(tx_extra_fields, tx_extra_pub_key),
+            "sanity_check_pending_tx: tx_extra missing tx pub key");
+        CHECK_AND_ASSERT_THROW_MES(tx_extra_pub_key.pub_key == reconstruct_pubkey,
+            "sanity_check_pending_tx: tx_extra unable to reproduce tx pubkey");
+        if (reconstruct_additional_pubkeys.size())
+        {
+            cryptonote::tx_extra_additional_pub_keys tx_extra_additional_pub_keys;
+            CHECK_AND_ASSERT_THROW_MES(cryptonote::find_tx_extra_field_by_type(tx_extra_fields, tx_extra_additional_pub_keys),
+                "sanity_check_pending_tx: tx_extra missing extra tx pub keys");
+            CHECK_AND_ASSERT_THROW_MES(tx_extra_additional_pub_keys.data.size() == reconstruct_additional_pubkeys.size(),
+                "sanity_check_pending_tx: tx_extra extra tx pub keys size mismatch");
+            for (size_t p = 0; p < reconstruct_additional_pubkeys.size(); ++p)
+            {
+                const auto &extra_pk = tx_extra_additional_pub_keys.data.at(p);
+                const auto &re_pk = reconstruct_additional_pubkeys.at(p);
+                CHECK_AND_ASSERT_THROW_MES(extra_pk == re_pk,
+                    "sanity_check_pending_tx: tx_extra unable to reproduce extra tx pubkey");
+            }
+        }
+    }
+
+    // Extract tx inputs
+    std::vector<crypto::key_image> ext_key_images;
+    std::vector<rct::xmr_amount> ext_input_amounts;
+    std::vector<std::vector<uint64_t>> ext_key_offsets;
+    const bool all_are_txin_to_key = std::all_of(ptx.tx.vin.begin(), ptx.tx.vin.end(),
+        [&](const cryptonote::txin_v& s_e) -> bool
+        {
+            CHECKED_GET_SPECIFIC_VARIANT(s_e, const cryptonote::txin_to_key, in, false);
+            ext_key_images.push_back(in.k_image);
+            ext_input_amounts.push_back(in.amount);
+            ext_key_offsets.push_back(in.key_offsets);
+            return true;
+        }
+    );
+    std::string ext_key_images_str;
+    for (const auto &ki : ext_key_images)
+    {
+        ext_key_images_str += boost::to_string(ki) + " ";
+    }
+    CHECK_AND_ASSERT_THROW_MES(all_are_txin_to_key,
+        "sanity_check_pending_tx: all inputs are not txin_to_key");
+    CHECK_AND_ASSERT_THROW_MES(ptx.key_images == ext_key_images_str,
+        "sanity_check_pending_tx: failed reconstructing key_images field");
+
+    const std::unordered_set<crypto::key_image> ext_key_images_set(ext_key_images.begin(), ext_key_images.end());
+    CHECK_AND_ASSERT_THROW_MES(ext_key_images.size() == ext_key_images_set.size(),
+        "sanity_check_pending_tx: duplicate key images");
+
+    // Inputs
+    // - We don't check the validity of tx_source_entry::real_out_additional_tx_keys.
+    CHECK_AND_ASSERT_THROW_MES(construct.sources.size() > 0,
+        "sanity_check_pending_tx: tx has no inputs");
+    CHECK_AND_ASSERT_THROW_MES(construct.sources.size() == ptx.tx.vin.size(),
+        "sanity_check_pending_tx: source/vin size mismatch");
+    // NOTE: due to upstream inconsistencies, we are unable to validate mixRing reliably
+    // if (recovered_from_serialized)
+    // {
+    //     CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.mixRing.size() == 0,
+    //         "sanity_check_pending_tx: ptx is recovered from serialized but mixRing.size() != 0");
+    // }
+    // else
+    // {
+    //     CHECK_AND_ASSERT_THROW_MES(construct.sources.size() == ptx.tx.rct_signatures.mixRing.size(),
+    //         "sanity_check_pending_tx: source(" << construct.sources.size() << ")/mixring("
+    //         << ptx.tx.rct_signatures.mixRing.size() << ") size mismatch");
+    // }
+    CHECK_AND_ASSERT_THROW_MES(construct.sources.size() == ptx.selected_transfers.size(),
+        "sanity_check_pending_tx: selected_transfers invalid size");
+    CHECK_AND_ASSERT_THROW_MES(construct.selected_transfers == ptx.selected_transfers,
+        "sanity_check_pending_tx: selected_transfers inconsistent");
+    // - For SOME REASON, construction data sources are not always in the same order as inputs, so we need to fix that
+    auto sources_ordered = construct.sources;
+    std::vector<size_t> ins_order;
+    for (const size_t selected_transfer : ptx.selected_transfers)
+    {
+        CHECK_AND_ASSERT_THROW_MES(selected_transfer < transfers.size(),
+            "sanity_check_pending_tx: invalid transfers index");
+        const auto &transfer = transfers.at(selected_transfer);
+        for (size_t i = 0; i < sources_ordered.size(); ++i)
+        {
+            const auto &src = sources_ordered.at(i);
+            CHECK_AND_ASSERT_THROW_MES(src.real_output < src.outputs.size(),
+                "sanity_check_pending_tx: ring sig index " << src.real_output << " out of input set size "
+                << src.outputs.size());
+            // Check both global output index and amount since 'global output index' is actually
+            // 'index in global array of same-amount outputs'.
+            if (src.outputs[src.real_output].first != transfer.m_global_output_index
+                || src.amount != transfer.m_amount
+                || src.outputs[src.real_output].second.dest != rct::pk2rct(transfer.get_public_key()))
+                continue;
+            ins_order.push_back(i);
+            break;
+        }
+    }
+    CHECK_AND_ASSERT_THROW_MES(ins_order.size() == sources_ordered.size(),
+        "sanity_check_pending_tx: global index mismatch between sources and tx");
+    tools::apply_permutation(ins_order, sources_ordered);
+
+    // Similarly, tx vins may not be in the same order as the selected transfers.
+    // Check selected transfers key images and map to tx vins order.
+    // Note: if the wallet hasn't imported key images, it won't be able to line up the order.
+    std::vector<size_t> inp_order(ptx.selected_transfers.size());
+    size_t found_kis_count = 0;
+    CHECK_AND_ASSERT_THROW_MES(ext_key_images.size() == ptx.selected_transfers.size(),
+        "sanity_check_pending_tx: extracted key images size mismatch to selected transfers");
+    for (size_t i = 0; i < ext_key_images.size() && transfer_ki_accessible; ++i)
+    {
+        const crypto::key_image &ki = ext_key_images[i];
+        for (size_t j = 0; j < ptx.selected_transfers.size(); ++j)
+        {
+            const auto &selected_transfer = ptx.selected_transfers.at(j);
+            CHECK_AND_ASSERT_THROW_MES(selected_transfer < transfers.size(),
+                "sanity_check_pending_tx: invalid transfers index");
+            if ((*transfer_ki_resolver)(selected_transfer) != ki)
+                continue;
+            inp_order.at(j) = i;
+            ++found_kis_count;
+            break;
+        }
+    }
+    CHECK_AND_ASSERT_THROW_MES(!transfer_ki_accessible || found_kis_count == ptx.selected_transfers.size(),
+        "sanity_check_pending_tx: ki mismatch between selected transfers and tx");
+
+    boost::multiprecision::uint128_t input_amnt = 0;
+    for (size_t i = 0; i < ptx.selected_transfers.size(); ++i)
+    {
+        const auto &selected_transfer = ptx.selected_transfers.at(i);
+        CHECK_AND_ASSERT_THROW_MES(selected_transfer < transfers.size(),
+            "sanity_check_pending_tx: invalid transfers index");
+
+        const auto &src = sources_ordered.at(i);
+        const auto &transfer = transfers.at(selected_transfer);
+
+        if (ptx.tx.version == 1)
+        {
+            CHECK_AND_ASSERT_THROW_MES(!src.rct,
+                "sanity_check_pending_tx: rct sources not allowed for v1 txs");
+        }
+        CHECK_AND_ASSERT_THROW_MES(src.amount == transfer.m_amount,
+            "sanity_check_pending_tx: transfer amount mismatch");
+        CHECK_AND_ASSERT_THROW_MES(src.real_output < src.outputs.size(),
+            "sanity_check_pending_tx: ring sig index out of input set size");
+        // `src.mask` may be uninitialized for v1 txs
+        const rct::key commitment = rct::commit(src.amount, ptx.tx.version > 1 ? src.mask : rct::identity());
+        // non-rct inputs to > v1 txs should always have identity masks set
+        if (ptx.tx.version > 1 && !src.rct)
+        {
+            CHECK_AND_ASSERT_THROW_MES(src.mask == rct::identity(),
+                "sanity_check_pending_tx: pre-rct input in > v1 tx has non-identity mask recorded");
+        }
+        CHECK_AND_ASSERT_THROW_MES(src.outputs[src.real_output].second.mask == commitment,
+            "sanity_check_pending_tx: failed reproducing real input's amount commitment");
+
+        // NOTE: due to upstream inconsistencies, we are unable to validate mixRing reliably.
+        // if (!recovered_from_serialized)
+        // {
+        //     const auto &input_ring = ptx.tx.rct_signatures.mixRing.at(i);
+        //     CHECK_AND_ASSERT_THROW_MES(input_ring.size() == src.outputs.size(),
+        //         "sanity_check_pending_tx: input ring size mismatch");
+        //     for (size_t r = 0; r < input_ring.size(); ++r)
+        //     {
+        //         CHECK_AND_ASSERT_THROW_MES(input_ring.at(r) == src.outputs.at(r).second,
+        //             "sanity_check_pending_tx: input ring member mismatch");
+        //     }
+        // }
+
+        if (!allow_read_only)
+        {
+            CHECK_AND_ASSERT_THROW_MES(!transfer.m_spent,
+                "sanity_check_pending_tx: non-read-only transfer - is marked as spent");
+            CHECK_AND_ASSERT_THROW_MES(!transfer.m_frozen,
+                "sanity_check_pending_tx: non-read-only transfer - is marked as frozen");
+        }
+        CHECK_AND_ASSERT_THROW_MES(src.rct == transfer.m_rct,
+            "sanity_check_pending_tx: transfer - 'is rct' mismatch");
+        CHECK_AND_ASSERT_THROW_MES(src.real_output_in_tx_index == transfer.m_internal_output_index,
+            "sanity_check_pending_tx: transfer - input's output-set index mismatch");
+        CHECK_AND_ASSERT_THROW_MES(src.outputs[src.real_output].second.dest == rct::pk2rct(transfer.get_public_key()),
+            "sanity_check_pending_tx: transfer - onetime addr mismatch");
+
+        if (transfer.m_mask != rct::identity())
+        {
+            // NOTE: as of this writing wallet2::sign_tx when called by a cold wallet is the only caller
+            // where transfer.m_mask may be set to the placeholder identity when the mask should be
+            // something else, since it's the only caller that didn't scan the chain which would have set
+            // it to the expected value. All other callers should have transfer.m_mask equal to src.mask.
+            CHECK_AND_ASSERT_THROW_MES(src.mask == transfer.m_mask,
+                "sanity_check_pending_tx: transfer - mask mismatch");
+        }
+
+        input_amnt += src.amount;
+        // We need `inp_order` from here, which is only valid if key images are available.
+        if (!transfer_ki_accessible)
+            continue;
+
+        const uint64_t ext_input_amount = ext_input_amounts.at(inp_order.at(i));
+        const std::vector<uint64_t> &ext_relative_key_offsets = ext_key_offsets.at(inp_order.at(i));
+
+        // Note: input amounts are cleartext for v1 outputs (even in an rct tx) because ring members are
+        // looked up in same-amount buckets.
+        CHECK_AND_ASSERT_THROW_MES(src.rct
+            ? ext_input_amount == 0
+            : ext_input_amount == src.amount,
+            "sanity_check_pending_tx: amount extracted from inputs is wrong");
+
+        // Key offsets line up
+        std::vector<uint64_t> src_absolute_key_offsets;
+        for (const auto &src_out : src.outputs)
+            src_absolute_key_offsets.push_back(src_out.first);
+        const std::vector<uint64_t> ext_absolute_key_offsets = cryptonote::relative_output_offsets_to_absolute(
+            ext_relative_key_offsets
+        );
+        CHECK_AND_ASSERT_THROW_MES(src_absolute_key_offsets == ext_absolute_key_offsets,
+            "sanity_check_pending_tx: key offsets mismatch");
+    }
+
+    CHECK_AND_ASSERT_THROW_MES(input_amnt <= UINT64_MAX,
+        "sanity_check_pending_tx: input amount > 2^64 - 1");
+
+    // Check change address
+    std::optional<cryptonote::subaddress_index> recognized_change_index;
+    recognized_change_index = cryptonote::sanity_check_change_address(ptx.change_dts.addr, subaddresses, account_keys);
+    for (const auto &dst_entr : construct.splitted_dsts)
+    {
+        CHECK_AND_ASSERT_THROW_MES(dst_entr.amount > 0 || ptx.tx.version > 1,
+            "sanity_check_pending_tx: Destination with wrong amount: " << dst_entr.amount);
+        const bool matches_change_addr = dst_entr.addr == ptx.change_dts.addr;
+        const bool is_bad_change_dst = matches_change_addr && dst_entr.amount > 0 && !recognized_change_index;
+        CHECK_AND_ASSERT_THROW_MES(!is_bad_change_dst,
+            "sanity_check_pending_tx: Non-zero amount change address is not recognized as belonging to the sender account");
+    }
+
+    // Destination consistency
+    CHECK_AND_ASSERT_THROW_MES(construct.change_dts == ptx.change_dts,
+        "sanity_check_pending_tx: change_dts inconsistent");
+    CHECK_AND_ASSERT_THROW_MES(construct.dests == ptx.dests,
+        "sanity_check_pending_tx: destination vecs are inconsistent");
+    std::vector<cryptonote::tx_destination_entry> splitted_dsts_repro;
+
+    if (ptx.tx.version == 1)
+    {
+        splitted_dsts_repro = construct.splitted_dsts;
+
+        // In v1, 'splitted' means destinations may be split into amount digits (e.g. sweep_unmixable).
+        // We iter splitted dests and accumulate them, then match amounts/keys with dests + change_dest
+        // (which also need to be accumulated in case of duplicates).
+        // The map keys use `account_public_address` instead of the spendkey in case of malicous construction.
+
+        // Accumulate splitted dests
+        std::unordered_map<cryptonote::account_public_address, boost::multiprecision::uint128_t> acc_split;
+        for (const auto &split : construct.splitted_dsts)
+        {
+            auto res = acc_split.try_emplace(split.addr, 0);
+            auto &amnt = res.first->second;
+            amnt += split.amount;
+        }
+
+        // Accumulate dests
+        std::unordered_map<cryptonote::account_public_address, boost::multiprecision::uint128_t> acc_dest;
+        if (ptx.change_dts.amount > 0)
+            acc_dest.emplace(ptx.change_dts.addr, ptx.change_dts.amount);
+
+        for (const auto &dest : ptx.dests)
+        {
+            auto res = acc_dest.try_emplace(dest.addr, 0);
+            auto &amnt = res.first->second;
+            amnt += dest.amount;
+        }
+
+        // Consistency check
+        CHECK_AND_ASSERT_THROW_MES(acc_split.size() == acc_dest.size(),
+            "sanity_check_pending_tx: v1 dest inconsistency (size)");
+        for (const auto &split : acc_split)
+        {
+            const auto &dest_res = acc_dest.find(split.first);
+            CHECK_AND_ASSERT_THROW_MES(dest_res != acc_dest.cend(),
+                "sanity_check_pending_tx: v1 dest inconsistency (addr matching)");
+            CHECK_AND_ASSERT_THROW_MES(split.second == dest_res->second,
+                "sanity_check_pending_tx: v1 dest inconsistency (amount matching)");
+            CHECK_AND_ASSERT_THROW_MES(split.second <= UINT64_MAX,
+                "sanity_check_pending_tx: v1 dest error (amount out of bounds)");
+        }
+    }
+    else //(ptx.tx.version > 1)
+    {
+        splitted_dsts_repro = ptx.dests;
+
+        if (ptx.change_dts.amount > 0 || ptx.dests.size() == 1)
+        {
+            splitted_dsts_repro.push_back(ptx.change_dts);
+        }
+        CHECK_AND_ASSERT_THROW_MES(construct.splitted_dsts.size() == splitted_dsts_repro.size(),
+            "sanity_check_pending_tx: failed checking splitted_dsts size");
+        CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.ecdhInfo.size() == splitted_dsts_repro.size(),
+            "sanity_check_pending_tx: ecdhInfo size mismatch");
+        CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.outPk.size() == splitted_dsts_repro.size(),
+            "sanity_check_pending_tx: outPk size mismatch");
+    }
+
+    boost::multiprecision::uint128_t output_amnt = 0;
+    size_t integrated_count = 0;
+    std::unordered_map<crypto::public_key, cryptonote::tx_destination_entry> dest_duplicates{};
+
+    for (size_t i = 0; i < construct.splitted_dsts.size(); ++i)
+    {
+        const auto &dest = construct.splitted_dsts.at(i);
+
+        // All integrated addresses must include the original string.
+        CHECK_AND_ASSERT_THROW_MES(!dest.is_integrated || dest.original.size() > 0,
+            "sanity_check_pending_tx: integrated destination missing original address string");
+
+        // Check original dst string
+        if (dest.original.size() > 0)
+        {
+            std::string address;
+            std::string extracted_payment_id;
+            uint64_t _amount;
+            std::string _tx_description;
+            std::string _recipient_name;
+            std::vector<std::string> _unknown_parameters;
+            std::string _error;
+            const bool parsed_uri = wallet2::parse_uri_impl(dest.original,
+                nettype,
+                address,
+                extracted_payment_id,
+                _amount,
+                _tx_description,
+                _recipient_name,
+                _unknown_parameters,
+                _error);
+
+            if (!parsed_uri)
+            {
+                // Treat original as the address string
+                address = dest.original;
+                extracted_payment_id = "";
+            }
+
+            // The `address` string may be an explicit address OR a URI that we can't access or assess here.
+            cryptonote::address_parse_info parse_info;
+            const bool parsed_info = cryptonote::get_account_address_from_str(parse_info, nettype, address);
+            if (parsed_info)
+            {
+                CHECK_AND_ASSERT_THROW_MES(parse_info.is_subaddress == dest.is_subaddress,
+                    "sanity_check_pending_tx: destination addr string mismatch - is_subaddress");
+                CHECK_AND_ASSERT_THROW_MES(parse_info.has_payment_id == dest.is_integrated,
+                    "sanity_check_pending_tx: destination addr string mismatch - is_integrated");
+                CHECK_AND_ASSERT_THROW_MES(parse_info.address == dest.addr,
+                    "sanity_check_pending_tx: destination addr string mismatch - address");
+
+                if (parse_info.has_payment_id && !extracted_payment_id.empty())
+                {
+                    CHECK_AND_ASSERT_THROW_MES(extracted_payment_id == epee::string_tools::pod_to_hex(parse_info.payment_id),
+                        "sanity_check_pending_tx: destination addr string mismatch - internal payment id inconsistency");
+                }
+
+                // Even if the URI was parsed successfully, the payment ID may only be stored in the internal
+                // 'address' substring.
+                if (parse_info.has_payment_id && extracted_payment_id.empty())
+                    extracted_payment_id = epee::string_tools::pod_to_hex(parse_info.payment_id);
+            }
+
+            if (dest.is_integrated)
+            {
+                ++integrated_count;
+                CHECK_AND_ASSERT_THROW_MES(integrated_count == 1,
+                    "sanity_check_pending_tx: more than one integrated address detected");
+
+                // NOTE: Due to upstream chaos, we are unable to reliably validate the construction data's payment ID.
+                // `tx_construction_data::extra` is *sometimes* pre-decrypted, so we handle it here by extracting and
+                // comparing directly with our decrypted version.
+                // std::optional<crypto::hash8> decrypted_payment_id8 = std::nullopt;
+                // if (cleartext_payment_id)
+                // {
+                //     std::vector<cryptonote::tx_extra_field> construct_tx_extra_fields;
+                //     CHECK_AND_ASSERT_THROW_MES(cryptonote::parse_tx_extra(construct.extra, construct_tx_extra_fields),
+                //         "sanity_check_pending_tx: construct.extra extraction failure");
+
+                //     cryptonote::tx_extra_nonce extra_nonce;
+                //     CHECK_AND_ASSERT_THROW_MES(cryptonote::find_tx_extra_field_by_type(construct_tx_extra_fields,
+                //         extra_nonce),
+                //         "reconstruct_payment_id: expected decrypted payment id is missing");
+                //     CHECK_AND_ASSERT_THROW_MES(cryptonote::get_encrypted_payment_id_from_tx_extra_nonce(
+                //             extra_nonce.nonce,
+                //             *decrypted_payment_id8
+                //         ),
+                //         "reconstruct_payment_id: expected decrypted payment id is missing");
+                // }
+
+                if (!extracted_payment_id.empty())
+                {
+                    reconstruct_payment_id(extracted_payment_id,
+                        parsed_info ? std::optional{parse_info.payment_id} : std::nullopt,
+                        // decrypted_payment_id8,
+                        ptx.change_dts.addr,
+                        construct.splitted_dsts,
+                        redacted ? std::nullopt : std::optional{ptx.tx_key},
+                        tx_extra_fields);
+                }
+            }
+        }
+
+        // Check reproduced dests
+        const auto &it = std::find_if(splitted_dsts_repro.cbegin(), splitted_dsts_repro.cend(),
+            [&dest](const auto &a) {
+                return a == dest;
+            }
+        );
+        CHECK_AND_ASSERT_THROW_MES(it != splitted_dsts_repro.cend(),
+            "sanity_check_pending_tx: failed checking splitted_dsts consistency");
+
+        // Check that addresses keys are canonical.
+        // Without this check, the following duplicate address check may be weakened.
+        CHECK_AND_ASSERT_THROW_MES(rct::isInMainSubgroup(rct::pk2rct(dest.addr.m_spend_public_key)),
+            "sanity_check_pending_tx: destination spendkey is not in the main subgroup (suspicious!)");
+        CHECK_AND_ASSERT_THROW_MES(rct::isInMainSubgroup(rct::pk2rct(dest.addr.m_view_public_key)),
+            "sanity_check_pending_tx: destination viewkey is not in the main subgroup (suspicious!)");
+
+        // Check that duplicate addresses are all either subaddresses or normal addresses (ignoring is_integrated).
+        const auto &dup = dest_duplicates.find(dest.addr.m_spend_public_key);
+        if (dup != dest_duplicates.cend())
+        {
+            CHECK_AND_ASSERT_THROW_MES(dup->second.addr == dest.addr,
+                "sanity_check_pending_tx: destinations have duplicate spend keys but not duplicate view keys");
+            CHECK_AND_ASSERT_THROW_MES(dup->second.is_subaddress == dest.is_subaddress,
+                "sanity_check_pending_tx: duplicate destinations do not have matching subaddress designations");
+            // NOTE: only one destination may have an integrated address, so we do not require duplicate destinations to
+            // have the same is_integrated designation.
+        }
+        else
+        {
+            dest_duplicates[dest.addr.m_spend_public_key] = dest;
+        }
+
+        // Carefully erase found copies one by one in case of duplicates.
+        splitted_dsts_repro.erase(it);
+
+        // Double-check v1 amounts (normally checked in the tx-out checker if not redacted)
+        if (ptx.tx.version == 1)
+        {
+            CHECK_AND_ASSERT_THROW_MES(ptx.tx.vout.at(i).amount == dest.amount,
+                "sanity_check_pending_tx: v1 tx output amount doesn't match destination amount");
+        }
+
+        output_amnt += dest.amount;
+    }
+    CHECK_AND_ASSERT_THROW_MES(splitted_dsts_repro.size() == 0,
+        "sanity_check_pending_tx: failed checking splitted_dsts consistency");
+
+    // Balance check
+    output_amnt += ptx.fee;
+
+    if (ptx.tx.version > 1)
+    {
+        CHECK_AND_ASSERT_THROW_MES(ptx.tx.rct_signatures.txnFee == ptx.fee,
+                "sanity_check_pending_tx: inconsistent fee for tx.v > 1 transaction");
+    }
+    if (ptx.dust_added_to_fee)
+    {
+        CHECK_AND_ASSERT_THROW_MES(ptx.dust <= ptx.fee,
+            "sanity_check_pending_tx: invalid dust amount when dust added to fee");
+    }
+    CHECK_AND_ASSERT_THROW_MES(output_amnt <= UINT64_MAX,
+            "sanity_check_pending_tx: output amount > 2^64 - 1");
+    CHECK_AND_ASSERT_THROW_MES(output_amnt == input_amnt,
+            "sanity_check_pending_tx: output amount != input amount");
+}
+//-------------------------------------------------------------------------------------------------------------------
+} //namespace wallet
+} //namespace tools

src/wallet/pending_tx_validation.h

@@ -0,0 +1,81 @@
+// Copyright (c) 2025-2026, The Monero Project
+//
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without modification, are
+// permitted provided that the following conditions are met:
+//
+// 1. Redistributions of source code must retain the above copyright notice, this list of
+//    conditions and the following disclaimer.
+//
+// 2. Redistributions in binary form must reproduce the above copyright notice, this list
+//    of conditions and the following disclaimer in the documentation and/or other
+//    materials provided with the distribution.
+//
+// 3. Neither the name of the copyright holder nor the names of its contributors may be
+//    used to endorse or promote products derived from this software without specific
+//    prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY
+// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+// MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+// THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+// INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+// STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#pragma once
+
+//local headers
+#include "crypto/crypto.h"
+#include "cryptonote_basic/account.h"
+#include "cryptonote_basic/subaddress_index.h"
+#include "cryptonote_config.h"
+#include "wallet2.h"
+#include "wallet2_basic/wallet2_types.h"
+
+//third party headers
+
+//standard headers
+#include <functional>
+#include <optional>
+#include <unordered_map>
+#include <vector>
+
+namespace tools
+{
+namespace wallet
+{
+/**
+ * brief: sanity_check_pending_tx - validate `pending_tx` consistency with itself and with with `transfer_details`
+ *        Assumes `ptx` version is >= v16.
+ *        WARNING: Unable to verify destination addresses that are URLs and not explicit addresses.
+ *        WARNING: Due to upstream chaos, we are unable to reliably validate that `tx_construction_data::extra` matches
+ *        with `ptx.tx.tx_extra`.
+ *        NOTE: Due to upstream inconsistencies, we are unable to validate mixRing reliably (it is not serialized in
+ *        final txs).
+ * param: ptx - the pending_tx to validate
+ * param: nettype - the network that will receive the tx (e.g. mainnet/stressnet/testnet)
+ * param: account_keys - the keys of the tx author (only the private view key and base address are needed)
+ * param: subaddresses - the tx author's subaddress map
+ * param: transfers - transfer_details from inside `wallet2` (required because `ptx` includes transfer references)
+ * param: redacted - if `true` then:
+ *        - We assume `ptx.tx_key` and `ptx.additional_tx_keys` are nullified.
+ * param: transfer_ki_resolver - if not-std::nullopt then this will be used to access `transfers` key images to
+          verify key images line up to wallets' imported key images
+        callback: const crypto::key_image _your_lambda_(const size_t transfer_idx)
+          - The callback should panic if `transfer_idx` is invalid.
+ * param: allow_read_only - if `true` then `ptx` transfers may be already-spent or frozen
+ */
+void sanity_check_pending_tx(const wallet2::pending_tx &ptx,
+    const cryptonote::network_type nettype,
+    const cryptonote::account_keys &account_keys,
+    const std::unordered_map<crypto::public_key, cryptonote::subaddress_index> &subaddresses,
+    const std::vector<wallet2_basic::transfer_details> &transfers,
+    const bool redacted,
+    const std::optional<std::function<const crypto::key_image(const size_t)>> &transfer_ki_resolver,
+    const bool allow_read_only);
+} //namespace wallet
+} //namespace tools

src/wallet/wallet2.cpp

@@ -93,6 +93,7 @@ using namespace epee;
 #include "device/device_cold.hpp"
 #include "device_trezor/device_trezor.hpp"
 #include "net/socks_connect.h"
+#include "pending_tx_validation.h"
 #include "wallet2_basic/wallet2_boost_serialization.h"
 #include "wallet2_basic/wallet2_serialization.h"
 
@@ -7242,6 +7243,43 @@ void wallet2::get_unconfirmed_payments(std::list<std::pair<crypto::hash,wallet2:
   }
 }
 //----------------------------------------------------------------------------------------------------
+// `expect_imported_key_images = false` overlaps with `transfer_ki_resolver = std::nullopt` but
+// we include both to reduce callsite complexity. (lack of useful enums in C++)
+void wallet2::sanity_check_pending_tx(const wallet2::pending_tx &ptx,
+  const bool redacted,
+  const bool expect_imported_key_images,
+  std::optional<std::function<const crypto::key_image(const size_t)>> transfer_ki_resolver,
+  const bool allow_read_only) const
+{
+  if (expect_imported_key_images)
+  {
+    // NOTE: Update this code if there is a usecase for checking both m_transfers and using a custom resolver.
+    CHECK_AND_ASSERT_THROW_MES(!transfer_ki_resolver,
+      "sanity_check_pending_tx (wallet2): expected imported key images but a ki resolver was provided");
+
+    const std::function<const crypto::key_image(const size_t)> temp =
+      [this](const size_t i)
+      {
+        CHECK_AND_ASSERT_THROW_MES(i < m_transfers.size(),
+          "sanity_check_pending_tx (wallet2): transfer - selected transfer idx out of known transfers");
+        const auto &transfer = m_transfers.at(i);
+        CHECK_AND_ASSERT_THROW_MES(transfer.m_key_image_known,
+          "sanity_check_pending_tx (wallet2): transfer - KI is expected but unknown");
+        return transfer.m_key_image;
+      };
+    transfer_ki_resolver = temp;
+  }
+
+  wallet::sanity_check_pending_tx(ptx,
+    this->nettype(),
+    m_account.get_keys(),
+    m_subaddresses,
+    m_transfers,
+    redacted,
+    transfer_ki_resolver,
+    allow_read_only);
+}
+//----------------------------------------------------------------------------------------------------
 void wallet2::rescan_spent()
 {
   // This is RPC call that can take a long time if there are many outputs,
@@ -7923,6 +7961,18 @@ bool wallet2::sign_tx(unsigned_tx_set &exported_txs, std::vector<wallet2::pendin
     signed_txes.key_images[i] = m_transfers[i].m_key_image;
   }
 
+  // check the local tx copies
+  for (const auto &ptx : txs)
+  {
+    for (const size_t idx : ptx.selected_transfers)
+    {
+      THROW_WALLET_EXCEPTION_IF(idx >= m_transfers.size(), error::wallet_internal_error,
+        "Cold wallet signing: No record of output " + std::to_string(idx) + " in this wallet, run "
+        "`export_outputs all` on the online wallet and `import_outputs` here.");
+    }
+    this->sanity_check_pending_tx(ptx, false, true, std::nullopt, false);
+  }
+
   return true;
 }
 //----------------------------------------------------------------------------------------------------
@@ -8004,7 +8054,7 @@ bool wallet2::load_tx(const std::string &signed_filename, std::vector<tools::wal
     return false;
   }
 
-  return parse_tx_from_str(s, ptx, accept_func);
+  return this->parse_tx_from_str(s, ptx, accept_func);
 }
 //----------------------------------------------------------------------------------------------------
 bool wallet2::parse_tx_from_str(const std::string &signed_tx_st, std::vector<tools::wallet2::pending_tx> &ptx, std::function<bool(const signed_tx_set &)> accept_func)
@@ -8055,6 +8105,36 @@ bool wallet2::parse_tx_from_str(const std::string &signed_tx_st, std::vector<too
     LOG_PRINT_L0("Unsupported version in signed transaction");
     return false;
   }
+
+  try
+  {
+    // validate before mutating state or displaying to user
+    // - Verify with the assumption signed txs are redacted.
+    for (const auto &ptx : signed_txs.ptx)
+    {
+      // Manually check `signed_txs.key_images` so local state is not mutated before we validate.
+      // We inject the key image checker because `ptx` internal sorting ambiguity makes it cumbersome to
+      // directly validate key images here.
+      // NOTE: These txs may be READ-ONLY, which means spent/frozen inputs are allowed.
+      const auto &kis = signed_txs.key_images;
+      this->sanity_check_pending_tx(ptx,
+        true,
+        false,
+        { [&kis](const size_t i) {
+          CHECK_AND_ASSERT_THROW_MES(i < kis.size(), "failed loading signed tx: ptx selected transfer "
+            "is outside the bounds of imported key images");
+          return kis.at(i);
+        } },
+        true);
+    }
+  }
+  catch (const std::exception &e)
+  {
+    LOG_PRINT_L0("Failed to validate signed transaction: " << e.what());
+    return false;
+  }
+
+  // print result for user
   LOG_PRINT_L0("Loaded signed tx data from binary: " << signed_txs.ptx.size() << " transactions");
   for (auto &c_ptx: signed_txs.ptx) LOG_PRINT_L0(cryptonote::obj_to_json_str(c_ptx.tx));
 
@@ -8065,13 +8145,25 @@ bool wallet2::parse_tx_from_str(const std::string &signed_tx_st, std::vector<too
   }
 
   // import key images
-  bool r = import_key_images(signed_txs.key_images);
+  bool r = this->import_key_images(signed_txs.key_images);
   if (!r) return false;
 
   // remember key images for this tx, for when we get those txes from the blockchain
   for (const auto &e: signed_txs.tx_key_images)
     m_cold_key_images.insert(e);
 
+  try
+  {
+    // extra/redundant validation making sure key images line up
+    for (const auto &ptx : signed_txs.ptx)
+      this->sanity_check_pending_tx(ptx, true, true, std::nullopt, true);
+  }
+  catch (const std::exception &e)
+  {
+    LOG_PRINT_L0("Failed to validate signed transaction (post import): " << e.what());
+    return false;
+  }
+
   ptx = signed_txs.ptx;
 
   return true;
@@ -8126,6 +8218,9 @@ bool wallet2::save_multisig_tx(const multisig_tx_set &txs, const std::string &fi
 //----------------------------------------------------------------------------------------------------
 wallet2::multisig_tx_set wallet2::make_multisig_tx_set(const std::vector<pending_tx>& ptx_vector) const
 {
+  for (const auto &ptx : ptx_vector)
+    this->sanity_check_pending_tx(ptx, false, true, std::nullopt, false);
+
   multisig_tx_set txs;
   txs.m_ptx = ptx_vector;
 
@@ -8185,18 +8280,21 @@ bool wallet2::parse_multisig_tx_from_str(std::string multisig_tx_st, multisig_tx
     return false;
   }
 
-  // sanity checks
-  for (const auto &ptx: exported_txs.m_ptx)
+  try
   {
-    CHECK_AND_ASSERT_MES(ptx.selected_transfers.size() == ptx.tx.vin.size(), false, "Mismatched selected_transfers/vin sizes");
-    for (size_t idx: ptx.selected_transfers)
-      CHECK_AND_ASSERT_MES(idx < m_transfers.size(), false, "Transfer index out of range");
-    CHECK_AND_ASSERT_MES(ptx.construction_data.selected_transfers.size() == ptx.tx.vin.size(), false, "Mismatched cd selected_transfers/vin sizes");
-    for (size_t idx: ptx.construction_data.selected_transfers)
-      CHECK_AND_ASSERT_MES(idx < m_transfers.size(), false, "Transfer index out of range");
-    CHECK_AND_ASSERT_MES(ptx.construction_data.sources.size() == ptx.tx.vin.size(), false, "Mismatched sources/vin sizes");
-    CHECK_AND_ASSERT_MES(!ptx.tx.vin.empty(), false, "Multisig tx has no inputs");
-    CHECK_AND_ASSERT_MES(!ptx.construction_data.sources.empty(), false, "Multisig tx has no sources");
+    // sanity checks
+    for (const auto &ptx: exported_txs.m_ptx)
+    {
+      // If key images have not been imported then this could fail if we check key images. We'd rather fail elsewhere
+      // with a better error message.
+      // Note: These may be READ-ONLY, so spent/frozen inputs are allowed.
+      this->sanity_check_pending_tx(ptx, false, false, std::nullopt, true);
+    }
+  }
+  catch (const std::exception &e)
+  {
+    LOG_PRINT_L0("Failed to validate multisig tx data: " << e.what());
+    return false;
   }
 
   return true;
@@ -8279,14 +8377,33 @@ bool wallet2::sign_multisig_tx(multisig_tx_set &exported_txs_inout, std::vector<
   THROW_WALLET_EXCEPTION_IF(frozen(exported_txs),
     error::wallet_internal_error, "Will not sign multisig tx containing frozen outputs")
 
+  // We only sign if key images have been imported to ensure:
+  // A) signatures can only be extracted from us if our key images are valid (the sanity checker will make sure
+  // the tx has the same key images as in our m_transfers, and a valid tx signature will mean key images are valid)
+  // B) signatures involving honest signers will always have imported key images, so at least one honest signer
+  // will be able to detect spends from such txs
+  // NOTE: This goes outside the main loop to harden against a theoretical synchronization issue where this triggers
+  // after we get half-way through partial signing, causing our next import to run into 'export needed' because
+  // the previous attempt had cleared some private nonces. At this time exports/imports are atomic so that bug can't
+  // occur.
+  for (size_t n = 0; n < exported_txs.m_ptx.size(); ++n)
+  {
+    for (const size_t idx : exported_txs.m_ptx[n].construction_data.selected_transfers)
+    {
+      if (idx >= m_transfers.size() || !m_transfers[idx].m_key_image_known)
+        THROW_WALLET_EXCEPTION(error::multisig_import_needed);
+    }
+  }
+
   // The 'exported_txs' contains a set of different transactions for the multisig group to try to sign. Each of those
   //   transactions has a set of 'signing attempts' corresponding to all the possible signing groups within the multisig.
   // - Here, we will partially sign as many of those signing attempts as possible, for each proposed transaction.
   for (size_t n = 0; n < exported_txs.m_ptx.size(); ++n)
   {
     tools::wallet2::pending_tx &ptx = exported_txs.m_ptx[n];
-    THROW_WALLET_EXCEPTION_IF(ptx.multisig_sigs.empty(), error::wallet_internal_error, "No signatures found in multisig tx");
     const tools::wallet2::tx_construction_data &sd = ptx.construction_data;
+    THROW_WALLET_EXCEPTION_IF(ptx.multisig_sigs.empty(), error::wallet_internal_error, "No signatures found in multisig tx");
+
     LOG_PRINT_L1(" " << (n+1) << ": " << sd.sources.size() << " inputs, ring size " << (sd.sources[0].outputs.size()) <<
         ", signed by " << exported_txs.m_signers.size() << "/" << m_multisig_threshold);
 
@@ -8314,6 +8431,11 @@ bool wallet2::sign_multisig_tx(multisig_tx_set &exported_txs_inout, std::vector<
       "error: multisig::signing::tx_builder_ringct_t::init"
     );
 
+    // Fully validate
+    // NOTE: This must occur after `tx_builder_ringct_t::init` because that function edits `ptx.tx` in-place.
+    // Spaghetti is as spaghetti does.
+    this->sanity_check_pending_tx(ptx, false, true, std::nullopt, false);
+
     // go through each signing attempt for this transaction (each signing attempt corresponds to some subgroup of signers
     //   of size 'threshold')
     for (auto &sig: ptx.multisig_sigs)
@@ -11372,6 +11494,18 @@ void wallet2::cold_sign_tx(const std::vector<pending_tx>& ptx_vector, signed_tx_
   tx_device_aux = aux_data.tx_device_aux;
 
   MDEBUG("Signed tx data from hw: " << exported_txs.ptx.size() << " transactions");
+
+  // Double-check final values.
+  // Cold wallets are not expected to have a complete store of key images.
+  // Cold devices (e.g. trezor) may or may not redact outputs, so we set `redact = true`.
+  // TODO: Redacting means we can't fully validate ptx and that we must trust the cold wallet.
+  // For robustness it would be better for both devices to distrust each other. Note that all redacted
+  // info is left in plaintext in the `pending_tx` construction data, so it's unclear *why* anything
+  // is redacted in the first place.
+  for (const auto &ptx : exported_txs.ptx)
+    this->sanity_check_pending_tx(ptx, true, false, std::nullopt, false);
+
+  // Print
   for (auto &c_ptx: exported_txs.ptx) LOG_PRINT_L0(cryptonote::obj_to_json_str(c_ptx.tx));
 }
 //----------------------------------------------------------------------------------------------------
@@ -14870,7 +15004,27 @@ std::string wallet2::make_uri(const std::string &address, const std::string &pay
   return uri;
 }
 //----------------------------------------------------------------------------------------------------
-bool wallet2::parse_uri(const std::string &uri, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector<std::string> &unknown_parameters, std::string &error)
+bool wallet2::parse_uri(const std::string &uri,
+  std::string &address,
+  std::string &payment_id,
+  uint64_t &amount,
+  std::string &tx_description,
+  std::string &recipient_name,
+  std::vector<std::string> &unknown_parameters,
+  std::string &error)
+{
+  return wallet2::parse_uri_impl(uri,
+    this->nettype(),
+    address,
+    payment_id,
+    amount,
+    tx_description,
+    recipient_name,
+    unknown_parameters,
+    error);
+}
+//----------------------------------------------------------------------------------------------------
+bool wallet2::parse_uri_impl(const std::string &uri, const cryptonote::network_type nettype, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector<std::string> &unknown_parameters, std::string &error)
 {
   if (uri.substr(0, 7) != "monero:")
   {
@@ -14883,7 +15037,7 @@ bool wallet2::parse_uri(const std::string &uri, std::string &address, std::strin
   address = ptr ? remainder.substr(0, ptr-remainder.c_str()) : remainder;
 
   cryptonote::address_parse_info info;
-  if(!get_account_address_from_str(info, nettype(), address))
+  if(!get_account_address_from_str(info, nettype, address))
   {
     error = std::string("URI has wrong address: ") + address;
     return false;

src/wallet/wallet2.h

@@ -485,7 +485,9 @@ private:
     struct signed_tx_set
     {
       std::vector<pending_tx> ptx;
+      // All key images in `m_transfers` from the signing wallet. Does not include key images in `ptx`.
       std::vector<crypto::key_image> key_images;
+      // All key images in `ptx` for outputs being received by the wallet (e.g. change, churn).
       std::unordered_map<crypto::public_key, crypto::key_image> tx_key_images;
 
       BEGIN_SERIALIZE_OBJECT()
@@ -932,6 +934,7 @@ private:
       uint64_t min_height, uint64_t max_height = (uint64_t)-1, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
     void get_unconfirmed_payments_out(std::list<std::pair<crypto::hash,wallet2::unconfirmed_transfer_details>>& unconfirmed_payments, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
     void get_unconfirmed_payments(std::list<std::pair<crypto::hash,wallet2::pool_payment_details>>& unconfirmed_payments, const boost::optional<uint32_t>& subaddr_account = boost::none, const std::set<uint32_t>& subaddr_indices = {}) const;
+    void sanity_check_pending_tx(const wallet2::pending_tx &ptx, const bool redacted, const bool expect_imported_key_images, std::optional<std::function<const crypto::key_image(const size_t)>> transfer_ki_resolver, const bool allow_read_only) const;
 
     uint64_t get_blockchain_current_height() const { return m_blockchain.size(); }
     void rescan_spent();
@@ -1358,6 +1361,7 @@ private:
 
     std::string make_uri(const std::string &address, const std::string &payment_id, uint64_t amount, const std::string &tx_description, const std::string &recipient_name, std::string &error) const;
     bool parse_uri(const std::string &uri, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector<std::string> &unknown_parameters, std::string &error);
+    static bool parse_uri_impl(const std::string &uri, const cryptonote::network_type, std::string &address, std::string &payment_id, uint64_t &amount, std::string &tx_description, std::string &recipient_name, std::vector<std::string> &unknown_parameters, std::string &error);
 
     uint64_t get_blockchain_height_by_date(uint16_t year, uint8_t month, uint8_t day);    // 1<=month<=12, 1<=day<=31
 

tests/functional_tests/cold_signing.py

@@ -38,6 +38,7 @@ import random
 SEED = 'velvet lymph giddy number token physics poetry unquoted nibs useful sabotage limits benches lifestyle eden nitrogen anvil fewest avoid batch vials washing fences goat unquoted'
 STANDARD_ADDRESS = '42ey1afDFnn4886T7196doS9GPMzexD9gXpsZJDwVjeRVdFCSoHnv7KPbBeGpzJBzHRCAs9UxqeoyFQMYbqSWYTfJJQAWDm'
 SUBADDRESS = '84QRUYawRNrU3NN1VpFRndSukeyEb3Xpv8qZjjsoJZnTYpDYceuUTpog13D7qPxpviS7J29bSgSkR11hFFoXWk2yNdsR9WF'
+SUBADDRESS2 = '85M4M1RVRcoEeC8sdSxN1ef6GhQYChSfKPWkB4FLKYJiSWuMXXT4Ewv8BHCRzSJB4ZYvXUcFxN4DPcVu6uwoPNRvQ1QwaXB'
 
 class ColdSigningTest():
     def run_test(self):
@@ -49,6 +50,8 @@ class ColdSigningTest():
         for piecemeal_output_export in [False, True]:
             self.self_transfer_to_subaddress(piecemeal_output_export)
         self.transfer_after_empty_export_import()
+        self.transfer_to_multi_subaddresses()
+        self.sweep()
 
     def reset(self):
         print('Resetting blockchain')
@@ -87,12 +90,12 @@ class ColdSigningTest():
         assert self.cold_wallet.get_address().address == self.hot_wallet.get_address().address
         assert self.cold_wallet.get_address().address == STANDARD_ADDRESS
 
-    def mine(self):
+    def mine(self, n_blocks = 80):
         print("Mining some blocks")
         daemon = Daemon()
         wallet = Wallet()
 
-        daemon.generateblocks(STANDARD_ADDRESS, 80)
+        daemon.generateblocks(STANDARD_ADDRESS, n_blocks)
         wallet.refresh()
 
     def export_import(self, piecemeal_output_export):
@@ -265,6 +268,48 @@ class ColdSigningTest():
         self.create_tx(STANDARD_ADDRESS, False)
         assert start_len == len(self.hot_wallet.get_transfers()['in']) - 1
 
+    def sign_and_submit(self, unsigned_txset):
+        print("Signing transaction with cold wallet")
+        res = self.cold_wallet.sign_transfer(unsigned_txset)
+
+        print("Submitting transaction with hot wallet")
+        res = self.hot_wallet.submit_transfer(res.signed_txset)
+        assert len(res.tx_hash_list) > 0
+        tx_hash_list = res.tx_hash_list
+
+        # Make sure it ends up in the chain
+        daemon = Daemon()
+        daemon.generateblocks(STANDARD_ADDRESS, 1)
+        self.hot_wallet.refresh()
+
+        res = self.hot_wallet.get_transfers()
+        def in_hash_list(txid, tx_hash_list):
+            return len([x for x in tx_hash_list if x == txid]) > 0
+        assert len([x for x in (res['pending'] if 'pending' in res else []) if in_hash_list(x.txid, tx_hash_list)]) == 0
+        assert len([x for x in (res['out'] if 'out' in res else []) if in_hash_list(x.txid, tx_hash_list)]) > 0
+
+    def transfer_to_multi_subaddresses(self):
+        # This test triggers the non-standard additional keys case
+        # const bool need_additional_txkeys = num_subaddresses > 0 && (num_stdaddresses > 0 || num_subaddresses > 1);
+        print("Transfer to 2 subaddresses in 1 tx")
+        dst1 = {'address': SUBADDRESS, 'amount': 1000000000000}
+        dst2 = {'address': SUBADDRESS2, 'amount': 1000000000000}
+
+        self.export_import(False)
+        res = self.hot_wallet.transfer([dst1, dst2])
+
+        self.sign_and_submit(res.unsigned_txset)
+
+    def sweep(self):
+        print("Mine 10 blocks so all non-coinbase outs from prior txs unlock")
+        self.mine(10)
+
+        print("Sweeping the wallet")
+        self.export_import(False)
+        res = self.hot_wallet.sweep_all(STANDARD_ADDRESS)
+
+        self.sign_and_submit(res.unsigned_txset)
+
 class Guard:
     def __enter__(self):
         for i in range(2):