Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

featureDisallowIncoming: Opt-out of incoming Checks, PayChans, NFTokenOffers and Trustlines #4336

Merged
merged 5 commits into from
Dec 20, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/ripple/app/tx/impl/CreateCheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,14 @@ CreateCheck::preclaim(PreclaimContext const& ctx)
return tecNO_DST;
}

if ((sleDst->getFlags() & lsfRequireDestTag) &&
!ctx.tx.isFieldPresent(sfDestinationTag))
uint32_t flags = sleDst->getFlags();
RichardAH marked this conversation as resolved.
Show resolved Hide resolved

// 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.
Expand Down
43 changes: 38 additions & 5 deletions src/ripple/app/tx/impl/NFTokenCreateOffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,44 @@ 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;
auto const destination = ctx.tx[~sfDestination];
if (destination)
RichardAH marked this conversation as resolved.
Show resolved Hide resolved
{
// If a destination is specified, the destination must already be in
// the ledger.
if (!ctx.view.exists(keylet::account(*destination)))
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

auto const sleDst = ctx.view.read(keylet::account(*destination));
RichardAH marked this conversation as resolved.
Show resolved Hide resolved

if (sleDst->getFlags() & lsfDisallowIncomingNFTOffer)
intelliot marked this conversation as resolved.
Show resolved Hide resolved
return tecNO_PERMISSION;
}
}

auto const owner = ctx.tx[~sfOwner];
if (owner)
{
// 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;
}
Expand Down
12 changes: 9 additions & 3 deletions src/ripple/app/tx/impl/PayChan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -217,14 +217,20 @@ 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])
uint32_t 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))
RichardAH marked this conversation as resolved.
Show resolved Hide resolved
return tecNO_TARGET;
}

Expand Down
34 changes: 34 additions & 0 deletions src/ripple/app/tx/impl/SetAccount.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,21 @@ SetAccount::preflight(PreflightContext const& ctx)
return temMALFORMED;
}

if (ctx.rules.enabled(featureDisallowIncoming))
{
if ((uSetFlag == asfDisallowIncomingNFTOffer &&
uClearFlag == asfDisallowIncomingNFTOffer) ||
(uSetFlag == asfDisallowIncomingCheck &&
uClearFlag == asfDisallowIncomingCheck) ||
(uSetFlag == asfDisallowIncomingPayChan &&
uClearFlag == asfDisallowIncomingPayChan))
RichardAH marked this conversation as resolved.
Show resolved Hide resolved
{
JLOG(j.trace())
<< "Malformed transaction: Contradictory flags set.";
return temINVALID_FLAG;
}
}

return preflight2(ctx);
}

Expand Down Expand Up @@ -538,6 +553,25 @@ 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 (uFlagsIn != uFlagsOut)
sle->setFieldU32(sfFlags, uFlagsOut);

Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/Feature.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
9 changes: 9 additions & 0 deletions src/ripple/protocol/LedgerFormats.h
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,15 @@ 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

// ltOFFER
lsfPassive = 0x00010000,
Expand Down
6 changes: 6 additions & 0 deletions src/ripple/protocol/TxFlags.h
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ 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;

// OfferCreate flags:
constexpr std::uint32_t tfPassive = 0x00010000;
Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/Feature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
98 changes: 98 additions & 0 deletions src/test/app/Check_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class dest_tag

class Check_test : public beast::unit_test::suite
{
FeatureBitset const disallowIncoming{featureDisallowIncoming};

static uint256
getCheckIndex(AccountID const& account, std::uint32_t uSequence)
{
Expand Down Expand Up @@ -293,6 +295,100 @@ class Check_test : public beast::unit_test::suite
BEAST_EXPECT(checksOnAccount(env, bob).size() == bobCount + 7);
}

void
testCreateDisallowIncoming(FeatureBitset features)
{
// Explore many of the valid ways to create a check.
RichardAH marked this conversation as resolved.
Show resolved Hide resolved
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);

// Note that no trust line has been set up for alice, but alice can
// still write a check for USD. You don't have to have the funds
// necessary to cover a check in order to write a check.
RichardAH marked this conversation as resolved.
Show resolved Hide resolved
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));
};
// from to

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);
}
RichardAH marked this conversation as resolved.
Show resolved Hide resolved

void
testCreateInvalid(FeatureBitset features)
{
Expand Down Expand Up @@ -2602,6 +2698,7 @@ class Check_test : public beast::unit_test::suite
{
testEnabled(features);
testCreateValid(features);
testCreateDisallowIncoming(features);
testCreateInvalid(features);
testCashXRP(features);
testCashIOU(features);
Expand All @@ -2621,6 +2718,7 @@ class Check_test : public beast::unit_test::suite
using namespace test::jtx;
auto const sa = supported_amendments();
testWithFeats(sa - featureCheckCashMakesTrustLine);
testWithFeats(sa - disallowIncoming);
RichardAH marked this conversation as resolved.
Show resolved Hide resolved
testWithFeats(sa);

testTrustLineCreation(sa); // Test with featureCheckCashMakesTrustLine
Expand Down
Loading