Proposal create bug fixes (#8139)

This commit is contained in:
Kassaking7
2026-09-08 15:27:38 -04:00
committed by GitHub
parent 2d80b9f832
commit d51aea5d79
8 changed files with 431 additions and 16 deletions

View File

@@ -21,7 +21,7 @@ class TransactionProposalCreateBuilder;
* Type: ttTRANSACTION_PROPOSAL_CREATE (92)
* Delegable: Delegation::NotDelegable
* Amendment: featureCosign
* Privileges: NoPriv
* Privileges: Privilege::NoPriv
*
* Immutable wrapper around STTx providing type-safe field access.
* Use TransactionProposalCreateBuilder to construct new transactions.

View File

@@ -385,6 +385,23 @@ preflight(
PreclaimResult
preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, OpenView const& view);
/**
* Type-erased overload of Transactor::invokeCheckPermission.
*
* Dispatches on the transaction type to Transactor::invokeCheckPermission<T>
* so a caller that only has an STTx still gets the same verdict submission
* uses: transaction-level permission, then granular permissions and
* checkGranularSandbox, then that type's checkGranularSemantics. Does not
* check SignerList or signing keys.
*
* An unknown transaction type (should not occur after a successful preflight)
* is treated as an internal invariant violation: UNREACHABLE is fired and the
* type-erased fallback return is temUNKNOWN, mirroring the sibling
* invokePreflight/invokePreclaim/invokeApply overloads in this header.
*/
NotTEC
invokeCheckPermission(ReadView const& view, STTx const& tx);
/**
* Compute only the expected base fee for a transaction.
*

View File

@@ -375,6 +375,27 @@ preflight(
}
}
NotTEC
invokeCheckPermission(ReadView const& view, STTx const& tx)
{
try
{
return withTxnType(view.rules(), tx.getTxnType(), [&]<typename T>() {
return Transactor::invokeCheckPermission<T>(view, tx);
});
}
catch (UnknownTxnType const& e)
{
// Should never happen
// LCOV_EXCL_START
JLOG(debugLog().fatal()) << "Unknown transaction type in invokeCheckPermission: "
<< e.txnType;
UNREACHABLE("xrpl::invokeCheckPermission : unknown transaction type");
return temUNKNOWN;
// LCOV_EXCL_STOP
}
}
PreclaimResult
preclaim(PreflightResult const& preflightResult, ServiceRegistry& registry, OpenView const& view)
{

View File

@@ -5,7 +5,6 @@
#include <xrpl/ledger/ApplyView.h>
#include <xrpl/ledger/View.h>
#include <xrpl/ledger/helpers/AccountRootHelpers.h>
#include <xrpl/ledger/helpers/DelegateHelpers.h>
#include <xrpl/ledger/helpers/DirectoryHelpers.h>
#include <xrpl/ledger/helpers/ProposalHelpers.h>
#include <xrpl/ledger/helpers/SponsorHelpers.h>
@@ -188,12 +187,38 @@ TransactionProposalCreate::preclaim(PreclaimContext const& ctx)
if (!sleSigners)
return false;
auto const accountSigners = SignerEntries::deserialize(*sleSigners, ctx.j, "ledger");
if (!accountSigners)
return std::unexpected(TER{accountSigners.error()});
// deserialize itself returns unexpected(temMALFORMED) when
// sfSignerEntries is missing or an element is not an sfSignerEntry.
// Those are the right codes for a transaction object. Here the object
// is an on-ledger ltSIGNER_LIST (sfSignerEntries is SoeRequired;
// each element is an sfSignerEntry). A corrupt SLE can still throw
// from the STObject accessors deserialize calls: getFieldArray
// ("Wrong field type") or getAccountID/getFieldU16 ("Field not
// found") when an sfSignerEntry is missing required fields. Either
// the expected<> error or a throw is unexpected ledger state, not a
// malformed TransactionProposalCreate, so tefBAD_LEDGER (rather
// than tefINTERNAL, which is reserved for truly unreachable code
// paths) is the right code.
try
{
auto const accountSigners =
SignerEntries::deserialize(*sleSigners, ctx.j, "ledger");
if (!accountSigners)
{
JLOG(ctx.j.fatal()) << "TransactionProposalCreate: unparseable SignerList: "
<< transToken(accountSigners.error());
return std::unexpected(tefBAD_LEDGER);
}
return std::ranges::any_of(
*accountSigners, [&](auto const& entry) { return entry.account == proposer; });
return std::ranges::any_of(
*accountSigners, [&](auto const& entry) { return entry.account == proposer; });
}
catch (std::exception const& e)
{
JLOG(ctx.j.fatal())
<< "TransactionProposalCreate: unparseable SignerList: " << e.what();
return std::unexpected(tefBAD_LEDGER);
}
};
auto isSigner = isAuthorizedFor(target);
@@ -201,18 +226,29 @@ TransactionProposalCreate::preclaim(PreclaimContext const& ctx)
return isSigner.error();
// A delegate that the target has granted permission over the
// proposed transaction's own type — or one of that delegate's own
// signers — is equally authorized: it will need to help complete
// the proposed transaction's own authorization anyway once the
// proposal is submitted.
// proposed transaction — or one of that delegate's own signers — is
// equally authorized: it will need to help complete the proposed
// transaction's own authorization anyway once the proposal is
// submitted. xrpl::invokeCheckPermission is the type-erased
// submission hierarchy (not checkTxPermission alone, which would
// reject a matching granular grant). Qualify xrpl:: so the inherited
// Transactor template is not chosen; it cannot deduce T here. A
// failed grant is still "not authorized" and becomes
// tecNO_PERMISSION below — Create is already signed, so do not leak
// the pre-sign terNO_DELEGATE_PERMISSION.
if (!*isSigner && proposedTx.isFieldPresent(sfDelegate))
{
AccountID const delegateAccount = proposedTx.getAccountID(sfDelegate);
// NOLINTNEXTLINE(readability-suspicious-call-argument)
auto const sleDelegate = ctx.view.read(keylet::delegate(target, delegateAccount));
if (sleDelegate &&
isTesSuccess(checkTxPermission(sleDelegate, STTx{STObject{proposedTx}})))
STTx const proposedStTx{STObject{proposedTx}};
if (isTesSuccess(xrpl::invokeCheckPermission(ctx.view, proposedStTx)))
{
// A grant cannot exist without a funded authorize (DelegateSet
// uses tecNO_TARGET; AccountDelete of the delegatee removes the
// Delegate SLE). Do not treat a missing account as a Create-time
// user error — that would extra-validate the proposed tx. If
// permission passed anyway, the ledger is corrupt.
if (!ctx.view.exists(keylet::account(delegateAccount)))
return tefINTERNAL; // LCOV_EXCL_LINE
isSigner = isAuthorizedFor(delegateAccount);
if (!isSigner)
return isSigner.error();

View File

@@ -339,8 +339,20 @@ Batch::preflight(PreflightContext const& ctx)
return temINVALID_FLAG;
auto const innerAccount = stx.getAccountID(sfAccount);
// TransactionProposalCreate preflights a proposed Batch with
// TapDryRun | TapProposal so signature-presence checks are deferred
// to collection time (On-Chain Cosigner spec §5.3.1.2). Inner
// preflight used to pass only TapBatch, so those bits never reached
// the inners: an unsigned account-reserve SponsorshipTransfer then
// demanded sfSponsorSignature and the Create failed with
// temINVALID_INNER_BATCH. Spec §6.1.1 names an inner Sponsor as a
// collectable slot, so forward TapProposal/TapDryRun. Always OR in
// TapBatch — PreflightContext with a parentBatchId requires it.
// LoanSet already short-circuits on tfInnerBatchTxn; it is also in
// kDisabledTxTypes, so it never reaches this call.
ApplyFlags const innerFlags = TapBatch | (ctx.flags & (TapProposal | TapDryRun));
if (auto const preflightResult =
xrpl::preflight(ctx.registry, ctx.rules, parentBatchId, stx, TapBatch, ctx.j);
xrpl::preflight(ctx.registry, ctx.rules, parentBatchId, stx, innerFlags, ctx.j);
!isTesSuccess(preflightResult.ter))
{
JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: "

View File

@@ -6,6 +6,7 @@
#include <test/jtx/delegate.h>
#include <test/jtx/deposit.h>
#include <test/jtx/fee.h>
#include <test/jtx/flags.h>
#include <test/jtx/multisign.h>
#include <test/jtx/noop.h>
#include <test/jtx/offer.h>
@@ -20,14 +21,19 @@
#include <xrpl/basics/strHex.h>
#include <xrpl/beast/unit_test/suite.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/json/json_value.h>
#include <xrpl/ledger/OpenView.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Keylet.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/STArray.h>
#include <xrpl/protocol/STLedgerEntry.h>
#include <xrpl/protocol/STObject.h>
#include <xrpl/protocol/SeqProxy.h>
#include <xrpl/protocol/TER.h>
#include <xrpl/protocol/TxFlags.h>
#include <xrpl/protocol/jss.h>
@@ -36,6 +42,7 @@
#include <cstddef>
#include <cstdint>
#include <functional>
#include <memory>
#include <string>
#include <vector>
@@ -698,6 +705,119 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite
}
}
// An on-ledger ltSIGNER_LIST that cannot be read as signer entries is
// unexpected ledger state, not a malformed transaction. preclaim must
// surface tefBAD_LEDGER (not temMALFORMED, not tefINTERNAL which is
// reserved for truly unreachable paths, and not the tefEXCEPTION that
// applySteps would wrap an uncaught throw with).
//
// SignerEntries::deserialize returns unexpected(temMALFORMED) when
// sfSignerEntries is missing or an element is not named sfSignerEntry.
// It still throws from STObject accessors when an sfSignerEntry is
// missing required fields (getAccountID → "Field not found: Account").
// Do not close() after the synthetic corruption: a closed ledger would
// drop the overlay and restore a well-formed list.
void
testCorruptSignerList(FeatureBitset features)
{
testcase("unparseable on-ledger SignerList is tefBAD_LEDGER");
using namespace jtx;
using namespace std::chrono_literals;
auto setup = [&](Env& env, Account const& target, Account const& signer) {
env.fund(XRP(10000), target, signer);
env.close();
env(signers(target, 1, {{signer, 1}}));
env.close();
// Ticket first: createTicket closes, which would drop a later
// open-ledger overlay and restore a well-formed SignerList.
return proposal::createTicket(env, target);
};
auto proposeAsSigner =
[&](Env& env, Account const& target, Account const& signer, std::uint32_t ticketSeq) {
env(proposal::create(
signer,
proposal::unsignedPayload(env, pay(target, signer, XRP(1)), ticketSeq),
proposal::expiration(env, 100s)),
Ter(tefBAD_LEDGER),
proposal::verify::create());
BEAST_EXPECT(!proposal::entry(env, target, ticketSeq));
};
{
Env env{*this, features};
Account const target{"targetMissing"};
Account const signer{"signerMissing"};
std::uint32_t const ticketSeq = setup(env, target, signer);
auto const signerListKeylet = keylet::signerList(target.id());
BEAST_EXPECT(env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) {
auto const sle = view.read(signerListKeylet);
if (!sle)
return false;
auto replacement = std::make_shared<SLE>(*sle);
if (!replacement->delField(sfSignerEntries))
return false;
view.rawReplace(replacement);
return true;
}));
BEAST_EXPECT(env.le(signerListKeylet));
proposeAsSigner(env, target, signer, ticketSeq);
}
{
Env env{*this, features};
Account const target{"targetBadEntry"};
Account const signer{"signerBadEntry"};
std::uint32_t const ticketSeq = setup(env, target, signer);
auto const signerListKeylet = keylet::signerList(target.id());
BEAST_EXPECT(env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) {
auto const sle = view.read(signerListKeylet);
if (!sle)
return false;
auto replacement = std::make_shared<SLE>(*sle);
STArray badEntries;
badEntries.pushBack(STObject{sfSigner});
replacement->setFieldArray(sfSignerEntries, badEntries);
view.rawReplace(replacement);
return true;
}));
BEAST_EXPECT(env.le(signerListKeylet));
proposeAsSigner(env, target, signer, ticketSeq);
}
{
Env env{*this, features};
Account const target{"targetMissingAccount"};
Account const signer{"signerMissingAccount"};
std::uint32_t const ticketSeq = setup(env, target, signer);
auto const signerListKeylet = keylet::signerList(target.id());
BEAST_EXPECT(env.app().getOpenLedger().modify([&](OpenView& view, beast::Journal) {
auto const sle = view.read(signerListKeylet);
if (!sle)
return false;
auto replacement = std::make_shared<SLE>(*sle);
STArray badEntries;
// Right inner name, but no sfAccount: deserialize calls
// getAccountID and throws (Field not found), which the
// catch maps to tefBAD_LEDGER.
badEntries.pushBack(STObject{sfSignerEntry});
replacement->setFieldArray(sfSignerEntries, badEntries);
view.rawReplace(replacement);
return true;
}));
BEAST_EXPECT(env.le(signerListKeylet));
proposeAsSigner(env, target, signer, ticketSeq);
}
}
// The target account may delegate authority over the proposed
// transaction's own type to another account (Permission Delegation,
// XLS-75); if it does, that delegate — or an account on the delegate's
@@ -781,6 +901,81 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite
}
}
// A delegate holding only a granular permission (XLS-75) that would
// authorize submitting the proposed transaction may also create a
// proposal for it. A granular grant that fails checkGranularSandbox
// still cannot.
void
testDelegatedGranularProposedTx(FeatureBitset features)
{
testcase("proposer authorized through granular delegate permission");
using namespace jtx;
using namespace std::chrono_literals;
Env env{*this, features};
Account const gw{"gw"}; // issuer / proposed-tx Account
Account const alice{"alice"}; // holder of the trust line being authorized
Account const bob{"bob"}; // delegate with TrustlineAuthorize only
env.fund(XRP(10000), gw, alice, bob);
env(fset(gw, asfRequireAuth));
env.close();
env(trust(alice, gw["USD"](50)));
env.close();
env(delegate::set(gw, bob, {"TrustlineAuthorize"}));
env.close();
auto delegatedTrustSet = [&](std::uint32_t ticketSeq, std::uint32_t flags) {
json::Value tx = trust(gw, gw["USD"](0), alice, flags);
tx[sfDelegate.jsonName] = bob.human();
return proposal::unsignedPayload(env, tx, ticketSeq);
};
// TrustlineAuthorize is sufficient for a tfSetfAuth TrustSet against
// an existing line whose limit is unchanged — the same shape that
// submits successfully under invokeCheckPermission.
{
std::uint32_t const ticketSeq = proposal::createTicket(env, gw);
env(proposal::create(
bob, delegatedTrustSet(ticketSeq, tfSetfAuth), proposal::expiration(env, 100s)),
proposal::verify::create());
env.close();
BEAST_EXPECT(proposal::entry(env, gw, ticketSeq));
}
// tfSetFreeze is not in TrustlineAuthorize's sandbox.
{
std::uint32_t const ticketSeq = proposal::createTicket(env, gw);
env(proposal::create(
bob,
delegatedTrustSet(ticketSeq, tfSetFreeze),
proposal::expiration(env, 100s)),
Ter(tecNO_PERMISSION),
proposal::verify::create());
env.close();
BEAST_EXPECT(!proposal::entry(env, gw, ticketSeq));
}
// sfQualityOut is a valid TrustSet field but not in the granular
// template, so checkGranularSandbox rejects it.
{
std::uint32_t const ticketSeq = proposal::createTicket(env, gw);
json::Value tx = trust(gw, gw["USD"](0), alice, tfSetfAuth);
tx[sfDelegate.jsonName] = bob.human();
tx[sfQualityOut.jsonName] = 100;
env(proposal::create(
bob,
proposal::unsignedPayload(env, tx, ticketSeq),
proposal::expiration(env, 100s)),
Ter(tecNO_PERMISSION),
proposal::verify::create());
env.close();
BEAST_EXPECT(!proposal::entry(env, gw, ticketSeq));
}
}
// The target account must be able to authorize a transaction through a
// SignerList, so a pseudo-account (here an AMM's) cannot be a target even
// though it exists on-ledger (On-Chain Cosigner spec §5.3.2.5).
@@ -1282,6 +1477,75 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite
BEAST_EXPECT(ownerCount(env, target) == 1 + proposal::kBatchProposalOwnerCount);
}
// A proposed Batch's inner preflight must receive TapProposal, or an
// unsigned account-reserve SponsorshipTransfer is rejected at Create
// (On-Chain Cosigner spec §6.1.1: an inner Sponsor is a collectable
// signature slot). Other inner types that do not key on TapProposal keep
// their existing preflight result.
void
testProposedBatchInnerSponsorship(FeatureBitset features)
{
testcase("proposed batch inner account-reserve SponsorshipTransfer");
using namespace jtx;
using namespace std::chrono_literals;
Env env{*this, features};
Account const target{"target"};
Account const bob{"bob"}; // named as Sponsor; signature collected later
env.fund(XRP(10000), target, bob);
env.close();
auto unsignedInnerSponsorship = [&](Account const& account) {
json::Value tx = sponsor::transfer(account, tfSponsorshipCreate);
tx[sfSponsor.getJsonName()] = bob.human();
tx[sfSponsorFlags.getJsonName()] = spfSponsorReserve;
return tx;
};
// Payment (unaffected by TapProposal) plus an unsigned inner
// account-reserve SponsorshipTransfer. Create succeeds only if
// TapProposal reaches the inner.
{
std::uint32_t const ticketSeq = proposal::createTicket(env, target);
auto const seq = env.seq(target);
json::Value const proposedTx = proposal::unsignedBatch(
env,
target,
ticketSeq,
tfAllOrNothing,
{proposal::innerTx(pay(target, bob, XRP(1)), seq),
proposal::innerTx(unsignedInnerSponsorship(target), seq + 1)});
env(proposal::create(target, proposedTx, proposal::expiration(env, 100s)),
proposal::verify::create());
env.close();
BEAST_EXPECT(proposal::entry(env, target, ticketSeq));
}
// Structural inner failures are unchanged: missing sfSponsor is still
// temMALFORMED in SponsorshipTransfer::preflight, collapsed by Batch
// to temINVALID_INNER_BATCH. TapProposal does not skip that.
{
std::uint32_t const ticketSeq = proposal::createTicket(env, target);
auto const seq = env.seq(target);
json::Value const proposedTx = proposal::unsignedBatch(
env,
target,
ticketSeq,
tfAllOrNothing,
{proposal::innerTx(pay(target, bob, XRP(1)), seq),
proposal::innerTx(sponsor::transfer(target, tfSponsorshipCreate), seq + 1)});
env(proposal::create(target, proposedTx, proposal::expiration(env, 100s)),
Ter(temINVALID_INNER_BATCH),
proposal::verify::create());
env.close();
BEAST_EXPECT(!proposal::entry(env, target, ticketSeq));
}
}
void
run() override
{
@@ -1299,7 +1563,9 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite
// Preclaim
testPreclaim(all);
testProposerAuthorization(all);
testCorruptSignerList(all);
testDelegatedProposedTx(all);
testDelegatedGranularProposedTx(all);
testPseudoTarget(all);
// Apply
@@ -1312,6 +1578,7 @@ struct TransactionProposalCreate_test : public beast::unit_test::Suite
testFeeSponsored(all);
testBatchReserve(all);
testMultiAccountBatch(all);
testProposedBatchInnerSponsorship(all);
}
};

View File

@@ -9,6 +9,7 @@
#include <test/jtx/owners.h> // IWYU pragma: keep
#include <test/jtx/pay.h>
#include <test/jtx/permissioned_domains.h>
#include <test/jtx/proposal.h>
#include <test/jtx/sig.h>
#include <test/jtx/sponsor.h>
#include <test/jtx/ticket.h>
@@ -35,6 +36,7 @@
#include <xrpl/tx/transactors/nft/NFTokenMint.h>
#include <algorithm>
#include <chrono> // IWYU pragma: keep
#include <cstdint>
#include <iterator>
#include <optional>
@@ -1699,6 +1701,64 @@ public:
BEAST_EXPECT(res[jss::result].isMember(jss::marker));
}
// A TransactionProposal blocks AccountDelete (it is not in
// nonObligationDeleter) but was omitted from kDeletionBlockers, so
// deletion_blockers_only reported a clean directory. Tickets on the
// same account are auto-removed at delete time and must stay omitted.
void
testDeletionBlockersProposal()
{
testcase("deletion_blockers_only includes TransactionProposal");
using namespace jtx;
using namespace std::chrono_literals;
Env env{*this, testableAmendments()};
Account const alice{"alice"};
Account const bob{"bob"};
env.fund(XRP(10000), alice, bob);
env.close();
std::uint32_t const ticketSeq = proposal::createTicket(env, alice);
env(proposal::create(
alice,
proposal::unsignedPayload(env, pay(alice, bob, XRP(1)), ticketSeq),
proposal::expiration(env, 100s)),
proposal::verify::create());
env.close();
json::Value params;
params[jss::account] = alice.human();
params[jss::deletion_blockers_only] = true;
params[jss::ledger_index] = "validated";
{
auto const resp = env.rpc("json", "account_objects", to_string(params));
auto const& aobjs = resp[jss::result][jss::account_objects];
if (BEAST_EXPECT(aobjs.isArray() && aobjs.size() == 1))
{
BEAST_EXPECT(aobjs[0u][sfLedgerEntryType.jsonName] == jss::TransactionProposal);
BEAST_EXPECT(aobjs[0u][sfOwner.jsonName] == alice.human());
}
}
{
params[jss::type] = jss::transaction_proposal;
auto const resp = env.rpc("json", "account_objects", to_string(params));
auto const& aobjs = resp[jss::result][jss::account_objects];
if (BEAST_EXPECT(aobjs.isArray() && aobjs.size() == 1))
BEAST_EXPECT(aobjs[0u][sfLedgerEntryType.jsonName] == jss::TransactionProposal);
}
{
params[jss::type] = jss::check;
auto const resp = env.rpc("json", "account_objects", to_string(params));
auto const& aobjs = resp[jss::result][jss::account_objects];
BEAST_EXPECT(aobjs.isArray() && aobjs.size() == 0);
}
}
void
run() override
{
@@ -1711,6 +1771,7 @@ public:
testAccountObjectMarker();
testSponsoredFilter();
testAccountObjectDoesntShowCancelledOffers();
testDeletionBlockersProposal();
}
};

View File

@@ -314,6 +314,7 @@ doAccountObjects(rpc::JsonContext& context)
{.name = jss::permissioned_domain, .type = ltPERMISSIONED_DOMAIN},
{.name = jss::vault, .type = ltVAULT},
{.name = jss::sponsorship, .type = ltSPONSORSHIP},
{.name = jss::transaction_proposal, .type = ltTRANSACTION_PROPOSAL},
};
typeFilter.emplace();