Skip to content

Commit

Permalink
Include context on most calls to acquire
Browse files Browse the repository at this point in the history
  • Loading branch information
ximinez committed Sep 6, 2024
1 parent f290324 commit 4a265d4
Show file tree
Hide file tree
Showing 9 changed files with 61 additions and 23 deletions.
6 changes: 5 additions & 1 deletion src/ripple/app/ledger/InboundLedgers.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,11 @@ class InboundLedgers
// Callers should use this if they possibly need an authoritative
// response immediately.
virtual std::shared_ptr<Ledger const>
acquire(uint256 const& hash, std::uint32_t seq, InboundLedger::Reason) = 0;
acquire(
uint256 const& hash,
std::uint32_t seq,
InboundLedger::Reason,
const char* context = nullptr) = 0;

// Callers should use this if they are known to be executing on the Job
// Queue. TODO review whether all callers of acquire() can use this
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/app/ledger/impl/InboundLedgers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ class InboundLedgersImp : public InboundLedgers
acquire(
uint256 const& hash,
std::uint32_t seq,
InboundLedger::Reason reason) override
InboundLedger::Reason reason,
const char* context = nullptr) override
{
auto doAcquire = [&, seq, reason]() -> std::shared_ptr<Ledger const> {
assert(hash.isNonZero());
Expand Down Expand Up @@ -99,6 +100,7 @@ class InboundLedgersImp : public InboundLedgers
<< " NeedNetworkLedger: "
<< (app_.getOPs().isNeedNetworkLedger() ? "yes" : "no")
<< " Reason: " << to_string(reason)
<< " Context: " << (context ? context : "n/a")
<< " Should acquire: " << (shouldAcquire ? "true." : "false.");

/* Acquiring ledgers is somewhat expensive. It requires lots of
Expand Down
23 changes: 18 additions & 5 deletions src/ripple/app/ledger/impl/LedgerCleaner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ class LedgerCleanerImp : public LedgerCleaner
app_.getInboundLedgers().acquire(
ledger->info().hash,
ledger->info().seq,
InboundLedger::Reason::GENERIC);
InboundLedger::Reason::GENERIC,
"getLedgerHash");
}
return hash ? *hash : beast::zero; // kludge
}
Expand All @@ -273,13 +274,19 @@ class LedgerCleanerImp : public LedgerCleaner
bool doTxns)
{
auto nodeLedger = app_.getInboundLedgers().acquire(
ledgerHash, ledgerIndex, InboundLedger::Reason::GENERIC);
ledgerHash,
ledgerIndex,
InboundLedger::Reason::GENERIC,
"doLedger");
if (!nodeLedger)
{
JLOG(j_.debug()) << "Ledger " << ledgerIndex << " not available";
app_.getLedgerMaster().clearLedger(ledgerIndex);
app_.getInboundLedgers().acquire(
ledgerHash, ledgerIndex, InboundLedger::Reason::GENERIC);
ledgerHash,
ledgerIndex,
InboundLedger::Reason::GENERIC,
"doLedger not available");
return false;
}

Expand All @@ -305,7 +312,10 @@ class LedgerCleanerImp : public LedgerCleaner
JLOG(j_.debug()) << "Ledger " << ledgerIndex << " is missing nodes";
app_.getLedgerMaster().clearLedger(ledgerIndex);
app_.getInboundLedgers().acquire(
ledgerHash, ledgerIndex, InboundLedger::Reason::GENERIC);
ledgerHash,
ledgerIndex,
InboundLedger::Reason::GENERIC,
"doLedger missing nodes");
return false;
}

Expand Down Expand Up @@ -359,7 +369,10 @@ class LedgerCleanerImp : public LedgerCleaner
// We found the hash and sequence of a better reference
// ledger.
referenceLedger = app_.getInboundLedgers().acquire(
refHash, refIndex, InboundLedger::Reason::GENERIC);
refHash,
refIndex,
InboundLedger::Reason::GENERIC,
"getHash");
if (referenceLedger)
ledgerHash =
getLedgerHash(referenceLedger, ledgerIndex);
Expand Down
25 changes: 16 additions & 9 deletions src/ripple/app/ledger/impl/LedgerMaster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1020,7 +1020,8 @@ void
LedgerMaster::failedSave(std::uint32_t seq, uint256 const& hash)
{
clearLedger(seq);
app_.getInboundLedgers().acquire(hash, seq, InboundLedger::Reason::GENERIC);
app_.getInboundLedgers().acquire(
hash, seq, InboundLedger::Reason::GENERIC, "failedSave");
}

// Check if the specified ledger can become the new last fully-validated
Expand Down Expand Up @@ -1068,7 +1069,7 @@ LedgerMaster::checkAccept(uint256 const& hash, std::uint32_t seq)
// FIXME: We may not want to fetch a ledger with just one
// trusted validation
ledger = app_.getInboundLedgers().acquire(
hash, seq, InboundLedger::Reason::GENERIC);
hash, seq, InboundLedger::Reason::GENERIC, "checkAccept");
}

if (ledger)
Expand Down Expand Up @@ -1442,7 +1443,10 @@ LedgerMaster::findNewLedgersToPublish(
// Can we try to acquire the ledger we need?
if (!ledger && (++acqCount < ledger_fetch_size_))
ledger = app_.getInboundLedgers().acquire(
*hash, seq, InboundLedger::Reason::GENERIC);
*hash,
seq,
InboundLedger::Reason::GENERIC,
"findNewLedgersToPublish");
}

// Did we acquire the next ledger we need to publish?
Expand Down Expand Up @@ -1631,15 +1635,17 @@ LedgerMaster::updatePaths()
app_.getInboundLedgers().acquire(
lastLedger->info().parentHash,
lastLedger->info().seq - 1,
InboundLedger::Reason::GENERIC);
InboundLedger::Reason::GENERIC,
"updatePaths open");
}
else
{
// this ledger is the problem
app_.getInboundLedgers().acquire(
lastLedger->info().hash,
lastLedger->info().seq,
InboundLedger::Reason::GENERIC);
InboundLedger::Reason::GENERIC,
"updatePaths closed");
}
}
}
Expand Down Expand Up @@ -1858,7 +1864,7 @@ LedgerMaster::walkHashBySeq(
if (!ledger)
{
if (auto const l = app_.getInboundLedgers().acquire(
*refHash, refIndex, reason))
*refHash, refIndex, reason, "walkHashBySeq"))
{
ledgerHash = hashOfSeq(*l, index, m_journal);
assert(ledgerHash);
Expand Down Expand Up @@ -1979,8 +1985,8 @@ LedgerMaster::fetchForHistory(
{
if (!app_.getInboundLedgers().isFailure(*hash))
{
ledger =
app_.getInboundLedgers().acquire(*hash, missing, reason);
ledger = app_.getInboundLedgers().acquire(
*hash, missing, reason, "fetchForHistory");
if (!ledger && missing != fetch_seq_ &&
missing > app_.getNodeStore().earliestLedgerSeq())
{
Expand Down Expand Up @@ -2061,7 +2067,8 @@ LedgerMaster::fetchForHistory(
if (auto h = getLedgerHashForHistory(seq, reason))
{
assert(h->isNonZero());
app_.getInboundLedgers().acquire(*h, seq, reason);
app_.getInboundLedgers().acquire(
*h, seq, reason, "fetchForHistory no ledger");
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/ripple/app/misc/NetworkOPs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1724,7 +1724,10 @@ NetworkOPsImp::checkLastClosedLedger(

if (!consensus)
consensus = app_.getInboundLedgers().acquire(
closedLedger, 0, InboundLedger::Reason::CONSENSUS);
closedLedger,
0,
InboundLedger::Reason::CONSENSUS,
"checkLastClosedLedger");

if (consensus &&
(!m_ledgerMaster.canBeCurrent(consensus) ||
Expand Down
10 changes: 8 additions & 2 deletions src/ripple/rpc/impl/RPCHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1080,7 +1080,10 @@ getLedgerByContext(RPC::JsonContext& context)
// ledger they want. Try to get it.

if (auto il = context.app.getInboundLedgers().acquire(
*refHash, refIndex, InboundLedger::Reason::GENERIC))
*refHash,
refIndex,
InboundLedger::Reason::GENERIC,
"getLedgerByContext no hash"))
{
Json::Value jvResult = RPC::make_error(
rpcLGR_NOT_FOUND,
Expand Down Expand Up @@ -1112,7 +1115,10 @@ getLedgerByContext(RPC::JsonContext& context)
// Try to get the desired ledger
// Verify all nodes even if we think we have it
auto ledger = context.app.getInboundLedgers().acquire(
ledgerHash, ledgerIndex, InboundLedger::Reason::GENERIC);
ledgerHash,
ledgerIndex,
InboundLedger::Reason::GENERIC,
"getLedgerByContext");

// In standalone mode, accept the ledger from the ledger cache
if (!ledger && context.app.config().standalone())
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/shamap/impl/NodeFamily.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ NodeFamily::acquire(uint256 const& hash, std::uint32_t seq)
JLOG(j_.error()) << "Missing node in " << to_string(hash);

app_.getInboundLedgers().acquire(
hash, seq, InboundLedger::Reason::GENERIC);
hash, seq, InboundLedger::Reason::GENERIC, "NodeFamily::acquire");
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/ripple/shamap/impl/ShardFamily.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ ShardFamily::acquire(uint256 const& hash, std::uint32_t seq)
JLOG(j_.error()) << "Missing node in " << to_string(hash);

app_.getInboundLedgers().acquire(
hash, seq, InboundLedger::Reason::SHARD);
hash, seq, InboundLedger::Reason::SHARD, "ShardFamily::acquire");
}
}

Expand Down
7 changes: 5 additions & 2 deletions src/test/app/LedgerReplay_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,11 @@ class MagicInboundLedgers : public InboundLedgers
virtual ~MagicInboundLedgers() = default;

virtual std::shared_ptr<Ledger const>
acquire(uint256 const& hash, std::uint32_t seq, InboundLedger::Reason)
override
acquire(
uint256 const& hash,
std::uint32_t seq,
InboundLedger::Reason,
const char*) override
{
if (bhvr == InboundLedgersBehavior::DropAll)
return {};
Expand Down

0 comments on commit 4a265d4

Please sign in to comment.