ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2025-12-06 17:27:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor: Makes HashRouter flags more type-safe (#5371)

Browse Source 
This change addresses the issue #5336: Refactor HashRouter flags to be more type-safe.

* Switched numeric flags to enum type.
* Updated unit tests
This commit is contained in:
Valentin Balaschenko
2025-07-23 13:03:12 +01:00
committed by GitHub
parent 7ff4f79d30
commit c233df720a
10 changed files with 219 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
src/test/app/Batch_test.cpp
View File

@@ -3652,14 +3652,18 @@ class Batch_test : public beast::unit_test::suite
{ {
// Submit a tx with tfInnerBatchTxn // Submit a tx with tfInnerBatchTxn
uint256 const txBad = submitTx(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() // Validate: NetworkOPs::processTransaction()
{ {
uint256 const txid = processTxn(tfInnerBatchTxn); uint256 const txid = processTxn(tfInnerBatchTxn);
// HashRouter::getFlags() should return SF_BAD // HashRouter::getFlags() should return LedgerFlags::BAD
BEAST_EXPECT(env.app().getHashRouter().getFlags(txid) == SF_BAD); BEAST_EXPECT(
env.app().getHashRouter().getFlags(txid) ==
HashRouterFlags::BAD);
} }
} }

127
src/test/app/HashRouter_test.cpp
View File

@@ -45,15 +45,19 @@ class HashRouter_test : public beast::unit_test::suite
TestStopwatch stopwatch; TestStopwatch stopwatch;
HashRouter router(getSetup(2s, 1s), stopwatch); HashRouter router(getSetup(2s, 1s), stopwatch);
uint256 const key1(1); HashRouterFlags key1(HashRouterFlags::PRIVATE1);
uint256 const key2(2); HashRouterFlags key2(HashRouterFlags::PRIVATE2);
uint256 const key3(3); HashRouterFlags key3(HashRouterFlags::PRIVATE3);
auto const ukey1 = uint256{static_cast<std::uint64_t>(key1)};
auto const ukey2 = uint256{static_cast<std::uint64_t>(key2)};
auto const ukey3 = uint256{static_cast<std::uint64_t>(key3)};
// t=0 // t=0
router.setFlags(key1, 11111); router.setFlags(ukey1, HashRouterFlags::PRIVATE1);
BEAST_EXPECT(router.getFlags(key1) == 11111); BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::PRIVATE1);
router.setFlags(key2, 22222); router.setFlags(ukey2, HashRouterFlags::PRIVATE2);
BEAST_EXPECT(router.getFlags(key2) == 22222); BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE2);
// key1 : 0 // key1 : 0
// key2 : 0 // key2 : 0
// key3: null // key3: null
@@ -62,7 +66,7 @@ class HashRouter_test : public beast::unit_test::suite
// Because we are accessing key1 here, it // Because we are accessing key1 here, it
// will NOT be expired for another two ticks // will NOT be expired for another two ticks
BEAST_EXPECT(router.getFlags(key1) == 11111); BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::PRIVATE1);
// key1 : 1 // key1 : 1
// key2 : 0 // key2 : 0
// key3 null // key3 null
@@ -70,9 +74,9 @@ class HashRouter_test : public beast::unit_test::suite
++stopwatch; ++stopwatch;
// t=3 // t=3
router.setFlags(key3, 33333); // force expiration router.setFlags(ukey3, HashRouterFlags::PRIVATE3); // force expiration
BEAST_EXPECT(router.getFlags(key1) == 11111); BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::PRIVATE1);
BEAST_EXPECT(router.getFlags(key2) == 0); BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::UNDEFINED);
} }
void void
@@ -83,15 +87,21 @@ class HashRouter_test : public beast::unit_test::suite
TestStopwatch stopwatch; TestStopwatch stopwatch;
HashRouter router(getSetup(2s, 1s), stopwatch); HashRouter router(getSetup(2s, 1s), stopwatch);
uint256 const key1(1); HashRouterFlags key1(HashRouterFlags::PRIVATE1);
uint256 const key2(2); HashRouterFlags key2(HashRouterFlags::PRIVATE2);
uint256 const key3(3); HashRouterFlags key3(HashRouterFlags::PRIVATE3);
uint256 const key4(4); HashRouterFlags key4(HashRouterFlags::PRIVATE4);
auto const ukey1 = uint256{static_cast<std::uint64_t>(key1)};
auto const ukey2 = uint256{static_cast<std::uint64_t>(key2)};
auto const ukey3 = uint256{static_cast<std::uint64_t>(key3)};
auto const ukey4 = uint256{static_cast<std::uint64_t>(key4)};
BEAST_EXPECT(key1 != key2 && key2 != key3 && key3 != key4); BEAST_EXPECT(key1 != key2 && key2 != key3 && key3 != key4);
// t=0 // t=0
router.setFlags(key1, 12345); router.setFlags(ukey1, HashRouterFlags::BAD);
BEAST_EXPECT(router.getFlags(key1) == 12345); BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::BAD);
// key1 : 0 // key1 : 0
// key2 : null // key2 : null
// key3 : null // key3 : null
@@ -103,26 +113,27 @@ class HashRouter_test : public beast::unit_test::suite
// so key1 will be expired after the second // so key1 will be expired after the second
// call to setFlags. // call to setFlags.
// t=1 // t=1
router.setFlags(key2, 9999);
BEAST_EXPECT(router.getFlags(key1) == 12345); router.setFlags(ukey2, HashRouterFlags::PRIVATE5);
BEAST_EXPECT(router.getFlags(key2) == 9999); BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::BAD);
BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE5);
// key1 : 1 // key1 : 1
// key2 : 1 // key2 : 1
// key3 : null // key3 : null
++stopwatch; ++stopwatch;
// t=2 // t=2
BEAST_EXPECT(router.getFlags(key2) == 9999); BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE5);
// key1 : 1 // key1 : 1
// key2 : 2 // key2 : 2
// key3 : null // key3 : null
++stopwatch; ++stopwatch;
// t=3 // t=3
router.setFlags(key3, 2222); router.setFlags(ukey3, HashRouterFlags::BAD);
BEAST_EXPECT(router.getFlags(key1) == 0); BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::UNDEFINED);
BEAST_EXPECT(router.getFlags(key2) == 9999); BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE5);
BEAST_EXPECT(router.getFlags(key3) == 2222); BEAST_EXPECT(router.getFlags(ukey3) == HashRouterFlags::BAD);
// key1 : 3 // key1 : 3
// key2 : 3 // key2 : 3
// key3 : 3 // key3 : 3
@@ -130,10 +141,10 @@ class HashRouter_test : public beast::unit_test::suite
++stopwatch; ++stopwatch;
// t=4 // t=4
// No insertion, no expiration // No insertion, no expiration
router.setFlags(key1, 7654); router.setFlags(ukey1, HashRouterFlags::SAVED);
BEAST_EXPECT(router.getFlags(key1) == 7654); BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::SAVED);
BEAST_EXPECT(router.getFlags(key2) == 9999); BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::PRIVATE5);
BEAST_EXPECT(router.getFlags(key3) == 2222); BEAST_EXPECT(router.getFlags(ukey3) == HashRouterFlags::BAD);
// key1 : 4 // key1 : 4
// key2 : 4 // key2 : 4
// key3 : 4 // key3 : 4
@@ -142,11 +153,11 @@ class HashRouter_test : public beast::unit_test::suite
++stopwatch; ++stopwatch;
// t=6 // t=6
router.setFlags(key4, 7890); router.setFlags(ukey4, HashRouterFlags::TRUSTED);
BEAST_EXPECT(router.getFlags(key1) == 0); BEAST_EXPECT(router.getFlags(ukey1) == HashRouterFlags::UNDEFINED);
BEAST_EXPECT(router.getFlags(key2) == 0); BEAST_EXPECT(router.getFlags(ukey2) == HashRouterFlags::UNDEFINED);
BEAST_EXPECT(router.getFlags(key3) == 0); BEAST_EXPECT(router.getFlags(ukey3) == HashRouterFlags::UNDEFINED);
BEAST_EXPECT(router.getFlags(key4) == 7890); BEAST_EXPECT(router.getFlags(ukey4) == HashRouterFlags::TRUSTED);
// key1 : 6 // key1 : 6
// key2 : 6 // key2 : 6
// key3 : 6 // key3 : 6
@@ -168,18 +179,18 @@ class HashRouter_test : public beast::unit_test::suite
uint256 const key4(4); uint256 const key4(4);
BEAST_EXPECT(key1 != key2 && key2 != key3 && key3 != key4); 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); router.addSuppression(key1);
BEAST_EXPECT(router.addSuppressionPeer(key2, 15)); BEAST_EXPECT(router.addSuppressionPeer(key2, 15));
BEAST_EXPECT(router.addSuppressionPeer(key3, 20, flags)); BEAST_EXPECT(router.addSuppressionPeer(key3, 20, flags));
BEAST_EXPECT(flags == 0); BEAST_EXPECT(flags == HashRouterFlags::UNDEFINED);
++stopwatch; ++stopwatch;
BEAST_EXPECT(!router.addSuppressionPeer(key1, 2)); BEAST_EXPECT(!router.addSuppressionPeer(key1, 2));
BEAST_EXPECT(!router.addSuppressionPeer(key2, 3)); BEAST_EXPECT(!router.addSuppressionPeer(key2, 3));
BEAST_EXPECT(!router.addSuppressionPeer(key3, 4, flags)); BEAST_EXPECT(!router.addSuppressionPeer(key3, 4, flags));
BEAST_EXPECT(flags == 0); BEAST_EXPECT(flags == HashRouterFlags::UNDEFINED);
BEAST_EXPECT(router.addSuppressionPeer(key4, 5)); BEAST_EXPECT(router.addSuppressionPeer(key4, 5));
} }
@@ -192,9 +203,9 @@ class HashRouter_test : public beast::unit_test::suite
HashRouter router(getSetup(2s, 1s), stopwatch); HashRouter router(getSetup(2s, 1s), stopwatch);
uint256 const key1(1); uint256 const key1(1);
BEAST_EXPECT(router.setFlags(key1, 10)); BEAST_EXPECT(router.setFlags(key1, HashRouterFlags::PRIVATE1));
BEAST_EXPECT(!router.setFlags(key1, 10)); BEAST_EXPECT(!router.setFlags(key1, HashRouterFlags::PRIVATE1));
BEAST_EXPECT(router.setFlags(key1, 20)); BEAST_EXPECT(router.setFlags(key1, HashRouterFlags::PRIVATE2));
} }
void void
@@ -250,7 +261,7 @@ class HashRouter_test : public beast::unit_test::suite
HashRouter router(getSetup(5s, 1s), stopwatch); HashRouter router(getSetup(5s, 1s), stopwatch);
uint256 const key(1); uint256 const key(1);
HashRouter::PeerShortID peer = 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));
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>;
HF f1 = HF::BAD;
HF f2 = HF::SAVED;
HF combined = f1 | f2;
BEAST_EXPECT(
static_cast<UHF>(combined) ==
(static_cast<UHF>(f1) | static_cast<UHF>(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: public:
void void
run() override run() override
@@ -375,6 +419,7 @@ public:
testRelay(); testRelay();
testProcess(); testProcess();
testSetup(); testSetup();
testFlagsOps();
} }
}; };

3
src/xrpld/app/ledger/Ledger.cpp
View File

@@ -996,7 +996,8 @@ pendSaveValidated(
bool isSynchronous, bool isSynchronous,
bool isCurrent) bool isCurrent)
{ {
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 // We have tried to save this ledger recently
auto stream = app.journal("Ledger").debug(); auto stream = app.journal("Ledger").debug();

14
src/xrpld/app/misc/HashRouter.cpp
View File

@@ -65,7 +65,10 @@ HashRouter::addSuppressionPeerWithStatus(uint256 const& key, PeerShortID peer)
} }
bool bool
HashRouter::addSuppressionPeer(uint256 const& key, PeerShortID peer, int& flags) HashRouter::addSuppressionPeer(
uint256 const& key,
PeerShortID peer,
HashRouterFlags& flags)
{ {
std::lock_guard lock(mutex_); std::lock_guard lock(mutex_);
@@ -79,7 +82,7 @@ bool
HashRouter::shouldProcess( HashRouter::shouldProcess(
uint256 const& key, uint256 const& key,
PeerShortID peer, PeerShortID peer,
int& flags, HashRouterFlags& flags,
std::chrono::seconds tx_interval) std::chrono::seconds tx_interval)
{ {
std::lock_guard lock(mutex_); std::lock_guard lock(mutex_);
@@ -91,7 +94,7 @@ HashRouter::shouldProcess(
return s.shouldProcess(suppressionMap_.clock().now(), tx_interval); return s.shouldProcess(suppressionMap_.clock().now(), tx_interval);
} }
int HashRouterFlags
HashRouter::getFlags(uint256 const& key) HashRouter::getFlags(uint256 const& key)
{ {
std::lock_guard lock(mutex_); std::lock_guard lock(mutex_);
@@ -100,9 +103,10 @@ HashRouter::getFlags(uint256 const& key)
} }
bool 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<bool>(flags), "ripple::HashRouter::setFlags : valid input");
std::lock_guard lock(mutex_); std::lock_guard lock(mutex_);

82
src/xrpld/app/misc/HashRouter.h
View File

@@ -31,20 +31,59 @@
namespace ripple { namespace ripple {
// TODO convert these macros to int constants or an enum enum class HashRouterFlags : std::uint16_t {
#define SF_BAD 0x02 // Temporarily bad // Public flags
#define SF_SAVED 0x04 UNDEFINED = 0x00,
#define SF_HELD 0x08 // Held by LedgerMaster after potential processing failure BAD = 0x02, // Temporarily bad
#define SF_TRUSTED 0x10 // comes from trusted source 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. // Private flags (used internally in apply.cpp)
// Do not attempt to read, set, or reuse. // Do not attempt to read, set, or reuse.
#define SF_PRIVATE1 0x0100 PRIVATE1 = 0x0100,
#define SF_PRIVATE2 0x0200 PRIVATE2 = 0x0200,
#define SF_PRIVATE3 0x0400 PRIVATE3 = 0x0400,
#define SF_PRIVATE4 0x0800 PRIVATE4 = 0x0800,
#define SF_PRIVATE5 0x1000 PRIVATE5 = 0x1000,
#define SF_PRIVATE6 0x2000 PRIVATE6 = 0x2000
};
constexpr HashRouterFlags
operator|(HashRouterFlags lhs, HashRouterFlags rhs)
{
return static_cast<HashRouterFlags>(
static_cast<std::underlying_type_t<HashRouterFlags>>(lhs) |
static_cast<std::underlying_type_t<HashRouterFlags>>(rhs));
}
constexpr HashRouterFlags&
operator|=(HashRouterFlags& lhs, HashRouterFlags rhs)
{
lhs = lhs | rhs;
return lhs;
}
constexpr HashRouterFlags
operator&(HashRouterFlags lhs, HashRouterFlags rhs)
{
return static_cast<HashRouterFlags>(
static_cast<std::underlying_type_t<HashRouterFlags>>(lhs) &
static_cast<std::underlying_type_t<HashRouterFlags>>(rhs));
}
constexpr HashRouterFlags&
operator&=(HashRouterFlags& lhs, HashRouterFlags rhs)
{
lhs = lhs & rhs;
return lhs;
}
constexpr bool
any(HashRouterFlags flags)
{
return static_cast<std::underlying_type_t<HashRouterFlags>>(flags) != 0;
}
class Config; class Config;
@@ -101,14 +140,14 @@ private:
peers_.insert(peer); peers_.insert(peer);
} }
int HashRouterFlags
getFlags(void) const getFlags(void) const
{ {
return flags_; return flags_;
} }
void void
setFlags(int flagsToSet) setFlags(HashRouterFlags flagsToSet)
{ {
flags_ |= flagsToSet; flags_ |= flagsToSet;
} }
@@ -154,7 +193,7 @@ private:
} }
private: private:
int flags_ = 0; HashRouterFlags flags_ = HashRouterFlags::UNDEFINED;
std::set<PeerShortID> peers_; std::set<PeerShortID> peers_;
// This could be generalized to a map, if more // This could be generalized to a map, if more
// than one flag needs to expire independently. // than one flag needs to expire independently.
@@ -190,14 +229,17 @@ public:
addSuppressionPeerWithStatus(uint256 const& key, PeerShortID peer); addSuppressionPeerWithStatus(uint256 const& key, PeerShortID peer);
bool 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 // Add a peer suppression and return whether the entry should be processed
bool bool
shouldProcess( shouldProcess(
uint256 const& key, uint256 const& key,
PeerShortID peer, PeerShortID peer,
int& flags, HashRouterFlags& flags,
std::chrono::seconds tx_interval); std::chrono::seconds tx_interval);
/** Set the flags on a hash. /** Set the flags on a hash.
@@ -205,9 +247,9 @@ public:
@return `true` if the flags were changed. `false` if unchanged. @return `true` if the flags were changed. `false` if unchanged.
*/ */
bool bool
setFlags(uint256 const& key, int flags); setFlags(uint256 const& key, HashRouterFlags flags);
int HashRouterFlags
getFlags(uint256 const& key); getFlags(uint256 const& key);
/** Determines whether the hashed item should be relayed. /** Determines whether the hashed item should be relayed.

21
src/xrpld/app/misc/NetworkOPs.cpp
View File

@@ -1207,7 +1207,7 @@ NetworkOPsImp::submitTransaction(std::shared_ptr<STTx const> const& iTrans)
auto const txid = trans->getTransactionID(); auto const txid = trans->getTransactionID();
auto const flags = app_.getHashRouter().getFlags(txid); auto const flags = app_.getHashRouter().getFlags(txid);
if ((flags & SF_BAD) != 0) if ((flags & HashRouterFlags::BAD) != HashRouterFlags::UNDEFINED)
{ {
JLOG(m_journal.warn()) << "Submitted transaction cached bad"; JLOG(m_journal.warn()) << "Submitted transaction cached bad";
return; return;
@@ -1251,7 +1251,7 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr<Transaction>& transaction)
{ {
auto const newFlags = app_.getHashRouter().getFlags(transaction->getID()); auto const newFlags = app_.getHashRouter().getFlags(transaction->getID());
if ((newFlags & SF_BAD) != 0) if ((newFlags & HashRouterFlags::BAD) != HashRouterFlags::UNDEFINED)
{ {
// cached bad // cached bad
JLOG(m_journal.warn()) << transaction->getID() << ": cached bad!\n"; JLOG(m_journal.warn()) << transaction->getID() << ": cached bad!\n";
@@ -1270,7 +1270,8 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr<Transaction>& transaction)
{ {
transaction->setStatus(INVALID); transaction->setStatus(INVALID);
transaction->setResult(temINVALID_FLAG); transaction->setResult(temINVALID_FLAG);
app_.getHashRouter().setFlags(transaction->getID(), SF_BAD); app_.getHashRouter().setFlags(
transaction->getID(), HashRouterFlags::BAD);
return false; return false;
} }
@@ -1289,7 +1290,8 @@ NetworkOPsImp::preProcessTransaction(std::shared_ptr<Transaction>& transaction)
JLOG(m_journal.info()) << "Transaction has bad signature: " << reason; JLOG(m_journal.info()) << "Transaction has bad signature: " << reason;
transaction->setStatus(INVALID); transaction->setStatus(INVALID);
transaction->setResult(temBAD_SIGNATURE); transaction->setResult(temBAD_SIGNATURE);
app_.getHashRouter().setFlags(transaction->getID(), SF_BAD); app_.getHashRouter().setFlags(
transaction->getID(), HashRouterFlags::BAD);
return false; return false;
} }
@@ -1412,7 +1414,8 @@ NetworkOPsImp::processTransactionSet(CanonicalTXSet const& set)
JLOG(m_journal.trace()) JLOG(m_journal.trace())
<< "Exception checking transaction: " << reason; << "Exception checking transaction: " << reason;
} }
app_.getHashRouter().setFlags(tx->getTransactionID(), SF_BAD); app_.getHashRouter().setFlags(
tx->getTransactionID(), HashRouterFlags::BAD);
continue; continue;
} }
@@ -1538,7 +1541,8 @@ NetworkOPsImp::apply(std::unique_lock<std::mutex>& batchLock)
e.transaction->setResult(e.result); e.transaction->setResult(e.result);
if (isTemMalformed(e.result)) if (isTemMalformed(e.result))
app_.getHashRouter().setFlags(e.transaction->getID(), SF_BAD); app_.getHashRouter().setFlags(
e.transaction->getID(), HashRouterFlags::BAD);
#ifdef DEBUG #ifdef DEBUG
if (e.result != tesSUCCESS) if (e.result != tesSUCCESS)
@@ -1626,7 +1630,8 @@ NetworkOPsImp::apply(std::unique_lock<std::mutex>& batchLock)
// (5) ledgers into the future. (Remember that an // (5) ledgers into the future. (Remember that an
// unseated optional compares as less than all seated // unseated optional compares as less than all seated
// values, so it has to be checked explicitly first.) // 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 // checks before setting. If the flag is set, it returns
// false, which means it's been held once without one of // false, which means it's been held once without one of
// the other conditions, so don't hold it again. Time's // the other conditions, so don't hold it again. Time's
@@ -1635,7 +1640,7 @@ NetworkOPsImp::apply(std::unique_lock<std::mutex>& batchLock)
if (e.local || if (e.local ||
(ledgersLeft && ledgersLeft <= LocalTxs::holdLedgers) || (ledgersLeft && ledgersLeft <= LocalTxs::holdLedgers) ||
app_.getHashRouter().setFlags( app_.getHashRouter().setFlags(
e.transaction->getID(), SF_HELD)) e.transaction->getID(), HashRouterFlags::HELD))
{ {
// transaction should be held // transaction should be held
JLOG(m_journal.debug()) JLOG(m_journal.debug())

14
src/xrpld/app/tx/detail/Escrow.cpp
View File

@@ -34,13 +34,13 @@
#include <xrpl/protocol/TxFlags.h> #include <xrpl/protocol/TxFlags.h>
#include <xrpl/protocol/XRPAmount.h> #include <xrpl/protocol/XRPAmount.h>
namespace ripple {
// During an EscrowFinish, the transaction must specify both // During an EscrowFinish, the transaction must specify both
// a condition and a fulfillment. We track whether that // a condition and a fulfillment. We track whether that
// fulfillment matches and validates the condition. // fulfillment matches and validates the condition.
#define SF_CF_INVALID SF_PRIVATE5 constexpr HashRouterFlags SF_CF_INVALID = HashRouterFlags::PRIVATE5;
#define SF_CF_VALID SF_PRIVATE6 constexpr HashRouterFlags SF_CF_VALID = HashRouterFlags::PRIVATE6;
namespace ripple {
/* /*
Escrow Escrow
@@ -663,7 +663,7 @@ EscrowFinish::preflight(PreflightContext const& ctx)
// If we haven't checked the condition, check it // If we haven't checked the condition, check it
// now. Whether it passes or not isn't important // now. Whether it passes or not isn't important
// in preflight. // in preflight.
if (!(flags & (SF_CF_INVALID | SF_CF_VALID))) if (!any(flags & (SF_CF_INVALID | SF_CF_VALID)))
{ {
if (checkCondition(*fb, *cb)) if (checkCondition(*fb, *cb))
router.setFlags(id, SF_CF_VALID); router.setFlags(id, SF_CF_VALID);
@@ -1064,7 +1064,7 @@ EscrowFinish::doApply()
// It's unlikely that the results of the check will // It's unlikely that the results of the check will
// expire from the hash router, but if it happens, // expire from the hash router, but if it happens,
// simply re-run the check. // 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]; auto const fb = ctx_.tx[~sfFulfillment];
@@ -1081,7 +1081,7 @@ EscrowFinish::doApply()
// If the check failed, then simply return an error // If the check failed, then simply return an error
// and don't look at anything else. // and don't look at anything else.
if (flags & SF_CF_INVALID) if (any(flags & SF_CF_INVALID))
return tecCRYPTOCONDITION_ERROR; return tecCRYPTOCONDITION_ERROR;
// Check against condition in the ledger entry: // Check against condition in the ledger entry:

27
src/xrpld/app/tx/detail/apply.cpp
View File

@@ -27,11 +27,16 @@
namespace ripple { namespace ripple {
// These are the same flags defined as SF_PRIVATE1-4 in HashRouter.h // These are the same flags defined as HashRouterFlags::PRIVATE1-4 in
#define SF_SIGBAD SF_PRIVATE1 // Signature is bad // HashRouter.h
#define SF_SIGGOOD SF_PRIVATE2 // Signature is good constexpr HashRouterFlags SF_SIGBAD =
#define SF_LOCALBAD SF_PRIVATE3 // Local checks failed HashRouterFlags::PRIVATE1; // Signature is bad
#define SF_LOCALGOOD SF_PRIVATE4 // Local checks passed 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
//------------------------------------------------------------------------------ //------------------------------------------------------------------------------
@@ -66,11 +71,11 @@ checkValidity(
return {Validity::Valid, ""}; return {Validity::Valid, ""};
} }
if (flags & SF_SIGBAD) if (any(flags & SF_SIGBAD))
// Signature is known bad // Signature is known bad
return {Validity::SigBad, "Transaction has bad signature."}; return {Validity::SigBad, "Transaction has bad signature."};
if (!(flags & SF_SIGGOOD)) if (!any(flags & SF_SIGGOOD))
{ {
// Don't know signature state. Check it. // Don't know signature state. Check it.
auto const requireCanonicalSig = auto const requireCanonicalSig =
@@ -88,12 +93,12 @@ checkValidity(
} }
// Signature is now known good // Signature is now known good
if (flags & SF_LOCALBAD) if (any(flags & SF_LOCALBAD))
// ...but the local checks // ...but the local checks
// are known bad. // are known bad.
return {Validity::SigGoodOnly, "Local checks failed."}; return {Validity::SigGoodOnly, "Local checks failed."};
if (flags & SF_LOCALGOOD) if (any(flags & SF_LOCALGOOD))
// ...and the local checks // ...and the local checks
// are known good. // are known good.
return {Validity::Valid, ""}; return {Validity::Valid, ""};
@@ -112,7 +117,7 @@ checkValidity(
void void
forceValidity(HashRouter& router, uint256 const& txid, Validity validity) forceValidity(HashRouter& router, uint256 const& txid, Validity validity)
{ {
int flags = 0; HashRouterFlags flags = HashRouterFlags::UNDEFINED;
switch (validity) switch (validity)
{ {
case Validity::Valid: case Validity::Valid:
@@ -125,7 +130,7 @@ forceValidity(HashRouter& router, uint256 const& txid, Validity validity)
// would be silly to call directly // would be silly to call directly
break; break;
} }
if (flags) if (any(flags))
router.setFlags(txid, flags); router.setFlags(txid, flags);
} }

25
src/xrpld/overlay/detail/PeerImp.cpp
View File

@@ -1296,13 +1296,13 @@ PeerImp::handleTransaction(
} }
// LCOV_EXCL_STOP // LCOV_EXCL_STOP
int flags; HashRouterFlags flags;
constexpr std::chrono::seconds tx_interval = 10s; constexpr std::chrono::seconds tx_interval = 10s;
if (!app_.getHashRouter().shouldProcess(txID, id_, flags, tx_interval)) if (!app_.getHashRouter().shouldProcess(txID, id_, flags, tx_interval))
{ {
// we have seen this transaction recently // we have seen this transaction recently
if (flags & SF_BAD) if (any(flags & HashRouterFlags::BAD))
{ {
fee_.update(Resource::feeUselessData, "known bad"); fee_.update(Resource::feeUselessData, "known bad");
JLOG(p_journal_.debug()) << "Ignoring known bad tx " << txID; JLOG(p_journal_.debug()) << "Ignoring known bad tx " << txID;
@@ -1329,7 +1329,7 @@ PeerImp::handleTransaction(
{ {
// Skip local checks if a server we trust // Skip local checks if a server we trust
// put the transaction in its open ledger // put the transaction in its open ledger
flags |= SF_TRUSTED; flags |= HashRouterFlags::TRUSTED;
} }
// for non-validator nodes only -- localPublicKey is set for // for non-validator nodes only -- localPublicKey is set for
@@ -2841,7 +2841,7 @@ PeerImp::doTransactions(
void void
PeerImp::checkTransaction( PeerImp::checkTransaction(
int flags, HashRouterFlags flags,
bool checkSignature, bool checkSignature,
std::shared_ptr<STTx const> const& stx, std::shared_ptr<STTx const> const& stx,
bool batch) bool batch)
@@ -2866,7 +2866,8 @@ PeerImp::checkTransaction(
(stx->getFieldU32(sfLastLedgerSequence) < (stx->getFieldU32(sfLastLedgerSequence) <
app_.getLedgerMaster().getValidLedgerIndex())) app_.getLedgerMaster().getValidLedgerIndex()))
{ {
app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); app_.getHashRouter().setFlags(
stx->getTransactionID(), HashRouterFlags::BAD);
charge(Resource::feeUselessData, "expired tx"); charge(Resource::feeUselessData, "expired tx");
return; return;
} }
@@ -2925,8 +2926,10 @@ PeerImp::checkTransaction(
<< "Exception checking transaction: " << validReason; << "Exception checking transaction: " << validReason;
} }
// Probably not necessary to set SF_BAD, but doesn't hurt. // Probably not necessary to set HashRouterFlags::BAD, but
app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); // doesn't hurt.
app_.getHashRouter().setFlags(
stx->getTransactionID(), HashRouterFlags::BAD);
charge( charge(
Resource::feeInvalidSignature, Resource::feeInvalidSignature,
"check transaction signature failure"); "check transaction signature failure");
@@ -2949,12 +2952,13 @@ PeerImp::checkTransaction(
JLOG(p_journal_.trace()) JLOG(p_journal_.trace())
<< "Exception checking transaction: " << reason; << "Exception checking transaction: " << reason;
} }
app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); app_.getHashRouter().setFlags(
stx->getTransactionID(), HashRouterFlags::BAD);
charge(Resource::feeInvalidSignature, "tx (impossible)"); charge(Resource::feeInvalidSignature, "tx (impossible)");
return; return;
} }
bool const trusted(flags & SF_TRUSTED); bool const trusted = any(flags & HashRouterFlags::TRUSTED);
app_.getOPs().processTransaction( app_.getOPs().processTransaction(
tx, trusted, false, NetworkOPs::FailHard::no); tx, trusted, false, NetworkOPs::FailHard::no);
} }
@@ -2962,7 +2966,8 @@ PeerImp::checkTransaction(
{ {
JLOG(p_journal_.warn()) JLOG(p_journal_.warn())
<< "Exception in " << __func__ << ": " << ex.what(); << "Exception in " << __func__ << ": " << ex.what();
app_.getHashRouter().setFlags(stx->getTransactionID(), SF_BAD); app_.getHashRouter().setFlags(
stx->getTransactionID(), HashRouterFlags::BAD);
using namespace std::string_literals; using namespace std::string_literals;
charge(Resource::feeInvalidData, "tx "s + ex.what()); charge(Resource::feeInvalidData, "tx "s + ex.what());
} }

3
src/xrpld/overlay/detail/PeerImp.h
View File

@@ -22,6 +22,7 @@
#include <xrpld/app/consensus/RCLCxPeerPos.h> #include <xrpld/app/consensus/RCLCxPeerPos.h>
#include <xrpld/app/ledger/detail/LedgerReplayMsgHandler.h> #include <xrpld/app/ledger/detail/LedgerReplayMsgHandler.h>
#include <xrpld/app/misc/HashRouter.h>
#include <xrpld/overlay/Squelch.h> #include <xrpld/overlay/Squelch.h>
#include <xrpld/overlay/detail/OverlayImpl.h> #include <xrpld/overlay/detail/OverlayImpl.h>
#include <xrpld/overlay/detail/ProtocolVersion.h> #include <xrpld/overlay/detail/ProtocolVersion.h>
@@ -612,7 +613,7 @@ private:
void void
checkTransaction( checkTransaction(
int flags, HashRouterFlags flags,
bool checkSignature, bool checkSignature,
std::shared_ptr<STTx const> const& stx, std::shared_ptr<STTx const> const& stx,
bool batch); bool batch);