From 5f12c22fbe45951521d03c999fcf58b2a439442e Mon Sep 17 00:00:00 2001 From: RichardAH Date: Tue, 20 Dec 2022 02:35:35 +0100 Subject: [PATCH 1/3] `featureDisallowIncoming`: Opt-out of incoming Checks, PayChans, NFTokenOffers and Trustlines (#4336) featureDisallowIncoming is a new amendment that would allow users to opt-out of incoming Checks, Payment Channels, NFTokenOffers, and trust lines. This commit includes tests. Adds four new AccountSet Flags: 1. asfDisallowIncomingNFTOffer 2. asfDisallowIncomingCheck 3. asfDisallowIncomingPayChan 4. asfDisallowIncomingTrustline --- src/ripple/app/tx/impl/CreateCheck.cpp | 10 +- src/ripple/app/tx/impl/NFTokenCreateOffer.cpp | 41 +++- src/ripple/app/tx/impl/PayChan.cpp | 13 +- src/ripple/app/tx/impl/SetAccount.cpp | 24 ++ src/ripple/app/tx/impl/SetTrust.cpp | 14 ++ src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/LedgerFormats.h | 11 + src/ripple/protocol/TxFlags.h | 7 + src/ripple/protocol/impl/Feature.cpp | 1 + src/test/app/Check_test.cpp | 98 ++++++++ src/test/app/NFToken_test.cpp | 133 ++++++++++ src/test/app/PayChan_test.cpp | 230 ++++++++++++------ src/test/app/SetTrust_test.cpp | 143 +++++++++-- src/test/rpc/AccountSet_test.cpp | 15 +- 14 files changed, 644 insertions(+), 99 deletions(-) diff --git a/src/ripple/app/tx/impl/CreateCheck.cpp b/src/ripple/app/tx/impl/CreateCheck.cpp index a59a7c12e..f5c2cbfbf 100644 --- a/src/ripple/app/tx/impl/CreateCheck.cpp +++ b/src/ripple/app/tx/impl/CreateCheck.cpp @@ -90,8 +90,14 @@ CreateCheck::preclaim(PreclaimContext const& ctx) return tecNO_DST; } - if ((sleDst->getFlags() & lsfRequireDestTag) && - !ctx.tx.isFieldPresent(sfDestinationTag)) + auto const flags = sleDst->getFlags(); + + // Check if the destination has disallowed incoming checks + if (ctx.view.rules().enabled(featureDisallowIncoming) && + (flags & lsfDisallowIncomingCheck)) + return tecNO_PERMISSION; + + if ((flags & lsfRequireDestTag) && !ctx.tx.isFieldPresent(sfDestinationTag)) { // The tag is basically account-specific information we don't // understand, but we can require someone to fill it in. diff --git a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp index 80e4c3964..695efdd0a 100644 --- a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp @@ -165,11 +165,42 @@ NFTokenCreateOffer::preclaim(PreclaimContext const& ctx) return tecUNFUNDED_OFFER; } - // If a destination is specified, the destination must already be in - // the ledger. - if (auto const destination = ctx.tx[~sfDestination]; - destination && !ctx.view.exists(keylet::account(*destination))) - return tecNO_DST; + if (auto const destination = ctx.tx[~sfDestination]) + { + // If a destination is specified, the destination must already be in + // the ledger. + auto const sleDst = ctx.view.read(keylet::account(*destination)); + + if (!sleDst) + return tecNO_DST; + + // check if the destination has disallowed incoming offers + if (ctx.view.rules().enabled(featureDisallowIncoming)) + { + // flag cannot be set unless amendment is enabled but + // out of an abundance of caution check anyway + + if (sleDst->getFlags() & lsfDisallowIncomingNFTOffer) + return tecNO_PERMISSION; + } + } + + if (auto const owner = ctx.tx[~sfOwner]) + { + // Check if the owner (buy offer) has disallowed incoming offers + if (ctx.view.rules().enabled(featureDisallowIncoming)) + { + auto const sleOwner = ctx.view.read(keylet::account(*owner)); + + // defensively check + // it should not be possible to specify owner that doesn't exist + if (!sleOwner) + return tecNO_TARGET; + + if (sleOwner->getFlags() & lsfDisallowIncomingNFTOffer) + return tecNO_PERMISSION; + } + } return tesSUCCESS; } diff --git a/src/ripple/app/tx/impl/PayChan.cpp b/src/ripple/app/tx/impl/PayChan.cpp index aab3dcc5a..1667bddcd 100644 --- a/src/ripple/app/tx/impl/PayChan.cpp +++ b/src/ripple/app/tx/impl/PayChan.cpp @@ -217,14 +217,21 @@ PayChanCreate::preclaim(PreclaimContext const& ctx) auto const sled = ctx.view.read(keylet::account(dst)); if (!sled) return tecNO_DST; - if (((*sled)[sfFlags] & lsfRequireDestTag) && - !ctx.tx[~sfDestinationTag]) + + auto const flags = sled->getFlags(); + + // Check if they have disallowed incoming payment channels + if (ctx.view.rules().enabled(featureDisallowIncoming) && + (flags & lsfDisallowIncomingPayChan)) + return tecNO_PERMISSION; + + if ((flags & lsfRequireDestTag) && !ctx.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; // Obeying the lsfDisallowXRP flag was a bug. Piggyback on // featureDepositAuth to remove the bug. if (!ctx.view.rules().enabled(featureDepositAuth) && - ((*sled)[sfFlags] & lsfDisallowXRP)) + (flags & lsfDisallowXRP)) return tecNO_TARGET; } diff --git a/src/ripple/app/tx/impl/SetAccount.cpp b/src/ripple/app/tx/impl/SetAccount.cpp index 85fe290ca..5c7d4369a 100644 --- a/src/ripple/app/tx/impl/SetAccount.cpp +++ b/src/ripple/app/tx/impl/SetAccount.cpp @@ -538,6 +538,30 @@ SetAccount::doApply() sle->makeFieldAbsent(sfNFTokenMinter); } + // Set or clear flags for disallowing various incoming instruments + if (ctx_.view().rules().enabled(featureDisallowIncoming)) + { + if (uSetFlag == asfDisallowIncomingNFTOffer) + uFlagsOut |= lsfDisallowIncomingNFTOffer; + else if (uClearFlag == asfDisallowIncomingNFTOffer) + uFlagsOut &= ~lsfDisallowIncomingNFTOffer; + + if (uSetFlag == asfDisallowIncomingCheck) + uFlagsOut |= lsfDisallowIncomingCheck; + else if (uClearFlag == asfDisallowIncomingCheck) + uFlagsOut &= ~lsfDisallowIncomingCheck; + + if (uSetFlag == asfDisallowIncomingPayChan) + uFlagsOut |= lsfDisallowIncomingPayChan; + else if (uClearFlag == asfDisallowIncomingPayChan) + uFlagsOut &= ~lsfDisallowIncomingPayChan; + + if (uSetFlag == asfDisallowIncomingTrustline) + uFlagsOut |= lsfDisallowIncomingTrustline; + else if (uClearFlag == asfDisallowIncomingTrustline) + uFlagsOut &= ~lsfDisallowIncomingTrustline; + } + if (uFlagsIn != uFlagsOut) sle->setFieldU32(sfFlags, uFlagsOut); diff --git a/src/ripple/app/tx/impl/SetTrust.cpp b/src/ripple/app/tx/impl/SetTrust.cpp index 23af19c7b..acbbedabf 100644 --- a/src/ripple/app/tx/impl/SetTrust.cpp +++ b/src/ripple/app/tx/impl/SetTrust.cpp @@ -128,6 +128,20 @@ SetTrust::preclaim(PreclaimContext const& ctx) } } + // If the destination has opted to disallow incoming trustlines + // then honour that flag + if (ctx.view.rules().enabled(featureDisallowIncoming)) + { + auto const sleDst = ctx.view.read(keylet::account(uDstAccountID)); + + if (!sleDst) + return tecNO_DST; + + auto const dstFlags = sleDst->getFlags(); + if (dstFlags & lsfDisallowIncomingTrustline) + return tecNO_PERMISSION; + } + return tesSUCCESS; } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index fac54c2fa..d4e65a31a 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -74,7 +74,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 53; +static constexpr std::size_t numFeatures = 54; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -340,6 +340,7 @@ extern uint256 const featureNonFungibleTokensV1_1; extern uint256 const fixTrustLinesToSelf; extern uint256 const fixRemoveNFTokenAutoTrustLine; extern uint256 const featureImmediateOfferKilled; +extern uint256 const featureDisallowIncoming; } // namespace ripple diff --git a/src/ripple/protocol/LedgerFormats.h b/src/ripple/protocol/LedgerFormats.h index 2dd04b126..45258a3d0 100644 --- a/src/ripple/protocol/LedgerFormats.h +++ b/src/ripple/protocol/LedgerFormats.h @@ -232,6 +232,17 @@ enum LedgerSpecificFlags { lsfDefaultRipple = 0x00800000, // True, trust lines allow rippling by default lsfDepositAuth = 0x01000000, // True, all deposits require authorization +/* // reserved for Hooks amendment + lsfTshCollect = 0x02000000, // True, allow TSH collect-calls to acc hooks +*/ + lsfDisallowIncomingNFTOffer = + 0x04000000, // True, reject new incoming NFT offers + lsfDisallowIncomingCheck = + 0x08000000, // True, reject new checks + lsfDisallowIncomingPayChan = + 0x10000000, // True, reject new paychans + lsfDisallowIncomingTrustline = + 0x20000000, // True, reject new trustlines (only if no issued assets) // ltOFFER lsfPassive = 0x00010000, diff --git a/src/ripple/protocol/TxFlags.h b/src/ripple/protocol/TxFlags.h index 0ad088c41..c42182198 100644 --- a/src/ripple/protocol/TxFlags.h +++ b/src/ripple/protocol/TxFlags.h @@ -79,6 +79,13 @@ constexpr std::uint32_t asfGlobalFreeze = 7; constexpr std::uint32_t asfDefaultRipple = 8; constexpr std::uint32_t asfDepositAuth = 9; constexpr std::uint32_t asfAuthorizedNFTokenMinter = 10; +/* // reserved for Hooks amendment +constexpr std::uint32_t asfTshCollect = 11; +*/ +constexpr std::uint32_t asfDisallowIncomingNFTOffer = 12; +constexpr std::uint32_t asfDisallowIncomingCheck = 13; +constexpr std::uint32_t asfDisallowIncomingPayChan = 14; +constexpr std::uint32_t asfDisallowIncomingTrustline = 15; // OfferCreate flags: constexpr std::uint32_t tfPassive = 0x00010000; diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index fa0d167ef..5903603f9 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -450,6 +450,7 @@ REGISTER_FEATURE(NonFungibleTokensV1_1, Supported::yes, DefaultVote::no) REGISTER_FIX (fixTrustLinesToSelf, Supported::yes, DefaultVote::no); REGISTER_FIX (fixRemoveNFTokenAutoTrustLine, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(ImmediateOfferKilled, Supported::yes, DefaultVote::no); +REGISTER_FEATURE(DisallowIncoming, Supported::yes, DefaultVote::no); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp index 31a2e572e..8f0c0ec46 100644 --- a/src/test/app/Check_test.cpp +++ b/src/test/app/Check_test.cpp @@ -85,6 +85,8 @@ public: class Check_test : public beast::unit_test::suite { + FeatureBitset const disallowIncoming{featureDisallowIncoming}; + static uint256 getCheckIndex(AccountID const& account, std::uint32_t uSequence) { @@ -293,6 +295,100 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT(checksOnAccount(env, bob).size() == bobCount + 7); } + void + testCreateDisallowIncoming(FeatureBitset features) + { + testcase("Create valid with disallow incoming"); + + using namespace test::jtx; + + // test flag doesn't set unless amendment enabled + { + Env env{*this, features - disallowIncoming}; + Account const alice{"alice"}; + env.fund(XRP(10000), alice); + env(fset(alice, asfDisallowIncomingCheck)); + env.close(); + auto const sle = env.le(alice); + uint32_t flags = sle->getFlags(); + BEAST_EXPECT(!(flags & lsfDisallowIncomingCheck)); + } + + Account const gw{"gateway"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + IOU const USD{gw["USD"]}; + + Env env{*this, features | disallowIncoming}; + + STAmount const startBalance{XRP(1000).value()}; + env.fund(startBalance, gw, alice, bob); + + /* + * Attempt to create two checks from `from` to `to` and + * require they both result in error/success code `expected` + */ + auto writeTwoChecksDI = [&env, &USD, this]( + Account const& from, + Account const& to, + TER expected) { + std::uint32_t const fromOwnerCount{ownerCount(env, from)}; + std::uint32_t const toOwnerCount{ownerCount(env, to)}; + + std::size_t const fromCkCount{checksOnAccount(env, from).size()}; + std::size_t const toCkCount{checksOnAccount(env, to).size()}; + + env(check::create(from, to, XRP(2000)), ter(expected)); + env.close(); + + env(check::create(from, to, USD(50)), ter(expected)); + env.close(); + + if (expected == tesSUCCESS) + { + BEAST_EXPECT( + checksOnAccount(env, from).size() == fromCkCount + 2); + BEAST_EXPECT(checksOnAccount(env, to).size() == toCkCount + 2); + + env.require(owners(from, fromOwnerCount + 2)); + env.require( + owners(to, to == from ? fromOwnerCount + 2 : toOwnerCount)); + return; + } + + BEAST_EXPECT(checksOnAccount(env, from).size() == fromCkCount); + BEAST_EXPECT(checksOnAccount(env, to).size() == toCkCount); + + env.require(owners(from, fromOwnerCount)); + env.require(owners(to, to == from ? fromOwnerCount : toOwnerCount)); + }; + + // enable the DisallowIncoming flag on both bob and alice + env(fset(bob, asfDisallowIncomingCheck)); + env(fset(alice, asfDisallowIncomingCheck)); + env.close(); + + // both alice and bob can't receive checks + writeTwoChecksDI(alice, bob, tecNO_PERMISSION); + writeTwoChecksDI(gw, alice, tecNO_PERMISSION); + + // remove the flag from alice but not from bob + env(fclear(alice, asfDisallowIncomingCheck)); + env.close(); + + // now bob can send alice a cheque but not visa-versa + writeTwoChecksDI(bob, alice, tesSUCCESS); + writeTwoChecksDI(alice, bob, tecNO_PERMISSION); + + // remove bob's flag too + env(fclear(bob, asfDisallowIncomingCheck)); + env.close(); + + // now they can send checks freely + writeTwoChecksDI(bob, alice, tesSUCCESS); + writeTwoChecksDI(alice, bob, tesSUCCESS); + } + void testCreateInvalid(FeatureBitset features) { @@ -2602,6 +2698,7 @@ class Check_test : public beast::unit_test::suite { testEnabled(features); testCreateValid(features); + testCreateDisallowIncoming(features); testCreateInvalid(features); testCashXRP(features); testCashIOU(features); @@ -2621,6 +2718,7 @@ public: using namespace test::jtx; auto const sa = supported_amendments(); testWithFeats(sa - featureCheckCashMakesTrustLine); + testWithFeats(sa - disallowIncoming); testWithFeats(sa); testTrustLineCreation(sa); // Test with featureCheckCashMakesTrustLine diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 2fb27f8a3..42a6eb4d3 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -29,6 +29,8 @@ namespace ripple { class NFToken_test : public beast::unit_test::suite { + FeatureBitset const disallowIncoming{featureDisallowIncoming}; + // Helper function that returns the owner count of an account root. static std::uint32_t ownerCount(test::jtx::Env const& env, test::jtx::Account const& acct) @@ -2975,6 +2977,135 @@ class NFToken_test : public beast::unit_test::suite } } + void + testCreateOfferDestinationDisallowIncoming(FeatureBitset features) + { + testcase("Create offer destination disallow incoming"); + + using namespace test::jtx; + + // test flag doesn't set unless amendment enabled + { + Env env{*this, features - disallowIncoming}; + Account const alice{"alice"}; + env.fund(XRP(10000), alice); + env(fset(alice, asfDisallowIncomingNFTOffer)); + env.close(); + auto const sle = env.le(alice); + uint32_t flags = sle->getFlags(); + BEAST_EXPECT(!(flags & lsfDisallowIncomingNFTOffer)); + } + + Env env{*this, features | disallowIncoming}; + + Account const issuer{"issuer"}; + Account const minter{"minter"}; + Account const buyer{"buyer"}; + Account const alice{"alice"}; + + env.fund(XRP(1000), issuer, minter, buyer, alice); + + env(token::setMinter(issuer, minter)); + env.close(); + + uint256 const nftokenID = + token::getNextID(env, issuer, 0, tfTransferable); + env(token::mint(minter, 0), + token::issuer(issuer), + txflags(tfTransferable)); + env.close(); + + // enable flag + env(fset(buyer, asfDisallowIncomingNFTOffer)); + env.close(); + + // a sell offer from the minter to the buyer should be rejected + { + env(token::createOffer(minter, nftokenID, drops(1)), + token::destination(buyer), + txflags(tfSellNFToken), + ter(tecNO_PERMISSION)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 0); + } + + // disable the flag + env(fclear(buyer, asfDisallowIncomingNFTOffer)); + env.close(); + + // create offer (allowed now) then cancel + { + uint256 const offerIndex = + keylet::nftoffer(minter, env.seq(minter)).key; + + env(token::createOffer(minter, nftokenID, drops(1)), + token::destination(buyer), + txflags(tfSellNFToken)); + env.close(); + + env(token::cancelOffer(minter, {offerIndex})); + env.close(); + } + + // create offer, enable flag, then cancel + { + uint256 const offerIndex = + keylet::nftoffer(minter, env.seq(minter)).key; + + env(token::createOffer(minter, nftokenID, drops(1)), + token::destination(buyer), + txflags(tfSellNFToken)); + env.close(); + + env(fset(buyer, asfDisallowIncomingNFTOffer)); + env.close(); + + env(token::cancelOffer(minter, {offerIndex})); + env.close(); + + env(fclear(buyer, asfDisallowIncomingNFTOffer)); + env.close(); + } + + // create offer then transfer + { + uint256 const offerIndex = + keylet::nftoffer(minter, env.seq(minter)).key; + + env(token::createOffer(minter, nftokenID, drops(1)), + token::destination(buyer), + txflags(tfSellNFToken)); + env.close(); + + env(token::acceptSellOffer(buyer, offerIndex)); + env.close(); + } + + // buyer now owns the token + + // enable flag again + env(fset(buyer, asfDisallowIncomingNFTOffer)); + env.close(); + + // a random offer to buy the token + { + env(token::createOffer(alice, nftokenID, drops(1)), + token::owner(buyer), + ter(tecNO_PERMISSION)); + env.close(); + } + + // minter offer to buy the token + { + env(token::createOffer(minter, nftokenID, drops(1)), + token::owner(buyer), + ter(tecNO_PERMISSION)); + env.close(); + } + } + void testCreateOfferExpiration(FeatureBitset features) { @@ -4929,6 +5060,7 @@ class NFToken_test : public beast::unit_test::suite testMintTaxon(features); testMintURI(features); testCreateOfferDestination(features); + testCreateOfferDestinationDisallowIncoming(features); testCreateOfferExpiration(features); testCancelOffers(features); testCancelTooManyOffers(features); @@ -4949,6 +5081,7 @@ public: FeatureBitset const fixNFTDir{fixNFTokenDirV1}; testWithFeats(all - fixNFTDir); + testWithFeats(all - disallowIncoming); testWithFeats(all); } }; diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index cf600a9fc..2a8ea360e 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -32,6 +32,8 @@ namespace ripple { namespace test { struct PayChan_test : public beast::unit_test::suite { + FeatureBitset const disallowIncoming{featureDisallowIncoming}; + static uint256 channel( jtx::Account const& account, @@ -175,12 +177,12 @@ struct PayChan_test : public beast::unit_test::suite } void - testSimple() + testSimple(FeatureBitset features) { testcase("simple"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); auto USDA = alice["USD"]; @@ -350,7 +352,91 @@ struct PayChan_test : public beast::unit_test::suite } void - testCancelAfter() + testDisallowIncoming(FeatureBitset features) + { + testcase("Disallow Incoming Flag"); + using namespace jtx; + + // test flag doesn't set unless amendment enabled + { + Env env{*this, features - disallowIncoming}; + Account const alice{"alice"}; + env.fund(XRP(10000), alice); + env(fset(alice, asfDisallowIncomingPayChan)); + env.close(); + auto const sle = env.le(alice); + uint32_t flags = sle->getFlags(); + BEAST_EXPECT(!(flags & lsfDisallowIncomingPayChan)); + } + + using namespace std::literals::chrono_literals; + Env env{*this, features | disallowIncoming}; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const cho = Account("cho"); + env.fund(XRP(10000), alice, bob, cho); + auto const pk = alice.pk(); + auto const settleDelay = 100s; + + // set flag on bob only + env(fset(bob, asfDisallowIncomingPayChan)); + env.close(); + + // channel creation from alice to bob is disallowed + { + auto const chan = channel(alice, bob, env.seq(alice)); + env(create(alice, bob, XRP(1000), settleDelay, pk), + ter(tecNO_PERMISSION)); + BEAST_EXPECT(!channelExists(*env.current(), chan)); + } + + // set flag on alice also + env(fset(alice, asfDisallowIncomingPayChan)); + env.close(); + + // channel creation from bob to alice is now disallowed + { + auto const chan = channel(bob, alice, env.seq(bob)); + env(create(bob, alice, XRP(1000), settleDelay, pk), + ter(tecNO_PERMISSION)); + BEAST_EXPECT(!channelExists(*env.current(), chan)); + } + + // remove flag from bob + env(fclear(bob, asfDisallowIncomingPayChan)); + env.close(); + + // now the channel between alice and bob can exist + { + auto const chan = channel(alice, bob, env.seq(alice)); + env(create(alice, bob, XRP(1000), settleDelay, pk), + ter(tesSUCCESS)); + BEAST_EXPECT(channelExists(*env.current(), chan)); + } + + // a channel from cho to alice isn't allowed + { + auto const chan = channel(cho, alice, env.seq(cho)); + env(create(cho, alice, XRP(1000), settleDelay, pk), + ter(tecNO_PERMISSION)); + BEAST_EXPECT(!channelExists(*env.current(), chan)); + } + + // remove flag from alice + env(fclear(alice, asfDisallowIncomingPayChan)); + env.close(); + + // now a channel from cho to alice is allowed + { + auto const chan = channel(cho, alice, env.seq(cho)); + env(create(cho, alice, XRP(1000), settleDelay, pk), + ter(tesSUCCESS)); + BEAST_EXPECT(channelExists(*env.current(), chan)); + } + } + + void + testCancelAfter(FeatureBitset features) { testcase("cancel after"); using namespace jtx; @@ -360,7 +446,7 @@ struct PayChan_test : public beast::unit_test::suite auto const carol = Account("carol"); { // If dst claims after cancel after, channel closes - Env env(*this); + Env env{*this, features}; env.fund(XRP(10000), alice, bob); auto const pk = alice.pk(); auto const settleDelay = 100s; @@ -392,7 +478,7 @@ struct PayChan_test : public beast::unit_test::suite } { // Third party can close after cancel after - Env env(*this); + Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol); auto const pk = alice.pk(); auto const settleDelay = 100s; @@ -415,12 +501,12 @@ struct PayChan_test : public beast::unit_test::suite } void - testExpiration() + testExpiration(FeatureBitset features) { testcase("expiration"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); auto const carol = Account("carol"); @@ -481,12 +567,12 @@ struct PayChan_test : public beast::unit_test::suite } void - testSettleDelay() + testSettleDelay(FeatureBitset features) { testcase("settle delay"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); env.fund(XRP(10000), alice, bob); @@ -541,12 +627,12 @@ struct PayChan_test : public beast::unit_test::suite } void - testCloseDry() + testCloseDry(FeatureBitset features) { testcase("close dry"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); env.fund(XRP(10000), alice, bob); @@ -575,13 +661,13 @@ struct PayChan_test : public beast::unit_test::suite } void - testDefaultAmount() + testDefaultAmount(FeatureBitset features) { // auth amount defaults to balance if not present testcase("default amount"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); env.fund(XRP(10000), alice, bob); @@ -630,7 +716,7 @@ struct PayChan_test : public beast::unit_test::suite } void - testDisallowXRP() + testDisallowXRP(FeatureBitset features) { // auth amount defaults to balance if not present testcase("Disallow XRP"); @@ -641,7 +727,7 @@ struct PayChan_test : public beast::unit_test::suite auto const bob = Account("bob"); { // Create a channel where dst disallows XRP - Env env(*this, supported_amendments() - featureDepositAuth); + Env env(*this, features - featureDepositAuth); env.fund(XRP(10000), alice, bob); env(fset(bob, asfDisallowXRP)); auto const chan = channel(alice, bob, env.seq(alice)); @@ -652,7 +738,7 @@ struct PayChan_test : public beast::unit_test::suite { // Create a channel where dst disallows XRP. Ignore that flag, // since it's just advisory. - Env env(*this); + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env(fset(bob, asfDisallowXRP)); auto const chan = channel(alice, bob, env.seq(alice)); @@ -663,7 +749,7 @@ struct PayChan_test : public beast::unit_test::suite { // Claim to a channel where dst disallows XRP // (channel is created before disallow xrp is set) - Env env(*this, supported_amendments() - featureDepositAuth); + Env env(*this, features - featureDepositAuth); env.fund(XRP(10000), alice, bob); auto const chan = channel(alice, bob, env.seq(alice)); env(create(alice, bob, XRP(1000), 3600s, alice.pk())); @@ -677,7 +763,7 @@ struct PayChan_test : public beast::unit_test::suite // Claim to a channel where dst disallows XRP (channel is // created before disallow xrp is set). Ignore that flag // since it is just advisory. - Env env(*this); + Env env{*this, features}; env.fund(XRP(10000), alice, bob); auto const chan = channel(alice, bob, env.seq(alice)); env(create(alice, bob, XRP(1000), 3600s, alice.pk())); @@ -690,14 +776,14 @@ struct PayChan_test : public beast::unit_test::suite } void - testDstTag() + testDstTag(FeatureBitset features) { // auth amount defaults to balance if not present testcase("Dst Tag"); using namespace jtx; using namespace std::literals::chrono_literals; // Create a channel where dst disallows XRP - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); env.fund(XRP(10000), alice, bob); @@ -720,7 +806,7 @@ struct PayChan_test : public beast::unit_test::suite } void - testDepositAuth() + testDepositAuth(FeatureBitset features) { testcase("Deposit Authorization"); using namespace jtx; @@ -731,7 +817,7 @@ struct PayChan_test : public beast::unit_test::suite auto const carol = Account("carol"); auto USDA = alice["USD"]; { - Env env(*this); + Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol); env(fset(bob, asfDepositAuth)); @@ -844,13 +930,13 @@ struct PayChan_test : public beast::unit_test::suite } void - testMultiple() + testMultiple(FeatureBitset features) { // auth amount defaults to balance if not present testcase("Multiple channels to the same account"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); env.fund(XRP(10000), alice, bob); @@ -867,13 +953,13 @@ struct PayChan_test : public beast::unit_test::suite } void - testAccountChannelsRPC() + testAccountChannelsRPC(FeatureBitset features) { testcase("AccountChannels RPC"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); auto const charlie = Account("charlie", KeyType::ed25519); @@ -922,7 +1008,7 @@ struct PayChan_test : public beast::unit_test::suite } void - testAccountChannelsRPCMarkers() + testAccountChannelsRPCMarkers(FeatureBitset features) { testcase("Account channels RPC markers"); @@ -941,7 +1027,7 @@ struct PayChan_test : public beast::unit_test::suite return r; }(); - Env env(*this); + Env env{*this, features}; env.fund(XRP(10000), alice); for (auto const& a : bobs) { @@ -1038,7 +1124,7 @@ struct PayChan_test : public beast::unit_test::suite } void - testAccountChannelsRPCSenderOnly() + testAccountChannelsRPCSenderOnly(FeatureBitset features) { // Check that the account_channels command only returns channels owned // by the account @@ -1049,7 +1135,7 @@ struct PayChan_test : public beast::unit_test::suite auto const alice = Account("alice"); auto const bob = Account("bob"); - Env env(*this); + Env env{*this, features}; env.fund(XRP(10000), alice, bob); // Create a channel from alice to bob and from bob to alice @@ -1075,12 +1161,12 @@ struct PayChan_test : public beast::unit_test::suite } void - testAuthVerifyRPC() + testAuthVerifyRPC(FeatureBitset features) { testcase("PayChan Auth/Verify RPC"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); auto const charlie = Account("charlie", KeyType::ed25519); @@ -1415,12 +1501,12 @@ struct PayChan_test : public beast::unit_test::suite } void - testOptionalFields() + testOptionalFields(FeatureBitset features) { testcase("Optional Fields"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); auto const carol = Account("carol"); @@ -1466,12 +1552,12 @@ struct PayChan_test : public beast::unit_test::suite } void - testMalformedPK() + testMalformedPK(FeatureBitset features) { testcase("malformed pk"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); auto USDA = alice["USD"]; @@ -1536,7 +1622,7 @@ struct PayChan_test : public beast::unit_test::suite } void - testMetaAndOwnership() + testMetaAndOwnership(FeatureBitset features) { testcase("Metadata & Ownership"); @@ -1565,8 +1651,7 @@ struct PayChan_test : public beast::unit_test::suite { // Test without adding the paychan to the recipient's owner // directory - Env env( - *this, supported_amendments() - fixPayChanRecipientOwnerDir); + Env env(*this, features - fixPayChanRecipientOwnerDir); env.fund(XRP(10000), alice, bob); env(create(alice, bob, XRP(1000), settleDelay, pk)); env.close(); @@ -1587,7 +1672,7 @@ struct PayChan_test : public beast::unit_test::suite { // Test with adding the paychan to the recipient's owner directory - Env env(*this); + Env env{*this, features}; env.fund(XRP(10000), alice, bob); env(create(alice, bob, XRP(1000), settleDelay, pk)); env.close(); @@ -1609,8 +1694,7 @@ struct PayChan_test : public beast::unit_test::suite { // Test removing paychans created before adding to the recipient's // owner directory - Env env( - *this, supported_amendments() - fixPayChanRecipientOwnerDir); + Env env(*this, features - fixPayChanRecipientOwnerDir); env.fund(XRP(10000), alice, bob); // create the channel before the amendment activates env(create(alice, bob, XRP(1000), settleDelay, pk)); @@ -1644,7 +1728,7 @@ struct PayChan_test : public beast::unit_test::suite } void - testAccountDelete() + testAccountDelete(FeatureBitset features) { testcase("Account Delete"); using namespace test::jtx; @@ -1678,8 +1762,8 @@ struct PayChan_test : public beast::unit_test::suite for (bool const withOwnerDirFix : {false, true}) { auto const amd = withOwnerDirFix - ? supported_amendments() - : supported_amendments() - fixPayChanRecipientOwnerDir; + ? features + : features - fixPayChanRecipientOwnerDir; Env env{*this, amd}; env.fund(XRP(10000), alice, bob, carol); env.close(); @@ -1771,8 +1855,7 @@ struct PayChan_test : public beast::unit_test::suite { // test resurrected account - Env env{ - *this, supported_amendments() - fixPayChanRecipientOwnerDir}; + Env env{*this, features - fixPayChanRecipientOwnerDir}; env.fund(XRP(10000), alice, bob, carol); env.close(); auto const feeDrops = env.current()->fees().base; @@ -1878,12 +1961,12 @@ struct PayChan_test : public beast::unit_test::suite } void - testUsingTickets() + testUsingTickets(FeatureBitset features) { testcase("using tickets"); using namespace jtx; using namespace std::literals::chrono_literals; - Env env(*this); + Env env{*this, features}; auto const alice = Account("alice"); auto const bob = Account("bob"); auto USDA = alice["USD"]; @@ -2039,28 +2122,39 @@ struct PayChan_test : public beast::unit_test::suite BEAST_EXPECT(env.seq(bob) == bobSeq); } + void + testWithFeats(FeatureBitset features) + { + testSimple(features); + testDisallowIncoming(features); + testCancelAfter(features); + testSettleDelay(features); + testExpiration(features); + testCloseDry(features); + testDefaultAmount(features); + testDisallowXRP(features); + testDstTag(features); + testDepositAuth(features); + testMultiple(features); + testAccountChannelsRPC(features); + testAccountChannelsRPCMarkers(features); + testAccountChannelsRPCSenderOnly(features); + testAuthVerifyRPC(features); + testOptionalFields(features); + testMalformedPK(features); + testMetaAndOwnership(features); + testAccountDelete(features); + testUsingTickets(features); + } + +public: void run() override { - testSimple(); - testCancelAfter(); - testSettleDelay(); - testExpiration(); - testCloseDry(); - testDefaultAmount(); - testDisallowXRP(); - testDstTag(); - testDepositAuth(); - testMultiple(); - testAccountChannelsRPC(); - testAccountChannelsRPCMarkers(); - testAccountChannelsRPCSenderOnly(); - testAuthVerifyRPC(); - testOptionalFields(); - testMalformedPK(); - testMetaAndOwnership(); - testAccountDelete(); - testUsingTickets(); + using namespace test::jtx; + FeatureBitset const all{supported_amendments()}; + testWithFeats(all - disallowIncoming); + testWithFeats(all); } }; diff --git a/src/test/app/SetTrust_test.cpp b/src/test/app/SetTrust_test.cpp index 45a9e5c76..fce9c4295 100644 --- a/src/test/app/SetTrust_test.cpp +++ b/src/test/app/SetTrust_test.cpp @@ -26,9 +26,14 @@ namespace test { class SetTrust_test : public beast::unit_test::suite { + FeatureBitset const disallowIncoming{featureDisallowIncoming}; + public: void - testFreeTrustlines(bool thirdLineCreatesLE, bool createOnHighAcct) + testFreeTrustlines( + FeatureBitset features, + bool thirdLineCreatesLE, + bool createOnHighAcct) { if (thirdLineCreatesLE) testcase("Allow two free trustlines"); @@ -36,7 +41,7 @@ public: testcase("Dynamic reserve for trustline"); using namespace jtx; - Env env(*this); + Env env(*this, features); auto const gwA = Account{"gwA"}; auto const gwB = Account{"gwB"}; @@ -107,14 +112,14 @@ public: } void - testTicketSetTrust() + testTicketSetTrust(FeatureBitset features) { testcase("SetTrust using a ticket"); using namespace jtx; // Verify that TrustSet transactions can use tickets. - Env env{*this}; + Env env{*this, features}; auto const gw = Account{"gateway"}; auto const alice = Account{"alice"}; auto const USD = gw["USD"]; @@ -152,12 +157,12 @@ public: } void - testMalformedTransaction() + testMalformedTransaction(FeatureBitset features) { testcase("SetTrust checks for malformed transactions"); using namespace jtx; - Env env{*this}; + Env env{*this, features}; auto const gw = Account{"gateway"}; auto const alice = Account{"alice"}; @@ -199,14 +204,17 @@ public: } void - testModifyQualityOfTrustline(bool createQuality, bool createOnHighAcct) + testModifyQualityOfTrustline( + FeatureBitset features, + bool createQuality, + bool createOnHighAcct) { testcase << "SetTrust " << (createQuality ? "creates" : "removes") << " quality of trustline for " << (createOnHighAcct ? "high" : "low") << " account"; using namespace jtx; - Env env{*this}; + Env env{*this, features}; auto const alice = Account{"alice"}; auto const bob = Account{"bob"}; @@ -249,20 +257,119 @@ public: } void - run() override + testDisallowIncoming(FeatureBitset features) { - testFreeTrustlines(true, false); - testFreeTrustlines(false, true); - testFreeTrustlines(false, true); + testcase("Create trustline with disallow incoming"); + + using namespace test::jtx; + + // test flag doesn't set unless amendment enabled + { + Env env{*this, features - disallowIncoming}; + Account const alice{"alice"}; + env.fund(XRP(10000), alice); + env(fset(alice, asfDisallowIncomingTrustline)); + env.close(); + auto const sle = env.le(alice); + uint32_t flags = sle->getFlags(); + BEAST_EXPECT(!(flags & lsfDisallowIncomingTrustline)); + } + + Env env{*this, features | disallowIncoming}; + + auto const gw = Account{"gateway"}; + auto const alice = Account{"alice"}; + auto const bob = Account{"bob"}; + auto const USD = gw["USD"]; + + env.fund(XRP(10000), gw, alice, bob); + env.close(); + + // Set flag on gateway + env(fset(gw, asfDisallowIncomingTrustline)); + env.close(); + + // Create a trustline which will fail + env(trust(alice, USD(1000)), ter(tecNO_PERMISSION)); + env.close(); + + // Unset the flag + env(fclear(gw, asfDisallowIncomingTrustline)); + env.close(); + + // Create a trustline which will now succeed + env(trust(alice, USD(1000))); + env.close(); + + // Now the payment succeeds. + env(pay(gw, alice, USD(200))); + env.close(); + + // Set flag on gateway again + env(fset(gw, asfDisallowIncomingTrustline)); + env.close(); + + // Destroy the balance by sending it back + env(pay(gw, alice, USD(200))); + env.close(); + + // The trustline still exists in default state + // So a further payment should work + env(pay(gw, alice, USD(200))); + env.close(); + + // Also set the flag on bob + env(fset(bob, asfDisallowIncomingTrustline)); + env.close(); + + // But now bob can't open a trustline because he didn't already have one + env(trust(bob, USD(1000)), ter(tecNO_PERMISSION)); + env.close(); + + // The gateway also can't open this trustline because bob has the flag + // set + env(trust(gw, bob["USD"](1000)), ter(tecNO_PERMISSION)); + env.close(); + + // Unset the flag only on the gateway + env(fclear(gw, asfDisallowIncomingTrustline)); + env.close(); + + // Now bob can open a trustline + env(trust(bob, USD(1000))); + env.close(); + + // And the gateway can send bob a balance + env(pay(gw, bob, USD(200))); + env.close(); + } + + void + testWithFeats(FeatureBitset features) + { + testFreeTrustlines(features, true, false); + testFreeTrustlines(features, false, true); + testFreeTrustlines(features, false, true); // true, true case doesn't matter since creating a trustline ledger // entry requires reserve from the creator // independent of hi/low account ids for endpoints - testTicketSetTrust(); - testMalformedTransaction(); - testModifyQualityOfTrustline(false, false); - testModifyQualityOfTrustline(false, true); - testModifyQualityOfTrustline(true, false); - testModifyQualityOfTrustline(true, true); + testTicketSetTrust(features); + testMalformedTransaction(features); + testModifyQualityOfTrustline(features, false, false); + testModifyQualityOfTrustline(features, false, true); + testModifyQualityOfTrustline(features, true, false); + testModifyQualityOfTrustline(features, true, true); + testDisallowIncoming(features); + } + +public: + void + run() override + { + using namespace test::jtx; + auto const sa = supported_amendments(); + testWithFeats(sa - disallowIncoming); + testWithFeats(sa); } }; BEAST_DEFINE_TESTSUITE(SetTrust, app, ripple); diff --git a/src/test/rpc/AccountSet_test.cpp b/src/test/rpc/AccountSet_test.cpp index 8e1ec790b..b3ca4c9f0 100644 --- a/src/test/rpc/AccountSet_test.cpp +++ b/src/test/rpc/AccountSet_test.cpp @@ -75,6 +75,7 @@ public: // elsewhere. continue; } + if (flag == asfAuthorizedNFTokenMinter) { // The asfAuthorizedNFTokenMinter flag requires the @@ -82,8 +83,18 @@ public: // the transaction. It is tested elsewhere. continue; } - else if ( - std::find(goodFlags.begin(), goodFlags.end(), flag) != + + if (flag == asfDisallowIncomingCheck || + flag == asfDisallowIncomingPayChan || + flag == asfDisallowIncomingNFTOffer || + flag == asfDisallowIncomingTrustline) + { + // These flags are part of the DisallowIncoming amendment + // and are tested elsewhere + continue; + } + + if (std::find(goodFlags.begin(), goodFlags.end(), flag) != goodFlags.end()) { // Good flag From c50eb7773feb21c5f5d0cf76e5df8689486b46d2 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Tue, 3 Jan 2023 07:24:45 -1000 Subject: [PATCH 2/3] RPC tooBusy response has 503 HTTP status if "ripplerpc": "3.0": (#4143) Fixes #4005 Makes it possible for internal RPC Error Codes to associate themselves with a non-OK (200) HTTP status code. There are quite a number of RPC responses in addition to tooBusy that now have non-OK HTTP status codes. The new return HTTP return codes are only enabled by including "ripplerpc": "3.0" or higher in the original request. Otherwise the historical value, 200, continues to be returned. This ensures that this is not a breaking change. --- src/ripple/net/impl/RPCCall.cpp | 11 +- src/ripple/protocol/ErrorCodes.h | 26 +++- src/ripple/protocol/impl/ErrorCodes.cpp | 180 +++++++++++------------ src/ripple/rpc/impl/ServerHandlerImp.cpp | 26 +++- src/ripple/server/impl/JSONRPCUtil.cpp | 17 ++- src/test/rpc/LedgerRPC_test.cpp | 2 +- 6 files changed, 154 insertions(+), 108 deletions(-) diff --git a/src/ripple/net/impl/RPCCall.cpp b/src/ripple/net/impl/RPCCall.cpp index eb4906f3a..b475afe9d 100644 --- a/src/ripple/net/impl/RPCCall.cpp +++ b/src/ripple/net/impl/RPCCall.cpp @@ -1396,16 +1396,7 @@ struct RPCCallImp // callbackFuncP. // Receive reply - if (iStatus == 401) - Throw( - "incorrect rpcuser or rpcpassword (authorization failed)"); - else if ( - (iStatus >= 400) && (iStatus != 400) && (iStatus != 404) && - (iStatus != 500)) // ? - Throw( - std::string("server returned HTTP error ") + - std::to_string(iStatus)); - else if (strData.empty()) + if (strData.empty()) Throw("no response from server"); // Parse reply diff --git a/src/ripple/protocol/ErrorCodes.h b/src/ripple/protocol/ErrorCodes.h index 98a8cf43a..ee33eee06 100644 --- a/src/ripple/protocol/ErrorCodes.h +++ b/src/ripple/protocol/ErrorCodes.h @@ -163,12 +163,15 @@ enum warning_code_i { namespace RPC { -/** Maps an rpc error code to its token and default message. */ +/** Maps an rpc error code to its token, default message, and HTTP status. */ struct ErrorInfo { // Default ctor needed to produce an empty std::array during constexpr eval. constexpr ErrorInfo() - : code(rpcUNKNOWN), token("unknown"), message("An unknown error code.") + : code(rpcUNKNOWN) + , token("unknown") + , message("An unknown error code.") + , http_status(200) { } @@ -176,13 +179,26 @@ struct ErrorInfo error_code_i code_, char const* token_, char const* message_) - : code(code_), token(token_), message(message_) + : code(code_), token(token_), message(message_), http_status(200) + { + } + + constexpr ErrorInfo( + error_code_i code_, + char const* token_, + char const* message_, + int http_status_) + : code(code_) + , token(token_) + , message(message_) + , http_status(http_status_) { } error_code_i code; Json::StaticString token; Json::StaticString message; + int http_status; }; /** Returns an ErrorInfo that reflects the error code. */ @@ -332,6 +348,10 @@ not_validator_error() bool contains_error(Json::Value const& json); +/** Returns http status that corresponds to the error code. */ +int +error_code_http_status(error_code_i code); + } // namespace RPC /** Returns a single string with the contents of an RPC error. */ diff --git a/src/ripple/protocol/impl/ErrorCodes.cpp b/src/ripple/protocol/impl/ErrorCodes.cpp index e4a9acf46..bb3b2d47a 100644 --- a/src/ripple/protocol/impl/ErrorCodes.cpp +++ b/src/ripple/protocol/impl/ErrorCodes.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include #include #include @@ -26,105 +27,96 @@ namespace RPC { namespace detail { -// clang-format off // Unordered array of ErrorInfos, so we don't have to maintain the list // ordering by hand. // // This array will be omitted from the object file; only the sorted version // will remain in the object file. But the string literals will remain. -constexpr static ErrorInfo unorderedErrorInfos[]{ - {rpcACT_MALFORMED, "actMalformed", "Account malformed."}, - {rpcACT_NOT_FOUND, "actNotFound", "Account not found."}, - {rpcALREADY_MULTISIG, "alreadyMultisig", "Already multisigned."}, - {rpcALREADY_SINGLE_SIG, "alreadySingleSig", "Already single-signed."}, - {rpcAMENDMENT_BLOCKED, "amendmentBlocked", "Amendment blocked, need upgrade."}, - {rpcEXPIRED_VALIDATOR_LIST, "unlBlocked", "Validator list expired."}, - {rpcATX_DEPRECATED, "deprecated", "Use the new API or specify a ledger range."}, - {rpcBAD_KEY_TYPE, "badKeyType", "Bad key type."}, - {rpcBAD_FEATURE, "badFeature", "Feature unknown or invalid."}, - {rpcBAD_ISSUER, "badIssuer", "Issuer account malformed."}, - {rpcBAD_MARKET, "badMarket", "No such market."}, - {rpcBAD_SECRET, "badSecret", "Secret does not match account."}, - {rpcBAD_SEED, "badSeed", "Disallowed seed."}, - {rpcBAD_SYNTAX, "badSyntax", "Syntax error."}, - {rpcCHANNEL_MALFORMED, "channelMalformed", "Payment channel is malformed."}, - {rpcCHANNEL_AMT_MALFORMED, "channelAmtMalformed", "Payment channel amount is malformed."}, - {rpcCOMMAND_MISSING, "commandMissing", "Missing command entry."}, - {rpcDB_DESERIALIZATION, "dbDeserialization", "Database deserialization error."}, - {rpcDST_ACT_MALFORMED, "dstActMalformed", "Destination account is malformed."}, - {rpcDST_ACT_MISSING, "dstActMissing", "Destination account not provided."}, - {rpcDST_ACT_NOT_FOUND, "dstActNotFound", "Destination account not found."}, - {rpcDST_AMT_MALFORMED, "dstAmtMalformed", "Destination amount/currency/issuer is malformed."}, - {rpcDST_AMT_MISSING, "dstAmtMissing", "Destination amount/currency/issuer is missing."}, - {rpcDST_ISR_MALFORMED, "dstIsrMalformed", "Destination issuer is malformed."}, - {rpcEXCESSIVE_LGR_RANGE, "excessiveLgrRange", "Ledger range exceeds 1000."}, - {rpcFORBIDDEN, "forbidden", "Bad credentials."}, - {rpcFAILED_TO_FORWARD, "failedToForward", "Failed to forward request to p2p node"}, - {rpcHIGH_FEE, "highFee", "Current transaction fee exceeds your limit."}, - {rpcINTERNAL, "internal", "Internal error."}, - {rpcINVALID_LGR_RANGE, "invalidLgrRange", "Ledger range is invalid."}, - {rpcINVALID_PARAMS, "invalidParams", "Invalid parameters."}, - {rpcJSON_RPC, "json_rpc", "JSON-RPC transport error."}, - {rpcLGR_IDXS_INVALID, "lgrIdxsInvalid", "Ledger indexes invalid."}, - {rpcLGR_IDX_MALFORMED, "lgrIdxMalformed", "Ledger index malformed."}, - {rpcLGR_NOT_FOUND, "lgrNotFound", "Ledger not found."}, - {rpcLGR_NOT_VALIDATED, "lgrNotValidated", "Ledger not validated."}, - {rpcMASTER_DISABLED, "masterDisabled", "Master key is disabled."}, - {rpcNOT_ENABLED, "notEnabled", "Not enabled in configuration."}, - {rpcNOT_IMPL, "notImpl", "Not implemented."}, - {rpcNOT_READY, "notReady", "Not ready to handle this request."}, - {rpcNOT_SUPPORTED, "notSupported", "Operation not supported."}, - {rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable."}, - {rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable."}, - {rpcNOT_SYNCED, "notSynced", "Not synced to the network."}, - {rpcNO_EVENTS, "noEvents", "Current transport does not support events."}, - {rpcNO_NETWORK, "noNetwork", "Not synced to the network."}, - {rpcNO_PERMISSION, "noPermission", "You don't have permission for this command."}, - {rpcNO_PF_REQUEST, "noPathRequest", "No pathfinding request in progress."}, - {rpcPUBLIC_MALFORMED, "publicMalformed", "Public key is malformed."}, - {rpcREPORTING_UNSUPPORTED, "reportingUnsupported", "Requested operation not supported by reporting mode server"}, - {rpcSIGNING_MALFORMED, "signingMalformed", "Signing of transaction is malformed."}, - {rpcSLOW_DOWN, "slowDown", "You are placing too much load on the server."}, - {rpcSRC_ACT_MALFORMED, "srcActMalformed", "Source account is malformed."}, - {rpcSRC_ACT_MISSING, "srcActMissing", "Source account not provided."}, - {rpcSRC_ACT_NOT_FOUND, "srcActNotFound", "Source account not found."}, - {rpcSRC_CUR_MALFORMED, "srcCurMalformed", "Source currency is malformed."}, - {rpcSRC_ISR_MALFORMED, "srcIsrMalformed", "Source issuer is malformed."}, - {rpcSTREAM_MALFORMED, "malformedStream", "Stream malformed."}, - {rpcTOO_BUSY, "tooBusy", "The server is too busy to help you now."}, - {rpcTXN_NOT_FOUND, "txnNotFound", "Transaction not found."}, - {rpcUNKNOWN_COMMAND, "unknownCmd", "Unknown method."}, - {rpcSENDMAX_MALFORMED, "sendMaxMalformed", "SendMax amount malformed."}, - {rpcOBJECT_NOT_FOUND, "objectNotFound", "The requested object was not found."}}; -// clang-format on - -// C++ does not allow you to return an array from a function. You must -// return an object which may in turn contain an array. The following -// struct is simply defined so the enclosed array can be returned from a -// constexpr function. // -// In C++17 this struct can be replaced by a std::array. But in C++14 -// the constexpr methods of a std::array are not sufficient to perform the -// necessary work at compile time. -template -struct ErrorInfoArray -{ - // Visual Studio doesn't treat a templated aggregate as an aggregate. - // So, for Visual Studio, we define a constexpr default constructor. - constexpr ErrorInfoArray() : infos{} - { - } +// There's a certain amount of tension in determining the correct HTTP +// status to associate with a given RPC error. Initially all RPC errors +// returned 200 (OK). And that's the default behavior if no HTTP status code +// is specified below. +// +// The codes currently selected target the load balancer fail-over use case. +// If a query fails on one node but is likely to have a positive outcome +// on a different node, then the failure should return a 4xx/5xx range +// status code. - ErrorInfo infos[N]; -}; +// clang-format off +constexpr static ErrorInfo unorderedErrorInfos[]{ + {rpcACT_MALFORMED, "actMalformed", "Account malformed."}, + {rpcACT_NOT_FOUND, "actNotFound", "Account not found."}, + {rpcALREADY_MULTISIG, "alreadyMultisig", "Already multisigned."}, + {rpcALREADY_SINGLE_SIG, "alreadySingleSig", "Already single-signed."}, + {rpcAMENDMENT_BLOCKED, "amendmentBlocked", "Amendment blocked, need upgrade.", 503}, + {rpcEXPIRED_VALIDATOR_LIST, "unlBlocked", "Validator list expired.", 503}, + {rpcATX_DEPRECATED, "deprecated", "Use the new API or specify a ledger range.", 400}, + {rpcBAD_KEY_TYPE, "badKeyType", "Bad key type.", 400}, + {rpcBAD_FEATURE, "badFeature", "Feature unknown or invalid.", 500}, + {rpcBAD_ISSUER, "badIssuer", "Issuer account malformed.", 400}, + {rpcBAD_MARKET, "badMarket", "No such market.", 404}, + {rpcBAD_SECRET, "badSecret", "Secret does not match account.", 403}, + {rpcBAD_SEED, "badSeed", "Disallowed seed.", 403}, + {rpcBAD_SYNTAX, "badSyntax", "Syntax error.", 400}, + {rpcCHANNEL_MALFORMED, "channelMalformed", "Payment channel is malformed.", 400}, + {rpcCHANNEL_AMT_MALFORMED, "channelAmtMalformed", "Payment channel amount is malformed.", 400}, + {rpcCOMMAND_MISSING, "commandMissing", "Missing command entry.", 400}, + {rpcDB_DESERIALIZATION, "dbDeserialization", "Database deserialization error.", 502}, + {rpcDST_ACT_MALFORMED, "dstActMalformed", "Destination account is malformed.", 400}, + {rpcDST_ACT_MISSING, "dstActMissing", "Destination account not provided.", 400}, + {rpcDST_ACT_NOT_FOUND, "dstActNotFound", "Destination account not found.", 404}, + {rpcDST_AMT_MALFORMED, "dstAmtMalformed", "Destination amount/currency/issuer is malformed.", 400}, + {rpcDST_AMT_MISSING, "dstAmtMissing", "Destination amount/currency/issuer is missing.", 400}, + {rpcDST_ISR_MALFORMED, "dstIsrMalformed", "Destination issuer is malformed.", 400}, + {rpcEXCESSIVE_LGR_RANGE, "excessiveLgrRange", "Ledger range exceeds 1000.", 400}, + {rpcFORBIDDEN, "forbidden", "Bad credentials.", 403}, + {rpcFAILED_TO_FORWARD, "failedToForward", "Failed to forward request to p2p node", 503}, + {rpcHIGH_FEE, "highFee", "Current transaction fee exceeds your limit.", 402}, + {rpcINTERNAL, "internal", "Internal error.", 500}, + {rpcINVALID_LGR_RANGE, "invalidLgrRange", "Ledger range is invalid.", 400}, + {rpcINVALID_PARAMS, "invalidParams", "Invalid parameters.", 400}, + {rpcJSON_RPC, "json_rpc", "JSON-RPC transport error.", 500}, + {rpcLGR_IDXS_INVALID, "lgrIdxsInvalid", "Ledger indexes invalid.", 400}, + {rpcLGR_IDX_MALFORMED, "lgrIdxMalformed", "Ledger index malformed.", 400}, + {rpcLGR_NOT_FOUND, "lgrNotFound", "Ledger not found.", 404}, + {rpcLGR_NOT_VALIDATED, "lgrNotValidated", "Ledger not validated.", 202}, + {rpcMASTER_DISABLED, "masterDisabled", "Master key is disabled.", 403}, + {rpcNOT_ENABLED, "notEnabled", "Not enabled in configuration.", 501}, + {rpcNOT_IMPL, "notImpl", "Not implemented.", 501}, + {rpcNOT_READY, "notReady", "Not ready to handle this request.", 503}, + {rpcNOT_SUPPORTED, "notSupported", "Operation not supported.", 501}, + {rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable.", 503}, + {rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable.", 503}, + {rpcNOT_SYNCED, "notSynced", "Not synced to the network.", 503}, + {rpcNO_EVENTS, "noEvents", "Current transport does not support events.", 405}, + {rpcNO_NETWORK, "noNetwork", "Not synced to the network.", 503}, + {rpcNO_PERMISSION, "noPermission", "You don't have permission for this command.", 401}, + {rpcNO_PF_REQUEST, "noPathRequest", "No pathfinding request in progress.", 404}, + {rpcOBJECT_NOT_FOUND, "objectNotFound", "The requested object was not found.", 404}, + {rpcPUBLIC_MALFORMED, "publicMalformed", "Public key is malformed.", 400}, + {rpcREPORTING_UNSUPPORTED, "reportingUnsupported", "Requested operation not supported by reporting mode server", 405}, + {rpcSENDMAX_MALFORMED, "sendMaxMalformed", "SendMax amount malformed.", 400}, + {rpcSIGNING_MALFORMED, "signingMalformed", "Signing of transaction is malformed.", 400}, + {rpcSLOW_DOWN, "slowDown", "You are placing too much load on the server.", 429}, + {rpcSRC_ACT_MALFORMED, "srcActMalformed", "Source account is malformed.", 400}, + {rpcSRC_ACT_MISSING, "srcActMissing", "Source account not provided.", 400}, + {rpcSRC_ACT_NOT_FOUND, "srcActNotFound", "Source account not found.", 404}, + {rpcSRC_CUR_MALFORMED, "srcCurMalformed", "Source currency is malformed.", 400}, + {rpcSRC_ISR_MALFORMED, "srcIsrMalformed", "Source issuer is malformed.", 400}, + {rpcSTREAM_MALFORMED, "malformedStream", "Stream malformed.", 400}, + {rpcTOO_BUSY, "tooBusy", "The server is too busy to help you now.", 503}, + {rpcTXN_NOT_FOUND, "txnNotFound", "Transaction not found.", 404}, + {rpcUNKNOWN_COMMAND, "unknownCmd", "Unknown method.", 405}}; +// clang-format on // Sort and validate unorderedErrorInfos at compile time. Should be // converted to consteval when get to C++20. template constexpr auto -sortErrorInfos(ErrorInfo const (&unordered)[N]) -> ErrorInfoArray +sortErrorInfos(ErrorInfo const (&unordered)[N]) -> std::array { - ErrorInfoArray ret; + std::array ret = {}; for (ErrorInfo const& info : unordered) { @@ -135,12 +127,10 @@ sortErrorInfos(ErrorInfo const (&unordered)[N]) -> ErrorInfoArray static_assert(rpcSUCCESS == 0, "Unexpected error_code_i layout."); int const index{info.code - 1}; - if (ret.infos[index].code != rpcUNKNOWN) + if (ret[index].code != rpcUNKNOWN) throw(std::invalid_argument("Duplicate error_code_i in list")); - ret.infos[index].code = info.code; - ret.infos[index].token = info.token; - ret.infos[index].message = info.message; + ret[index] = info; } // Verify that all entries are filled in starting with 1 and proceeding @@ -150,7 +140,7 @@ sortErrorInfos(ErrorInfo const (&unordered)[N]) -> ErrorInfoArray // rpcUNKNOWN. But other than that all entries should match their index. int codeCount{0}; int expect{rpcBAD_SYNTAX - 1}; - for (ErrorInfo const& info : ret.infos) + for (ErrorInfo const& info : ret) { ++expect; if (info.code == rpcUNKNOWN) @@ -181,7 +171,7 @@ get_error_info(error_code_i code) { if (code <= rpcSUCCESS || code > rpcLAST) return detail::unknownError; - return detail::sortedErrorInfos.infos[code - 1]; + return detail::sortedErrorInfos[code - 1]; } Json::Value @@ -208,6 +198,12 @@ contains_error(Json::Value const& json) return false; } +int +error_code_http_status(error_code_i code) +{ + return get_error_info(code).http_status; +} + } // namespace RPC std::string diff --git a/src/ripple/rpc/impl/ServerHandlerImp.cpp b/src/ripple/rpc/impl/ServerHandlerImp.cpp index cb70fdcab..f269283b8 100644 --- a/src/ripple/rpc/impl/ServerHandlerImp.cpp +++ b/src/ripple/rpc/impl/ServerHandlerImp.cpp @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -970,6 +971,29 @@ ServerHandlerImp::processRequest( } } } + + // If we're returning an error_code, use that to determine the HTTP status. + int const httpStatus = [&reply]() { + // This feature is enabled with ripplerpc version 3.0 and above. + // Before ripplerpc version 3.0 always return 200. + if (reply.isMember(jss::ripplerpc) && + reply[jss::ripplerpc].isString() && + reply[jss::ripplerpc].asString() >= "3.0") + { + // If there's an error_code, use that to determine the HTTP Status. + if (reply.isMember(jss::error) && + reply[jss::error].isMember(jss::error_code) && + reply[jss::error][jss::error_code].isInt()) + { + int const errCode = reply[jss::error][jss::error_code].asInt(); + return RPC::error_code_http_status( + static_cast(errCode)); + } + } + // Return OK. + return 200; + }(); + auto response = to_string(reply); rpc_time_.notify(std::chrono::duration_cast( @@ -988,7 +1012,7 @@ ServerHandlerImp::processRequest( stream << "Reply: " << response.substr(0, maxSize); } - HTTPReply(200, response, output, rpcJ); + HTTPReply(httpStatus, response, output, rpcJ); } //------------------------------------------------------------------------------ diff --git a/src/ripple/server/impl/JSONRPCUtil.cpp b/src/ripple/server/impl/JSONRPCUtil.cpp index f5bb815a9..12d12829c 100644 --- a/src/ripple/server/impl/JSONRPCUtil.cpp +++ b/src/ripple/server/impl/JSONRPCUtil.cpp @@ -61,7 +61,7 @@ HTTPReply( { JLOG(j.trace()) << "HTTP Reply " << nStatus << " " << content; - if (nStatus == 401) + if (content.empty() && nStatus == 401) { output("HTTP/1.0 401 Authorization Required\r\n"); output(getHTTPHeaderTimestamp()); @@ -100,18 +100,33 @@ HTTPReply( case 200: output("HTTP/1.1 200 OK\r\n"); break; + case 202: + output("HTTP/1.1 202 Accepted\r\n"); + break; case 400: output("HTTP/1.1 400 Bad Request\r\n"); break; + case 401: + output("HTTP/1.1 401 Authorization Required\r\n"); + break; case 403: output("HTTP/1.1 403 Forbidden\r\n"); break; case 404: output("HTTP/1.1 404 Not Found\r\n"); break; + case 405: + output("HTTP/1.1 405 Method Not Allowed\r\n"); + break; + case 429: + output("HTTP/1.1 429 Too Many Requests\r\n"); + break; case 500: output("HTTP/1.1 500 Internal Server Error\r\n"); break; + case 501: + output("HTTP/1.1 501 Not Implemented\r\n"); + break; case 503: output("HTTP/1.1 503 Server is overloaded\r\n"); break; diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 2580c4bfe..5494a81da 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -1675,7 +1675,7 @@ class LedgerRPC_test : public beast::unit_test::suite void testLedgerAccountsOption() { - testcase("Ledger Request, Accounts Option"); + testcase("Ledger Request, Accounts Hashes"); using namespace test::jtx; Env env{*this}; From b19287c6d0e17f5b38ceb78405d0d121904d311b Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Wed, 4 Jan 2023 15:45:19 -0800 Subject: [PATCH 3/3] Add a unit test for invalid memos (#4287) --- Builds/CMake/RippledCore.cmake | 1 + src/test/protocol/Memo_test.cpp | 123 ++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 src/test/protocol/Memo_test.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 82a57995a..b3366aaaa 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -909,6 +909,7 @@ if (tests) src/test/protocol/InnerObjectFormats_test.cpp src/test/protocol/Issue_test.cpp src/test/protocol/Hooks_test.cpp + src/test/protocol/Memo_test.cpp src/test/protocol/PublicKey_test.cpp src/test/protocol/Quality_test.cpp src/test/protocol/STAccount_test.cpp diff --git a/src/test/protocol/Memo_test.cpp b/src/test/protocol/Memo_test.cpp new file mode 100644 index 000000000..b39482e42 --- /dev/null +++ b/src/test/protocol/Memo_test.cpp @@ -0,0 +1,123 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2022 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include +#include +#include + +namespace ripple { + +class Memo_test : public beast::unit_test::suite +{ +public: + void + testMemos() + { + testcase("Test memos"); + + using namespace test::jtx; + Account alice{"alice"}; + + Env env(*this); + env.fund(XRP(10000), alice); + env.close(); + + // Lambda that returns a valid JTx with a memo that we can hack up. + // This is the basis for building tests of invalid states. + auto makeJtxWithMemo = [&env, &alice]() { + JTx example = noop(alice); + memo const exampleMemo{"tic", "tac", "toe"}; + exampleMemo(env, example); + return example; + }; + + // A valid memo. + env(makeJtxWithMemo()); + env.close(); + + { + // Make sure that too big a memo is flagged as invalid. + JTx memoSize = makeJtxWithMemo(); + memoSize.jv[sfMemos.jsonName][0u][sfMemo.jsonName] + [sfMemoData.jsonName] = std::string(2020, '0'); + env(memoSize, ter(temINVALID)); + + // This memo is just barely small enough. + memoSize.jv[sfMemos.jsonName][0u][sfMemo.jsonName] + [sfMemoData.jsonName] = std::string(2018, '1'); + env(memoSize); + } + { + // Put a non-Memo in the Memos array. + JTx memoNonMemo = noop(alice); + auto& jv = memoNonMemo.jv; + auto& ma = jv[sfMemos.jsonName]; + auto& mi = ma[ma.size()]; + auto& m = mi[sfCreatedNode.jsonName]; // CreatedNode in Memos + m[sfMemoData.jsonName] = "3030303030"; + + env(memoNonMemo, ter(temINVALID)); + } + { + // Put an invalid field in a Memo object. + JTx memoExtra = makeJtxWithMemo(); + memoExtra + .jv[sfMemos.jsonName][0u][sfMemo.jsonName][sfFlags.jsonName] = + 13; + env(memoExtra, ter(temINVALID)); + } + { + // Put a character that is not allowed in a URL in a MemoType field. + JTx memoBadChar = makeJtxWithMemo(); + memoBadChar.jv[sfMemos.jsonName][0u][sfMemo.jsonName] + [sfMemoType.jsonName] = + strHex(std::string_view("ONE