Skip to content

Commit b62abc7

Browse files
committed
Merge bitcoin#34436: refactor: add overflow-safe CeilDiv helper and use it in unsigned callsites
02d047f refactor: add overflow-safe `CeilDiv` helper (Lőrinc) Pull request description: ### Problem The codebase has many open-coded ceiling-division expressions (for example `(x+y-1)/y`) scattered across files. These are less readable, duplicate logic, and can be overflow-prone in edge cases. ### Fix Introduce a small overflow-safe integer helper, `CeilDiv()`, and use it in existing **unsigned** callsites where the conversion is straightforward and noise-free. ### What this PR does * Adds `CeilDiv()` to `src/util/overflow.h` for unsigned integral inputs. * Keeps the precondition check `assert(divisor > 0)`. * Replaces selected unsigned ceiling-division expressions with `CeilDiv(...)`. * Adds focused unit tests in `src/test/util_tests.cpp` for the migrated patterns. --- This is a pure refactor with no intended behavioral change. Signed arithmetic callsites are intentionally left unchanged in this PR. This PR changed a few more things originally but based on feedback reverted to the simplest cases only. ACKs for top commit: rustaceanrob: ACK 02d047f hodlinator: ACK 02d047f sedited: ACK 02d047f Tree-SHA512: b09336031f487e6ce289822e0ffeb8cfc8cfe8a2f4f3f49470748dfbd0a6cbab97498674cb8686dd2bd4ab6dd0b79cfdf2da00041fee12d109892e1bc5dde0ff
2 parents 7c21413 + 02d047f commit b62abc7

File tree

15 files changed

+85
-19
lines changed

15 files changed

+85
-19
lines changed

src/arith_uint256.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,9 @@
55

66
#include <arith_uint256.h>
77

8-
#include <uint256.h>
98
#include <crypto/common.h>
9+
#include <uint256.h>
10+
#include <util/overflow.h>
1011

1112
#include <cassert>
1213

@@ -194,7 +195,7 @@ arith_uint256& arith_uint256::SetCompact(uint32_t nCompact, bool* pfNegative, bo
194195

195196
uint32_t arith_uint256::GetCompact(bool fNegative) const
196197
{
197-
int nSize = (bits() + 7) / 8;
198+
int nSize = CeilDiv(bits(), 8u);
198199
uint32_t nCompact = 0;
199200
if (nSize <= 3) {
200201
nCompact = GetLow64() << 8 * (3 - nSize);

src/common/bloom.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include <span.h>
1313
#include <streams.h>
1414
#include <util/fastrange.h>
15+
#include <util/overflow.h>
1516

1617
#include <algorithm>
1718
#include <cmath>
@@ -166,7 +167,7 @@ CRollingBloomFilter::CRollingBloomFilter(const unsigned int nElements, const dou
166167
* restrict it to the range 1-50. */
167168
nHashFuncs = std::max(1, std::min((int)round(logFpRate / log(0.5)), 50));
168169
/* In this rolling bloom filter, we'll store between 2 and 3 generations of nElements / 2 entries. */
169-
nEntriesPerGeneration = (nElements + 1) / 2;
170+
nEntriesPerGeneration = CeilDiv(nElements, 2u);
170171
uint32_t nMaxElements = nEntriesPerGeneration * 3;
171172
/* The maximum fpRate = pow(1.0 - exp(-nHashFuncs * nMaxElements / nFilterBits), nHashFuncs)
172173
* => pow(fpRate, 1.0 / nHashFuncs) = 1.0 - exp(-nHashFuncs * nMaxElements / nFilterBits)
@@ -182,7 +183,7 @@ CRollingBloomFilter::CRollingBloomFilter(const unsigned int nElements, const dou
182183
* treated as set in generation 1, 2, or 3 respectively.
183184
* These bits are stored in separate integers: position P corresponds to bit
184185
* (P & 63) of the integers data[(P >> 6) * 2] and data[(P >> 6) * 2 + 1]. */
185-
data.resize(((nFilterBits + 63) / 64) << 1);
186+
data.resize(CeilDiv(nFilterBits, 64u) << 1);
186187
reset();
187188
}
188189

src/cuckoocache.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#define BITCOIN_CUCKOOCACHE_H
77

88
#include <util/fastrange.h>
9+
#include <util/overflow.h>
910

1011
#include <algorithm>
1112
#include <array>
@@ -63,7 +64,7 @@ class bit_packed_atomic_flags
6364
explicit bit_packed_atomic_flags(uint32_t size)
6465
{
6566
// pad out the size if needed
66-
size = (size + 7) / 8;
67+
size = CeilDiv(size, 8u);
6768
mem.reset(new std::atomic<uint8_t>[size]);
6869
for (uint32_t i = 0; i < size; ++i)
6970
mem[i].store(0xFF);

src/flatfile.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include <tinyformat.h>
99
#include <util/fs_helpers.h>
1010
#include <util/log.h>
11+
#include <util/overflow.h>
1112

1213
#include <stdexcept>
1314

@@ -59,8 +60,8 @@ size_t FlatFileSeq::Allocate(const FlatFilePos& pos, size_t add_size, bool& out_
5960
{
6061
out_of_space = false;
6162

62-
unsigned int n_old_chunks = (pos.nPos + m_chunk_size - 1) / m_chunk_size;
63-
unsigned int n_new_chunks = (pos.nPos + add_size + m_chunk_size - 1) / m_chunk_size;
63+
unsigned int n_old_chunks = CeilDiv(pos.nPos, m_chunk_size);
64+
unsigned int n_new_chunks = CeilDiv(pos.nPos + add_size, m_chunk_size);
6465
if (n_new_chunks > n_old_chunks) {
6566
size_t old_size = pos.nPos;
6667
size_t new_size = n_new_chunks * m_chunk_size;

src/key_io.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <script/interpreter.h>
1010
#include <script/solver.h>
1111
#include <tinyformat.h>
12+
#include <util/overflow.h>
1213
#include <util/strencodings.h>
1314

1415
#include <algorithm>
@@ -72,7 +73,7 @@ class DestinationEncoder
7273
return {};
7374
}
7475
std::vector<unsigned char> data = {(unsigned char)id.GetWitnessVersion()};
75-
data.reserve(1 + (program.size() * 8 + 4) / 5);
76+
data.reserve(1 + CeilDiv(program.size() * 8, 5u));
7677
ConvertBits<8, 5, true>([&](unsigned char c) { data.push_back(c); }, program.begin(), program.end());
7778
return bech32::Encode(bech32::Encoding::BECH32M, m_params.Bech32HRP(), data);
7879
}

src/merkleblock.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,14 @@
55

66
#include <merkleblock.h>
77

8-
#include <hash.h>
98
#include <consensus/consensus.h>
9+
#include <hash.h>
10+
#include <util/overflow.h>
1011

1112

1213
std::vector<unsigned char> BitsToBytes(const std::vector<bool>& bits)
1314
{
14-
std::vector<unsigned char> ret((bits.size() + 7) / 8);
15+
std::vector<unsigned char> ret(CeilDiv(bits.size(), 8u));
1516
for (unsigned int p = 0; p < bits.size(); p++) {
1617
ret[p / 8] |= bits[p] << (p % 8);
1718
}
@@ -174,7 +175,7 @@ uint256 CPartialMerkleTree::ExtractMatches(std::vector<Txid> &vMatch, std::vecto
174175
if (fBad)
175176
return uint256();
176177
// verify that all bits were consumed (except for the padding caused by serializing it as a byte sequence)
177-
if ((nBitsUsed+7)/8 != (vBits.size()+7)/8)
178+
if (CeilDiv(nBitsUsed, 8u) != CeilDiv(vBits.size(), 8u))
178179
return uint256();
179180
// verify that all hashes were consumed
180181
if (nHashUsed != vHash.size())

src/rest.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include <undo.h>
2929
#include <util/any.h>
3030
#include <util/check.h>
31+
#include <util/overflow.h>
3132
#include <util/strencodings.h>
3233
#include <validation.h>
3334

@@ -993,7 +994,7 @@ static bool rest_getutxos(const std::any& context, HTTPRequest* req, const std::
993994
std::vector<CCoin> outs;
994995
std::string bitmapStringRepresentation;
995996
std::vector<bool> hits;
996-
bitmap.resize((vOutPoints.size() + 7) / 8);
997+
bitmap.resize(CeilDiv(vOutPoints.size(), 8u));
997998
ChainstateManager* maybe_chainman = GetChainman(context, req);
998999
if (!maybe_chainman) return false;
9991000
ChainstateManager& chainman = *maybe_chainman;

src/serialize.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
#include <compat/endian.h>
1212
#include <prevector.h>
1313
#include <span.h>
14+
#include <util/overflow.h>
1415

1516
#include <algorithm>
1617
#include <concepts>
@@ -425,7 +426,7 @@ template<typename Stream, VarIntMode Mode, typename I>
425426
void WriteVarInt(Stream& os, I n)
426427
{
427428
CheckVarIntMode<Mode, I>();
428-
unsigned char tmp[(sizeof(n)*8+6)/7];
429+
unsigned char tmp[CeilDiv(sizeof(n) * 8, 7u)];
429430
int len=0;
430431
while(true) {
431432
tmp[len] = (n & 0x7F) | (len ? 0x80 : 0x00);

src/support/allocators/pool.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include <utility>
1616

1717
#include <util/check.h>
18+
#include <util/overflow.h>
1819

1920
/**
2021
* A memory resource similar to std::pmr::unsynchronized_pool_resource, but
@@ -127,7 +128,7 @@ class PoolResource final
127128
*/
128129
[[nodiscard]] static constexpr std::size_t NumElemAlignBytes(std::size_t bytes)
129130
{
130-
return (bytes + ELEM_ALIGN_BYTES - 1) / ELEM_ALIGN_BYTES + (bytes == 0);
131+
return CeilDiv(bytes, ELEM_ALIGN_BYTES) + (bytes == 0);
131132
}
132133

133134
/**

src/test/util_tests.cpp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1834,4 +1834,41 @@ BOOST_AUTO_TEST_CASE(mib_string_literal_test)
18341834
BOOST_CHECK_EXCEPTION(operator""_MiB(static_cast<unsigned long long>(max_mib) + 1), std::overflow_error, HasReason("MiB value too large for size_t byte conversion"));
18351835
}
18361836

1837+
BOOST_AUTO_TEST_CASE(ceil_div_test)
1838+
{
1839+
// Type combinations used by current CeilDiv callsites.
1840+
BOOST_CHECK((std::is_same_v<decltype(CeilDiv(uint32_t{0}, 8u)), uint32_t>));
1841+
BOOST_CHECK((std::is_same_v<decltype(CeilDiv(size_t{0}, 8u)), size_t>));
1842+
BOOST_CHECK((std::is_same_v<decltype(CeilDiv(unsigned{0}, size_t{1})), size_t>));
1843+
1844+
// `common/bloom.cpp` and `cuckoocache.h` patterns.
1845+
BOOST_CHECK_EQUAL(CeilDiv(uint32_t{3}, 2u), uint32_t{2});
1846+
BOOST_CHECK_EQUAL(CeilDiv(uint32_t{65}, 64u), uint32_t{2});
1847+
BOOST_CHECK_EQUAL(CeilDiv(uint32_t{9}, 8u), uint32_t{2});
1848+
1849+
// `key_io.cpp`, `rest.cpp`, `merkleblock.cpp`, `strencodings.cpp` patterns.
1850+
BOOST_CHECK_EQUAL(CeilDiv(size_t{9}, 8u), size_t{2});
1851+
BOOST_CHECK_EQUAL(CeilDiv(size_t{10}, 3u), size_t{4});
1852+
BOOST_CHECK_EQUAL(CeilDiv(size_t{11}, 5u), size_t{3});
1853+
BOOST_CHECK_EQUAL(CeilDiv(size_t{41} * 8, 5u), size_t{66});
1854+
1855+
// `flatfile.cpp` mixed unsigned/size_t pattern.
1856+
BOOST_CHECK_EQUAL(CeilDiv(unsigned{10}, size_t{4}), size_t{3});
1857+
1858+
// `util/feefrac.h` fast-path rounding-up pattern.
1859+
constexpr int64_t fee{12345};
1860+
constexpr int32_t at_size{67};
1861+
constexpr int32_t size{10};
1862+
BOOST_CHECK_EQUAL(CeilDiv(uint64_t(fee) * at_size, uint32_t(size)),
1863+
(uint64_t(fee) * at_size + uint32_t(size) - 1) / uint32_t(size));
1864+
1865+
// `bitset.h` template parameter pattern.
1866+
constexpr unsigned bits{129};
1867+
constexpr size_t digits{std::numeric_limits<size_t>::digits};
1868+
BOOST_CHECK_EQUAL(CeilDiv(bits, digits), (bits + digits - 1) / digits);
1869+
1870+
// `serialize.h` varint scratch-buffer pattern.
1871+
BOOST_CHECK_EQUAL(CeilDiv(sizeof(uint64_t) * 8, 7u), (sizeof(uint64_t) * 8 + 6) / 7);
1872+
}
1873+
18371874
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)