From 9ba4e8d3ba86ed04512e3d16306dc698ace09ca1 Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Wed, 23 Jul 2025 13:03:12 +0100 Subject: [PATCH] refactor: Makes HashRouter flags more type-safe (#5371) This change addresses the issue #5336: Refactor HashRouter flags to be more type-safe. * Switched numeric flags to enum type. * Updated unit tests --- Builds/levelization/results/loops.txt | 2 +- src/test/app/Batch_test.cpp | 10 +- src/test/app/HashRouter_test.cpp | 127 ++++++++++++++++-------- src/test/app/Import_test.cpp | 3 +- src/test/app/XahauGenesis_test.cpp | 6 +- src/test/consensus/UNLReport_test.cpp | 21 ++-- src/test/rpc/LedgerData_test.cpp | 3 +- src/xrpld/app/hook/detail/applyHook.cpp | 2 +- src/xrpld/app/ledger/Ledger.cpp | 3 +- src/xrpld/app/misc/HashRouter.cpp | 14 ++- src/xrpld/app/misc/HashRouter.h | 85 ++++++++++++---- src/xrpld/app/misc/NetworkOPs.cpp | 21 ++-- src/xrpld/app/misc/detail/TxQ.cpp | 20 ++-- src/xrpld/app/tx/detail/Escrow.cpp | 14 +-- src/xrpld/app/tx/detail/Transactor.cpp | 13 +-- src/xrpld/app/tx/detail/apply.cpp | 29 +++--- src/xrpld/overlay/detail/PeerImp.cpp | 25 +++-- src/xrpld/overlay/detail/PeerImp.h | 3 +- 18 files changed, 265 insertions(+), 136 deletions(-) diff --git a/Builds/levelization/results/loops.txt b/Builds/levelization/results/loops.txt index a826d14dd..af1293919 100644 --- a/Builds/levelization/results/loops.txt +++ b/Builds/levelization/results/loops.txt @@ -26,7 +26,7 @@ Loop: xrpld.app xrpld.nodestore xrpld.app > xrpld.nodestore Loop: xrpld.app xrpld.overlay - xrpld.overlay ~= xrpld.app + xrpld.overlay > xrpld.app Loop: xrpld.app xrpld.peerfinder xrpld.peerfinder ~= xrpld.app diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index ca5d6bfd8..3b4c76135 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -3661,14 +3661,18 @@ class Batch_test : public beast::unit_test::suite { // Submit a tx with tfInnerBatchTxn uint256 const txBad = submitTx(tfInnerBatchTxn); - BEAST_EXPECT(env.app().getHashRouter().getFlags(txBad) == 0); + BEAST_EXPECT( + env.app().getHashRouter().getFlags(txBad) == + HashRouterFlags::UNDEFINED); } // Validate: NetworkOPs::processTransaction() { uint256 const txid = processTxn(tfInnerBatchTxn); - // HashRouter::getFlags() should return SF_BAD - BEAST_EXPECT(env.app().getHashRouter().getFlags(txid) == SF_BAD); + // HashRouter::getFlags() should return LedgerFlags::BAD + BEAST_EXPECT( + env.app().getHashRouter().getFlags(txid) == + HashRouterFlags::BAD); } } diff --git a/src/test/app/HashRouter_test.cpp b/src/test/app/HashRouter_test.cpp index 0737116f1..44170e152 100644 --- a/src/test/app/HashRouter_test.cpp +++ b/src/test/app/HashRouter_test.cpp @@ -45,15 +45,19 @@ class HashRouter_test : public beast::unit_test::suite TestStopwatch stopwatch; HashRouter router(getSetup(2s, 1s), stopwatch); - uint256 const key1(1); - uint256 const key2(2); - uint256 const key3(3); + HashRouterFlags key1(HashRouterFlags::PRIVATE1); + HashRouterFlags key2(HashRouterFlags::PRIVATE2); + HashRouterFlags key3(HashRouterFlags::PRIVATE3); + + auto const ukey1 = uint256{static_cast(key1)}; + auto const ukey2 = uint256{static_cast(key2)}; + auto const ukey3 = uint256{static_cast(key3)}; // t=0 - router.setFlags(key1, 11111); - BEAST_EXPECT(router.getFlags(key1) == 11111); - router.setFlags(key2, 22222); - BEAST_EXPECT(router.getFlags(key2) == 22222); + router.setFlags(ukey1, HashRouterFlags::PRIVATE1); + BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::PRIVATE1); + router.setFlags(ukey2, HashRouterFlags::PRIVATE2); + BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE2); // key1 : 0 // key2 : 0 // key3: null @@ -62,7 +66,7 @@ class HashRouter_test : public beast::unit_test::suite // Because we are accessing key1 here, it // will NOT be expired for another two ticks - BEAST_EXPECT(router.getFlags(key1) == 11111); + BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::PRIVATE1); // key1 : 1 // key2 : 0 // key3 null @@ -70,9 +74,9 @@ class HashRouter_test : public beast::unit_test::suite ++stopwatch; // t=3 - router.setFlags(key3, 33333); // force expiration - BEAST_EXPECT(router.getFlags(key1) == 11111); - BEAST_EXPECT(router.getFlags(key2) == 0); + router.setFlags(ukey3, HashRouterFlags::PRIVATE3); // force expiration + BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::PRIVATE1); + BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::UNDEFINED); } void @@ -83,15 +87,21 @@ class HashRouter_test : public beast::unit_test::suite TestStopwatch stopwatch; HashRouter router(getSetup(2s, 1s), stopwatch); - uint256 const key1(1); - uint256 const key2(2); - uint256 const key3(3); - uint256 const key4(4); + HashRouterFlags key1(HashRouterFlags::PRIVATE1); + HashRouterFlags key2(HashRouterFlags::PRIVATE2); + HashRouterFlags key3(HashRouterFlags::PRIVATE3); + HashRouterFlags key4(HashRouterFlags::PRIVATE4); + + auto const ukey1 = uint256{static_cast(key1)}; + auto const ukey2 = uint256{static_cast(key2)}; + auto const ukey3 = uint256{static_cast(key3)}; + auto const ukey4 = uint256{static_cast(key4)}; + BEAST_EXPECT(key1 != key2 && key2 != key3 && key3 != key4); // t=0 - router.setFlags(key1, 12345); - BEAST_EXPECT(router.getFlags(key1) == 12345); + router.setFlags(ukey1, HashRouterFlags::BAD); + BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::BAD); // key1 : 0 // key2 : null // key3 : null @@ -103,26 +113,27 @@ class HashRouter_test : public beast::unit_test::suite // so key1 will be expired after the second // call to setFlags. // t=1 - router.setFlags(key2, 9999); - BEAST_EXPECT(router.getFlags(key1) == 12345); - BEAST_EXPECT(router.getFlags(key2) == 9999); + + router.setFlags(ukey2, HashRouterFlags::PRIVATE5); + BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::BAD); + BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE5); // key1 : 1 // key2 : 1 // key3 : null ++stopwatch; // t=2 - BEAST_EXPECT(router.getFlags(key2) == 9999); + BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE5); // key1 : 1 // key2 : 2 // key3 : null ++stopwatch; // t=3 - router.setFlags(key3, 2222); - BEAST_EXPECT(router.getFlags(key1) == 0); - BEAST_EXPECT(router.getFlags(key2) == 9999); - BEAST_EXPECT(router.getFlags(key3) == 2222); + router.setFlags(ukey3, HashRouterFlags::BAD); + BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::UNDEFINED); + BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE5); + BEAST_EXPECT(router.getFlags(ukey3) == HashRouterFlags::BAD); // key1 : 3 // key2 : 3 // key3 : 3 @@ -130,10 +141,10 @@ class HashRouter_test : public beast::unit_test::suite ++stopwatch; // t=4 // No insertion, no expiration - router.setFlags(key1, 7654); - BEAST_EXPECT(router.getFlags(key1) == 7654); - BEAST_EXPECT(router.getFlags(key2) == 9999); - BEAST_EXPECT(router.getFlags(key3) == 2222); + router.setFlags(ukey1, HashRouterFlags::SAVED); + BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::SAVED); + BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE5); + BEAST_EXPECT(router.getFlags(ukey3) == HashRouterFlags::BAD); // key1 : 4 // key2 : 4 // key3 : 4 @@ -142,11 +153,11 @@ class HashRouter_test : public beast::unit_test::suite ++stopwatch; // t=6 - router.setFlags(key4, 7890); - BEAST_EXPECT(router.getFlags(key1) == 0); - BEAST_EXPECT(router.getFlags(key2) == 0); - BEAST_EXPECT(router.getFlags(key3) == 0); - BEAST_EXPECT(router.getFlags(key4) == 7890); + router.setFlags(ukey4, HashRouterFlags::TRUSTED); + BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::UNDEFINED); + BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::UNDEFINED); + BEAST_EXPECT(router.getFlags(ukey3) == HashRouterFlags::UNDEFINED); + BEAST_EXPECT(router.getFlags(ukey4) == HashRouterFlags::TRUSTED); // key1 : 6 // key2 : 6 // key3 : 6 @@ -168,18 +179,18 @@ class HashRouter_test : public beast::unit_test::suite uint256 const key4(4); BEAST_EXPECT(key1 != key2 && key2 != key3 && key3 != key4); - int flags = 12345; // This value is ignored + HashRouterFlags flags(HashRouterFlags::BAD); // This value is ignored router.addSuppression(key1); BEAST_EXPECT(router.addSuppressionPeer(key2, 15)); BEAST_EXPECT(router.addSuppressionPeer(key3, 20, flags)); - BEAST_EXPECT(flags == 0); + BEAST_EXPECT(flags == HashRouterFlags::UNDEFINED); ++stopwatch; BEAST_EXPECT(!router.addSuppressionPeer(key1, 2)); BEAST_EXPECT(!router.addSuppressionPeer(key2, 3)); BEAST_EXPECT(!router.addSuppressionPeer(key3, 4, flags)); - BEAST_EXPECT(flags == 0); + BEAST_EXPECT(flags == HashRouterFlags::UNDEFINED); BEAST_EXPECT(router.addSuppressionPeer(key4, 5)); } @@ -192,9 +203,9 @@ class HashRouter_test : public beast::unit_test::suite HashRouter router(getSetup(2s, 1s), stopwatch); uint256 const key1(1); - BEAST_EXPECT(router.setFlags(key1, 10)); - BEAST_EXPECT(!router.setFlags(key1, 10)); - BEAST_EXPECT(router.setFlags(key1, 20)); + BEAST_EXPECT(router.setFlags(key1, HashRouterFlags::PRIVATE1)); + BEAST_EXPECT(!router.setFlags(key1, HashRouterFlags::PRIVATE1)); + BEAST_EXPECT(router.setFlags(key1, HashRouterFlags::PRIVATE2)); } void @@ -250,7 +261,7 @@ class HashRouter_test : public beast::unit_test::suite HashRouter router(getSetup(5s, 1s), stopwatch); uint256 const key(1); HashRouter::PeerShortID peer = 1; - int flags; + HashRouterFlags flags; BEAST_EXPECT(router.shouldProcess(key, peer, flags, 1s)); BEAST_EXPECT(!router.shouldProcess(key, peer, flags, 1s)); @@ -364,6 +375,39 @@ class HashRouter_test : public beast::unit_test::suite } } + void + testFlagsOps() + { + testcase("Bitwise Operations"); + + using HF = HashRouterFlags; + using UHF = std::underlying_type_t; + + HF f1 = HF::BAD; + HF f2 = HF::SAVED; + HF combined = f1 | f2; + + BEAST_EXPECT( + static_cast(combined) == + (static_cast(f1) | static_cast(f2))); + + HF temp = f1; + temp |= f2; + BEAST_EXPECT(temp == combined); + + HF intersect = combined & f1; + BEAST_EXPECT(intersect == f1); + + HF temp2 = combined; + temp2 &= f1; + BEAST_EXPECT(temp2 == f1); + + BEAST_EXPECT(any(f1)); + BEAST_EXPECT(any(f2)); + BEAST_EXPECT(any(combined)); + BEAST_EXPECT(!any(HF::UNDEFINED)); + } + public: void run() override @@ -375,6 +419,7 @@ public: testRelay(); testProcess(); testSetup(); + testFlagsOps(); } }; diff --git a/src/test/app/Import_test.cpp b/src/test/app/Import_test.cpp index b92d89d46..f55d0e341 100644 --- a/src/test/app/Import_test.cpp +++ b/src/test/app/Import_test.cpp @@ -5512,7 +5512,8 @@ class Import_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); diff --git a/src/test/app/XahauGenesis_test.cpp b/src/test/app/XahauGenesis_test.cpp index 93605ae41..1b99db015 100644 --- a/src/test/app/XahauGenesis_test.cpp +++ b/src/test/app/XahauGenesis_test.cpp @@ -187,7 +187,8 @@ struct XahauGenesis_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; @@ -3718,7 +3719,8 @@ struct XahauGenesis_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); diff --git a/src/test/consensus/UNLReport_test.cpp b/src/test/consensus/UNLReport_test.cpp index d5a46c35b..e4184a2d8 100644 --- a/src/test/consensus/UNLReport_test.cpp +++ b/src/test/consensus/UNLReport_test.cpp @@ -207,7 +207,8 @@ class UNLReport_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); @@ -239,7 +240,8 @@ class UNLReport_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); @@ -260,7 +262,8 @@ class UNLReport_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); @@ -317,7 +320,8 @@ class UNLReport_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); @@ -349,7 +353,8 @@ class UNLReport_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); @@ -373,7 +378,8 @@ class UNLReport_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); @@ -432,7 +438,8 @@ class UNLReport_test : public beast::unit_test::suite uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); diff --git a/src/test/rpc/LedgerData_test.cpp b/src/test/rpc/LedgerData_test.cpp index 9b44d6fd4..dbcc37120 100644 --- a/src/test/rpc/LedgerData_test.cpp +++ b/src/test/rpc/LedgerData_test.cpp @@ -487,7 +487,8 @@ public: uint256 txID = tx.getTransactionID(); auto s = std::make_shared(); tx.add(*s); - env.app().getHashRouter().setFlags(txID, SF_PRIVATE2); + env.app().getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); view.rawTxInsert(txID, std::move(s), nullptr); return true; }); diff --git a/src/xrpld/app/hook/detail/applyHook.cpp b/src/xrpld/app/hook/detail/applyHook.cpp index 03eab1480..6b4fdad7b 100644 --- a/src/xrpld/app/hook/detail/applyHook.cpp +++ b/src/xrpld/app/hook/detail/applyHook.cpp @@ -1682,7 +1682,7 @@ hook::finalizeHookResult( auto& id = tpTrans->getID(); JLOG(j.trace()) << "HookEmit[" << HR_ACC() << "]: " << id; - applyCtx.app.getHashRouter().setFlags(id, SF_EMITTED); + applyCtx.app.getHashRouter().setFlags(id, HashRouterFlags::EMITTED); std::shared_ptr ptr = tpTrans->getSTransaction(); diff --git a/src/xrpld/app/ledger/Ledger.cpp b/src/xrpld/app/ledger/Ledger.cpp index c33d79b72..b3111f881 100644 --- a/src/xrpld/app/ledger/Ledger.cpp +++ b/src/xrpld/app/ledger/Ledger.cpp @@ -1038,7 +1038,8 @@ pendSaveValidated( bool isCurrent, std::function callback) { - if (!app.getHashRouter().setFlags(ledger->info().hash, SF_SAVED)) + if (!app.getHashRouter().setFlags( + ledger->info().hash, HashRouterFlags::SAVED)) { // We have tried to save this ledger recently auto stream = app.journal("Ledger").debug(); diff --git a/src/xrpld/app/misc/HashRouter.cpp b/src/xrpld/app/misc/HashRouter.cpp index dc87b2bce..b241d6a98 100644 --- a/src/xrpld/app/misc/HashRouter.cpp +++ b/src/xrpld/app/misc/HashRouter.cpp @@ -65,7 +65,10 @@ HashRouter::addSuppressionPeerWithStatus(uint256 const& key, PeerShortID peer) } bool -HashRouter::addSuppressionPeer(uint256 const& key, PeerShortID peer, int& flags) +HashRouter::addSuppressionPeer( + uint256 const& key, + PeerShortID peer, + HashRouterFlags& flags) { std::lock_guard lock(mutex_); @@ -79,7 +82,7 @@ bool HashRouter::shouldProcess( uint256 const& key, PeerShortID peer, - int& flags, + HashRouterFlags& flags, std::chrono::seconds tx_interval) { std::lock_guard lock(mutex_); @@ -91,7 +94,7 @@ HashRouter::shouldProcess( return s.shouldProcess(suppressionMap_.clock().now(), tx_interval); } -int +HashRouterFlags HashRouter::getFlags(uint256 const& key) { std::lock_guard lock(mutex_); @@ -100,9 +103,10 @@ HashRouter::getFlags(uint256 const& key) } bool -HashRouter::setFlags(uint256 const& key, int flags) +HashRouter::setFlags(uint256 const& key, HashRouterFlags flags) { - XRPL_ASSERT(flags, "ripple::HashRouter::setFlags : valid input"); + XRPL_ASSERT( + static_cast(flags), "ripple::HashRouter::setFlags : valid input"); std::lock_guard lock(mutex_); diff --git a/src/xrpld/app/misc/HashRouter.h b/src/xrpld/app/misc/HashRouter.h index c58b0f122..8dfa78751 100644 --- a/src/xrpld/app/misc/HashRouter.h +++ b/src/xrpld/app/misc/HashRouter.h @@ -31,22 +31,60 @@ namespace ripple { -// TODO convert these macros to int constants or an enum -#define SF_BAD 0x02 // Temporarily bad -#define SF_SAVED 0x04 -#define SF_HELD 0x08 // Held by LedgerMaster after potential processing failure -#define SF_TRUSTED 0x10 // comes from trusted source +enum class HashRouterFlags : std::uint16_t { + // Public flags + UNDEFINED = 0x00, + BAD = 0x02, // Temporarily bad + SAVED = 0x04, + HELD = 0x08, // Held by LedgerMaster after potential processing failure + TRUSTED = 0x10, // Comes from a trusted source -// Private flags, used internally in apply.cpp. -// Do not attempt to read, set, or reuse. -#define SF_PRIVATE1 0x0100 -#define SF_PRIVATE2 0x0200 -#define SF_PRIVATE3 0x0400 -#define SF_PRIVATE4 0x0800 -#define SF_PRIVATE5 0x1000 -#define SF_PRIVATE6 0x2000 + // Private flags (used internally in apply.cpp) + // Do not attempt to read, set, or reuse. + PRIVATE1 = 0x0100, + PRIVATE2 = 0x0200, + PRIVATE3 = 0x0400, + PRIVATE4 = 0x0800, + PRIVATE5 = 0x1000, + PRIVATE6 = 0x2000, + EMITTED = 0x4000, +}; -#define SF_EMITTED 0x4000 +constexpr HashRouterFlags +operator|(HashRouterFlags lhs, HashRouterFlags rhs) +{ + return static_cast( + static_cast>(lhs) | + static_cast>(rhs)); +} + +constexpr HashRouterFlags& +operator|=(HashRouterFlags& lhs, HashRouterFlags rhs) +{ + lhs = lhs | rhs; + return lhs; +} + +constexpr HashRouterFlags +operator&(HashRouterFlags lhs, HashRouterFlags rhs) +{ + return static_cast( + static_cast>(lhs) & + static_cast>(rhs)); +} + +constexpr HashRouterFlags& +operator&=(HashRouterFlags& lhs, HashRouterFlags rhs) +{ + lhs = lhs & rhs; + return lhs; +} + +constexpr bool +any(HashRouterFlags flags) +{ + return static_cast>(flags) != 0; +} class Config; @@ -103,14 +141,14 @@ private: peers_.insert(peer); } - int + HashRouterFlags getFlags(void) const { return flags_; } void - setFlags(int flagsToSet) + setFlags(HashRouterFlags flagsToSet) { flags_ |= flagsToSet; } @@ -140,7 +178,7 @@ private: Stopwatch::time_point const& now, std::chrono::seconds relayTime) { - if (flags_ & SF_EMITTED) + if (any(flags_ & HashRouterFlags::EMITTED)) return false; if (relayed_ && *relayed_ + relayTime > now) @@ -159,7 +197,7 @@ private: } private: - int flags_ = 0; + HashRouterFlags flags_ = HashRouterFlags::UNDEFINED; std::set peers_; // This could be generalized to a map, if more // than one flag needs to expire independently. @@ -195,14 +233,17 @@ public: addSuppressionPeerWithStatus(uint256 const& key, PeerShortID peer); bool - addSuppressionPeer(uint256 const& key, PeerShortID peer, int& flags); + addSuppressionPeer( + uint256 const& key, + PeerShortID peer, + HashRouterFlags& flags); // Add a peer suppression and return whether the entry should be processed bool shouldProcess( uint256 const& key, PeerShortID peer, - int& flags, + HashRouterFlags& flags, std::chrono::seconds tx_interval); /** Set the flags on a hash. @@ -210,9 +251,9 @@ public: @return `true` if the flags were changed. `false` if unchanged. */ bool - setFlags(uint256 const& key, int flags); + setFlags(uint256 const& key, HashRouterFlags flags); - int + HashRouterFlags getFlags(uint256 const& key); /** Determines whether the hashed item should be relayed. diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 13d5e505f..ae1702d39 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1144,7 +1144,7 @@ NetworkOPsImp::submitTransaction(std::shared_ptr const& iTrans) auto const txid = trans->getTransactionID(); auto const flags = app_.getHashRouter().getFlags(txid); - if ((flags & SF_BAD) != 0) + if ((flags & HashRouterFlags::BAD) != HashRouterFlags::UNDEFINED) { // RH NOTE: Warning removed here due to ConsesusSet using this function // which continually triggers this bar. Doesn't seem dangerous, just @@ -1209,7 +1209,7 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr& transaction) auto const newFlags = app_.getHashRouter().getFlags(transaction->getID()); - if ((newFlags & SF_BAD) != 0) + if ((newFlags & HashRouterFlags::BAD) != HashRouterFlags::UNDEFINED) { // cached bad JLOG(m_journal.warn()) << transaction->getID() << ": cached bad!\n"; @@ -1226,7 +1226,8 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr& transaction) { transaction->setStatus(INVALID); transaction->setResult(temINVALID_FLAG); - app_.getHashRouter().setFlags(transaction->getID(), SF_BAD); + app_.getHashRouter().setFlags( + transaction->getID(), HashRouterFlags::BAD); return false; } @@ -1245,7 +1246,8 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr& transaction) JLOG(m_journal.info()) << "Transaction has bad signature: " << reason; transaction->setStatus(INVALID); transaction->setResult(temBAD_SIGNATURE); - app_.getHashRouter().setFlags(transaction->getID(), SF_BAD); + app_.getHashRouter().setFlags( + transaction->getID(), HashRouterFlags::BAD); return false; } @@ -1396,7 +1398,8 @@ NetworkOPsImp::processTransactionSet(CanonicalTXSet const& set) JLOG(m_journal.trace()) << "Exception checking transaction: " << reason; } - app_.getHashRouter().setFlags(tx->getTransactionID(), SF_BAD); + app_.getHashRouter().setFlags( + tx->getTransactionID(), HashRouterFlags::BAD); continue; } @@ -1522,7 +1525,8 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) e.transaction->setResult(e.result); if (isTemMalformed(e.result)) - app_.getHashRouter().setFlags(e.transaction->getID(), SF_BAD); + app_.getHashRouter().setFlags( + e.transaction->getID(), HashRouterFlags::BAD); #ifdef DEBUG if (!isTesSuccess(e.result)) @@ -1610,7 +1614,8 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) // (5) ledgers into the future. (Remember that an // unseated optional compares as less than all seated // values, so it has to be checked explicitly first.) - // 3. The SF_HELD flag is not set on the txID. (setFlags + // 3. The HashRouterFlags::BAD flag is not set on the txID. + // (setFlags // checks before setting. If the flag is set, it returns // false, which means it's been held once without one of // the other conditions, so don't hold it again. Time's @@ -1619,7 +1624,7 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) if (e.local || (ledgersLeft && ledgersLeft <= LocalTxs::holdLedgers) || app_.getHashRouter().setFlags( - e.transaction->getID(), SF_HELD)) + e.transaction->getID(), HashRouterFlags::HELD)) { // transaction should be held JLOG(m_journal.debug()) diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index 93d10e176..ec26498c8 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -1482,7 +1482,9 @@ TxQ::accept(Application& app, OpenView& view) for (STTx const& txn : debugTxInjectQueue) { auto txnHash = txn.getTransactionID(); - app.getHashRouter().setFlags(txnHash, SF_EMITTED | SF_PRIVATE2); + app.getHashRouter().setFlags( + txnHash, + HashRouterFlags::EMITTED | HashRouterFlags::PRIVATE2); auto const& emitted = const_cast(txn).downcast(); @@ -1553,8 +1555,8 @@ TxQ::accept(Application& app, OpenView& view) auto s = std::make_shared(); cronTx.add(*s); - app.getHashRouter().setFlags(txID, SF_PRIVATE2); - app.getHashRouter().setFlags(txID, SF_EMITTED); + app.getHashRouter().setFlags(txID, HashRouterFlags::PRIVATE2); + app.getHashRouter().setFlags(txID, HashRouterFlags::EMITTED); view.rawTxInsert(txID, std::move(s), nullptr); ledgerChanged = true; } @@ -1641,7 +1643,8 @@ TxQ::accept(Application& app, OpenView& view) auto seq = view.info().seq; auto txnHash = stpTrans->getTransactionID(); - app.getHashRouter().setFlags(txnHash, SF_EMITTED); + app.getHashRouter().setFlags( + txnHash, HashRouterFlags::EMITTED); if (stpTrans->getFieldU32(sfLastLedgerSequence) < seq) { @@ -1670,8 +1673,10 @@ TxQ::accept(Application& app, OpenView& view) auto s = std::make_shared(); efTx.add(*s); - app.getHashRouter().setFlags(txID, SF_PRIVATE2); - app.getHashRouter().setFlags(txID, SF_EMITTED); + app.getHashRouter().setFlags( + txID, HashRouterFlags::PRIVATE2); + app.getHashRouter().setFlags( + txID, HashRouterFlags::EMITTED); view.rawTxInsert(txID, std::move(s), nullptr); ledgerChanged = true; @@ -1691,7 +1696,8 @@ TxQ::accept(Application& app, OpenView& view) // set if (fls >= view.info().seq) { - app.getHashRouter().setFlags(txnHash, SF_PRIVATE2); + app.getHashRouter().setFlags( + txnHash, HashRouterFlags::PRIVATE2); view.rawTxInsert( stpTrans->getTransactionID(), std::move(s), diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index 8401c6838..31862c0c3 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -33,13 +33,13 @@ #include #include +namespace ripple { + // During an EscrowFinish, the transaction must specify both // a condition and a fulfillment. We track whether that // fulfillment matches and validates the condition. -#define SF_CF_INVALID SF_PRIVATE5 -#define SF_CF_VALID SF_PRIVATE6 - -namespace ripple { +constexpr HashRouterFlags SF_CF_INVALID = HashRouterFlags::PRIVATE5; +constexpr HashRouterFlags SF_CF_VALID = HashRouterFlags::PRIVATE6; /* Escrow @@ -430,7 +430,7 @@ EscrowFinish::preflight(PreflightContext const& ctx) // If we haven't checked the condition, check it // now. Whether it passes or not isn't important // in preflight. - if (!(flags & (SF_CF_INVALID | SF_CF_VALID))) + if (!any(flags & (SF_CF_INVALID | SF_CF_VALID))) { if (checkCondition(*fb, *cb)) router.setFlags(id, SF_CF_VALID); @@ -560,7 +560,7 @@ EscrowFinish::doApply() // It's unlikely that the results of the check will // expire from the hash router, but if it happens, // simply re-run the check. - if (cb && !(flags & (SF_CF_INVALID | SF_CF_VALID))) + if (cb && !any(flags & (SF_CF_INVALID | SF_CF_VALID))) { auto const fb = ctx_.tx[~sfFulfillment]; @@ -577,7 +577,7 @@ EscrowFinish::doApply() // If the check failed, then simply return an error // and don't look at anything else. - if (flags & SF_CF_INVALID) + if (any(flags & SF_CF_INVALID)) return tecCRYPTOCONDITION_ERROR; // Check against condition in the ledger entry: diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 3fd128e8e..11db8205d 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -143,8 +143,8 @@ preflight1(PreflightContext const& ctx) // in their prevalidated form so this is safe if (ctx.rules.enabled(featureHooks) && hook::isEmittedTxn(ctx.tx)) { - if ((ctx.app.getHashRouter().getFlags(ctx.tx.getTransactionID()) & - SF_EMITTED) || + if (any(ctx.app.getHashRouter().getFlags(ctx.tx.getTransactionID()) & + HashRouterFlags::EMITTED) || (ctx.flags & tapPREFLIGHT_EMIT)) { if (ctx.tx.getSeqProxy().isTicket() && @@ -916,8 +916,8 @@ Transactor::checkSign(PreclaimContext const& ctx) { // ensure the txn was either emitted here or it's in preflight testing // during emission - if ((ctx.app.getHashRouter().getFlags(ctx.tx.getTransactionID()) & - SF_EMITTED) || + if (any(ctx.app.getHashRouter().getFlags(ctx.tx.getTransactionID()) & + HashRouterFlags::EMITTED) || (ctx.flags & tapPREFLIGHT_EMIT)) return tesSUCCESS; @@ -2057,8 +2057,9 @@ Transactor::operator()() if ((ctx_.flags() & tapPREFLIGHT_EMIT) || (view().flags() & tapPREFLIGHT_EMIT) || (ctx_.isEmittedTxn() && - !(ctx_.app.getHashRouter().getFlags(ctx_.tx.getTransactionID()) & - SF_EMITTED))) + !any( + ctx_.app.getHashRouter().getFlags(ctx_.tx.getTransactionID()) & + HashRouterFlags::EMITTED))) return {tecINTERNAL, false}; if (auto const& trap = ctx_.app.trapTxID(); diff --git a/src/xrpld/app/tx/detail/apply.cpp b/src/xrpld/app/tx/detail/apply.cpp index b60e74dbf..48e21c1b7 100644 --- a/src/xrpld/app/tx/detail/apply.cpp +++ b/src/xrpld/app/tx/detail/apply.cpp @@ -27,11 +27,16 @@ namespace ripple { -// These are the same flags defined as SF_PRIVATE1-4 in HashRouter.h -#define SF_SIGBAD SF_PRIVATE1 // Signature is bad -#define SF_SIGGOOD SF_PRIVATE2 // Signature is good -#define SF_LOCALBAD SF_PRIVATE3 // Local checks failed -#define SF_LOCALGOOD SF_PRIVATE4 // Local checks passed +// These are the same flags defined as HashRouterFlags::PRIVATE1-4 in +// HashRouter.h +constexpr HashRouterFlags SF_SIGBAD = + HashRouterFlags::PRIVATE1; // Signature is bad +constexpr HashRouterFlags SF_SIGGOOD = + HashRouterFlags::PRIVATE2; // Signature is good +constexpr HashRouterFlags SF_LOCALBAD = + HashRouterFlags::PRIVATE3; // Local checks failed +constexpr HashRouterFlags SF_LOCALGOOD = + HashRouterFlags::PRIVATE4; // Local checks passed //------------------------------------------------------------------------------ @@ -58,7 +63,7 @@ checkValidity( { // pass, this is a txn being preflighted emit api } - else if (flags & SF_EMITTED) + else if (any(flags & HashRouterFlags::EMITTED)) { // pass, this txn came out of the emission directory } @@ -96,11 +101,11 @@ checkValidity( return {Validity::Valid, ""}; } - if (flags & SF_SIGBAD) + if (any(flags & SF_SIGBAD)) // Signature is known bad return {Validity::SigBad, "Transaction has bad signature."}; - if (!(flags & SF_SIGGOOD)) + if (!any(flags & SF_SIGGOOD)) { // Don't know signature state. Check it. auto const requireCanonicalSig = @@ -118,12 +123,12 @@ checkValidity( } // Signature is now known good - if (flags & SF_LOCALBAD) + if (any(flags & SF_LOCALBAD)) // ...but the local checks // are known bad. return {Validity::SigGoodOnly, "Local checks failed."}; - if (flags & SF_LOCALGOOD) + if (any(flags & SF_LOCALGOOD)) // ...and the local checks // are known good. return {Validity::Valid, ""}; @@ -142,7 +147,7 @@ checkValidity( void forceValidity(HashRouter& router, uint256 const& txid, Validity validity) { - int flags = 0; + HashRouterFlags flags = HashRouterFlags::UNDEFINED; switch (validity) { case Validity::Valid: @@ -155,7 +160,7 @@ forceValidity(HashRouter& router, uint256 const& txid, Validity validity) // would be silly to call directly break; } - if (flags) + if (any(flags)) router.setFlags(txid, flags); } diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 545c5cf4e..b47bbf4ac 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1306,13 +1306,13 @@ PeerImp::handleTransaction( } // LCOV_EXCL_STOP - int flags; + HashRouterFlags flags; constexpr std::chrono::seconds tx_interval = 10s; if (!app_.getHashRouter().shouldProcess(txID, id_, flags, tx_interval)) { // we have seen this transaction recently - if (flags & SF_BAD) + if (any(flags & HashRouterFlags::BAD)) { fee_.update(Resource::feeUselessData, "known bad"); JLOG(p_journal_.debug()) << "Ignoring known bad tx " << txID; @@ -1339,7 +1339,7 @@ PeerImp::handleTransaction( { // Skip local checks if a server we trust // put the transaction in its open ledger - flags |= SF_TRUSTED; + flags |= HashRouterFlags::TRUSTED; } // for non-validator nodes only -- localPublicKey is set for @@ -2851,7 +2851,7 @@ PeerImp::doTransactions( void PeerImp::checkTransaction( - int flags, + HashRouterFlags flags, bool checkSignature, std::shared_ptr const& stx, bool batch) @@ -2885,7 +2885,8 @@ PeerImp::checkTransaction( (stx->getFieldU32(sfLastLedgerSequence) < app_.getLedgerMaster().getValidLedgerIndex())) { - app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); + app_.getHashRouter().setFlags( + stx->getTransactionID(), HashRouterFlags::BAD); charge(Resource::feeUselessData, "expired tx"); return; } @@ -2944,8 +2945,10 @@ PeerImp::checkTransaction( << "Exception checking transaction: " << validReason; } - // Probably not necessary to set SF_BAD, but doesn't hurt. - app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); + // Probably not necessary to set HashRouterFlags::BAD, but + // doesn't hurt. + app_.getHashRouter().setFlags( + stx->getTransactionID(), HashRouterFlags::BAD); charge( Resource::feeInvalidSignature, "check transaction signature failure"); @@ -2968,12 +2971,13 @@ PeerImp::checkTransaction( JLOG(p_journal_.trace()) << "Exception checking transaction: " << reason; } - app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); + app_.getHashRouter().setFlags( + stx->getTransactionID(), HashRouterFlags::BAD); charge(Resource::feeInvalidSignature, "tx (impossible)"); return; } - bool const trusted(flags & SF_TRUSTED); + bool const trusted = any(flags & HashRouterFlags::TRUSTED); app_.getOPs().processTransaction( tx, trusted, false, NetworkOPs::FailHard::no); } @@ -2981,7 +2985,8 @@ PeerImp::checkTransaction( { JLOG(p_journal_.warn()) << "Exception in " << __func__ << ": " << ex.what(); - app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); + app_.getHashRouter().setFlags( + stx->getTransactionID(), HashRouterFlags::BAD); using namespace std::string_literals; charge(Resource::feeInvalidData, "tx "s + ex.what()); } diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index d5f8e4d17..5aa49fd15 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -612,7 +613,7 @@ private: void checkTransaction( - int flags, + HashRouterFlags flags, bool checkSignature, std::shared_ptr const& stx, bool batch);