From afd5949afd24b25eada28708e3b8592acc407c13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Wo=CC=81jcik?= <3044353+pwojcikdev@users.noreply.github.com> Date: Sat, 26 Oct 2024 11:12:48 +0200 Subject: [PATCH] Use saturation arithmetic for incrementing account numbers --- nano/core_test/numbers.cpp | 95 +++++++++++++++++++++++++++ nano/lib/numbers.hpp | 27 ++++++++ nano/node/backlog_scan.cpp | 2 +- nano/node/bootstrap/crawlers.hpp | 2 +- nano/node/bootstrap/database_scan.cpp | 4 +- nano/node/json_handler.cpp | 4 +- nano/node/node.cpp | 2 +- 7 files changed, 129 insertions(+), 7 deletions(-) diff --git a/nano/core_test/numbers.cpp b/nano/core_test/numbers.cpp index ec8bed3f4e..7b6e77015e 100644 --- a/nano/core_test/numbers.cpp +++ b/nano/core_test/numbers.cpp @@ -649,3 +649,98 @@ TEST (uint512_union, hash) } } } + +TEST (sat_math, add_sat) +{ + // Test uint128_t + { + nano::uint128_t max = std::numeric_limits::max (); + nano::uint128_t one = 1; + nano::uint128_t large_val = max - 100; + + // Normal addition + ASSERT_EQ (nano::add_sat (one, one), nano::uint128_t (2)); + + // Saturation at max + ASSERT_EQ (nano::add_sat (max, one), max); + ASSERT_EQ (nano::add_sat (large_val, nano::uint128_t (200)), max); + ASSERT_EQ (nano::add_sat (max, max), max); + } + // Test uint256_t + { + nano::uint256_t max = std::numeric_limits::max (); + nano::uint256_t one = 1; + nano::uint256_t large_val = max - 100; + + // Normal addition + ASSERT_EQ (nano::add_sat (one, one), nano::uint256_t (2)); + + // Saturation at max + ASSERT_EQ (nano::add_sat (max, one), max); + ASSERT_EQ (nano::add_sat (large_val, nano::uint256_t (200)), max); + ASSERT_EQ (nano::add_sat (max, max), max); + } + // Test uint512_t + { + nano::uint512_t max = std::numeric_limits::max (); + nano::uint512_t one = 1; + nano::uint512_t large_val = max - 100; + + // Normal addition + ASSERT_EQ (nano::add_sat (one, one), nano::uint512_t (2)); + + // Saturation at max + ASSERT_EQ (nano::add_sat (max, one), max); + ASSERT_EQ (nano::add_sat (large_val, nano::uint512_t (200)), max); + ASSERT_EQ (nano::add_sat (max, max), max); + } +} + +TEST (sat_math, sub_sat) +{ + // Test uint128_t + { + nano::uint128_t max = std::numeric_limits::max (); + nano::uint128_t min = std::numeric_limits::min (); + nano::uint128_t one = 1; + nano::uint128_t hundred (100); + + // Normal subtraction + ASSERT_EQ (nano::sub_sat (hundred, one), nano::uint128_t (99)); + + // Saturation at min + ASSERT_EQ (nano::sub_sat (min, one), min); + ASSERT_EQ (nano::sub_sat (hundred, nano::uint128_t (200)), min); + ASSERT_EQ (nano::sub_sat (min, max), min); + } + // Test uint256_t + { + nano::uint256_t max = std::numeric_limits::max (); + nano::uint256_t min = std::numeric_limits::min (); + nano::uint256_t one = 1; + nano::uint256_t hundred (100); + + // Normal subtraction + ASSERT_EQ (nano::sub_sat (hundred, one), nano::uint256_t (99)); + + // Saturation at min + ASSERT_EQ (nano::sub_sat (min, one), min); + ASSERT_EQ (nano::sub_sat (hundred, nano::uint256_t (200)), min); + ASSERT_EQ (nano::sub_sat (min, max), min); + } + // Test uint512_t + { + nano::uint512_t max = std::numeric_limits::max (); + nano::uint512_t min = std::numeric_limits::min (); + nano::uint512_t one = 1; + nano::uint512_t hundred (100); + + // Normal subtraction + ASSERT_EQ (nano::sub_sat (hundred, one), nano::uint512_t (99)); + + // Saturation at min + ASSERT_EQ (nano::sub_sat (min, one), min); + ASSERT_EQ (nano::sub_sat (hundred, nano::uint512_t (200)), min); + ASSERT_EQ (nano::sub_sat (min, max), min); + } +} \ No newline at end of file diff --git a/nano/lib/numbers.hpp b/nano/lib/numbers.hpp index ccf42ba357..b32c32075b 100644 --- a/nano/lib/numbers.hpp +++ b/nano/lib/numbers.hpp @@ -507,6 +507,33 @@ namespace difficulty uint64_t from_multiplier (double const, uint64_t const); double to_multiplier (uint64_t const, uint64_t const); } + +/** + * Add to or substract from a value without overflow + * TODO: C++26 replace with std::add_sat and std::sub_sat + */ +template +T add_sat (T const & value, T const & diff) noexcept +{ + static_assert (std::numeric_limits::is_specialized, "std::numeric_limits must be specialized"); + return (value > std::numeric_limits::max () - diff) ? std::numeric_limits::max () : value + diff; +} +template +T sub_sat (T const & value, T const & diff) noexcept +{ + static_assert (std::numeric_limits::is_specialized, "std::numeric_limits must be specialized"); + return (value < std::numeric_limits::min () + diff) ? std::numeric_limits::min () : value - diff; +} +template +T inc_sat (T const & value) noexcept +{ + return add_sat (value, static_cast (1)); +} +template +T dec_sat (T const & value) noexcept +{ + return sub_sat (value, static_cast (1)); +} } /* diff --git a/nano/node/backlog_scan.cpp b/nano/node/backlog_scan.cpp index 9d922c042d..880ae8d052 100644 --- a/nano/node/backlog_scan.cpp +++ b/nano/node/backlog_scan.cpp @@ -126,7 +126,7 @@ void nano::backlog_scan::populate_backlog (nano::unique_lock & lock activated.push_back (info); } - next = account.number () + 1; // TODO: Prevent account overflow + next = inc_sat (account.number ()); } done = (it == end); diff --git a/nano/node/bootstrap/crawlers.hpp b/nano/node/bootstrap/crawlers.hpp index a29d53fa9a..dec7ac8de1 100644 --- a/nano/node/bootstrap/crawlers.hpp +++ b/nano/node/bootstrap/crawlers.hpp @@ -134,7 +134,7 @@ struct pending_database_crawler if (it != end) { // If that fails, perform a fresh lookup - seek (starting_account.number () + 1); + seek (inc_sat (starting_account.number ())); } update_current (); diff --git a/nano/node/bootstrap/database_scan.cpp b/nano/node/bootstrap/database_scan.cpp index dc88a9b308..0135129392 100644 --- a/nano/node/bootstrap/database_scan.cpp +++ b/nano/node/bootstrap/database_scan.cpp @@ -78,7 +78,7 @@ std::deque nano::bootstrap::account_database_scanner::next_batch { auto const & [account, info] = crawler.current.value (); result.push_back (account); - next = account.number () + 1; // TODO: Handle account number overflow + next = inc_sat (account.number ()); } // Empty current value indicates the end of the table @@ -106,7 +106,7 @@ std::deque nano::bootstrap::pending_database_scanner::next_batch { auto const & [key, info] = crawler.current.value (); result.push_back (key.account); - next = key.account.number () + 1; // TODO: Handle account number overflow + next = inc_sat (key.account.number ()); } // Empty current value indicates the end of the table diff --git a/nano/node/json_handler.cpp b/nano/node/json_handler.cpp index af496a963e..681fcaba8a 100644 --- a/nano/node/json_handler.cpp +++ b/nano/node/json_handler.cpp @@ -2173,7 +2173,7 @@ void nano::json_handler::delegators () { auto transaction (node.ledger.tx_begin_read ()); boost::property_tree::ptree delegators; - for (auto i (node.store.account.begin (transaction, start_account.number () + 1)), n (node.store.account.end (transaction)); i != n && delegators.size () < count; ++i) + for (auto i (node.store.account.begin (transaction, inc_sat (start_account.number ()))), n (node.store.account.end (transaction)); i != n && delegators.size () < count; ++i) { nano::account_info const & info (i->second); if (info.representative == representative) @@ -4189,7 +4189,7 @@ void nano::json_handler::unopened () break; } // Skip existing accounts - iterator = node.store.pending.begin (transaction, nano::pending_key (account.number () + 1, 0)); + iterator = node.store.pending.begin (transaction, nano::pending_key (inc_sat (account.number ()), 0)); } else { diff --git a/nano/node/node.cpp b/nano/node/node.cpp index 32ddf699bd..62ee9680d7 100644 --- a/nano/node/node.cpp +++ b/nano/node/node.cpp @@ -856,7 +856,7 @@ bool nano::node::collect_ledger_pruning_targets (std::deque & read_operations += depth; if (read_operations >= batch_read_size_a) { - last_account_a = account.number () + 1; + last_account_a = inc_sat (account.number ()); finish_transaction = true; } else