Skip to content

Commit

Permalink
Improved TransactionResultPayload interface
Browse files Browse the repository at this point in the history
  • Loading branch information
@SirTyson
SirTyson committed May 16, 2024
1 parent e95fcb9 commit 709e745
Show file tree
Hide file tree
Showing 13 changed files with 730 additions and 451 deletions.
40 changes: 25 additions & 15 deletions src/herder/TransactionQueue.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,22 @@ isDuplicateTx(TransactionFrameBasePtr oldTx, TransactionFrameBasePtr newTx)
}
else if (oldEnv.type() == ENVELOPE_TYPE_TX_FEE_BUMP)
{
auto const& oldFeeBump = oldTx->toFeeBumpTransactionFrame();
return oldFeeBump.getInnerFullHash() == newTx->getFullHash();
std::shared_ptr<FeeBumpTransactionFrame const> feeBumpPtr{};
#ifdef BUILD_TESTS
if (oldTx->isTestTx())
{
auto testFrame =
std::dynamic_pointer_cast<TransactionTestFrame const>(oldTx);
feeBumpPtr =
std::dynamic_pointer_cast<FeeBumpTransactionFrame const>(
testFrame->getTxFramePtr());
}
else
#endif
feeBumpPtr =
std::dynamic_pointer_cast<FeeBumpTransactionFrame const>(oldTx);
releaseAssertOrThrow(feeBumpPtr);
return feeBumpPtr->getInnerFullHash() == newTx->getFullHash();
}
return false;
}
Expand Down Expand Up @@ -253,8 +267,7 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
int64_t newFullFee = tx->getFullFee();
if (newFullFee < 0 || tx->getInclusionFee() < 0)
{
auto resPayload =
TransactionResultPayload::create(tx->toTransactionFrame(), 0);
auto resPayload = tx->createResultPayload();
resPayload->getResult().result.code(txMALFORMED);
return {TransactionQueue::AddResult::ADD_STATUS_ERROR, resPayload};
}
Expand All @@ -281,8 +294,7 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
{
// If the transaction is older than the one in the queue, we
// reject it
auto resPayload = TransactionResultPayload::create(
tx->toTransactionFrame(), 0);
auto resPayload = tx->createResultPayload();
resPayload->getResult().result.code(txBAD_SEQ);
return {TransactionQueue::AddResult::ADD_STATUS_ERROR,
resPayload};
Expand All @@ -293,14 +305,13 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
// appropriate error message
if (tx->isSoroban())
{
auto resPayload = TransactionResultPayload::create(
tx->toTransactionFrame(), 0);
auto resPayload = tx->createResultPayload();
if (!tx->checkSorobanResourceAndSetError(
mApp,
mApp.getLedgerManager()
.getLastClosedLedgerHeader()
.header.ledgerVersion,
*resPayload))
resPayload))
{
return {AddResult::ADD_STATUS_ERROR, resPayload};
}
Expand All @@ -326,8 +337,8 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
int64_t minFee;
if (!canReplaceByFee(tx, currentTx, minFee))
{
auto resPayload = TransactionResultPayload::create(
tx->toTransactionFrame(), minFee);
auto resPayload = tx->createResultPayload();
resPayload->getResult().feeCharged = minFee;
resPayload->getResult().result.code(txINSUFFICIENT_FEE);
return {TransactionQueue::AddResult::ADD_STATUS_ERROR,
resPayload};
Expand Down Expand Up @@ -355,8 +366,8 @@ TransactionQueue::canAdd(TransactionFrameBasePtr tx,
ban({tx});
if (canAddRes.second != 0)
{
auto resPayload = TransactionResultPayload::create(
tx->toTransactionFrame(), canAddRes.second);
auto resPayload = tx->createResultPayload();
resPayload->getResult().feeCharged = canAddRes.second;
resPayload->getResult().result.code(txINSUFFICIENT_FEE);
return {TransactionQueue::AddResult::ADD_STATUS_ERROR, resPayload};
}
Expand Down Expand Up @@ -550,8 +561,7 @@ TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf)
// fast fail when Soroban tx is malformed
if ((tx->isSoroban() != (c1 || c2)) || !tx->XDRProvidesValidFee())
{
auto resPayload =
TransactionResultPayload::create(tx->toTransactionFrame(), 0);
auto resPayload = tx->createResultPayload();
resPayload->getResult().result.code(txMALFORMED);
return {TransactionQueue::AddResult::ADD_STATUS_ERROR, resPayload};
}
Expand Down
4 changes: 2 additions & 2 deletions src/ledger/LedgerManagerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1522,7 +1522,7 @@ LedgerManagerImpl::applyTransactions(
{
ZoneNamedN(txZone, "applyTransaction", true);
auto tx = txs.at(i);
auto& txResult = *txResults.at(i);
auto txResult = txResults.at(i);

auto txTime = mTransactionApply.TimeScope();
TransactionMetaFrame tm(ltx.loadHeader().current().ledgerVersion);
Expand All @@ -1546,7 +1546,7 @@ LedgerManagerImpl::applyTransactions(
tx->processPostApply(mApp, ltx, tm, txResult);
TransactionResultPair results;
results.transactionHash = tx->getContentsHash();
results.result = txResult.getResult();
results.result = txResult->getResult();
if (results.result.result.code() == TransactionResultCode::txSUCCESS)
{
if (tx->isSoroban())
Expand Down
2 changes: 1 addition & 1 deletion src/test/FuzzerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ class FuzzTransactionFrame : public TransactionFrame
FuzzTransactionFrame(Hash const& networkID,
TransactionEnvelope const& envelope)
: TransactionFrame(networkID, envelope)
, mResultPayload(TransactionResultPayload::create(*this, 0)){};
, mResultPayload(createResultPayload()){};

void
attemptApplication(Application& app, AbstractLedgerTxn& ltx)
Expand Down
124 changes: 46 additions & 78 deletions src/transactions/FeeBumpTransactionFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,42 +72,10 @@ FeeBumpTransactionFrame::FeeBumpTransactionFrame(
}
#endif

static void
updateResult(TransactionResultPayload& resPayload,
TransactionFrameBasePtr innerTx)
{
releaseAssert(resPayload.isFeeBump());
if (resPayload.getInnerResult().result.code() == txSUCCESS)
{
resPayload.getResult().result.code(txFEE_BUMP_INNER_SUCCESS);
}
else
{
resPayload.getResult().result.code(txFEE_BUMP_INNER_FAILED);
}

auto& irp = resPayload.getResult().result.innerResultPair();
irp.transactionHash = innerTx->getContentsHash();

auto const& res = resPayload.getInnerResult();
auto& innerRes = irp.result;
innerRes.feeCharged = res.feeCharged;
innerRes.result.code(res.result.code());
switch (innerRes.result.code())
{
case txSUCCESS:
case txFAILED:
innerRes.result.results() = res.result.results();
break;
default:
break;
}
}

bool
FeeBumpTransactionFrame::apply(Application& app, AbstractLedgerTxn& ltx,
TransactionMetaFrame& meta,
TransactionResultPayload& resPayload,
TransactionResultPayloadPtr resPayload,
Hash const& sorobanBasePrngSeed) const
{
try
Expand All @@ -131,14 +99,19 @@ FeeBumpTransactionFrame::apply(Application& app, AbstractLedgerTxn& ltx,

try
{
bool res = mInnerTx->apply(app, ltx, meta, resPayload, false,
sorobanBasePrngSeed);
// If this throws, then we may not have the correct TransactionResult so
// we must crash.
// Note that even after updateResult is called here, feeCharged will not
// be accurate for Soroban transactions until
// FeeBumpTransactionFrame::processPostApply is called.
updateResult(resPayload, mInnerTx);
auto feeBumpPayload =
std::dynamic_pointer_cast<FeeBumpTransactionResultPayload>(
resPayload);
releaseAssertOrThrow(feeBumpPayload);
bool res = mInnerTx->apply(app, ltx, meta,
feeBumpPayload->getInnerResultPayload(),
false, sorobanBasePrngSeed);
feeBumpPayload->updateResult(mInnerTx);
return res;
}
catch (std::exception& e)
Expand All @@ -156,15 +129,18 @@ FeeBumpTransactionFrame::apply(Application& app, AbstractLedgerTxn& ltx,
void
FeeBumpTransactionFrame::processPostApply(
Application& app, AbstractLedgerTxn& ltx, TransactionMetaFrame& meta,
TransactionResultPayload& resPayload) const
TransactionResultPayloadPtr resPayload) const
{
auto feeBumpPayload =
std::dynamic_pointer_cast<FeeBumpTransactionResultPayload>(resPayload);
// We must forward the Fee-bump source so the refund is applied to the
// correct account
// Note that we are not calling TransactionFrame::processPostApply, so if
// any logic is added there, we would have to reason through if that logic
// should also be reflected here.
int64_t refund =
mInnerTx->processRefund(app, ltx, meta, getFeeSourceID(), resPayload);
mInnerTx->processRefund(app, ltx, meta, getFeeSourceID(),
*(feeBumpPayload->getInnerResultPayload()));

// The result codes and a feeCharged without the refund are set in
// updateResult in FeeBumpTransactionFrame::apply. At this point, feeCharged
Expand All @@ -176,12 +152,13 @@ FeeBumpTransactionFrame::processPostApply(
// First update feeCharged of the inner result on the feeBump using
// mInnerTx
{
auto& irp = resPayload.getResult().result.innerResultPair();
auto& irp = feeBumpPayload->getResult().result.innerResultPair();
auto& innerRes = irp.result;
innerRes.feeCharged = resPayload.getInnerResult().feeCharged;
innerRes.feeCharged =
feeBumpPayload->getInnerResultPayload()->getResult().feeCharged;

// Now set the updated feeCharged on the fee bump.
resPayload.getResult().feeCharged -= refund;
feeBumpPayload->getResult().feeCharged -= refund;
}
}
}
Expand Down Expand Up @@ -212,19 +189,17 @@ FeeBumpTransactionFrame::checkValid(Application& app,
{
if (!XDRProvidesValidFee())
{
auto resPayload = TransactionResultPayload::create(*mInnerTx, 0);
resPayload->initializeFeeBumpResult();
auto resPayload = createResultPayload();
resPayload->getResult().result.code(txMALFORMED);
return {false, resPayload};
}

LedgerTxn ltx(ltxOuter);
int64_t minBaseFee = ltx.loadHeader().current().baseFee;
auto resPayload = createResultPayloadWithFeeCharged(
ltx.loadHeader().current(), minBaseFee, false);

// TODO: Fix
auto toRemove = resPayload->getResult().feeCharged;
auto resPayload =
std::dynamic_pointer_cast<FeeBumpTransactionResultPayload>(
createResultPayloadWithFeeCharged(ltx.loadHeader().current(),
minBaseFee, false));
releaseAssert(resPayload);

SignatureChecker signatureChecker{ltx.loadHeader().current().ledgerVersion,
Expand All @@ -236,32 +211,31 @@ FeeBumpTransactionFrame::checkValid(Application& app,
return {false, resPayload};
}

releaseAssert(resPayload->isFeeBump());
if (!signatureChecker.checkAllSignaturesUsed())
{
resPayload->getResult().result.code(txBAD_AUTH_EXTRA);
return {false, resPayload};
}

// Replace result payload with inner TX result then update accordingly
bool res;
std::tie(res, resPayload) = mInnerTx->checkValidWithOptionallyChargedFee(
auto [res, innerResPayload] = mInnerTx->checkValidWithOptionallyChargedFee(
app, ltx, current, false, lowerBoundCloseTimeOffset,
upperBoundCloseTimeOffset);
resPayload->initializeFeeBumpResult();
updateResult(*resPayload, mInnerTx);
resPayload->getResult().feeCharged = toRemove;
resPayload->swapInnerResultPayload(innerResPayload);
resPayload->updateResult(mInnerTx);

return {res, resPayload};
}

bool
FeeBumpTransactionFrame::checkSorobanResourceAndSetError(
Application& app, uint32_t ledgerVersion,
TransactionResultPayload& resPayload) const
TransactionResultPayloadPtr resPayload) const
{
return mInnerTx->checkSorobanResourceAndSetError(app, ledgerVersion,
resPayload);
auto feeBumpPayload =
std::dynamic_pointer_cast<FeeBumpTransactionResultPayload>(resPayload);
releaseAssertOrThrow(feeBumpPayload);
return mInnerTx->checkSorobanResourceAndSetError(
app, ledgerVersion, feeBumpPayload->getInnerResultPayload());
}

bool
Expand All @@ -271,7 +245,6 @@ FeeBumpTransactionFrame::commonValidPreSeqNum(
// this function does validations that are independent of the account state
// (stay true regardless of other side effects)

releaseAssert(resPayload.isFeeBump());
auto header = ltx.loadHeader();
if (protocolVersionIsBefore(header.current().ledgerVersion,
ProtocolVersion::V_13))
Expand Down Expand Up @@ -330,7 +303,6 @@ FeeBumpTransactionFrame::commonValid(SignatureChecker& signatureChecker,
AbstractLedgerTxn& ltxOuter, bool applying,
TransactionResultPayload& resPayload) const
{
releaseAssert(resPayload.isFeeBump());
LedgerTxn ltx(ltxOuter);
ValidationType res = ValidationType::kInvalid;

Expand Down Expand Up @@ -389,18 +361,6 @@ FeeBumpTransactionFrame::clearCached() const
}
#endif

FeeBumpTransactionFrame const&
FeeBumpTransactionFrame::toFeeBumpTransactionFrame() const
{
return *this;
}

TransactionFrame const&
FeeBumpTransactionFrame::toTransactionFrame() const
{
return *mInnerTx;
}

int64_t
FeeBumpTransactionFrame::getFullFee() const
{
Expand Down Expand Up @@ -563,7 +523,6 @@ FeeBumpTransactionFrame::processFeeSeqNum(AbstractLedgerTxn& ltx,
auto resPayload = createResultPayloadWithFeeCharged(
ltx.loadHeader().current(), baseFee, true);
releaseAssert(resPayload);
releaseAssert(resPayload->isFeeBump());

auto feeSource = stellar::loadAccount(ltx, getFeeSourceID());
if (!feeSource)
Expand Down Expand Up @@ -614,18 +573,27 @@ FeeBumpTransactionFrame::createResultPayloadWithFeeCharged(
LedgerHeader const& header, std::optional<int64_t> baseFee,
bool applying) const
{
auto resPayload =
auto innerResPayload =
mInnerTx->createResultPayloadWithFeeCharged(header, baseFee, applying);

resPayload->initializeFeeBumpResult();
resPayload->getResult().result.code(txFEE_BUMP_INNER_SUCCESS);

// feeCharged is updated accordingly to represent the cost of the
// transaction regardless of the failure modes.
resPayload->getResult().feeCharged = getFee(header, baseFee, applying);
auto feeCharged = getFee(header, baseFee, applying);
std::shared_ptr<FeeBumpTransactionResultPayload> resPayload(
new FeeBumpTransactionResultPayload(innerResPayload));
resPayload->getResult().result.code(txFEE_BUMP_INNER_SUCCESS);
resPayload->getResult().feeCharged = feeCharged;

return resPayload;
}

TransactionResultPayloadPtr
FeeBumpTransactionFrame::createResultPayload() const
{
return TransactionResultPayloadPtr(
new FeeBumpTransactionResultPayload(mInnerTx->createResultPayload()));
}

std::shared_ptr<StellarMessage const>
FeeBumpTransactionFrame::toStellarMessage() const
{
Expand Down
Loading

0 comments on commit 709e745

Please sign in to comment.