diff --git a/src/test/app/SetManifest_test.cpp b/src/test/app/SetManifest_test.cpp index 127b15a1b7..7f015edddc 100644 --- a/src/test/app/SetManifest_test.cpp +++ b/src/test/app/SetManifest_test.cpp @@ -111,6 +111,15 @@ struct SetManifest_test : public beast::unit_test::suite return env.rpc("json", "submit", to_string(params))[jss::result]; } + /** Submits an already formed SetManifest transaction. */ + static Json::Value + submit(jtx::Env& env, std::shared_ptr const& tx) + { + Serializer s; + tx->add(s); + return env.rpc("submit", strHex(s.slice()))[jss::result]; + } + static std::string engineResult(Json::Value const& result) { @@ -175,6 +184,43 @@ struct SetManifest_test : public beast::unit_test::suite return build(mulRatio(base, 12, 10, /*roundUp*/ true), true); } + /** An ordinary account-signed SetManifest envelope. + + This is the only lane allowed to create an account's first manifest + slot. It uses the account's current Sequence unless a test overrides + it, and its outer signature authenticates the complete transaction. + */ + static std::shared_ptr + signedEnvelope( + jtx::Env& env, + std::string const& manifest, + jtx::Account const& account, + std::optional sequence = std::nullopt, + std::optional fee = std::nullopt) + { + auto const build = [&](XRPAmount fee) { + auto tx = + std::make_shared(ttMANIFEST_SET, [&](STObject& obj) { + obj.setAccountID(sfAccount, account.id()); + obj.setFieldU32( + sfSequence, sequence.value_or(env.seq(account))); + obj.setFieldU32(sfNetworkID, env.app().config().NETWORK_ID); + obj.setFieldAmount(sfFee, fee); + obj.setFieldVL(sfSigningPubKey, account.pk().slice()); + + SerialIter mit{makeSlice(manifest)}; + obj.peekFieldObject(sfManifest).set(mit); + }); + tx->sign(account.pk(), account.sk()); + return tx; + }; + + auto const probe = build(XRPAmount{0}); + auto const baseFee = + SetManifest::calculateBaseFee(*env.current(), *probe); + return build(fee.value_or(baseFee)); + } + /** A mutable copy of a ledger object, suitable for the RawView interface. Round-tripped through the wire format rather than copy constructed. @@ -239,9 +285,25 @@ struct SetManifest_test : public beast::unit_test::suite env.fund(XRP(1000), master); env.close(); + auto const first = makeManifest(master, ephemeral, 1); + + // Possessing a valid manifest does not authorize creation of its + // account-backed slot. The refusal claims neither a fee nor Sequence. + auto const balanceBefore = env.balance(master); + auto const sequenceBefore = env.seq(master); + BEAST_EXPECT(!onLedgerManifestSequence(*env.current(), master.pk())); + BEAST_EXPECT(engineResult(submit(env, first)) == "tefBAD_AUTH"); + BEAST_EXPECT(!env.le(keylet::manifest(master.pk()))); + BEAST_EXPECT(!onLedgerManifestSequence(*env.current(), master.pk())); + BEAST_EXPECT(env.balance(master) == balanceBefore); + BEAST_EXPECT(env.seq(master) == sequenceBefore); + + // First registration is an ordinary account-signed transaction. BEAST_EXPECT( - engineResult(submit(env, makeManifest(master, ephemeral, 1))) == + engineResult(submit(env, signedEnvelope(env, first, master))) == "tesSUCCESS"); + BEAST_EXPECT( + onLedgerManifestSequence(*env.current(), master.pk()) == 1); env.close(); // A manifest is written twice so it can be found from either key, and @@ -276,10 +338,8 @@ struct SetManifest_test : public beast::unit_test::suite sleAcct->getFieldH256(sfManifestID) == keylet::manifest(master.pk()).key); - // The account sequence must be untouched: the transaction is unsigned - // and pinned to sequence 0, so consuming a sequence would let a third - // party burn the validator's sequence numbers -- and writing seq + 1 - // would reset the account to 1. + // Account-signed registration consumed exactly one ordinary Sequence. + BEAST_EXPECT(env.seq(master) == sequenceBefore + 1); BEAST_EXPECT(sleAcct->getFieldU32(sfSequence) == env.seq(master)); } @@ -294,11 +354,13 @@ struct SetManifest_test : public beast::unit_test::suite auto const master = Account("master", KeyType::ed25519); auto const eph1 = Account("eph1", KeyType::ed25519); auto const eph2 = Account("eph2", KeyType::ed25519); + auto const eph3 = Account("eph3", KeyType::ed25519); env.fund(XRP(1000), master); env.close(); - submit(env, makeManifest(master, eph1, 1)); + submit(env, signedEnvelope(env, makeManifest(master, eph1, 1), master)); env.close(); + auto const accountSequence = env.seq(master); // Rotating the ephemeral key erases both old copies and writes two // new ones, so the two can never drift apart. @@ -313,10 +375,33 @@ struct SetManifest_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::manifest(master.pk()))->getFieldU32(sfSequence) == 2); + BEAST_EXPECT(env.seq(master) == accountSequence); - // Replaying the manifest we just applied, and anything older, is - // rejected on sequence. This is what prevents replay: the transaction - // is unsigned, so nothing else would. + // The account-signed lane uses ordinary replay protection. A stale + // outer Sequence is rejected before the manifest sequence matters; + // the correct one both rotates the manifest and advances the account. + BEAST_EXPECT( + engineResult(submit( + env, + signedEnvelope( + env, + makeManifest(master, eph3, 3), + master, + accountSequence - 1))) == "tefPAST_SEQ"); + BEAST_EXPECT(!env.le(keylet::manifest(eph3.pk()))); + + BEAST_EXPECT( + engineResult(submit( + env, + signedEnvelope(env, makeManifest(master, eph3, 3), master))) == + "tesSUCCESS"); + env.close(); + BEAST_EXPECT(env.seq(master) == accountSequence + 1); + BEAST_EXPECT(!env.le(keylet::manifest(eph2.pk()))); + BEAST_EXPECT(env.le(keylet::manifest(eph3.pk()))); + + // Replaying older manifests through the unsigned lane is rejected by + // manifest sequence, independently of the account Sequence. BEAST_EXPECT( engineResult(submit(env, makeManifest(master, eph2, 2))) == "tefPAST_MANIFEST_SEQ"); @@ -338,7 +423,9 @@ struct SetManifest_test : public beast::unit_test::suite env.fund(XRP(1000), master); env.close(); - submit(env, makeManifest(master, ephemeral, 1)); + submit( + env, + signedEnvelope(env, makeManifest(master, ephemeral, 1), master)); env.close(); BEAST_EXPECT( @@ -383,7 +470,7 @@ struct SetManifest_test : public beast::unit_test::suite env.fund(XRP(1000), master); env.close(); - submit(env, makeManifest(master, eph1, 1)); + submit(env, signedEnvelope(env, makeManifest(master, eph1, 1), master)); env.close(); auto& cache = env.app().validatorManifests(); @@ -442,7 +529,7 @@ struct SetManifest_test : public beast::unit_test::suite env.fund(XRP(1000), master); env.close(); - submit(env, makeManifest(master, eph1, 1)); + submit(env, signedEnvelope(env, makeManifest(master, eph1, 1), master)); env.close(); auto& cache = env.app().validatorManifests(); @@ -539,11 +626,16 @@ struct SetManifest_test : public beast::unit_test::suite // An ephemeral key already claimed by a different account would // collide with -- and clobber -- that account's manifest object. - submit(env, makeManifest(master, ephemeral, 1)); + submit( + env, + signedEnvelope(env, makeManifest(master, ephemeral, 1), master)); env.close(); BEAST_EXPECT( - engineResult(submit(env, makeManifest(other, ephemeral, 1))) == + engineResult(submit( + env, + signedEnvelope( + env, makeManifest(other, ephemeral, 1), other))) == "tecDUPLICATE"); } @@ -563,6 +655,16 @@ struct SetManifest_test : public beast::unit_test::suite auto const good = makeManifest(master, ephemeral, 1); + // Establish the slot through the account-authorized lane so the + // unsigned envelopes below exercise update-envelope validation. + BEAST_EXPECT( + engineResult(submit( + env, + signedEnvelope( + env, makeManifest(master, ephemeral, 0), master))) == + "tesSUCCESS"); + env.close(); + // Sanity check: the unmodified envelope is the one Submit builds, so // every rejection below is attributable to the tweak and nothing else. BEAST_EXPECT( @@ -607,10 +709,7 @@ struct SetManifest_test : public beast::unit_test::suite obj.setFieldH256(sfAccountTxnID, uint256{1}); }}, {"TicketSequence", - [](STObject& obj) { obj.setFieldU32(sfTicketSequence, 1); }}, - {"SigningPubKey", [&](STObject& obj) { - obj.setFieldVL(sfSigningPubKey, master.pk().slice()); - }}}) + [](STObject& obj) { obj.setFieldU32(sfTicketSequence, 1); }}}) { BEAST_EXPECTS( applyDirect(env, envelope(env, good, master.id(), tweak)) == @@ -618,6 +717,21 @@ struct SetManifest_test : public beast::unit_test::suite name); } + // A non-empty signing key changes lanes. Without the corresponding + // account signature this is an invalid ordinary signed transaction, + // not a canonical unsigned envelope. + BEAST_EXPECT( + applyDirect( + env, envelope(env, good, master.id(), [&](STObject& obj) { + obj.setFieldVL(sfSigningPubKey, master.pk().slice()); + })) == temINVALID); + + BEAST_EXPECT( + applyDirect( + env, envelope(env, good, master.id(), [&](STObject& obj) { + obj.setFieldU32(sfLastLedgerSequence, env.current()->seq()); + })) == temMALFORMED); + // sfFee is the one envelope field preflight cannot bound, because the // base fee is not in scope until preclaim. checkFee() caps it instead. auto const priced = envelope(env, good, master.id()); @@ -629,12 +743,31 @@ struct SetManifest_test : public beast::unit_test::suite obj.setFieldAmount(sfFee, ceiling + XRPAmount{1}); })) == temBAD_FEE); + BEAST_EXPECT( + applyDirect( + env, envelope(env, good, master.id(), [&](STObject& obj) { + obj.setFieldAmount(sfFee, ceiling - XRPAmount{1}); + })) == temBAD_FEE); + // At the ceiling exactly, which is what Submit sends. BEAST_EXPECT( applyDirect( env, envelope(env, good, master.id(), [&](STObject& obj) { obj.setFieldAmount(sfFee, ceiling); })) == tesSUCCESS); + + // The account signature authenticates its envelope, so the signed + // lane deliberately uses ordinary configurable-fee semantics rather + // than the canonical unsigned Fee. + BEAST_EXPECT( + applyDirect( + env, + signedEnvelope( + env, + makeManifest(master, ephemeral, 2), + master, + std::nullopt, + ceiling + XRPAmount{100})) == tesSUCCESS); } void @@ -652,7 +785,9 @@ struct SetManifest_test : public beast::unit_test::suite env.close(); BEAST_EXPECT( - engineResult(submit(env, makeManifest(master, eph1, 1))) == + engineResult(submit( + env, + signedEnvelope(env, makeManifest(master, eph1, 1), master))) == "tesSUCCESS"); env.close(); @@ -711,9 +846,12 @@ struct SetManifest_test : public beast::unit_test::suite for (int i = 0; i < 4; ++i) { BEAST_EXPECT( - engineResult( - submit(env, makeManifest(masters[i], ephs[i], 1))) == - "tesSUCCESS"); + engineResult(submit( + env, + signedEnvelope( + env, + makeManifest(masters[i], ephs[i], 1), + masters[i]))) == "tesSUCCESS"); env.close(); } diff --git a/src/xrpld/app/hook/detail/HookAPI.cpp b/src/xrpld/app/hook/detail/HookAPI.cpp index dec51f781f..043ec4c44b 100644 --- a/src/xrpld/app/hook/detail/HookAPI.cpp +++ b/src/xrpld/app/hook/detail/HookAPI.cpp @@ -528,6 +528,16 @@ HookAPI::emit(Slice const& txBlob) const ripple::TxType txType = stpTrans->getTxnType(); + // SetManifest's account-signed lane must consume ordinary account replay + // protection, while its manifest-authorized lane is reserved for the + // protocol's canonical update envelope. Hook emission is neither. + if (txType == ttMANIFEST_SET) + { + JLOG(j.trace()) << "HookEmit[" << HC_ACC() + << "]: Hooks cannot emit SetManifest transactions."; + return Unexpected(EMISSION_FAILURE); + } + ripple::uint256 const& hookCanEmit = hookCtx.result.hookCanEmit; if (!hook::canEmit(txType, hookCanEmit)) { diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index a2c80eb6b0..e1beaa72cd 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1170,12 +1170,12 @@ NetworkOPsImp::publishNewerManifests(ReadView const& ledger) // Only for validators that have opted in by publishing on-ledger // already. Submitting spends the master key account's balance, so an // account that has never used the feature is left alone. - auto const sleMan = ledger.read(keylet::manifest(pk)); - if (!sleMan) + auto const ledgerSequence = onLedgerManifestSequence(ledger, pk); + if (!ledgerSequence) continue; auto const held = app_.validatorManifests().getRawManifest(pk); - if (!held || held->first <= sleMan->getFieldU32(sfSequence)) + if (!held || held->first <= *ledgerSequence) continue; auto const hex = makeSetManifestTx( diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index c66ec0f6ba..9552cf2396 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -1953,13 +1954,10 @@ TxQ::tryDirectApply( const bool isFirstImport = !sleAccount && view.rules().enabled(featureImport) && tx->getTxnType() == ttIMPORT; - // A manifest txn is pinned to sfSequence 0 (Transactor::checkSeqProxy), so - // it can never match the account sequence. Direct-apply it like a first - // Import: letting it fall through to the queue would reject it outright on - // sequence rather than hold it. Manifests are therefore exempt from fee - // escalation, since requiredFeeLevel is not consulted for them. + // Only the manifest-authorized lane is pinned to sfSequence 0. An + // account-signed SetManifest uses ordinary queue and fee behavior. const bool isManifest = view.rules().enabled(featureOnChainManifests) && - tx->getTxnType() == ttMANIFEST_SET; + isUnsignedSetManifest(*tx); const bool bypassQueue = isFirstImport || isManifest; diff --git a/src/xrpld/app/tx/detail/SetManifest.cpp b/src/xrpld/app/tx/detail/SetManifest.cpp index ebb54b6713..6efea2dada 100644 --- a/src/xrpld/app/tx/detail/SetManifest.cpp +++ b/src/xrpld/app/tx/detail/SetManifest.cpp @@ -34,6 +34,32 @@ namespace ripple { +bool +isUnsignedSetManifest(STTx const& tx) noexcept +{ + try + { + return tx.getTxnType() == ttMANIFEST_SET && + tx.isFieldPresent(sfSigningPubKey) && + tx.getSigningPubKey().empty() && + tx.isFieldPresent(sfTxnSignature) && tx.getSignature().empty() && + !tx.isFieldPresent(sfSigners); + } + catch (std::exception const&) + { + return false; + } +} + +std::optional +onLedgerManifestSequence(ReadView const& view, PublicKey const& masterKey) +{ + auto const sle = view.read(keylet::manifest(masterKey)); + if (!sle) + return std::nullopt; + return sle->getFieldU32(sfSequence); +} + TxConsequences SetManifest::makeTxConsequences(PreflightContext const& ctx) { @@ -95,17 +121,20 @@ SetManifest::preflight(PreflightContext const& ctx) // 3. not already revoked will be checked in preclaim because it depends on // lgr state - // 4. the envelope carries no account signature: authority comes solely - // from the manifest's own master/ephemeral signatures, which do not cover - // the envelope. Pin every envelope field a relayer could otherwise choose. - // The shape below must match the one checkValidity() recognises, or the - // txn falls through to the ordinary signature path and is rejected there. - // sfFee cannot be bounded here because the computed base fee is not in - // scope until preclaim; checkFee() bounds it instead. - if (!tx.isFieldPresent(sfSigningPubKey) || !tx.getSigningPubKey().empty() || - !tx.isFieldPresent(sfTxnSignature) || !tx.getSignature().empty() || - tx.isFieldPresent(sfSigners) || tx.isFieldPresent(sfAccountTxnID) || - tx.isFieldPresent(sfTicketSequence) || tx.getFieldU32(sfSequence) != 0) + // 4. Manifest-only authority is the canonical update lane. Pin every + // optional field that ordinary account signing would otherwise + // authenticate. sfFee is checked against the one computed value in + // checkFee(), where the ledger fee schedule is available. + if (isUnsignedSetManifest(tx) && + (tx.getFieldU32(sfSequence) != 0 || tx.isFieldPresent(sfFlags) || + tx.isFieldPresent(sfSourceTag) || tx.isFieldPresent(sfPreviousTxnID) || + tx.isFieldPresent(sfLastLedgerSequence) || + tx.isFieldPresent(sfAccountTxnID) || + tx.isFieldPresent(sfOperationLimit) || tx.isFieldPresent(sfMemos) || + tx.isFieldPresent(sfTicketSequence) || + tx.isFieldPresent(sfEmitDetails) || + tx.isFieldPresent(sfFirstLedgerSequence) || + tx.isFieldPresent(sfHookParameters) || tx.isFieldPresent(sfHookName))) { JLOG(j.warn()) << "SetManifest: envelope must be unsigned with Sequence 0."; @@ -136,6 +165,20 @@ SetManifest::preclaim(PreclaimContext const& ctx) if (!newManifest) return tefINTERNAL; // preflight already parsed this successfully + // Manifest-only authority may rotate or revoke an existing registration, + // but it cannot create the registration. Distinguish a genuinely empty + // slot from a corrupt AccountRoot that forgot an extant manifest object. + if (isUnsignedSetManifest(ctx.tx) && !sle->isFieldPresent(sfManifestID)) + { + if (ctx.view.exists(keylet::manifest(newManifest->masterKey))) + return tefBAD_LEDGER; + + JLOG(ctx.j.trace()) + << "SetManifest: unsigned envelope cannot create manifest slot. " + << id; + return tefBAD_AUTH; + } + // Replay protection. A byte-identical resubmission is rejected as // tefALREADY by checkPriorTxAndLastLedger, but sfFee may vary within the // band checkFee() allows, so the same manifest can also arrive under a @@ -205,6 +248,14 @@ SetManifest::doApply() if (!manifest || calcAccountID(manifest->masterKey) != account_) return tefINTERNAL; + // preclaim is the public stateful refusal. Keep the mutation boundary + // independently fail-closed so no future alternate apply path can turn an + // unsigned manifest into a first registration. + if (isUnsignedSetManifest(ctx_.tx) && !sle->isFieldPresent(sfManifestID)) + return view().exists(keylet::manifest(manifest->masterKey)) + ? tefBAD_LEDGER + : tefINTERNAL; + // A manifest is stored twice so it can be found from either key: // keylet::manifest(masterKey) -> obj1, sfManifestID -> obj2 // keylet::manifest(signingKey) -> obj2, sfManifestID -> obj1 @@ -312,7 +363,7 @@ SetManifest::calculateBaseFee(ReadView const& view, STTx const& tx) return Transactor::calculateBaseFee(view, tx) + manifestFee; } -/** The most sfFee may be: the same 1.2x headroom Submit applies. +/** The canonical unsigned sfFee: the same 1.2x headroom Submit applies. Kept in one place so the value Submit writes and the value preclaim will accept cannot drift apart. @@ -326,22 +377,26 @@ manifestFeeCeiling(XRPAmount baseFee) TER SetManifest::checkFee(PreclaimContext const& ctx, XRPAmount baseFee) { - // A ceiling is required because the envelope carries no account signature, - // so sfFee is chosen by whoever relays the txn -- and manifests are public: - // they are gossiped over the peer protocol and embedded in published UNLs, - // so the relayer need not be the master key holder. Uncapped, any observer - // of a not-yet-recorded manifest could wrap it with sfFee set to that - // validator's entire balance. The 20% band is headroom against a fee floor - // that has risen since the txn was built, and bounds what an attacker can - // burn to the same 20%. - if (ctx.tx[sfFee].xrp() > manifestFeeCeiling(baseFee)) + // Account-signed SetManifest transactions use ordinary fee semantics. + // Their outer signature authenticates the chosen Fee, and they may enter + // TxQ like any other account transaction. + if (!isUnsignedSetManifest(ctx.tx)) + return Transactor::checkFee(ctx, baseFee); + + // The manifest signature does not cover the outer transaction, so every + // valid relayer must derive the same Fee from the ledger fee schedule and + // manifest size. The 20% headroom remains, but is one exact value rather + // than a malleable band. Account-signed transactions returned above and + // authenticate their independently chosen Fee normally. + if (ctx.tx[sfFee].xrp() != manifestFeeCeiling(baseFee)) { - JLOG(ctx.j.trace()) << "SetManifest: fee above ceiling: " + JLOG(ctx.j.trace()) << "SetManifest: non-canonical unsigned fee: " << to_string(ctx.tx[sfFee].xrp()); return temBAD_FEE; } - // Floor and balance are the ordinary rules. + // Balance remains an ordinary rule. The exact canonical value is already + // at or above the ordinary base-fee floor by construction. return Transactor::checkFee(ctx, baseFee); } diff --git a/src/xrpld/app/tx/detail/SetManifest.h b/src/xrpld/app/tx/detail/SetManifest.h index aaea5a4c26..5f3d7ca0ed 100644 --- a/src/xrpld/app/tx/detail/SetManifest.h +++ b/src/xrpld/app/tx/detail/SetManifest.h @@ -24,17 +24,39 @@ #include #include #include +#include + +#include namespace ripple { -/** Encode the transaction that publishes `manifest` on-ledger. +/** Return whether a SetManifest envelope uses manifest-only authority. - A manifest transaction carries no account signature, so the protocol pins - the whole envelope: Sequence must be 0, SigningPubKey and TxnSignature must - be empty, and Fee must fall between the computed base fee and a ceiling - above it. SetManifest::preflight and SetManifest::checkFee reject anything - else. Every caller that submits a manifest builds it here so those rules - cannot drift apart from the ones the transactor enforces. + This is the one shared lane discriminator. An account-signed SetManifest + follows ordinary transaction signature, sequence, fee, and TxQ rules. + Only the exact empty outer-signature shape is eligible for the special + manifest-authorized lane; SetManifest::preflight pins its remaining + envelope fields. +*/ +bool +isUnsignedSetManifest(STTx const& tx) noexcept; + +/** Return the current on-ledger sequence for a registered master key. + + Absence is the anti-entropy boundary: background publication may update an + existing registration but must never bootstrap one. +*/ +std::optional +onLedgerManifestSequence(ReadView const& view, PublicKey const& masterKey); + +/** Encode a manifest-authorized update transaction for `manifest`. + + This lane carries no account signature and can update only an existing + manifest slot. Sequence must be 0, SigningPubKey and TxnSignature must be + empty, optional common fields must be absent, and Fee must equal the one + computed canonical value. SetManifest::preflight and SetManifest::checkFee + reject anything else. + Initial registration uses an ordinary account-signed SetManifest instead. Returns hex rather than an STTx because the manifest is appended to the encoded transaction verbatim, behind its object marker, instead of being diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 7b7c2402c5..e3c3adad83 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -604,11 +605,10 @@ Transactor::checkSeqProxy( return terNO_ACCOUNT; } - // A manifest txn is derived deterministically from the manifest alone, so - // it cannot depend on account state: preflight pins sfSequence to 0 and the - // account sequence is neither checked here nor consumed below. + // Only the manifest-authorized lane is independent of account sequence. + // Account-signed SetManifest transactions use ordinary replay protection. if (view.rules().enabled(featureOnChainManifests) && - tx.getTxnType() == ttMANIFEST_SET) + isUnsignedSetManifest(tx)) return tesSUCCESS; SeqProxy const a_seq = SeqProxy::sequence((*sle)[sfSequence]); @@ -762,15 +762,15 @@ Transactor::consumeSeqProxy(SLE::pointer const& sleAccount) if (ctx_.isEmittedTxn()) return tesSUCCESS; - // Manifest txns get the same treatment: pinned to sfSequence 0 and not - // signed by the account, so they neither consume nor reset its sequence. + // Manifest-authorized txns are pinned to sfSequence 0 and not signed by + // the account, so they neither consume nor reset its sequence. // Doing so would be actively harmful -- the write below is // seqProx.value() + 1, which for a seq-0 txn sets the account sequence to // 1 and makes every previously used sequence replayable. Handling it here // rather than in apply() also covers reset(), which re-consumes on the // tec / failed-invariant path. if (view().rules().enabled(featureOnChainManifests) && - ctx_.tx.getTxnType() == ttMANIFEST_SET) + isUnsignedSetManifest(ctx_.tx)) return tesSUCCESS; SeqProxy const seqProx = ctx_.tx.getSeqProxy(); @@ -916,10 +916,11 @@ Transactor::checkSign(PreclaimContext const& ctx) ctx.tx.getTxnType() == ttIMPORT) return tesSUCCESS; - // pass ttMANIFEST_SETs, their signatures are checked in preflight against - // the manifest's internal key logic + // The manifest-authorized lane is checked in preflight against the + // manifest's internal key logic. Account-signed SetManifest transactions + // continue through the ordinary single- or multi-signature path. if (ctx.view.rules().enabled(featureOnChainManifests) && - ctx.tx.getTxnType() == ttMANIFEST_SET) + isUnsignedSetManifest(ctx.tx)) return tesSUCCESS; if (ctx.flags & tapDRY_RUN) diff --git a/src/xrpld/app/tx/detail/apply.cpp b/src/xrpld/app/tx/detail/apply.cpp index c6d26cb43a..5156ab3a4f 100644 --- a/src/xrpld/app/tx/detail/apply.cpp +++ b/src/xrpld/app/tx/detail/apply.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -74,12 +75,8 @@ checkValidity( return {Validity::Valid, ""}; } - if (rules.enabled(featureOnChainManifests) && - tx.getTxnType() == ttMANIFEST_SET && - tx.isFieldPresent(sfTxnSignature) && - tx.getFieldVL(sfTxnSignature).empty() && - tx.isFieldPresent(sfSigningPubKey) && - tx.getFieldVL(sfSigningPubKey).empty() && tx.isFieldPresent(sfManifest)) + if (rules.enabled(featureOnChainManifests) && isUnsignedSetManifest(tx) && + tx.isFieldPresent(sfManifest)) { // perform alternative signature check over manifest STObject const& manObj = const_cast(tx)