Support for the Deposit Authorization account root flag #2239

scottschurr · 2017-10-03T15:01:36Z

Deposit Authorization allows an account to specify that it will not accept any deposit unless it signs the transaction. That means an account with with the DepositAuthorization flag set will not (usually) accept Payment transactions. It will accept Escrow and PaymentChannel deposits, but only if it signs the transaction that causes the transfer.

A legitimate concern was raised that an account with this flag set could put itself in an inoperable condition if it used all of its XRP. Without XRP it could not pay the necessary fee to sign a transaction to replenish its XRP.

Three approaches have been discussed to address this concern:

Have twice the fee to set the lsfDepositAuth flag on an account root. Then have no fee to clear the flag on the account root. Clearing the lsfDepositAuth flag for free would allow the account to accept more XRP.
If the account XRP balance falls below a particular level, then the lsfDepositAuth protections are removed, but only for small payments of XRP. The account's XRP balance must fall below the minimum reserve before an XRP Payment transaction will be accepted. Within that window, the largest XRP Payment that may be sent is equal to the XRP minimum reserve.
One other approach that has been suggested is that, as standard procedures for checks, creating a check takes twice the standard fee and cashing a check requires no fee. This would allow a check to replenish the XRP on an account, once checks are implemented and live on the network.

After discussions and deliberations, I've chosen approach 2: allow small XRP payments if the account balance gets low. Here are the reasons for the choice:

It puts the burden of the odd behavior only on those people who use the feature.
If desired, the rule should be relatively easy to change in the future, since the number of affected users should be small.
The other two approaches leave the network potentially exposed to zero-cost ledger spam if one of the protections is missed. Any individual transaction that can be transmitted with zero fee is a risk that must be watched very closely. Minimizing the number of these transactions helps make the network more secure.

Both Payment Channels and Escrow are affected, since they are (currently) the only methods that always allow funds to be deposited into an account with the lsfDepositAuth flag set.

We hope to get this pull request integrated into the 0.90.0 release chain early on for testing on the AltNet.

Reviewers: @nbougalis for Escrow experience, @seelabs for Payment Channels experience.

codecov-io · 2017-10-03T16:31:18Z

Codecov Report

Merging #2239 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2239   +/-   ##
========================================
  Coverage    70.26%   70.26%           
========================================
  Files          706      706           
  Lines        61410    61410           
========================================
  Hits         43147    43147           
  Misses       18263    18263

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc9e9f4...be45af3. Read the comment docs.

MarkusTeufelberger · 2017-10-04T10:16:13Z

Is having exactly the minimum reserve of XRP enough to get an account operable again (accounts can and do reach exactly 0 XRP)? Maybe a <= min reserve instead of < min reserve might be needed, but then again it might be possible to already operate owning exactly the min reserve.

scottschurr · 2017-10-04T15:31:46Z

@MarkusTeufelberger, Yes, having the minimum reserve is (usually) a bit more than necessary to make an account useful. The protocol allows people to chew into their reserve to pay fees. So once they have the minimum reserve (or less) they can pay fees for any transaction they care to. However they can't make an XRP payment that would take them below the minimum reserve.

The only situation where having the minimum reserve would not be sufficient is if the network fee rose above the minimum reserve. That's worth thinking about for an account that may be exclusively multisigned with 8 signers. They would have to always pay a fee of at least 9 times the current network fee. But, even for the multisigned case, I don't think a network fee that high is likely to become a steady-state condition.

Thanks for asking. Good question!

scottschurr · 2017-11-01T20:44:22Z

@nbougalis and @seelabs, I know you folks are frantic busy. Would you like me to pick a couple of other reviewers for this pull request? Thanks.

seelabs · 2017-11-01T21:01:43Z

@scottschurr Apologies. I'll plan on reviewing this early next week after my higher-priority tasks are finished.

seelabs

Left some minor feedback, but looks good.

seelabs · 2017-11-09T19:05:12Z

src/ripple/app/tx/impl/PayChan.cpp

+        // Check whether the destination account requires deposit authorization
+        if (txAccount != dst)
+        {
+            // If dst set lsfDepositAuth and signer is not dst, then fail.


Nit: I think we're better off leaving comments such as this out out of the code base. It's a comment that explains the one line of code below it, and that one line is already straight forward. I don't care about this particular comment, but it's something I noticed in a couple places in this PR.

Noted. You and I have a different threshold for when a comment is appropriate, mine being much lower than most software engineers. I've seldom met a comment I didn't like 😉. I doubt you'll train me out of writing non-essential comments at this point in my career. Speaking only for myself, they help me think about code I'm writing and reason about code I'm reading. But I'm happy to discuss removing any comments that you feel distract from the code.

seelabs · 2017-11-09T19:10:16Z

src/ripple/app/tx/impl/SetAccount.cpp


+    bool const sigWithMaster {[tx, acct = account_] ()


You are copying tx and acct here, I think you want refs.

Note: for an immediately invoked function expression, such as this one, I prefer capture all by ref [&]. What the lambda actually captures isn't important in such a context.

Good catch on the refs. Thanks.

I hear your preference on lambda captures. My personal preference is still to follow Scott Meyers' advice, "Item 31: Avoid default capture modes." I find his reasoning compelling, even for local lambdas.

seelabs · 2017-11-09T19:13:28Z

src/ripple/app/tx/impl/Payment.cpp

+    TER terResult {tesSUCCESS};
+
+    // Determine whether the destination requires deposit authorization.
+    bool const reqDepositAuth = [] (ReadView& view, std::uint32_t dstFlags)


The initialization is simple enough that it doesn't need the lambda:

bool const reqDepositAuth = sleDst->getFlags() & lsfDepositAuth && view().rules().enabled(featureDepositAuth);

seelabs · 2017-11-09T19:19:01Z

src/ripple/app/tx/impl/Payment.cpp

+        //  1. payment is not direct XRP to XRP and
+        //  2. the destination has lsfDepositAuth set,
+        // then the payment is not authorized.
+        terResult = tecNO_PERMISSION;


Instead of setting terResult would could return here (and in other places in this function). I think doing to simplifies the code. I took a cut at a refactor here: seelabs@d6efff9

Done. I incorporated your refactor as a [FOLD] commit.

scottschurr

Thanks for the review @seelabs, especially for spotting the lambda capture-by-value mistake. I think I've addressed most of your comments.

scottschurr · 2017-11-14T19:51:33Z

src/ripple/app/tx/impl/PayChan.cpp

+        // Check whether the destination account requires deposit authorization
+        if (txAccount != dst)
+        {
+            // If dst set lsfDepositAuth and signer is not dst, then fail.


Noted. You and I have a different threshold for when a comment is appropriate, mine being much lower than most software engineers. I've seldom met a comment I didn't like 😉. I doubt you'll train me out of writing non-essential comments at this point in my career. Speaking only for myself, they help me think about code I'm writing and reason about code I'm reading. But I'm happy to discuss removing any comments that you feel distract from the code.

scottschurr · 2017-11-14T20:58:21Z

src/ripple/app/tx/impl/Payment.cpp

+        //  1. payment is not direct XRP to XRP and
+        //  2. the destination has lsfDepositAuth set,
+        // then the payment is not authorized.
+        terResult = tecNO_PERMISSION;


Done. I incorporated your refactor as a [FOLD] commit.

scottschurr · 2017-11-14T22:41:25Z

src/ripple/app/tx/impl/SetAccount.cpp


+    bool const sigWithMaster {[tx, acct = account_] ()


Good catch on the refs. Thanks.

I hear your preference on lambda captures. My personal preference is still to follow Scott Meyers' advice, "Item 31: Avoid default capture modes." I find his reasoning compelling, even for local lambdas.

scottschurr · 2017-12-01T00:34:27Z

Rebased to 0.80.1.

scottschurr · 2017-12-02T02:51:28Z

Rebased to 0.90.0-b1.

ripplelabs-jenkins · 2017-12-02T03:23:46Z

Jenkins Build Summary

Built from this commit

Built at 20171222 - 00:06:54

Test Results

Build Type	Result	Status
clang.debug.unity	975 cases, 0 failed, t: 387s	PASS ✅
coverage	975 cases, 0 failed, t: 612s	PASS ✅
gcc.debug.unity	975 cases, 0 failed, t: 435s	PASS ✅
clang.debug.nounity	973 cases, 0 failed, t: 709s	PASS ✅
clang.release.unity	974 cases, 0 failed, t: 472s	PASS ✅
gcc.debug.nounity	973 cases, 0 failed, t: 653s	PASS ✅
gcc.release.unity	974 cases, 0 failed, t: 504s	PASS ✅

seelabs

re-reviewed Refactor Payment::doApply only (I believe this is the only change since I last reviewed).

👍

nbougalis

There's a transaction-breaking change here we need to fix.

nbougalis · 2017-12-09T07:24:26Z

src/ripple/app/tx/impl/Payment.cpp


    bool const bRipple = paths || sendMax || !saDstAmount.native ();
    // XXX Should sendMax be sufficient to imply ripple?

+    if (bRipple && reqDepositAuth)
+        // If the destination has lsfDepositAuth set, then only direct


I prefer such comments above the block instead of interjected between the if and the single-line statement within it, especially in the absence of surrounding braces. No change needed, really. Just passing commentary.

nbougalis · 2017-12-09T07:50:11Z

src/ripple/app/tx/impl/SetAccount.cpp

+    if (uSetFlag == asfDepositAuth || uClearFlag == asfDepositAuth)
+    {
+        if (!ctx.rules.enabled(featureDepositAuth))
+            return temDISABLED;


Versions of rippled without this code (i.e. any version before this patch is merged) with return tesSUCCESS when trying to process an operation that specifies asfDepositAuth (or indeed any unknown value) for either SetFlag or ClearFlag. So this is, ironically, a transaction-breaking change.

I propose that we either remove this block entirely from preflight and fall through into doApply which is what happens today (essentially treating this operation a nop if no other changes are made), or that we simply add a distinct fix amendment to add a block similar to this here:

if (ctx.rules.enabled(fixWhatever)) { if (uSetFlag >= asfMaxValue || uClearFlags >= asfMaxValue) return temINVALID_FLAG; }

We could then define asfMaxValue accordingly, and note that it needs to be maintained whenever new values are added.

Excellent catch. I've removed this code and extended the AccountSet_test.cpp to make it apparent whether the DepositAuth flag handling is similar to the other flags before and after the amendment goes live. Good eyes. 👀.

nbougalis · 2017-12-09T08:18:57Z

src/ripple/app/tx/impl/Escrow.cpp

+    if (ctx_.view().rules().enabled(featureDepositAuth))
+    {
+        // NOTE: Escrow payments cannot be used to fund accounts
+        if (! sled)


I don't think that it is possible for this situation to ever trigger. See: https://github.com/ripple/rippled/blob/3e5490ef6db9cbc3abdff04655d80952b288d097/src/ripple/app/tx/impl/Escrow.cpp#L229-L234.

I understand that this check was already present further down, and you just cloned it here for completeness, and maybe it's just better to actually move it in a single place, immediately after the lookup.

While this may seem like a transaction breaking change, I propose that it can't be. My rationale is this: we check in EscrowCreate whether the destination exists and refuse to create the escrow if that is the case. Since accounts cannot be deleted, then it is not possible for an escrow to exist without the destination account existing.

We could also add the following invariant check, which would catch this if it ever were to happen. (warning: I just typed this out here; it's hasn't been compile-tested):

/** * @brief Invariant: an escrow must not create a new account root. */ class NoFundingEscrow { bool fundedAccount_ = false; public: void visitEntry( uint256 const&, bool, std::shared_ptr<SLE const> const&, std::shared_ptr<SLE const> const&); bool finalize(STTx const&, TER, beast::Journal const&); }; void NoFundingEscrow::visitEntry( uint256 const&, bool isDelete, std::shared_ptr <SLE const> const& before, std::shared_ptr <SLE const> const& after) { // If an account root didn't exist before and exists after the transaction // then the transaction funded an account: if (!before && after && after->getType() == ltACCOUNT_ROOT) fundedAccount_ = true; } bool NoFundingEscrow::finalize(STTx const&tx, TER, beast::Journal const& j) { if (!fundedAccount_) return true; auto const type = tx.getTxnType(); if (type == ttESCROW_CREATE || type == ttESCROW_FINISH || type == ttESCROW_CANCEL) { JLOG(j.fatal()) << "Invariant failed: an escrow operation funded an account"; return false; } return true; }

Coincidentally, this invariant checker could be augmented to ensure that if an account root is created by a transaction (regardless of type) then its initial balance is greater than or equal to the reserve, because we don't allow partially funding a new account.

Okay. I'm good with the rationale that the situation ought never to occur unless the ledger is corrupt. I still want to check for the impossible case -- I would feel bad about accessing through an unchecked shared_ptr. But I'll check in only one place, that that check ought to never fire.

scottschurr

@nbougalis, thanks for the keen eye. I believe I've addressed the issues you found with the commit named [FOLD] Address review comments.

scottschurr · 2017-12-15T23:02:55Z

src/ripple/app/tx/impl/Escrow.cpp

+    if (ctx_.view().rules().enabled(featureDepositAuth))
+    {
+        // NOTE: Escrow payments cannot be used to fund accounts
+        if (! sled)


Okay. I'm good with the rationale that the situation ought never to occur unless the ledger is corrupt. I still want to check for the impossible case -- I would feel bad about accessing through an unchecked shared_ptr. But I'll check in only one place, that that check ought to never fire.

scottschurr · 2017-12-15T23:20:54Z

src/ripple/app/tx/impl/SetAccount.cpp

+    if (uSetFlag == asfDepositAuth || uClearFlag == asfDepositAuth)
+    {
+        if (!ctx.rules.enabled(featureDepositAuth))
+            return temDISABLED;


Excellent catch. I've removed this code and extended the AccountSet_test.cpp to make it apparent whether the DepositAuth flag handling is similar to the other flags before and after the amendment goes live. Good eyes. 👀.

The DepositAuth feature allows an account to require that it signs for any funds that are deposited to the account. For the time being this limits the account to accepting only XRP, although there are plans to allow IOU payments in the future. The lsfDepositAuth protections are not extended to offers. If an account creates an offer it is in effect saying, “I will accept funds from anyone who takes this offer.” Therefore, the typical user of the lsfDepositAuth flag will choose never to create any offers. But they can if they so choose. The DepositAuth feature leaves a small gap in its protections. An XRP payment is allowed to a destination account with the lsfDepositAuth flag set if: - The Destination XRP balance is less than or equal to the base reserve and - The value of the XRP Payment is less than or equal to the base reserve. This exception is intended to make it impossible for an account to wedge itself by spending all of its XRP on fees and leave itself unable to pay the fee to get more XRP. This commit - adds featureDepositAuth, - adds the lsfDepositAuth flag, - adds support for lsfDepositAuth in SetAccount.cpp - adds support in Payment.cpp for rejecting payments that don't meet the lsfDepositAuth requirements, - adds unit tests for Payment transactions to an an account with lsfDepositAuth set. - adds Escrow and PayChan support for lsfDepositAuth along with as unit tests.

scottschurr · 2017-12-21T23:30:45Z

Squashed.

mDuo13 · 2017-12-28T20:17:02Z

I'd like to see DepositAuth also allow incoming multi-signed Payments where the destination's active master or regular key pair is a signer on the Payment tx. Maybe that can come in a later PR.

seelabs · 2018-01-16T17:29:56Z

In 0.90.0-b3

scottschurr assigned nbougalis and seelabs Oct 3, 2017

scottschurr requested review from nbougalis and seelabs October 3, 2017 15:01

seelabs reviewed Nov 9, 2017

View reviewed changes

scottschurr force-pushed the deposit-auth-low-balance branch from d72755d to c4e1244 Compare November 15, 2017 20:54

scottschurr commented Nov 15, 2017

View reviewed changes

scottschurr added Rebase Tx Change labels Nov 17, 2017

scottschurr force-pushed the deposit-auth-low-balance branch from c4e1244 to 4fa77c1 Compare December 1, 2017 00:33

scottschurr removed the Rebase label Dec 1, 2017

scottschurr force-pushed the deposit-auth-low-balance branch from 4fa77c1 to 988a494 Compare December 2, 2017 02:50

seelabs approved these changes Dec 7, 2017

View reviewed changes

nbougalis suggested changes Dec 9, 2017

View reviewed changes

scottschurr force-pushed the deposit-auth-low-balance branch from 988a494 to 1f742df Compare December 16, 2017 01:15

scottschurr commented Dec 16, 2017

View reviewed changes

scottschurr force-pushed the deposit-auth-low-balance branch from 1f742df to 733422c Compare December 20, 2017 00:14

nbougalis added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Dec 20, 2017

scottschurr added 2 commits December 21, 2017 14:53

PayChan and Escrow should ignore DisallowXRP (RIPD-1462)

be45af3

scottschurr force-pushed the deposit-auth-low-balance branch from 733422c to be45af3 Compare December 21, 2017 23:30

nbougalis mentioned this pull request Jan 10, 2018

Proposed 0.90.0-b3 #2328

Merged

seelabs closed this Jan 16, 2018

intelliot mentioned this pull request Feb 15, 2018

Support for the Deposit Authorization account root flag XRPLF/xrpl.js#852

Merged

scottschurr deleted the deposit-auth-low-balance branch February 17, 2018 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for the Deposit Authorization account root flag #2239

Support for the Deposit Authorization account root flag #2239

scottschurr commented Oct 3, 2017

codecov-io commented Oct 3, 2017 •

edited

Loading

MarkusTeufelberger commented Oct 4, 2017

scottschurr commented Oct 4, 2017

scottschurr commented Nov 1, 2017

seelabs commented Nov 1, 2017

seelabs left a comment

seelabs Nov 9, 2017

scottschurr Nov 14, 2017

seelabs Nov 9, 2017

scottschurr Nov 14, 2017

seelabs Nov 9, 2017

seelabs Nov 9, 2017

scottschurr Nov 14, 2017

scottschurr left a comment

scottschurr Nov 14, 2017

scottschurr Nov 14, 2017

scottschurr Nov 14, 2017

scottschurr commented Dec 1, 2017

scottschurr commented Dec 2, 2017

ripplelabs-jenkins commented Dec 2, 2017 •

edited

Loading

seelabs left a comment

nbougalis left a comment

nbougalis Dec 9, 2017

nbougalis Dec 9, 2017

scottschurr Dec 15, 2017

nbougalis Dec 9, 2017 •

edited

Loading

scottschurr Dec 15, 2017

scottschurr left a comment •

edited

Loading

scottschurr Dec 15, 2017

scottschurr Dec 15, 2017

scottschurr commented Dec 21, 2017

mDuo13 commented Dec 28, 2017

seelabs commented Jan 16, 2018

Support for the Deposit Authorization account root flag #2239

Support for the Deposit Authorization account root flag #2239

Conversation

scottschurr commented Oct 3, 2017

codecov-io commented Oct 3, 2017 • edited Loading

Codecov Report

MarkusTeufelberger commented Oct 4, 2017

scottschurr commented Oct 4, 2017

scottschurr commented Nov 1, 2017

seelabs commented Nov 1, 2017

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr commented Dec 1, 2017

scottschurr commented Dec 2, 2017

ripplelabs-jenkins commented Dec 2, 2017 • edited Loading

Jenkins Build Summary

Test Results

seelabs left a comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis Dec 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr commented Dec 21, 2017

mDuo13 commented Dec 28, 2017

seelabs commented Jan 16, 2018

codecov-io commented Oct 3, 2017 •

edited

Loading

ripplelabs-jenkins commented Dec 2, 2017 •

edited

Loading

nbougalis Dec 9, 2017 •

edited

Loading

scottschurr left a comment •

edited

Loading