cryptonote_basic: fix add_extra_nonce_to_tx_extra() length

Author: jeffro256

src/common/varint.h

@@ -125,4 +125,14 @@ namespace tools {
     int read_varint(InputIt &&first, InputIt &&last, T &i) {
     return read_varint<std::numeric_limits<T>::digits>(std::forward<InputIt>(first), std::forward<InputIt>(last), i);
   }
+
+  template <typename T, typename = std::enable_if_t<std::is_integral_v<T> && std::is_unsigned_v<T>>>
+  constexpr std::size_t get_varint_byte_size(T val) {
+    std::size_t bytes = 0;
+    do {
+      ++bytes;
+      val >>= 7;
+    } while (val);
+    return bytes;
+  }
 }

src/cryptonote_basic/cryptonote_format_utils.cpp

@@ -717,16 +717,17 @@ namespace cryptonote
   bool add_extra_nonce_to_tx_extra(std::vector<uint8_t>& tx_extra, const blobdata& extra_nonce)
   {
     CHECK_AND_ASSERT_MES(extra_nonce.size() <= TX_EXTRA_NONCE_MAX_COUNT, false, "extra nonce could be 255 bytes max");
-    size_t start_pos = tx_extra.size();
-    tx_extra.resize(tx_extra.size() + 2 + extra_nonce.size());
+    unsigned char *p = tx_extra.data() + tx_extra.size();
+    const std::size_t len_varint_bytes = tools::get_varint_byte_size(extra_nonce.size());
+    tx_extra.resize(tx_extra.size() + 1 + len_varint_bytes + extra_nonce.size());
     //write tag
-    tx_extra[start_pos] = TX_EXTRA_NONCE;
+    *p = TX_EXTRA_NONCE;
+    ++p;
     //write len
-    ++start_pos;
-    tx_extra[start_pos] = static_cast<uint8_t>(extra_nonce.size());
+    tools::write_varint(p, extra_nonce.size());
+    assert(p == tx_extra.data() + tx_extra.size() - extra_nonce.size());
     //write data
-    ++start_pos;
-    memcpy(&tx_extra[start_pos], extra_nonce.data(), extra_nonce.size());
+    memcpy(p, extra_nonce.data(), extra_nonce.size());
     return true;
   }
   //---------------------------------------------------------------

tests/unit_tests/CMakeLists.txt

@@ -42,6 +42,7 @@ set(unit_tests_sources
   checkpoints.cpp
   command_line.cpp
   crypto.cpp
+  cryptonote_format_utils.cpp
   decompose_amount_into_digits.cpp
   device.cpp
   difficulty.cpp

tests/unit_tests/cryptonote_format_utils.cpp

@@ -0,0 +1,89 @@
+// Copyright (c) 2025, 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.
+
+#include "gtest/gtest.h"
+
+#include "crypto/generators.h"
+#include "cryptonote_basic/cryptonote_format_utils.h"
+
+TEST(cn_format_utils, add_extra_nonce_to_tx_extra)
+{
+    static constexpr std::size_t max_nonce_size = TX_EXTRA_NONCE_MAX_COUNT + 1; // we *can* test higher if desired
+
+    std::vector<std::uint8_t> extra_prefix;
+    cryptonote::add_tx_pub_key_to_extra(extra_prefix, crypto::get_H());
+
+    std::vector<std::uint8_t> extra;
+    std::string nonce;
+    std::vector<cryptonote::tx_extra_field> tx_extra_fields;
+    extra.reserve(extra_prefix.size() + max_nonce_size + 1 + 10);
+    nonce.reserve(max_nonce_size);
+    tx_extra_fields.reserve(2);
+    for (std::size_t nonce_size = 0; nonce_size <= max_nonce_size; ++nonce_size)
+    {
+        extra = extra_prefix;
+        nonce.resize(nonce_size);
+        memset(&nonce[0], '%', nonce.size());
+        tx_extra_fields.clear();
+
+        const std::size_t expected_extra_size = extra_prefix.size() + 1
+            + tools::get_varint_byte_size(nonce_size) + nonce_size;
+        const bool expected_success = nonce_size <= TX_EXTRA_NONCE_MAX_COUNT;
+
+        // add nonce and do detailed test
+        const bool add_success = cryptonote::add_extra_nonce_to_tx_extra(extra, nonce);
+        ASSERT_EQ(expected_success, add_success);
+        if (!expected_success)
+            continue;
+        ASSERT_EQ(expected_extra_size, extra.size());
+        ASSERT_EQ(0, memcmp(extra_prefix.data(), extra.data(), extra_prefix.size()));
+        const std::uint8_t *p = extra.data() + extra_prefix.size();
+        ASSERT_EQ(TX_EXTRA_NONCE, *p);
+        ++p;
+        std::size_t read_nonce_size = 0;
+        const int varint_size = tools::read_varint((const uint8_t*)(p), // copy p
+            (const uint8_t*) extra.data() + extra.size(),
+            read_nonce_size);
+        ASSERT_EQ(tools::get_varint_byte_size(nonce_size), varint_size);
+        p += varint_size;
+        for (std::size_t i = 0; i < nonce_size; ++i)
+        {
+            ASSERT_EQ('%', *p);
+            ++p;
+        }
+        ASSERT_EQ(extra.data() + extra.size(), p);
+
+        // do integration test with higher-level tx_extra parsing code
+        ASSERT_TRUE(cryptonote::parse_tx_extra(extra, tx_extra_fields));
+        ASSERT_EQ(2, tx_extra_fields.size());
+        const auto &pk_field = boost::get<cryptonote::tx_extra_pub_key>(tx_extra_fields.at(0));
+        ASSERT_EQ(crypto::get_H(), pk_field.pub_key);
+        const auto &nonce_field = boost::get<cryptonote::tx_extra_nonce>(tx_extra_fields.at(1));
+        ASSERT_EQ(nonce, nonce_field.nonce);
+    }
+}

tests/unit_tests/varint.cpp

@@ -62,3 +62,101 @@ TEST(varint, equal)
     ASSERT_TRUE(idx2 == idx);
   }
 }
+
+TEST(varint, max_uint64_t_bytes)
+{
+  unsigned char bytes[100]{};
+  unsigned char *p = bytes;
+  tools::write_varint(p, std::numeric_limits<std::uint64_t>::max());
+  std::size_t i = 0;
+  while (bytes[i]) ++i;
+  ASSERT_EQ(10, i);        // tests size of max 64-bit uint
+  ASSERT_EQ(bytes + i, p); // tests iterator is modified in-place
+}
+
+template <typename T>
+static void subtest_varint_byte_size_for_single_value(const std::size_t expected_bytes, const T val)
+{
+  static_assert(std::is_integral_v<T> && std::is_unsigned_v<T>);
+  static constexpr int DIGITS = std::numeric_limits<T>::digits;
+  static_assert(DIGITS == CHAR_BIT * sizeof(T));
+  static_assert(sizeof(T));
+  static_assert(sizeof(T) <= 8); // 64-bit or less
+
+  unsigned char bytes[100]{};
+  unsigned char *p = bytes;
+  tools::write_varint(p, val);
+  ASSERT_EQ(expected_bytes, p - bytes);
+  ASSERT_EQ(expected_bytes, tools::get_varint_byte_size(val));
+}
+
+template <typename T>
+static void subtest_varint_byte_size_for_type()
+{
+  static_assert(std::is_integral_v<T> && std::is_unsigned_v<T>);
+  static constexpr int DIGITS = std::numeric_limits<T>::digits;
+  static_assert(DIGITS == CHAR_BIT * sizeof(T));
+  static_assert(sizeof(T));
+
+  for (T n = 0; n < 128; ++n)
+  {
+    subtest_varint_byte_size_for_single_value(1, n);
+  }
+
+  for (T n = 128; n < 255; ++n)
+  {
+    subtest_varint_byte_size_for_single_value(2, n);
+  }
+
+  subtest_varint_byte_size_for_single_value(2, T{255});
+
+  if constexpr (DIGITS > 8)
+  {
+    static_assert(DIGITS >= 16);
+    for (T n = 256; n < 16384; ++n)
+    {
+      subtest_varint_byte_size_for_single_value(2, n);
+    }
+
+    for (T n = 16384; n < 65535; ++n)
+    {
+      subtest_varint_byte_size_for_single_value(3, n);
+    }
+
+    subtest_varint_byte_size_for_single_value(3, T{65535});
+
+    if constexpr (DIGITS > 16)
+    {
+      static_assert(DIGITS >= 32);
+      subtest_varint_byte_size_for_single_value(3, T{2097151});
+      subtest_varint_byte_size_for_single_value(4, T{2097152});
+      subtest_varint_byte_size_for_single_value(4, T{268435455});
+      subtest_varint_byte_size_for_single_value(5, T{268435456});
+      subtest_varint_byte_size_for_single_value(5, T{4294967295});
+
+      if constexpr (DIGITS > 32)
+      {
+        static_assert(DIGITS >= 64);
+        subtest_varint_byte_size_for_single_value(5, T{34359738367});
+        subtest_varint_byte_size_for_single_value(6, T{34359738368});
+        subtest_varint_byte_size_for_single_value(6, T{4398046511103});
+        subtest_varint_byte_size_for_single_value(7, T{4398046511104});
+        subtest_varint_byte_size_for_single_value(7, T{562949953421311});
+        subtest_varint_byte_size_for_single_value(8, T{562949953421312});
+        subtest_varint_byte_size_for_single_value(8, T{72057594037927935});
+        subtest_varint_byte_size_for_single_value(9, T{72057594037927936});
+        subtest_varint_byte_size_for_single_value(9, T{9223372036854775807});
+        subtest_varint_byte_size_for_single_value(10, T{9223372036854775808ull});
+        subtest_varint_byte_size_for_single_value(10, T{18446744073709551615ull});
+      }
+    }
+  }
+};
+
+TEST(varint, get_varint_byte_size)
+{
+  subtest_varint_byte_size_for_type<std::uint8_t>();
+  subtest_varint_byte_size_for_type<std::uint16_t>();
+  subtest_varint_byte_size_for_type<std::uint32_t>();
+  subtest_varint_byte_size_for_type<std::uint64_t>();
+}