Skip to content

Commit 8f2f531

Browse files
dangell7dangell7
authored
Fix: Improve error handling in Batch RPC response (#5503)
1 parent edb4f03 commit 8f2f531

File tree

4 files changed

+222
-22
lines changed

4 files changed

+222
-22
lines changed

‎src/libxrpl/protocol/STTx.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -760,6 +760,12 @@ isRawTransactionOkay(STObject const& st, std::string& reason)
760760
{
761761
TxType const tt =
762762
safe_cast<TxType>(raw.getFieldU16(sfTransactionType));
763+
if (tt == ttBATCH)
764+
{
765+
reason = "Raw Transactions may not contain batch transactions.";
766+
return false;
767+
}
768+
763769
raw.applyTemplate(getTxFormat(tt)->getSOTemplate());
764770
}
765771
catch (std::exception const& e)

‎src/test/app/Batch_test.cpp‎

Lines changed: 176 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include <xrpld/app/misc/HashRouter.h>
2525
#include <xrpld/app/misc/Transaction.h>
2626
#include <xrpld/app/tx/apply.h>
27+
#include <xrpld/app/tx/detail/Batch.h>
2728

2829
#include <xrpl/protocol/Batch.h>
2930
#include <xrpl/protocol/Feature.h>
@@ -317,15 +318,16 @@ class Batch_test : public beast::unit_test::suite
317318
env.close();
318319
}
319320

320-
// temINVALID: Batch: batch cannot have inner batch txn.
321+
// DEFENSIVE: temINVALID: Batch: batch cannot have inner batch txn.
322+
// ACTUAL: telENV_RPC_FAILED: isRawTransactionOkay()
321323
{
322324
auto const seq = env.seq(alice);
323325
auto const batchFee = batch::calcBatchFee(env, 0, 2);
324326
env(batch::outer(alice, seq, batchFee, tfAllOrNothing),
325327
batch::inner(
326328
batch::outer(alice, seq, batchFee, tfAllOrNothing), seq),
327329
batch::inner(pay(alice, bob, XRP(1)), seq + 2),
328-
ter(temINVALID));
330+
ter(telENV_RPC_FAILED));
329331
env.close();
330332
}
331333

@@ -3953,6 +3955,176 @@ class Batch_test : public beast::unit_test::suite
39533955
}
39543956
}
39553957

3958+
void
3959+
testValidateRPCResponse(FeatureBitset features)
3960+
{
3961+
// Verifying that the RPC response from submit includes
3962+
// the account_sequence_available, account_sequence_next,
3963+
// open_ledger_cost and validated_ledger_index fields.
3964+
testcase("Validate RPC response");
3965+
3966+
using namespace jtx;
3967+
Env env(*this);
3968+
Account const alice("alice");
3969+
Account const bob("bob");
3970+
env.fund(XRP(10000), alice, bob);
3971+
env.close();
3972+
3973+
// tes
3974+
{
3975+
auto const baseFee = env.current()->fees().base;
3976+
auto const aliceSeq = env.seq(alice);
3977+
auto jtx = env.jt(pay(alice, bob, XRP(1)));
3978+
3979+
Serializer s;
3980+
jtx.stx->add(s);
3981+
auto const jr = env.rpc("submit", strHex(s.slice()))[jss::result];
3982+
env.close();
3983+
3984+
BEAST_EXPECT(jr.isMember(jss::account_sequence_available));
3985+
BEAST_EXPECT(
3986+
jr[jss::account_sequence_available].asUInt() == aliceSeq + 1);
3987+
BEAST_EXPECT(jr.isMember(jss::account_sequence_next));
3988+
BEAST_EXPECT(
3989+
jr[jss::account_sequence_next].asUInt() == aliceSeq + 1);
3990+
BEAST_EXPECT(jr.isMember(jss::open_ledger_cost));
3991+
BEAST_EXPECT(jr[jss::open_ledger_cost] == to_string(baseFee));
3992+
BEAST_EXPECT(jr.isMember(jss::validated_ledger_index));
3993+
}
3994+
3995+
// tec failure
3996+
{
3997+
auto const baseFee = env.current()->fees().base;
3998+
auto const aliceSeq = env.seq(alice);
3999+
env(fset(bob, asfRequireDest));
4000+
auto jtx = env.jt(pay(alice, bob, XRP(1)), seq(aliceSeq));
4001+
4002+
Serializer s;
4003+
jtx.stx->add(s);
4004+
auto const jr = env.rpc("submit", strHex(s.slice()))[jss::result];
4005+
env.close();
4006+
4007+
BEAST_EXPECT(jr.isMember(jss::account_sequence_available));
4008+
BEAST_EXPECT(
4009+
jr[jss::account_sequence_available].asUInt() == aliceSeq + 1);
4010+
BEAST_EXPECT(jr.isMember(jss::account_sequence_next));
4011+
BEAST_EXPECT(
4012+
jr[jss::account_sequence_next].asUInt() == aliceSeq + 1);
4013+
BEAST_EXPECT(jr.isMember(jss::open_ledger_cost));
4014+
BEAST_EXPECT(jr[jss::open_ledger_cost] == to_string(baseFee));
4015+
BEAST_EXPECT(jr.isMember(jss::validated_ledger_index));
4016+
}
4017+
4018+
// tem failure
4019+
{
4020+
auto const baseFee = env.current()->fees().base;
4021+
auto const aliceSeq = env.seq(alice);
4022+
auto jtx = env.jt(pay(alice, bob, XRP(1)), seq(aliceSeq + 1));
4023+
4024+
Serializer s;
4025+
jtx.stx->add(s);
4026+
auto const jr = env.rpc("submit", strHex(s.slice()))[jss::result];
4027+
env.close();
4028+
4029+
BEAST_EXPECT(jr.isMember(jss::account_sequence_available));
4030+
BEAST_EXPECT(
4031+
jr[jss::account_sequence_available].asUInt() == aliceSeq);
4032+
BEAST_EXPECT(jr.isMember(jss::account_sequence_next));
4033+
BEAST_EXPECT(jr[jss::account_sequence_next].asUInt() == aliceSeq);
4034+
BEAST_EXPECT(jr.isMember(jss::open_ledger_cost));
4035+
BEAST_EXPECT(jr[jss::open_ledger_cost] == to_string(baseFee));
4036+
BEAST_EXPECT(jr.isMember(jss::validated_ledger_index));
4037+
}
4038+
}
4039+
4040+
void
4041+
testBatchCalculateBaseFee(FeatureBitset features)
4042+
{
4043+
using namespace jtx;
4044+
Env env(*this);
4045+
Account const alice("alice");
4046+
Account const bob("bob");
4047+
Account const carol("carol");
4048+
env.fund(XRP(10000), alice, bob, carol);
4049+
env.close();
4050+
4051+
auto getBaseFee = [&](JTx const& jtx) -> XRPAmount {
4052+
Serializer s;
4053+
jtx.stx->add(s);
4054+
return Batch::calculateBaseFee(*env.current(), *jtx.stx);
4055+
};
4056+
4057+
// bad: Inner Batch transaction found
4058+
{
4059+
auto const seq = env.seq(alice);
4060+
XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2);
4061+
auto jtx = env.jt(
4062+
batch::outer(alice, seq, batchFee, tfAllOrNothing),
4063+
batch::inner(
4064+
batch::outer(alice, seq, batchFee, tfAllOrNothing), seq),
4065+
batch::inner(pay(alice, bob, XRP(1)), seq + 2));
4066+
XRPAmount const txBaseFee = getBaseFee(jtx);
4067+
BEAST_EXPECT(txBaseFee == XRPAmount(INITIAL_XRP));
4068+
}
4069+
4070+
// bad: Raw Transactions array exceeds max entries.
4071+
{
4072+
auto const seq = env.seq(alice);
4073+
XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2);
4074+
4075+
auto jtx = env.jt(
4076+
batch::outer(alice, seq, batchFee, tfAllOrNothing),
4077+
batch::inner(pay(alice, bob, XRP(1)), seq + 1),
4078+
batch::inner(pay(alice, bob, XRP(1)), seq + 2),
4079+
batch::inner(pay(alice, bob, XRP(1)), seq + 3),
4080+
batch::inner(pay(alice, bob, XRP(1)), seq + 4),
4081+
batch::inner(pay(alice, bob, XRP(1)), seq + 5),
4082+
batch::inner(pay(alice, bob, XRP(1)), seq + 6),
4083+
batch::inner(pay(alice, bob, XRP(1)), seq + 7),
4084+
batch::inner(pay(alice, bob, XRP(1)), seq + 8),
4085+
batch::inner(pay(alice, bob, XRP(1)), seq + 9));
4086+
4087+
XRPAmount const txBaseFee = getBaseFee(jtx);
4088+
BEAST_EXPECT(txBaseFee == XRPAmount(INITIAL_XRP));
4089+
}
4090+
4091+
// bad: Signers array exceeds max entries.
4092+
{
4093+
auto const seq = env.seq(alice);
4094+
XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2);
4095+
4096+
auto jtx = env.jt(
4097+
batch::outer(alice, seq, batchFee, tfAllOrNothing),
4098+
batch::inner(pay(alice, bob, XRP(10)), seq + 1),
4099+
batch::inner(pay(alice, bob, XRP(5)), seq + 2),
4100+
batch::sig(
4101+
bob,
4102+
carol,
4103+
alice,
4104+
bob,
4105+
carol,
4106+
alice,
4107+
bob,
4108+
carol,
4109+
alice,
4110+
alice));
4111+
XRPAmount const txBaseFee = getBaseFee(jtx);
4112+
BEAST_EXPECT(txBaseFee == XRPAmount(INITIAL_XRP));
4113+
}
4114+
4115+
// good:
4116+
{
4117+
auto const seq = env.seq(alice);
4118+
XRPAmount const batchFee = batch::calcBatchFee(env, 0, 2);
4119+
auto jtx = env.jt(
4120+
batch::outer(alice, seq, batchFee, tfAllOrNothing),
4121+
batch::inner(pay(alice, bob, XRP(1)), seq + 1),
4122+
batch::inner(pay(bob, alice, XRP(2)), seq + 2));
4123+
XRPAmount const txBaseFee = getBaseFee(jtx);
4124+
BEAST_EXPECT(txBaseFee == batchFee);
4125+
}
4126+
}
4127+
39564128
void
39574129
testWithFeats(FeatureBitset features)
39584130
{
@@ -3983,6 +4155,8 @@ class Batch_test : public beast::unit_test::suite
39834155
testBatchTxQueue(features);
39844156
testBatchNetworkOps(features);
39854157
testBatchDelegate(features);
4158+
testValidateRPCResponse(features);
4159+
testBatchCalculateBaseFee(features);
39864160
}
39874161

39884162
public:

‎src/xrpld/app/misc/NetworkOPs.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1701,7 +1701,7 @@ NetworkOPsImp::apply(std::unique_lock<std::mutex>& batchLock)
17011701
}
17021702
}
17031703

1704-
if (!isTemMalformed(e.result) && validatedLedgerIndex)
1704+
if (validatedLedgerIndex)
17051705
{
17061706
auto [fee, accountSeq, availableSeq] =
17071707
app_.getTxQ().getTxRequiredFeeAndSeq(

‎src/xrpld/app/tx/detail/Batch.cpp‎

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,10 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx)
6161

6262
// LCOV_EXCL_START
6363
if (baseFee > maxAmount - view.fees().base)
64-
throw std::overflow_error("XRPAmount overflow");
64+
{
65+
JLOG(debugLog().error()) << "BatchTrace: Base fee overflow detected.";
66+
return XRPAmount{INITIAL_XRP};
67+
}
6568
// LCOV_EXCL_STOP
6669

6770
XRPAmount const batchBase = view.fees().base + baseFee;
@@ -72,32 +75,36 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx)
7275
{
7376
auto const& txns = tx.getFieldArray(sfRawTransactions);
7477

75-
XRPL_ASSERT(
76-
txns.size() <= maxBatchTxCount,
77-
"Raw Transactions array exceeds max entries.");
78-
7978
// LCOV_EXCL_START
8079
if (txns.size() > maxBatchTxCount)
81-
throw std::length_error(
82-
"Raw Transactions array exceeds max entries");
80+
{
81+
JLOG(debugLog().error())
82+
<< "BatchTrace: Raw Transactions array exceeds max entries.";
83+
return XRPAmount{INITIAL_XRP};
84+
}
8385
// LCOV_EXCL_STOP
8486

8587
for (STObject txn : txns)
8688
{
8789
STTx const stx = STTx{std::move(txn)};
8890

89-
XRPL_ASSERT(
90-
stx.getTxnType() != ttBATCH, "Inner Batch transaction found.");
91-
9291
// LCOV_EXCL_START
9392
if (stx.getTxnType() == ttBATCH)
94-
throw std::invalid_argument("Inner Batch transaction found");
93+
{
94+
JLOG(debugLog().error())
95+
<< "BatchTrace: Inner Batch transaction found.";
96+
return XRPAmount{INITIAL_XRP};
97+
}
9598
// LCOV_EXCL_STOP
9699

97100
auto const fee = ripple::calculateBaseFee(view, stx);
98101
// LCOV_EXCL_START
99102
if (txnFees > maxAmount - fee)
100-
throw std::overflow_error("XRPAmount overflow");
103+
{
104+
JLOG(debugLog().error())
105+
<< "BatchTrace: XRPAmount overflow in txnFees calculation.";
106+
return XRPAmount{INITIAL_XRP};
107+
}
101108
// LCOV_EXCL_STOP
102109
txnFees += fee;
103110
}
@@ -108,13 +115,14 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx)
108115
if (tx.isFieldPresent(sfBatchSigners))
109116
{
110117
auto const& signers = tx.getFieldArray(sfBatchSigners);
111-
XRPL_ASSERT(
112-
signers.size() <= maxBatchTxCount,
113-
"Batch Signers array exceeds max entries.");
114118

115119
// LCOV_EXCL_START
116120
if (signers.size() > maxBatchTxCount)
117-
throw std::length_error("Batch Signers array exceeds max entries");
121+
{
122+
JLOG(debugLog().error())
123+
<< "BatchTrace: Batch Signers array exceeds max entries.";
124+
return XRPAmount{INITIAL_XRP};
125+
}
118126
// LCOV_EXCL_STOP
119127

120128
for (STObject const& signer : signers)
@@ -128,16 +136,28 @@ Batch::calculateBaseFee(ReadView const& view, STTx const& tx)
128136

129137
// LCOV_EXCL_START
130138
if (signerCount > 0 && view.fees().base > maxAmount / signerCount)
131-
throw std::overflow_error("XRPAmount overflow");
139+
{
140+
JLOG(debugLog().error())
141+
<< "BatchTrace: XRPAmount overflow in signerCount calculation.";
142+
return XRPAmount{INITIAL_XRP};
143+
}
132144
// LCOV_EXCL_STOP
133145

134146
XRPAmount signerFees = signerCount * view.fees().base;
135147

136148
// LCOV_EXCL_START
137149
if (signerFees > maxAmount - txnFees)
138-
throw std::overflow_error("XRPAmount overflow");
150+
{
151+
JLOG(debugLog().error())
152+
<< "BatchTrace: XRPAmount overflow in signerFees calculation.";
153+
return XRPAmount{INITIAL_XRP};
154+
}
139155
if (txnFees + signerFees > maxAmount - batchBase)
140-
throw std::overflow_error("XRPAmount overflow");
156+
{
157+
JLOG(debugLog().error())
158+
<< "BatchTrace: XRPAmount overflow in total fee calculation.";
159+
return XRPAmount{INITIAL_XRP};
160+
}
141161
// LCOV_EXCL_STOP
142162

143163
// 10 drops per batch signature + sum of inner tx fees + batchBase

0 commit comments

Comments
 (0)