Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
17 changes: 15 additions & 2 deletions src/libxrpl/tx/transactors/proposal/TransactionProposalCreate.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,7 @@ TransactionProposalCreate::preflight(PreflightContext const& ctx)
return temINVALID;
}

if (proposedTx.isFieldPresent(sfFlags) &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we still need proposedTx.isFieldPresent(sfFlags) check, otherwise if the field doesn't exist it will throw. What if we move the STTx construction (line 128) to the start of preflight? would it avoid having need to check the existence of the fields

((proposedTx.getFieldU32(sfFlags) & tfInnerBatchTxn) != 0u))
if (proposedTx.isFlag(tfInnerBatchTxn))
{
JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn "
"carries tfInnerBatchTxn.";
Expand Down Expand Up @@ -104,6 +103,20 @@ TransactionProposalCreate::preflight(PreflightContext const& ctx)
if (!proposedTx.isFieldPresent(sfTicketSequence) || proposedTx.getFieldU32(sfSequence) != 0)
return temSEQ_AND_TICKET;
Comment on lines 123 to 124

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

based on recent discussion, we need to reserve ticket, so we should check if the ticket exists


// If this transaction itself is paying with a Ticket, and the proposed
// transaction is targeting that same account and Ticket, then applying
// this transaction consumes the very Ticket the proposal depends on
// before the proposal is even stored: the proposal would be dead on
// arrival, and its only recourse would be TransactionProposalCancel.
if (ctx.tx.getSeqProxy().isTicket() &&
proposedTx.getAccountID(sfAccount) == ctx.tx.getAccountID(sfAccount) &&
proposedTx.getFieldU32(sfTicketSequence) == ctx.tx.getSeqProxy().value())
{
JLOG(ctx.j.debug()) << "TransactionProposalCreate: proposed txn "
"reuses the Ticket this transaction itself consumes.";
return temMALFORMED;
}

// The proposed transaction must pass its own static checks under the
// current rules, so no statically-dead proposal can be stored. TapDryRun
// accepts the unsigned canonical form without a signature check;
Expand Down
35 changes: 35 additions & 0 deletions src/test/app/TransactionProposalCreate_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <test/jtx/TestHelpers.h>
#include <test/jtx/amount.h>
#include <test/jtx/batch.h>
#include <test/jtx/noop.h>
#include <test/jtx/pay.h>
#include <test/jtx/sponsor.h>
#include <test/jtx/ter.h>
Expand Down Expand Up @@ -175,6 +176,15 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite
reject(tx, temBAD_SIGNER);
}

// An unrecognized TransactionType cannot even be constructed as an
// STTx (there is no format to validate it against), so it is
// rejected the same as any other malformed payload.
{
json::Value tx = payload();
tx[jss::TransactionType] = 65535;
reject(tx, temMALFORMED);
}

// A pseudo-transaction is never submittable by an account.
{
json::Value tx = payload();
Expand Down Expand Up @@ -231,6 +241,31 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite
reject(tx, temSEQ_AND_TICKET);
}

// If this TransactionProposalCreate itself pays with a Ticket, and the
// proposed transaction targets that same account and Ticket, applying
// this transaction consumes the Ticket the proposal depends on before
// the proposal is even stored: it would be dead on arrival.
{
std::uint32_t const aliceTicketSeq = env.seq(alice) + 1;
env(ticket::create(alice, 1));
env.close();

json::Value const tx = unsignedPayload(env, alice, bob, aliceTicketSeq);
env(proposalCreate(alice, tx, expiration),
ticket::Use(aliceTicketSeq),
Ter(temMALFORMED));
env.close();
BEAST_EXPECT(!env.le(keylet::txProposal(alice.id(), aliceTicketSeq)));
BEAST_EXPECT(env.le(keylet::ticket(alice.id(), aliceTicketSeq)));
BEAST_EXPECT(ownerCount(env, alice) == 1);

// Consume the leftover Ticket so alice's ownerCount is back to
// zero for the remaining cases below.
env(noop(alice), ticket::Use(aliceTicketSeq));
env.close();
BEAST_EXPECT(ownerCount(env, alice) == 0);
}

// A payload that fails its own transaction type's preflight surfaces
// that type's own code, not a generic error (On-Chain Cosigner spec §5.3.1.2).
{
Expand Down
Loading