Skip to content

Commit 255ea43

Browse files
committed
Update TxQ lock safety
1 parent 2bc5cb2 commit 255ea43

2 files changed

Lines changed: 42 additions & 31 deletions

File tree

src/xrpld/app/misc/TxQ.h

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -745,7 +745,8 @@ class TxQ
745745
std::optional<TxQAccount::TxMap::iterator>
746746
removeFromByFee(
747747
std::optional<TxQAccount::TxMap::iterator> const& replacedTxIter,
748-
std::shared_ptr<STTx const> const& tx);
748+
std::shared_ptr<STTx const> const& tx,
749+
std::lock_guard<std::mutex> const&);
749750

750751
using FeeHook = boost::intrusive::member_hook<
751752
MaybeTx,
@@ -802,7 +803,7 @@ class TxQ
802803
/// Is the queue at least `fillPercentage` full?
803804
template <size_t fillPercentage = 100>
804805
bool
805-
isFull() const;
806+
isFull(std::lock_guard<std::mutex> const&) const;
806807

807808
/** Checks if the indicated transaction fits the conditions
808809
for being stored in the queue.
@@ -815,22 +816,26 @@ class TxQ
815816
std::shared_ptr<SLE const> const& sleAccount,
816817
AccountMap::iterator const&,
817818
std::optional<TxQAccount::TxMap::iterator> const&,
818-
std::lock_guard<std::mutex> const& lock);
819+
std::lock_guard<std::mutex> const&);
819820

820821
/// Erase and return the next entry in byFee_ (lower fee level)
821-
FeeMultiSet::iterator_type erase(FeeMultiSet::const_iterator_type);
822+
FeeMultiSet::iterator_type
823+
erase(FeeMultiSet::const_iterator_type, std::lock_guard<std::mutex> const&);
822824
/** Erase and return the next entry for the account (if fee level
823825
is higher), or next entry in byFee_ (lower fee level).
824826
Used to get the next "applyable" MaybeTx for accept().
825827
*/
826-
FeeMultiSet::iterator_type eraseAndAdvance(
827-
FeeMultiSet::const_iterator_type);
828+
FeeMultiSet::iterator_type
829+
eraseAndAdvance(
830+
FeeMultiSet::const_iterator_type,
831+
std::lock_guard<std::mutex> const&);
828832
/// Erase a range of items, based on TxQAccount::TxMap iterators
829833
TxQAccount::TxMap::iterator
830834
erase(
831835
TxQAccount& txQAccount,
832836
TxQAccount::TxMap::const_iterator begin,
833-
TxQAccount::TxMap::const_iterator end);
837+
TxQAccount::TxMap::const_iterator end,
838+
std::lock_guard<std::mutex> const&);
834839

835840
/**
836841
All-or-nothing attempt to try to apply the queued txs for
@@ -849,6 +854,7 @@ class TxQ
849854
std::size_t const txExtraCount,
850855
ApplyFlags flags,
851856
FeeMetrics::Snapshot const& metricsSnapshot,
857+
std::lock_guard<std::mutex> const&,
852858
beast::Journal j);
853859
};
854860

src/xrpld/app/misc/detail/TxQ.cpp

Lines changed: 29 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -373,7 +373,7 @@ TxQ::~TxQ()
373373

374374
template <size_t fillPercentage>
375375
bool
376-
TxQ::isFull() const
376+
TxQ::isFull(std::lock_guard<std::mutex> const&) const
377377
{
378378
static_assert(
379379
fillPercentage > 0 && fillPercentage <= 100, "Invalid fill percentage");
@@ -447,8 +447,9 @@ TxQ::canBeHeld(
447447
}
448448

449449
auto
450-
TxQ::erase(TxQ::FeeMultiSet::const_iterator_type candidateIter)
451-
-> FeeMultiSet::iterator_type
450+
TxQ::erase(
451+
TxQ::FeeMultiSet::const_iterator_type candidateIter,
452+
std::lock_guard<std::mutex> const&) -> FeeMultiSet::iterator_type
452453
{
453454
auto& txQAccount = byAccount_.at(candidateIter->account);
454455
auto const seqProx = candidateIter->seqProxy;
@@ -463,8 +464,9 @@ TxQ::erase(TxQ::FeeMultiSet::const_iterator_type candidateIter)
463464
}
464465

465466
auto
466-
TxQ::eraseAndAdvance(TxQ::FeeMultiSet::const_iterator_type candidateIter)
467-
-> FeeMultiSet::iterator_type
467+
TxQ::eraseAndAdvance(
468+
TxQ::FeeMultiSet::const_iterator_type candidateIter,
469+
std::lock_guard<std::mutex> const&) -> FeeMultiSet::iterator_type
468470
{
469471
auto& txQAccount = byAccount_.at(candidateIter->account);
470472
auto const accountIter =
@@ -505,7 +507,8 @@ auto
505507
TxQ::erase(
506508
TxQ::TxQAccount& txQAccount,
507509
TxQ::TxQAccount::TxMap::const_iterator begin,
508-
TxQ::TxQAccount::TxMap::const_iterator end) -> TxQAccount::TxMap::iterator
510+
TxQ::TxQAccount::TxMap::const_iterator end,
511+
std::lock_guard<std::mutex> const&) -> TxQAccount::TxMap::iterator
509512
{
510513
for (auto it = begin; it != end; ++it)
511514
{
@@ -526,6 +529,7 @@ TxQ::tryClearAccountQueueUpThruTx(
526529
std::size_t const txExtraCount,
527530
ApplyFlags flags,
528531
FeeMetrics::Snapshot const& metricsSnapshot,
532+
std::lock_guard<std::mutex> const& lock,
529533
beast::Journal j)
530534
{
531535
SeqProxy const tSeqProx{tx.getSeqProxy()};
@@ -603,11 +607,11 @@ TxQ::tryClearAccountQueueUpThruTx(
603607
{
604608
// All of the queued transactions applied, so remove them from the
605609
// queue.
606-
endTxIter = erase(accountIter->second, beginTxIter, endTxIter);
610+
endTxIter = erase(accountIter->second, beginTxIter, endTxIter, lock);
607611
// If `tx` is replacing a queued tx, delete that one, too.
608612
if (endTxIter != accountIter->second.transactions.end() &&
609613
endTxIter->first == tSeqProx)
610-
erase(accountIter->second, endTxIter, std::next(endTxIter));
614+
erase(accountIter->second, endTxIter, std::next(endTxIter), lock);
611615
}
612616

613617
return txResult;
@@ -1212,6 +1216,7 @@ TxQ::apply(
12121216
view.txCount(),
12131217
flags,
12141218
metricsSnapshot,
1219+
lock,
12151220
j);
12161221
if (result.applied)
12171222
{
@@ -1240,7 +1245,7 @@ TxQ::apply(
12401245
// If the queue is full, decide whether to drop the current
12411246
// transaction or the last transaction for the account with
12421247
// the lowest fee.
1243-
if (!replacedTxIter && isFull())
1248+
if (!replacedTxIter && isFull(lock))
12441249
{
12451250
auto lastRIter = byFee_.rbegin();
12461251
while (lastRIter != byFee_.rend() && lastRIter->account == account)
@@ -1303,7 +1308,7 @@ TxQ::apply(
13031308
<< " from queue with average fee of " << endEffectiveFeeLevel
13041309
<< " in favor of " << transactionID << " with fee of "
13051310
<< feeLevelPaid;
1306-
erase(byFee_.iterator_to(dropRIter->second));
1311+
erase(byFee_.iterator_to(dropRIter->second), lock);
13071312
}
13081313
else
13091314
{
@@ -1317,7 +1322,7 @@ TxQ::apply(
13171322
// Hold the transaction in the queue.
13181323
if (replacedTxIter)
13191324
{
1320-
replacedTxIter = removeFromByFee(replacedTxIter, tx);
1325+
replacedTxIter = removeFromByFee(replacedTxIter, tx, lock);
13211326
}
13221327

13231328
if (!accountIsInQueue)
@@ -1382,7 +1387,7 @@ TxQ::processClosedLedger(Application& app, ReadView const& view, bool timeLeap)
13821387
if (candidateIter->lastValid && *candidateIter->lastValid <= ledgerSeq)
13831388
{
13841389
byAccount_.at(candidateIter->account).dropPenalty = true;
1385-
candidateIter = erase(candidateIter);
1390+
candidateIter = erase(candidateIter, lock);
13861391
}
13871392
else
13881393
{
@@ -1486,7 +1491,7 @@ TxQ::accept(Application& app, OpenView& view)
14861491
<< " applied successfully with " << transToken(txnResult)
14871492
<< ". Remove from queue.";
14881493

1489-
candidateIter = eraseAndAdvance(candidateIter);
1494+
candidateIter = eraseAndAdvance(candidateIter, lock);
14901495
ledgerChanged = true;
14911496
}
14921497
else if (
@@ -1500,7 +1505,7 @@ TxQ::accept(Application& app, OpenView& view)
15001505
JLOG(j_.debug()) << "Queued transaction " << candidateIter->txID
15011506
<< " failed with " << transToken(txnResult)
15021507
<< ". Remove from queue.";
1503-
candidateIter = eraseAndAdvance(candidateIter);
1508+
candidateIter = eraseAndAdvance(candidateIter, lock);
15041509
}
15051510
else
15061511
{
@@ -1515,7 +1520,7 @@ TxQ::accept(Application& app, OpenView& view)
15151520
--candidateIter->retriesRemaining;
15161521
candidateIter->lastResult = txnResult;
15171522
if (account.dropPenalty && account.transactions.size() > 1 &&
1518-
isFull<95>())
1523+
isFull<95>(lock))
15191524
{
15201525
// The queue is close to full, this account has multiple
15211526
// txs queued, and this account has had a transaction
@@ -1530,7 +1535,7 @@ TxQ::accept(Application& app, OpenView& view)
15301535
<< transToken(txnResult)
15311536
<< ". Removing ticketed tx from account "
15321537
<< account.account;
1533-
candidateIter = eraseAndAdvance(candidateIter);
1538+
candidateIter = eraseAndAdvance(candidateIter, lock);
15341539
}
15351540
else
15361541
{
@@ -1551,7 +1556,7 @@ TxQ::accept(Application& app, OpenView& view)
15511556
<< account.account;
15521557
auto endIter = byFee_.iterator_to(dropRIter->second);
15531558
if (endIter != candidateIter)
1554-
erase(endIter);
1559+
erase(endIter, lock);
15551560
++candidateIter;
15561561
}
15571562
}
@@ -1698,8 +1703,8 @@ TxQ::tryDirectApply(
16981703
if (txSeqProx.isSeq() && txSeqProx != acctSeqProx)
16991704
return {};
17001705

1701-
FeeLevel64 const requiredFeeLevel = [this, &view, flags]() {
1702-
std::lock_guard lock(mutex_);
1706+
std::lock_guard lock(mutex_);
1707+
FeeLevel64 const requiredFeeLevel = [this, &view, flags, &lock]() {
17031708
return getRequiredFeeLevel(
17041709
view, flags, feeMetrics_.getSnapshot(), lock);
17051710
}();
@@ -1727,7 +1732,6 @@ TxQ::tryDirectApply(
17271732
{
17281733
// If the applied transaction replaced a transaction in the
17291734
// queue then remove the replaced transaction.
1730-
std::lock_guard lock(mutex_);
17311735

17321736
AccountMap::iterator accountIter = byAccount_.find(account);
17331737
if (accountIter != byAccount_.end())
@@ -1737,7 +1741,7 @@ TxQ::tryDirectApply(
17371741
txQAcct.transactions.find(txSeqProx);
17381742
existingIter != txQAcct.transactions.end())
17391743
{
1740-
removeFromByFee(existingIter, tx);
1744+
removeFromByFee(existingIter, tx, lock);
17411745
}
17421746
}
17431747
}
@@ -1749,7 +1753,8 @@ TxQ::tryDirectApply(
17491753
std::optional<TxQ::TxQAccount::TxMap::iterator>
17501754
TxQ::removeFromByFee(
17511755
std::optional<TxQAccount::TxMap::iterator> const& replacedTxIter,
1752-
std::shared_ptr<STTx const> const& tx)
1756+
std::shared_ptr<STTx const> const& tx,
1757+
std::lock_guard<std::mutex> const& lock)
17531758
{
17541759
if (replacedTxIter && tx)
17551760
{
@@ -1769,7 +1774,7 @@ TxQ::removeFromByFee(
17691774
deleteIter->account == (*tx)[sfAccount],
17701775
"ripple::TxQ::removeFromByFee : matching account");
17711776

1772-
erase(deleteIter);
1777+
erase(deleteIter, lock);
17731778
}
17741779
return std::nullopt;
17751780
}
@@ -1789,7 +1794,7 @@ TxQ::getMetrics(OpenView const& view) const
17891794
result.txPerLedger = snapshot.txnsExpected;
17901795
result.referenceFeeLevel = baseLevel;
17911796
result.minProcessingFeeLevel =
1792-
isFull() ? byFee_.rbegin()->feeLevel + FeeLevel64{1} : baseLevel;
1797+
isFull(lock) ? byFee_.rbegin()->feeLevel + FeeLevel64{1} : baseLevel;
17931798
result.medFeeLevel = snapshot.escalationMultiplier;
17941799
result.openLedgerFeeLevel = FeeMetrics::scaleFeeLevel(snapshot, view);
17951800

0 commit comments

Comments
 (0)