fixPayChanV1 #4717

dangell7 · 2023-09-20T08:22:18Z

This commit introduces a new fix amendment (fixPayChanV1) which prevents the creation of new PaymentChannelCreate transaction with a CancelAfter time less than the current ledger time. It piggy backs off of fix1571.

Once the amendment is activated, creating a new PaymentChannel will require that if you specify the CancelAfter time/value, that value must be greater than or equal to the current ledger time.

Currently users can create a payment channel where the CancelAfter time is before the current ledger time. This results in the payment channel being immediately closed on the next PaymentChannel transaction.

I did swap tecNO_PERMISSION for temBAD_EXPIRATION as I believe that is a more descriptive error message.

tadejgolobic · 2023-09-20T09:32:12Z

src/ripple/protocol/impl/Feature.cpp

@@ -456,6 +456,7 @@ REGISTER_FIX    (fixReducedOffersV1,            Supported::yes, VoteBehavior::De
 REGISTER_FEATURE(Clawback,                      Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FEATURE(AMM,                           Supported::yes, VoteBehavior::DefaultNo);
 REGISTER_FEATURE(XChainBridge,                  Supported::yes, VoteBehavior::DefaultNo);
+REGISTER_FIX    (fixPayChanV1,                  Supported::yes, VoteBehavior::DefaultNo);


Shouldn't this be Yes by default? Per my understanding, every fix amendment shout have vote by default set to yes.

REGISTER_FEATURE(fixPayChanV1, Supported::yes, VoteBehavior::DefaultYes);

Look above at all the fixes that don't have the VoteBehavior::DefaultYes.

Yes is reserved for things that need immediate approval. This doesn't need immediate approval imo.

I'm happy to change it though

I would advocate for keeping it as is, to avoid risk of amendment blocking, since this is not critical.

mvadari · 2023-09-20T14:44:39Z

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

+    {
+        auto const closeTime = ctx_.view().info().parentCloseTime;
+        if (ctx_.tx[~sfCancelAfter] && after(closeTime, ctx_.tx[sfCancelAfter]))
+            return temBAD_EXPIRATION;


Shouldn't tem codes should only be used in preflight?

I used temBAD_EXPIRATION because that is what was used in PaymentChannelFund. Its also a finite error code. This transaction cannot be reapplied and get a different error. It will always fail. However, because its so far into the tx in doApply, you could argue it should be a tec code and take the fee.

You guys tell me what to do :)

I don't believe we have good documentation for when to return different ter codes, and we should write some. In the meantime, here are my thoughts:

A transactions has three major phases:

Preflight. This is cheap. It happens before signature checks (which are expensive) and cannot read the ledger. In order to prevent broadcasting transactions to the network that fail signature checks, and therefore can't claim a fee, preflight is not allowed to return tec codes. (Note: there are several places where we check signatures: preflight2, invoke_preclaim, and in network ops; But not allowing preflight to return tec codes was modivated by the signature checks happening after preflight).

Preclaim. This is more expensive. This happens after signature checks (and after preflight) and has read access to the ledger.

Apply. This happens after preclaim and has read/write access to the ledger.

Two important points are:

Preflight is cheap and happens before signature checking.

If something returns a tem code it is not broadcast to the network. If something returns a tec code it is broadcast.

There are places in the code that return tem codes after preflight. The bad thing about this is someone can try to overload an individual server by making it do relatively expensive operations (like signature checks) and never pay fees for the work. The good thing is it saves the other servers on the network from doing the work because it is never broadcast.

However, I don't see a way to prevent someone from submitting bad signatures to an individual server and making that server do the same work that a transaction with a valid signature that returns a tem code would do.

It's important to note that tem codes should only be returned for transactions that can't succeed in any ledger. For example, if an account has an insufficient balance in the current open ledger, that transaction should return a tec code and should still be broadcast. In some cases we may want to return tec codes even when a tem would to so the transaction is recorded in a ledger and can easily be retrieved and replayed to diagnose errors.

Given that returning a tec code would not prevent any attacks that I can see, and there are benefits to not broadcasting transactions, I don't think we want to blanket disallow returning tem codes from preclaim or apply or change the ones that are already there.

However, in most cases preclaim and apply should return tec codes.

Just to add on to this, I am supportive of creating this fix amendment, but I believe the appropriate error code for this situation is probably tecEXPIRED:

The transaction tried to create an object (such as an Offer or a Check) whose provided Expiration time has already passed.

It would not surprise me if EscrowCreate is inconsistent with this. Ideally we should fix that too.

But here's why I think a tec-class code is appropriate:

The transaction isn't internally inconsistent or self-contradictory; it's only no good because there exists a previous ledger whose close time is higher than the specified expiration.

Since transactions can be delayed an indefinite amount of time between creation and confirmation¹, this error can occur for transactions that were valid initially, got relayed to a bunch of servers (maybe queued), and then happened to have their expiration pass while they were awaiting confirmation. As a general rule, we want transactions to claim a fee (tec result code) if they've been relayed through the network, so transactions whose only fault is something is past expiration should use a tec code not a tem code.

¹ Some example situations that could cause a delay:

Transaction was created and signed, saved, and then machine lost network connectivity or power. When it came back up an unspecified amount of time later, it submitted the transaction.

Transaction was relayed to some servers during a period of high load, then fees went up and it didn't get relayed to enough servers to achieve consensus. Later on when load was lower some system retried the same transaction..

The transaction was relayed through the network and on track to be validated, but then XRPL network had a temporary outage preventing new ledgers from being confirmed for a while (for example, half of validators became unable to talk to the other half due to a massive netsplit).

Thank you both for the very very detailed explanation. This will help me a lot. I'm going to change the code to tecEXPIRED.

intelliot · 2023-09-21T06:36:46Z

Although the issue can be resolved with an amendment, it's not necessarily a flaw at the protocol level. Instead, it's a potential user error. While many such errors can be detected, catching every single one might not be feasible. A strong case can be made for identifying these mistakes through checks in client libraries or at the API level. With this approach, it wouldn't be necessary to make changes that impact transaction processing. For instance, xrplcluster prevents transactions with extremely high fees, or payments to known scammers. Instead of trying to address every possible user error within the protocol, it's more practical to handle these in a layer in front of rippled. One of the benefits is that the fix can be rolled out immediately, permissionlessly and without total consensus.

intelliot · 2023-09-21T17:33:16Z

Important notes:

I don't see anything wrong with this amendment. It fixes a problem (even if it's a small problem with satisfactory alternative workarounds). I'm very happy to see developers open small fix PRs like this one.
My comment offered a holistic view, considering both the benefits of the amendment and the practicality and immediacy of other solutions.

If @dangell7 (or anyone else in the community) would like this amendment, please proceed. Reopen this PR or open a new one. This protocol change can happen in parallel, and in addition to, any fixes at the client or API service layers.

mvadari · 2023-10-03T17:09:18Z

@dangell7 it looks like tests are failing on this PR.

src/test/app/PayChan_test.cpp

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

src/ripple/protocol/Feature.h

intelliot · 2023-10-16T06:25:30Z

@dangell7 - confirming - is this ready for review? (or are there any additional changes you're planning to make?)

dangell7 · 2023-10-16T07:15:24Z

@intelliot Ready for review

mvadari · 2023-10-16T17:45:14Z

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

+    if (ctx_.view().rules().enabled(fixPayChanCancelAfter))
+    {
+        auto const closeTime = ctx_.view().info().parentCloseTime;
+        if (ctx_.tx[~sfCancelAfter] && after(closeTime, ctx_.tx[sfCancelAfter]))
+            return tecEXPIRED;
+    }


This check should happen before the SLE is created - perhaps around line 250.

intelliot · 2023-10-16T18:23:34Z

@dangell7 - do you have an opinion on whether this should be considered for the next release (2.0.0) or if it's ok for it to wait for 2024?

(The first release in 2024 is expected for Jan or Feb.)

dangell7 · 2023-10-27T10:07:30Z

@intelliot I have no opinion. 2024 is fine. I don't think many use paychan but its nice that this is fixed at some point.

scottschurr

Changes look good to me. Thanks for adding the test!

intelliot · 2024-01-08T22:46:21Z

bug fix. Should not require perf signoff. cc @sophiax851

intelliot · 2024-02-01T06:19:41Z

@dangell7

This PR has conflicts that must be resolved.
After resolving, please confirm that you still think this PR is ready to merge.

fixPayChanV1

8fccd90

tadejgolobic reviewed Sep 20, 2023

View reviewed changes

mvadari reviewed Sep 20, 2023

View reviewed changes

dangell7 closed this Sep 21, 2023

dangell7 reopened this Sep 21, 2023

dangell7 added 7 commits September 21, 2023 19:56

Merge branch 'develop' into fixPayChanV1

10be4e8

Merge branch 'develop' into fixPayChanV1

34eb379

Merge branch 'develop' into fixPayChanV1

ac560ca

Merge branch 'develop' into fixPayChanV1

e1d5b6d

Merge branch 'develop' into fixPayChanV1

7852c20

Update PayChan.cpp

9db774e

Merge branch 'develop' into fixPayChanV1

43ccf98

dangell7 added 2 commits October 4, 2023 09:17

Merge branch 'develop' into fixPayChanV1

733e3ca

update test tecEXPIRED

50bbcd4

mvadari reviewed Oct 4, 2023

View reviewed changes

src/test/app/PayChan_test.cpp Outdated Show resolved Hide resolved

dangell7 added 2 commits October 4, 2023 17:57

Update PayChan_test.cpp

cb7e575

Merge branch 'develop' into fixPayChanV1

4db1218

intelliot requested a review from mvadari October 6, 2023 05:09

mvadari reviewed Oct 6, 2023

View reviewed changes

src/ripple/app/tx/impl/PayChan.cpp Outdated Show resolved Hide resolved

mvadari reviewed Oct 6, 2023

View reviewed changes

src/ripple/protocol/Feature.h Outdated Show resolved Hide resolved

intelliot assigned dangell7 Oct 11, 2023

dangell7 added 3 commits October 11, 2023 21:30

Merge branch 'develop' into fixPayChanV1

c5a59c8

move after func to view.h

5496108

rename amendment fixPayChanCancelAfter

66e9924

intelliot requested review from seelabs and mvadari October 16, 2023 06:24

intelliot unassigned dangell7 Oct 16, 2023

Merge branch 'develop' into fixPayChanV1

16fc7b2

mvadari approved these changes Oct 16, 2023

View reviewed changes

mvadari reviewed Oct 16, 2023

View reviewed changes

intelliot assigned seelabs and dangell7 Oct 16, 2023

intelliot added this to the 2024 release milestone Oct 17, 2023

dangell7 added 2 commits October 17, 2023 16:52

move guard

d65afe7

Merge branch 'develop' into fixPayChanV1

417a050

mvadari approved these changes Oct 17, 2023

View reviewed changes

Merge branch 'develop' into fixPayChanV1

d123ae7

mvadari approved these changes Oct 30, 2023

View reviewed changes

scottschurr approved these changes Dec 13, 2023

View reviewed changes

intelliot added Tx Change API Change Amendment Testable Will Need Documentation Perf impact not expected Change is not expected to improve nor harm performance. labels Feb 1, 2024

intelliot modified the milestones: 2.1.0 (Mar 2024), 2.2.0 (Apr 2024) Feb 2, 2024

intelliot removed this from the 2.2.0 (June 2024) milestone May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixPayChanV1 #4717

fixPayChanV1 #4717

dangell7 commented Sep 20, 2023

tadejgolobic Sep 20, 2023

dangell7 Sep 20, 2023

jscottbranson Sep 20, 2023

mvadari Sep 20, 2023

dangell7 Sep 20, 2023 •

edited

Loading

seelabs Sep 22, 2023

mDuo13 Sep 22, 2023 •

edited

Loading

dangell7 Sep 25, 2023

intelliot commented Sep 21, 2023

intelliot commented Sep 21, 2023

mvadari commented Oct 3, 2023

intelliot commented Oct 16, 2023

dangell7 commented Oct 16, 2023

mvadari Oct 16, 2023

intelliot commented Oct 16, 2023

dangell7 commented Oct 27, 2023

scottschurr left a comment

intelliot commented Jan 8, 2024

intelliot commented Feb 1, 2024

fixPayChanV1 #4717

Are you sure you want to change the base?

fixPayChanV1 #4717

Conversation

dangell7 commented Sep 20, 2023

tadejgolobic Sep 20, 2023

Choose a reason for hiding this comment

dangell7 Sep 20, 2023

Choose a reason for hiding this comment

jscottbranson Sep 20, 2023

Choose a reason for hiding this comment

mvadari Sep 20, 2023

Choose a reason for hiding this comment

dangell7 Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

seelabs Sep 22, 2023

Choose a reason for hiding this comment

mDuo13 Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

dangell7 Sep 25, 2023

Choose a reason for hiding this comment

intelliot commented Sep 21, 2023

intelliot commented Sep 21, 2023

mvadari commented Oct 3, 2023

intelliot commented Oct 16, 2023

dangell7 commented Oct 16, 2023

mvadari Oct 16, 2023

Choose a reason for hiding this comment

intelliot commented Oct 16, 2023

dangell7 commented Oct 27, 2023

scottschurr left a comment

Choose a reason for hiding this comment

intelliot commented Jan 8, 2024

intelliot commented Feb 1, 2024

dangell7 Sep 20, 2023 •

edited

Loading

mDuo13 Sep 22, 2023 •

edited

Loading