Skip to content

Add option to validate transactions in the background #4617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: release/v22.3.0
Choose a base branch
from
+676 −338
Open

Add option to validate transactions in the background #4617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/stellar-core_example.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,11 @@ BACKGROUND_OVERLAY_PROCESSING = true
# performance on multicore machines. Note that this is not compatible with SQLite.
EXPERIMENTAL_PARALLEL_LEDGER_APPLY = false

# EXPERIMENTAL_BACKGROUND_TX_VALIDATION (bool) default false
# Validates transactions received over the network in the background, which can
# lead to better performance on multicore machines.
EXPERIMENTAL_BACKGROUND_TX_VALIDATION = false

# PREFERRED_PEERS (list of strings) default is empty
# These are IP:port strings that this server will add to its DB of peers.
# This server will try to always stay connected to the other peers on this list.
Expand Down
2 changes: 1 addition & 1 deletion src/bucket/test/BucketTestUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ LedgerManagerForBucketTests::sealLedgerTxnAndTransferEntriesToBucketList(
}

LedgerManagerForBucketTests&
BucketTestApplication::getLedgerManager()
BucketTestApplication::getLedgerManager() const
{
auto& lm = ApplicationImpl::getLedgerManager();
return static_cast<LedgerManagerForBucketTests&>(lm);
Expand Down
2 changes: 1 addition & 1 deletion src/bucket/test/BucketTestUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class BucketTestApplication : public TestApplication
{
}

virtual LedgerManagerForBucketTests& getLedgerManager() override;
virtual LedgerManagerForBucketTests& getLedgerManager() const override;

private:
virtual std::unique_ptr<LedgerManager>
Expand Down
5 changes: 3 additions & 2 deletions src/herder/Herder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,11 @@ class Herder
// generator, and therefore can skip certain expensive validity checks
virtual TransactionQueue::AddResult
recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
bool isLoadgenTx = false) = 0;
bool isPreValidated, bool isLoadgenTx = false) = 0;
#else
virtual TransactionQueue::AddResult
recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf) = 0;
recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
bool isPreValidated) = 0;
#endif
virtual void peerDoesntHave(stellar::MessageType type,
uint256 const& itemID, Peer::pointer peer) = 0;
Expand Down
14 changes: 10 additions & 4 deletions src/herder/HerderImpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,8 @@ HerderImpl::emitEnvelope(SCPEnvelope const& envelope)
}

TransactionQueue::AddResult
HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf
HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
bool isPreValidated
#ifdef BUILD_TESTS
,
bool isLoadgenTx
Expand Down Expand Up @@ -622,7 +623,7 @@ HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf
}
else if (!tx->isSoroban())
{
result = mTransactionQueue.tryAdd(tx, submittedFromSelf
result = mTransactionQueue.tryAdd(tx, submittedFromSelf, isPreValidated
#ifdef BUILD_TESTS
,
isLoadgenTx
Expand All @@ -631,7 +632,8 @@ HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf
}
else if (mSorobanTransactionQueue)
{
result = mSorobanTransactionQueue->tryAdd(tx, submittedFromSelf
result = mSorobanTransactionQueue->tryAdd(tx, submittedFromSelf,
isPreValidated
#ifdef BUILD_TESTS
,
isLoadgenTx
Expand Down Expand Up @@ -1174,6 +1176,9 @@ HerderImpl::lastClosedLedgerIncreased(bool latest, TxSetXDRFrameConstPtr txSet)
// applied transactions.
updateTransactionQueue(txSet);

// Update snapshots used for transaction validation
mApp.getOverlayManager().updateSnapshots();

// If we're in sync and there are no buffered ledgers to apply, trigger next
// ledger
if (latest)
Expand Down Expand Up @@ -2356,7 +2361,8 @@ HerderImpl::updateTransactionQueue(TxSetXDRFrameConstPtr externalizedTxSet)

auto invalidTxs = TxSetUtils::getInvalidTxList(
txs, mApp, 0,
getUpperBoundCloseTimeOffset(mApp, lhhe.header.scpValue.closeTime));
getUpperBoundCloseTimeOffset(mApp.getAppConnector(),
lhhe.header.scpValue.closeTime));
queue.ban(invalidTxs);

queue.rebroadcast();
Expand Down
8 changes: 4 additions & 4 deletions src/herder/HerderImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,11 @@ class HerderImpl : public Herder
#ifdef BUILD_TESTS
TransactionQueue::AddResult
recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
bool isLoadgenTx = false) override;
bool isPreValidated, bool isLoadgenTx = false) override;
#else
TransactionQueue::AddResult
recvTransaction(TransactionFrameBasePtr tx,
bool submittedFromSelf) override;
TransactionQueue::AddResult recvTransaction(TransactionFrameBasePtr tx,
bool submittedFromSelf,
bool isPreValidated) override;
#endif

EnvelopeStatus recvSCPEnvelope(SCPEnvelope const& envelope) override;
Expand Down
29 changes: 11 additions & 18 deletions src/herder/TransactionQueue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,8 @@ validateSorobanMemo(TransactionFrameBasePtr tx)
TransactionQueue::AddResult
TransactionQueue::canAdd(
TransactionFrameBasePtr tx, AccountStates::iterator& stateIter,
std::vector<std::pair<TransactionFrameBasePtr, bool>>& txsToEvict
std::vector<std::pair<TransactionFrameBasePtr, bool>>& txsToEvict,
bool isPreValidated
#ifdef BUILD_TESTS
,
bool isLoadgenTx
Expand All @@ -355,6 +356,7 @@ TransactionQueue::canAdd(

stateIter = mAccountStates.find(tx->getSourceID());
TransactionFrameBasePtr currentTx;
ExtendedLedgerSnapshot ls(mApp);
if (stateIter != mAccountStates.end())
{
auto const& transaction = stateIter->second.mTransaction;
Expand Down Expand Up @@ -386,14 +388,7 @@ TransactionQueue::canAdd(
if (tx->isSoroban())
{
auto txResult = tx->createSuccessResult();
if (!tx->checkSorobanResourceAndSetError(
mApp.getAppConnector(),
mApp.getLedgerManager()
.getLastClosedSorobanNetworkConfig(),
mApp.getLedgerManager()
.getLastClosedLedgerHeader()
.header.ledgerVersion,
txResult))
if (!tx->checkSorobanResourceAndSetError(ls, txResult))
{
return AddResult(AddResultCode::ADD_STATUS_ERROR, txResult);
}
Expand Down Expand Up @@ -433,7 +428,6 @@ TransactionQueue::canAdd(
}
}

LedgerSnapshot ls(mApp);
uint32_t ledgerVersion = ls.getLedgerHeader().current().ledgerVersion;
// Subtle: transactions are rejected based on the source account limit
// prior to this point. This is safe because we can't evict transactions
Expand Down Expand Up @@ -469,13 +463,11 @@ TransactionQueue::canAdd(
// Loadgen txs were generated by this local node, and therefore can skip
// validation, and be added directly to the queue.
auto txResult = tx->createSuccessResult();
#ifdef BUILD_TESTS
if (!isLoadgenTx)
#endif
if (!isPreValidated)
{
txResult =
tx->checkValid(mApp.getAppConnector(), ls, 0, 0,
getUpperBoundCloseTimeOffset(mApp, closeTime));
txResult = tx->checkValid(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there visible performance benefits when doing checkValid vs just calling verifySig on all the signers to populate the existing cache? Reason I'm asking is because skipping checkValid here means that we might be potentially adding invalid transactions to the queue (if ledger state changed while the transaction was being processed). This can be handled, but I wanted to make sure we'd actually get the benefits compared to a much simpler verifySig option. Note that in case of verifySig, we could simply call it in Peer::recvAuthenticatedMessage similar how it's done for SCP messages, and then still do checkValid on main thread. Because verifySig uses the global cache, we would just get a cache hit and avoid the extra cost of signature verification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From tracy testing, signature verification makes up 2/3s to 3/4s of checkValid. So it's the largest component, but it's also not everything. I'd expect that if we were to scope this down to just signature verifcation, we'd get results that are slightly worse than 2/3s of 3/4s of what we're seeing here, just due to potential lock contention within the cache and the small possibility for bad luck with eviction (it's a random eviction cache, right?).

As far as invalid transactions in the queue go, I was under the impression that the validity of transactions in the queue was already "best effort", especially with Soroban where we can't validate much anyway? That's not to say we should make the problem "worse" without a good reason, but surely we already handle invalid transactions in the queue, right?

ls, 0, 0,
getUpperBoundCloseTimeOffset(mApp.getAppConnector(), closeTime));
}
if (!txResult->isSuccess())
{
Expand Down Expand Up @@ -663,7 +655,8 @@ TransactionQueue::findAllAssetPairsInvolvedInPaymentLoops(
}

TransactionQueue::AddResult
TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf
TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf,
bool isPreValidated
#ifdef BUILD_TESTS
,
bool isLoadgenTx
Expand All @@ -689,7 +682,7 @@ TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf
AccountStates::iterator stateIter;

std::vector<std::pair<TransactionFrameBasePtr, bool>> txsToEvict;
auto const res = canAdd(tx, stateIter, txsToEvict
auto const res = canAdd(tx, stateIter, txsToEvict, isPreValidated
#ifdef BUILD_TESTS
,
isLoadgenTx
Expand Down
17 changes: 13 additions & 4 deletions src/herder/TransactionQueue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,19 @@ class TransactionQueue
static std::vector<AssetPair>
findAllAssetPairsInvolvedInPaymentLoops(TransactionFrameBasePtr tx);

/**
* Try to add `tx` to the queue. If the transaction is submitted from self
* (not over the network), then set `submittedFromSelf` to true. If the
* transaction has already been validated (via background transaction
* validation, or loadgen), then set `isPreValidated` to true. If the
* transaction is a loadgen transaction, set `isLoadgenTx` to true.
*/
#ifdef BUILD_TESTS
AddResult tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf,
bool isLoadgenTx = false);
bool isPreValidated, bool isLoadgenTx);
#else
AddResult tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf);
AddResult tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf,
bool isPreValidated);
#endif
void removeApplied(Transactions const& txs);
// Ban transactions that are no longer valid or have insufficient fee;
Expand Down Expand Up @@ -239,11 +247,12 @@ class TransactionQueue
TransactionQueue::AddResult
canAdd(TransactionFrameBasePtr tx, AccountStates::iterator& stateIter,
std::vector<std::pair<TransactionFrameBasePtr, bool>>& txsToEvict,
bool isLoadgenTx = false);
bool isPreValidated, bool isLoadgenTx = false);
#else
TransactionQueue::AddResult
canAdd(TransactionFrameBasePtr tx, AccountStates::iterator& stateIter,
std::vector<std::pair<TransactionFrameBasePtr, bool>>& txsToEvict);
std::vector<std::pair<TransactionFrameBasePtr, bool>>& txsToEvict,
bool isPreValidated);
#endif

void releaseFeeMaybeEraseAccountState(TransactionFrameBasePtr tx);
Expand Down
5 changes: 2 additions & 3 deletions src/herder/TxSetFrame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1814,13 +1814,12 @@ TxSetPhaseFrame::txsAreValid(Application& app,

// Grab read-only latest ledger state; This is only used to validate tx sets
// for LCL+1
LedgerSnapshot ls(app);
ExtendedLedgerSnapshot ls(app);
ls.getLedgerHeader().currentToModify().ledgerSeq =
app.getLedgerManager().getLastClosedLedgerNum() + 1;
for (auto const& tx : *this)
{
auto txResult = tx->checkValid(app.getAppConnector(), ls, 0,
lowerBoundCloseTimeOffset,
auto txResult = tx->checkValid(ls, 0, lowerBoundCloseTimeOffset,
upperBoundCloseTimeOffset);
if (!txResult->isSuccess())
{
Expand Down
5 changes: 2 additions & 3 deletions src/herder/TxSetUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ TxSetUtils::getInvalidTxList(TxFrameList const& txs, Application& app,
{
ZoneScoped;
releaseAssert(threadIsMain());
LedgerSnapshot ls(app);
ExtendedLedgerSnapshot ls(app);
// This is done so minSeqLedgerGap is validated against the next
// ledgerSeq, which is what will be used at apply time
ls.getLedgerHeader().currentToModify().ledgerSeq =
Expand All @@ -178,8 +178,7 @@ TxSetUtils::getInvalidTxList(TxFrameList const& txs, Application& app,

for (auto const& tx : txs)
{
auto txResult = tx->checkValid(app.getAppConnector(), ls, 0,
lowerBoundCloseTimeOffset,
auto txResult = tx->checkValid(ls, 0, lowerBoundCloseTimeOffset,
upperBoundCloseTimeOffset);
if (!txResult->isSuccess())
{
Expand Down
43 changes: 28 additions & 15 deletions src/herder/test/HerderTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,9 @@ TEST_CASE_VERSIONS("standalone", "[herder][acceptance]")

auto feedTx = [&](TransactionTestFramePtr tx,
TransactionQueue::AddResultCode expectedRes) {
REQUIRE(app->getHerder().recvTransaction(tx, false).code ==
expectedRes);
REQUIRE(
app->getHerder().recvTransaction(tx, false, false).code ==
expectedRes);
};

auto waitForExternalize = [&]() {
Expand Down Expand Up @@ -2934,11 +2935,11 @@ TEST_CASE("tx queue source account limit", "[herder][transactionqueue]")
auto [root, a1, b1, tx1, tx2] = makeTxs(app);

// Submit txs for the same account, should be good
REQUIRE(app->getHerder().recvTransaction(tx1, true).code ==
REQUIRE(app->getHerder().recvTransaction(tx1, true, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_PENDING);

// Second tx is rejected due to limit
REQUIRE(app->getHerder().recvTransaction(tx2, true).code ==
REQUIRE(app->getHerder().recvTransaction(tx2, true, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_TRY_AGAIN_LATER);

uint32_t lcl = app->getLedgerManager().getLastClosedLedgerNum();
Expand All @@ -2965,7 +2966,7 @@ TEST_CASE("tx queue source account limit", "[herder][transactionqueue]")

// Now submit the second tx (which was rejected earlier) and make sure
// it ends up in the ledger
REQUIRE(app->getHerder().recvTransaction(tx2, true).code ==
REQUIRE(app->getHerder().recvTransaction(tx2, true, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_PENDING);

lcl = app->getLedgerManager().getLastClosedLedgerNum();
Expand Down Expand Up @@ -3234,6 +3235,18 @@ TEST_CASE("overlay parallel processing")
});
}

SECTION("background transaction validation")
{
// Set threshold to 1 so all have to vote
simulation =
Topologies::core(4, 1, Simulation::OVER_TCP, networkID, [](int i) {
auto cfg = getTestConfig(i);
cfg.TESTING_UPGRADE_MAX_TX_SET_SIZE = 100;
cfg.BACKGROUND_TX_VALIDATION = true;
return cfg;
});
}

// Background ledger close requires postgres
#ifdef USE_POSTGRES
SECTION("background ledger close")
Expand Down Expand Up @@ -3645,7 +3658,7 @@ herderExternalizesValuesWithProtocol(uint32_t version,
auto sorobanTx = createUploadWasmTx(
*A, *root, 100, DEFAULT_TEST_RESOURCE_FEE, resources);
REQUIRE(
herderA.recvTransaction(sorobanTx, true).code ==
herderA.recvTransaction(sorobanTx, true, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_PENDING);
submitted = true;
}
Expand Down Expand Up @@ -4332,9 +4345,9 @@ TEST_CASE("do not flood invalid transactions", "[herder]")
// this will be invalid after tx1r gets applied
auto tx2r = root->tx({payment(*root, 1)});

herder.recvTransaction(tx1a, false);
herder.recvTransaction(tx1r, false);
herder.recvTransaction(tx2r, false);
herder.recvTransaction(tx1a, false, false);
herder.recvTransaction(tx1r, false, false);
herder.recvTransaction(tx2r, false, false);

size_t numBroadcast = 0;
tq.mTxBroadcastedEvent = [&](TransactionFrameBasePtr&) { ++numBroadcast; };
Expand Down Expand Up @@ -4447,7 +4460,7 @@ TEST_CASE("do not flood too many soroban transactions",

auto tx = createUploadWasmTx(*app, source, inclusionFee, 10'000'000,
resources);
REQUIRE(herder.recvTransaction(tx, false).code ==
REQUIRE(herder.recvTransaction(tx, false, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_PENDING);
return tx;
};
Expand Down Expand Up @@ -4622,7 +4635,7 @@ TEST_CASE("do not flood too many transactions", "[herder][transactionqueue]")
curFeeOffset--;
}

REQUIRE(herder.recvTransaction(tx, false).code ==
REQUIRE(herder.recvTransaction(tx, false, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_PENDING);
return tx;
};
Expand Down Expand Up @@ -4832,7 +4845,7 @@ TEST_CASE("do not flood too many transactions with DEX separation",
curFeeOffset--;
}

REQUIRE(herder.recvTransaction(tx, false).code ==
REQUIRE(herder.recvTransaction(tx, false, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_PENDING);
return tx;
};
Expand Down Expand Up @@ -5260,7 +5273,7 @@ TEST_CASE("exclude transactions by operation type", "[herder]")
auto acc = getAccount("acc");
auto tx = root->tx({createAccount(acc.getPublicKey(), 1)});

REQUIRE(app->getHerder().recvTransaction(tx, false).code ==
REQUIRE(app->getHerder().recvTransaction(tx, false, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_PENDING);
}

Expand All @@ -5275,7 +5288,7 @@ TEST_CASE("exclude transactions by operation type", "[herder]")
auto acc = getAccount("acc");
auto tx = root->tx({createAccount(acc.getPublicKey(), 1)});

REQUIRE(app->getHerder().recvTransaction(tx, false).code ==
REQUIRE(app->getHerder().recvTransaction(tx, false, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_FILTERED);
}

Expand All @@ -5291,7 +5304,7 @@ TEST_CASE("exclude transactions by operation type", "[herder]")
auto acc = getAccount("acc");
auto tx = root->tx({createAccount(acc.getPublicKey(), 1)});

REQUIRE(app->getHerder().recvTransaction(tx, false).code ==
REQUIRE(app->getHerder().recvTransaction(tx, false, false).code ==
TransactionQueue::AddResultCode::ADD_STATUS_PENDING);
}
}
Expand Down
Loading