From 5aa3d5e231eb55e604ea92a0a4cd5335e7fc5035 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Thu, 9 Apr 2026 16:08:17 +0100 Subject: [PATCH] refactor: Remove erroneous base_uint ctor from container (#46) --- include/xrpl/basics/base_uint.h | 30 +++++++++++++++++-- src/libxrpl/ledger/View.cpp | 3 +- src/libxrpl/protocol/AccountID.cpp | 4 +-- src/libxrpl/protocol/Indexes.cpp | 2 +- src/libxrpl/protocol/PublicKey.cpp | 2 +- src/libxrpl/protocol/STIssue.cpp | 4 +-- src/libxrpl/protocol/STVector256.cpp | 3 +- src/libxrpl/protocol/Seed.cpp | 2 +- src/test/app/Invariants_test.cpp | 4 +-- src/test/basics/base_uint_test.cpp | 5 ++-- src/test/protocol/STParsedJSON_test.cpp | 10 ++++--- .../ledger/detail/LedgerReplayMsgHandler.cpp | 19 +++++++----- src/xrpld/overlay/detail/PeerImp.cpp | 24 +++++++-------- 13 files changed, 73 insertions(+), 39 deletions(-) diff --git a/include/xrpl/basics/base_uint.h b/include/xrpl/basics/base_uint.h index dc8c34abad..a9c9334a85 100644 --- a/include/xrpl/basics/base_uint.h +++ b/include/xrpl/basics/base_uint.h @@ -66,6 +66,11 @@ struct is_contiguous_container : std::true_type { }; +template +struct always_false_t : std::bool_constant +{ +}; + } // namespace detail /** Integers of any length that is a multiple of 32-bits @@ -292,10 +297,31 @@ public: std::is_trivially_copyable::value>> explicit base_uint(Container const& c) { + // Use always_false_t so the static_assert condition is dependent + // and only triggers when this constructor template is instantiated. + static_assert( + detail::always_false_t::value, + "This constructor is not intended to be used and will be soon " + "removed. " + "Use base_uint::fromRaw instead."); + } + + template < + class Container, + class = std::enable_if_t< + detail::is_contiguous_container::value && + std::is_trivially_copyable::value>> + static base_uint + fromRaw(Container const& c) + { + base_uint result; XRPL_ASSERT( c.size() * sizeof(typename Container::value_type) == size(), - "ripple::base_uint::base_uint(Container auto) : input size match"); - std::memcpy(data_.data(), c.data(), size()); + "xrpl::base_uint::fromRaw(Container auto) : input size match"); + std::size_t const canCopy = + std::min(size(), c.size() * sizeof(typename Container::value_type)); + std::memcpy(result.data_.data(), c.data(), canCopy); + return result; } template diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 465e9ad442..6a8c611fc2 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -1157,7 +1157,8 @@ pseudoAccountAddress(ReadView const& view, uint256 const& pseudoOwnerKey) ripesha_hasher rsh; auto const hash = sha512Half(i, view.info().parentHash, pseudoOwnerKey); rsh(hash.data(), hash.size()); - AccountID const ret{static_cast(rsh)}; + AccountID const ret = + AccountID::fromRaw(static_cast(rsh)); if (!view.read(keylet::account(ret))) return ret; } diff --git a/src/libxrpl/protocol/AccountID.cpp b/src/libxrpl/protocol/AccountID.cpp index 109ac3d028..3f423f3c2c 100644 --- a/src/libxrpl/protocol/AccountID.cpp +++ b/src/libxrpl/protocol/AccountID.cpp @@ -126,7 +126,7 @@ parseBase58(std::string const& s) auto const result = decodeBase58Token(s, TokenType::AccountID); if (result.size() != AccountID::bytes) return std::nullopt; - return AccountID{result}; + return AccountID::fromRaw(result); } //------------------------------------------------------------------------------ @@ -171,7 +171,7 @@ calcAccountID(PublicKey const& pk) ripesha_hasher rsh; rsh(pk.data(), pk.size()); - return AccountID{static_cast(rsh)}; + return AccountID::fromRaw(static_cast(rsh)); } AccountID const& diff --git a/src/libxrpl/protocol/Indexes.cpp b/src/libxrpl/protocol/Indexes.cpp index d87241b719..87a5c50a43 100644 --- a/src/libxrpl/protocol/Indexes.cpp +++ b/src/libxrpl/protocol/Indexes.cpp @@ -406,7 +406,7 @@ nftpage_min(AccountID const& owner) { std::array buf{}; std::memcpy(buf.data(), owner.data(), owner.size()); - return {ltNFTOKEN_PAGE, uint256{buf}}; + return {ltNFTOKEN_PAGE, uint256::fromRaw(buf)}; } Keylet diff --git a/src/libxrpl/protocol/PublicKey.cpp b/src/libxrpl/protocol/PublicKey.cpp index 54b50c80ef..90c449ade5 100644 --- a/src/libxrpl/protocol/PublicKey.cpp +++ b/src/libxrpl/protocol/PublicKey.cpp @@ -323,7 +323,7 @@ calcNodeID(PublicKey const& pk) ripesha_hasher h; h(pk.data(), pk.size()); - return NodeID{static_cast(h)}; + return NodeID::fromRaw(static_cast(h)); } } // namespace ripple diff --git a/src/libxrpl/protocol/STIssue.cpp b/src/libxrpl/protocol/STIssue.cpp index 821e17f6a7..ff4a0195f0 100644 --- a/src/libxrpl/protocol/STIssue.cpp +++ b/src/libxrpl/protocol/STIssue.cpp @@ -46,7 +46,7 @@ STIssue::STIssue(SerialIter& sit, SField const& name) : STBase{name} { auto const currencyOrAccount = sit.get160(); - if (isXRP(static_cast(currencyOrAccount))) + if (isXRP(Currency::fromRaw(currencyOrAccount))) { asset_ = xrpIssue(); } @@ -57,7 +57,7 @@ STIssue::STIssue(SerialIter& sit, SField const& name) : STBase{name} // - 160 bits MPT issuer account // - 160 bits black hole account // - 32 bits sequence - AccountID account = static_cast(sit.get160()); + AccountID account = AccountID::fromRaw(sit.get160()); // MPT if (noAccount() == account) { diff --git a/src/libxrpl/protocol/STVector256.cpp b/src/libxrpl/protocol/STVector256.cpp index 3612b0cc4d..16bff7ea7c 100644 --- a/src/libxrpl/protocol/STVector256.cpp +++ b/src/libxrpl/protocol/STVector256.cpp @@ -47,7 +47,8 @@ STVector256::STVector256(SerialIter& sit, SField const& name) : STBase(name) mValue.reserve(cnt); for (std::size_t i = 0; i != cnt; ++i) - mValue.emplace_back(slice.substr(i * uint256::size(), uint256::size())); + mValue.push_back(uint256::fromRaw( + slice.substr(i * uint256::size(), uint256::size()))); } STBase* diff --git a/src/libxrpl/protocol/Seed.cpp b/src/libxrpl/protocol/Seed.cpp index 095c09e7c0..0846e7bb44 100644 --- a/src/libxrpl/protocol/Seed.cpp +++ b/src/libxrpl/protocol/Seed.cpp @@ -124,7 +124,7 @@ parseGenericSeed(std::string const& str, bool rfc1751) if (RFC1751::getKeyFromEnglish(key, str) == 1) { Blob const blob(key.rbegin(), key.rend()); - return Seed{uint128{blob}}; + return Seed{uint128::fromRaw(blob)}; } } diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 3dfac7bdeb..6ac8d4bf2c 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -3618,8 +3618,8 @@ class Invariants_test : public beast::unit_test::suite sleShares->setFieldU64(sfOwnerNode, *sharesPage); sleShares->at(sfFlags) = 0; - // Setting wrong pseudo acocunt ID - sleShares->at(sfIssuer) = AccountID(uint160(42)); + // Setting wrong pseudo account ID + sleShares->at(sfIssuer) = AccountID(42); sleShares->at(sfOutstandingAmount) = 0; sleShares->at(sfSequence) = sequence; diff --git a/src/test/basics/base_uint_test.cpp b/src/test/basics/base_uint_test.cpp index 458032d203..1dfe7a54ef 100644 --- a/src/test/basics/base_uint_test.cpp +++ b/src/test/basics/base_uint_test.cpp @@ -148,7 +148,7 @@ struct base_uint_test : beast::unit_test::suite Blob raw{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12}; BEAST_EXPECT(test96::bytes == raw.size()); - test96 u{raw}; + test96 u = test96::fromRaw(raw); uset.insert(u); BEAST_EXPECT(raw.size() == u.size()); BEAST_EXPECT(to_string(u) == "0102030405060708090A0B0C"); @@ -169,7 +169,8 @@ struct base_uint_test : beast::unit_test::suite // back into another base_uint (w) for comparison with the original nonhash<96> h; hash_append(h, u); - test96 w{std::vector(h.data_.begin(), h.data_.end())}; + test96 w = test96::fromRaw( + std::vector(h.data_.begin(), h.data_.end())); BEAST_EXPECT(w == u); test96 v{~u}; diff --git a/src/test/protocol/STParsedJSON_test.cpp b/src/test/protocol/STParsedJSON_test.cpp index dfdecb30ef..a27833c81c 100644 --- a/src/test/protocol/STParsedJSON_test.cpp +++ b/src/test/protocol/STParsedJSON_test.cpp @@ -374,7 +374,8 @@ class STParsedJSON_test : public beast::unit_test::suite 0xCD, 0xEF}; BEAST_EXPECT( - obj.object->getFieldH128(sfEmailHash) == uint128{expected}); + obj.object->getFieldH128(sfEmailHash) == + uint128::fromRaw(expected)); } // Valid lowercase hex string for UInt128 @@ -467,7 +468,7 @@ class STParsedJSON_test : public beast::unit_test::suite 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x01, 0x23, 0x45, 0x67}; BEAST_EXPECT( obj.object->getFieldH160(sfTakerPaysCurrency) == - uint160{expected}); + uint160::fromRaw(expected)); } // Valid lowercase hex string for UInt160 { @@ -555,7 +556,7 @@ class STParsedJSON_test : public beast::unit_test::suite 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}; BEAST_EXPECT( obj.object->getFieldH192(sfMPTokenIssuanceID) == - uint192{expected}); + uint192::fromRaw(expected)); } // Valid lowercase hex string for UInt192 @@ -655,7 +656,8 @@ class STParsedJSON_test : public beast::unit_test::suite 0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF, 0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF}; BEAST_EXPECT( - obj.object->getFieldH256(sfLedgerHash) == uint256{expected}); + obj.object->getFieldH256(sfLedgerHash) == + uint256::fromRaw(expected)); } // Valid lowercase hex string for UInt256 { diff --git a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp index 5bfad61bf4..ce4d7ee535 100644 --- a/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp +++ b/src/xrpld/app/ledger/detail/LedgerReplayMsgHandler.cpp @@ -56,8 +56,8 @@ LedgerReplayMsgHandler::processProofPathRequest( reply.set_ledgerhash(packet.ledgerhash()); reply.set_type(packet.type()); - uint256 const key(packet.key()); - uint256 const ledgerHash(packet.ledgerhash()); + uint256 const key = uint256::fromRaw(packet.key()); + uint256 const ledgerHash = uint256::fromRaw(packet.ledgerhash()); auto ledger = app_.getLedgerMaster().getLedgerByHash(ledgerHash); if (!ledger) { @@ -110,7 +110,9 @@ LedgerReplayMsgHandler::processProofPathResponse( protocol::TMProofPathResponse& reply = *msg; if (reply.has_error() || !reply.has_key() || !reply.has_ledgerhash() || !reply.has_type() || !reply.has_ledgerheader() || - reply.path_size() == 0) + reply.path_size() == 0 || + reply.ledgerhash().size() != uint256::size() || + reply.key().size() != uint256::size()) { JLOG(journal_.debug()) << "Bad message: Error reply"; return false; @@ -126,7 +128,7 @@ LedgerReplayMsgHandler::processProofPathResponse( // deserialize the header auto info = deserializeHeader( {reply.ledgerheader().data(), reply.ledgerheader().size()}); - uint256 replyHash(reply.ledgerhash()); + uint256 replyHash = uint256::fromRaw(reply.ledgerhash()); if (calculateLedgerHash(info) != replyHash) { JLOG(journal_.debug()) << "Bad message: Hash mismatch"; @@ -134,7 +136,7 @@ LedgerReplayMsgHandler::processProofPathResponse( } info.hash = replyHash; - uint256 key(reply.key()); + uint256 key = uint256::fromRaw(reply.key()); if (key != keylet::skip().key) { JLOG(journal_.debug()) @@ -192,7 +194,7 @@ LedgerReplayMsgHandler::processReplayDeltaRequest( } reply.set_ledgerhash(packet.ledgerhash()); - uint256 const ledgerHash{packet.ledgerhash()}; + uint256 const ledgerHash = uint256::fromRaw(packet.ledgerhash()); auto ledger = app_.getLedgerMaster().getLedgerByHash(ledgerHash); if (!ledger || !ledger->isImmutable()) { @@ -223,7 +225,8 @@ LedgerReplayMsgHandler::processReplayDeltaResponse( std::shared_ptr const& msg) { protocol::TMReplayDeltaResponse& reply = *msg; - if (reply.has_error() || !reply.has_ledgerheader()) + if (reply.has_error() || !reply.has_ledgerheader() || + !reply.has_ledgerhash() || reply.ledgerhash().size() != uint256::size()) { JLOG(journal_.debug()) << "Bad message: Error reply"; return false; @@ -231,7 +234,7 @@ LedgerReplayMsgHandler::processReplayDeltaResponse( auto info = deserializeHeader( {reply.ledgerheader().data(), reply.ledgerheader().size()}); - uint256 replyHash(reply.ledgerhash()); + uint256 replyHash = uint256::fromRaw(reply.ledgerhash()); if (calculateLedgerHash(info) != replyHash) { JLOG(journal_.debug()) << "Bad message: Hash mismatch"; diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 6de8ee909f..bc9c615946 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -164,7 +164,7 @@ PeerImp::run() return ret; if (auto const s = base64_decode(value); s.size() == uint256::size()) - return uint256{s}; + return uint256::fromRaw(s); return std::nullopt; }; @@ -1657,7 +1657,7 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } - uint256 const ledgerHash{m->ledgerhash()}; + uint256 const ledgerHash = uint256::fromRaw(m->ledgerhash()); // Otherwise check if received data for a candidate transaction set if (m->type() == protocol::liTS_CANDIDATE) @@ -1725,8 +1725,8 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } - uint256 const proposeHash{set.currenttxhash()}; - uint256 const prevLedger{set.previousledger()}; + uint256 const proposeHash = uint256::fromRaw(set.currenttxhash()); + uint256 const prevLedger = uint256::fromRaw(set.previousledger()); NetClock::time_point const closeTime{NetClock::duration{set.closetime()}}; @@ -2024,7 +2024,7 @@ PeerImp::onMessage(std::shared_ptr const& m) return; } - uint256 const hash{m->hash()}; + uint256 const hash = uint256::fromRaw(m->hash()); if (m->status() == protocol::tsHAVE) { @@ -2515,7 +2515,7 @@ PeerImp::onMessage(std::shared_ptr const& m) auto const& obj = packet.objects(i); if (obj.has_hash() && stringIsUint256Sized(obj.hash())) { - uint256 const hash{obj.hash()}; + uint256 const hash = uint256::fromRaw(obj.hash()); // VFALCO TODO Move this someplace more sensible so we dont // need to inject the NodeStore interfaces. std::uint32_t seq{obj.has_ledgerseq() ? obj.ledgerseq() : 0}; @@ -2589,7 +2589,7 @@ PeerImp::onMessage(std::shared_ptr const& m) if (pLDo) { - uint256 const hash{obj.hash()}; + uint256 const hash = uint256::fromRaw(obj.hash()); app_.getLedgerMaster().addFetchPack( hash, @@ -2648,7 +2648,7 @@ PeerImp::handleHaveTransactions( return; } - uint256 hash(m->hashes(i)); + uint256 hash = uint256::fromRaw(m->hashes(i)); auto txn = app_.getMasterTransaction().fetch_from_cache(hash); @@ -2786,7 +2786,7 @@ PeerImp::doFetchPack(std::shared_ptr const& packet) fee_.fee = Resource::feeHeavyBurdenPeer; - uint256 const hash{packet->ledgerhash()}; + uint256 const hash = uint256::fromRaw(packet->ledgerhash()); std::weak_ptr weak = shared_from_this(); auto elapsed = UptimeClock::now(); @@ -2823,7 +2823,7 @@ PeerImp::doTransactions( return; } - uint256 hash(obj.hash()); + uint256 hash = uint256::fromRaw(obj.hash()); auto txn = app_.getMasterTransaction().fetch_from_cache(hash); @@ -3195,7 +3195,7 @@ PeerImp::getLedger(std::shared_ptr const& m) if (m->has_ledgerhash()) { // Attempt to find ledger by hash - uint256 const ledgerHash{m->ledgerhash()}; + uint256 const ledgerHash = uint256::fromRaw(m->ledgerhash()); ledger = app_.getLedgerMaster().getLedgerByHash(ledgerHash); if (!ledger) { @@ -3286,7 +3286,7 @@ PeerImp::getTxSet(std::shared_ptr const& m) const { JLOG(p_journal_.trace()) << "getTxSet: TX set"; - uint256 const txSetHash{m->ledgerhash()}; + uint256 const txSetHash = uint256::fromRaw(m->ledgerhash()); std::shared_ptr shaMap{ app_.getInboundTransactions().getSet(txSetHash, false)}; if (!shaMap)