fix amendment: flag check for credentials #5250

oleks-rip · 2025-01-17T17:23:27Z

High Level Overview of Change

Add transaction flag checking functionality for Credentials

Context of Change

CredentialCreate / CredentialAccept / CredentialDelete transactions will check sfFlags field in preflight()

Type of Change

Bug fix (non-breaking change which fixes an issue)

codecov · 2025-01-17T17:41:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.2%. Comparing base (8103459) to head (80e9e62).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #5250   +/-   ##
=======================================
  Coverage     78.1%   78.2%           
=======================================
  Files          790     790           
  Lines        67622   67638   +16     
  Branches      8163    8163           
=======================================
+ Hits         52844   52862   +18     
+ Misses       14778   14776    -2

Files with missing lines	Coverage Δ
include/xrpl/protocol/Feature.h	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/Credentials.cpp	`96.9% <100.0%> (+0.2%)`	⬆️
src/xrpld/app/tx/detail/SetSignerList.cpp	`91.8% <100.0%> (+0.2%)`	⬆️

... and 2 files with indirect coverage changes

include/xrpl/protocol/detail/features.macro

src/test/app/Credentials_test.cpp

include/xrpl/protocol/detail/features.macro

dangell7 · 2025-01-20T11:45:29Z

The flag check is also missing on other transactions. Should we add the flag check to other transactions as well? I believe the other one is SignerListSet.

Bronek · 2025-01-20T12:57:52Z

@dangell7 makes sense, but it should be a different amendment (we do not want this amendment to be rejected by operators, because of compatibility anxiety)

include/xrpl/protocol/detail/features.macro

mvadari · 2025-01-22T14:53:45Z

@dangell7 makes sense, but it should be a different amendment (we do not want this amendment to be rejected by operators, because of compatibility anxiety)

I disagree @Bronek - I think we should cover everything in one amendment.

Bronek · 2025-01-22T16:50:20Z

@dangell7 makes sense, but it should be a different amendment (we do not want this amendment to be rejected by operators, because of compatibility anxiety)

I disagree @Bronek - I think we should cover everything in one amendment.

Fair enough. I am not going to oppose strongly, just wary of scope creep.

mvadari · 2025-01-22T16:54:33Z

@dangell7 makes sense, but it should be a different amendment (we do not want this amendment to be rejected by operators, because of compatibility anxiety)

I disagree @Bronek - I think we should cover everything in one amendment.

Fair enough. I am not going to oppose strongly, just wary of scope creep.

My concern is about amendment count creep - needing a separate amendment just to add one additional check feels kind of dumb. Agree that scope creep is a concern, but IMO this slight scope creep is fine (and I wouldn't want to expand it further).

New name is good. I have not done a full review, so I'm not going to approve. Since my issue has been addressed, I'm just going to dismiss my review.

mvadari · 2025-01-23T21:07:12Z

include/xrpl/protocol/detail/features.macro

@@ -29,6 +29,8 @@
 // If you add an amendment here, then do not forget to increment `numFeatures`
 // in include/xrpl/protocol/Feature.h.

+// Check flags in Credential transactions
+XRPL_FIX    (TransactionFlags,           Supported::yes, VoteBehavior::DefaultNo)


Suggested change

XRPL_FIX (TransactionFlags, Supported::yes, VoteBehavior::DefaultNo)

XRPL_FIX (InvalidFlags, Supported::yes, VoteBehavior::DefaultNo)

Something like this maybe? So it's clearer what's being fixed

Not so clear - it feels like we testing flag values themselves if they are valid or not.

Right, so you're now actually erroring for invalid flags.

Not exact. One case - Transaction can fail with valid flag, which not processing by it, that's what we are testing.
And another case when someone can create flag value that is not acceptable as a single flag. For example binary value 011. Or value that is already occupied.

I'm thinking from the perspective of someone who sees fixTransactionFlags by itself and doesn't look into the code (which will be the vast majority of people). That's confusing to me. vs fixInvalidFlags is clearer to me about what it's fixing - flags that are invalid are being handled properly now.

Exact the same reason, but opposite conclusion for me :-) fixInvalidFlags is dedicated to flags type/values checks, and fixTransactionFlags directly point that it has something to do with flags in transactions

What if we combine them? fixInvalidTxFlags

I think "fix invalid" is a bit overkill phrase. Fixing something is implying that it is invalid/incorrect/wrong. So fixInvalidFlags is equal to fixFlags by its meaning. Which is less informative than fixTransactionFlags.

For the community I think I would err on the side of over kill and go with fixInvalidTxFlags

src/test/app/MultiSign_test.cpp

oleks-rip · 2025-01-24T15:27:21Z

@Bronek @mvadari there is also Change transaction which doesn't check for flag combination (but use some of them). But this tx is not so common, it doesn't documented on the SITE and I didn't found it usage. Do you know more about it and should it check for flags or not?

Bronek · 2025-01-27T11:09:50Z

@Bronek @mvadari there is also Change transaction which doesn't check for flag combination (but use some of them). But this tx is not so common, it doesn't documented on the SITE and I didn't found it usage. Do you know more about it and should it check for flags or not?

It is a pseudo-transaction, responsible for all of:

using EnableAmendment = Change;
using SetFee = Change;
using UNLModify = Change;

I agree we should enforce its flags, and since this PR has mutated to enforcing flags perhaps it is also the right place to add enforcement to this PR.

There is a caveat: the enforcement of flags (as seen in this PR) is subject to amendment voting, and this Change transaction, as seen above, is a key part of the amendment voting process, so we could potentially break amendment voting by voting this change in. Would be good to have a 3rd opinion here, maybe @JoelKatz can comment.

ximinez · 2025-01-27T18:35:04Z

I agree we should enforce its flags, and since this PR has mutated to enforcing flags perhaps it is also the right place to add enforcement to this PR.

There is a caveat: the enforcement of flags (as seen in this PR) is subject to amendment voting, and this Change transaction, as seen above, is a key part of the amendment voting process, so we could potentially break amendment voting by voting this change in. Would be good to have a 3rd opinion here, maybe @JoelKatz can comment.

It wouldn't hurt to add the check, as long as it's gated by the amendment. Technically, amendments are not active until the ledger after the EnableAmendment is processed.

However, it's probably not necessary, because there are only two ways a node is ever going to process a pseudo-transaction:

It generates it itself.
It is in the proposed transaction set from a majority of the UNL. Each of the UNL nodes are going to generate the transaction itself.

In other words, user input is never going to result in a valid pseudo-transaction, so they are much less likely to have invalid flags.

Conclusion: It may not be necessary, but it wouldn't hurt.

oleks-rip · 2025-01-29T17:30:58Z

Conclusion: It may not be necessary, but it wouldn't hurt.

So better skip it to not to introduce some hidden bug

src/test/app/Credentials_test.cpp

oleks-rip · 2025-02-10T20:06:10Z

ready to merge

oleks-rip force-pushed the cred_fix branch from 23bda8d to 6137a6e Compare January 17, 2025 17:24

Bronek reviewed Jan 17, 2025

View reviewed changes

include/xrpl/protocol/detail/features.macro Outdated Show resolved Hide resolved

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

ximinez previously requested changes Jan 17, 2025

View reviewed changes

include/xrpl/protocol/detail/features.macro Outdated Show resolved Hide resolved

Bronek self-requested a review January 20, 2025 11:39

Bronek approved these changes Jan 20, 2025

View reviewed changes

oleks-rip force-pushed the cred_fix branch from 4444d42 to f043171 Compare January 21, 2025 19:45

mvadari reviewed Jan 22, 2025

View reviewed changes

include/xrpl/protocol/detail/features.macro Outdated Show resolved Hide resolved

oleks-rip force-pushed the cred_fix branch 3 times, most recently from b882a6e to 8c8a262 Compare January 23, 2025 20:26

Bronek self-requested a review January 23, 2025 21:06

Bronek approved these changes Jan 23, 2025

View reviewed changes

mvadari reviewed Jan 23, 2025

View reviewed changes

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

oleks-rip force-pushed the cred_fix branch from 8c8a262 to a77e4f6 Compare January 24, 2025 15:07

oleks-rip force-pushed the cred_fix branch 3 times, most recently from 526b7f8 to 185a1d7 Compare January 28, 2025 18:18

oleks-rip force-pushed the cred_fix branch from 185a1d7 to 173eff8 Compare January 29, 2025 17:33

mvadari changed the title ~~RXI-1397 flag check for credentials~~ fix amendment: flag check for credentials Jan 29, 2025

mvadari added Bug Tech Debt Non-urgent improvements Amendment labels Jan 29, 2025

oleks-rip force-pushed the cred_fix branch from 173eff8 to a508b9a Compare January 30, 2025 20:41

mvadari reviewed Jan 31, 2025

View reviewed changes

src/test/app/Credentials_test.cpp Show resolved Hide resolved

oleks-rip force-pushed the cred_fix branch 3 times, most recently from 0e2bbc5 to 6b325d9 Compare February 5, 2025 22:28

oleks-rip added 5 commits February 10, 2025 13:20

RXI-1397 flag check for credentials

f452475

Rename fix, correct typo

805390c

Review fixes

1d240a6

Rename flags test

5b2b78d

Review fixes 3

6a40ef4

oleks-rip force-pushed the cred_fix branch from 6b325d9 to feff79f Compare February 10, 2025 18:42

mvadari approved these changes Feb 10, 2025

View reviewed changes

Review fixes 4

80e9e62

oleks-rip force-pushed the cred_fix branch from feff79f to 80e9e62 Compare February 10, 2025 19:14

oleks-rip added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Feb 10, 2025

bthomee merged commit fa5a854 into XRPLF:develop Feb 10, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix amendment: flag check for credentials #5250

fix amendment: flag check for credentials #5250

oleks-rip commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading

dangell7 commented Jan 20, 2025

Bronek commented Jan 20, 2025

mvadari commented Jan 22, 2025

Bronek commented Jan 22, 2025

mvadari commented Jan 22, 2025

mvadari Jan 23, 2025

oleks-rip Jan 24, 2025 •

edited

Loading

mvadari Jan 31, 2025

oleks-rip Jan 31, 2025

mvadari Jan 31, 2025

oleks-rip Jan 31, 2025 •

edited

Loading

mvadari Jan 31, 2025

oleks-rip Jan 31, 2025

brettmollin Feb 5, 2025

oleks-rip Feb 10, 2025

oleks-rip commented Jan 24, 2025 •

edited

Loading

Bronek commented Jan 27, 2025

ximinez commented Jan 27, 2025

oleks-rip commented Jan 29, 2025

oleks-rip commented Feb 10, 2025

	XRPL_FIX (TransactionFlags, Supported::yes, VoteBehavior::DefaultNo)
	XRPL_FIX (InvalidFlags, Supported::yes, VoteBehavior::DefaultNo)

fix amendment: flag check for credentials #5250

fix amendment: flag check for credentials #5250

Conversation

oleks-rip commented Jan 17, 2025 • edited Loading

High Level Overview of Change

Context of Change

Type of Change

codecov bot commented Jan 17, 2025 • edited Loading

Codecov Report

dangell7 commented Jan 20, 2025

Bronek commented Jan 20, 2025

mvadari commented Jan 22, 2025

Bronek commented Jan 22, 2025

mvadari commented Jan 22, 2025

Choose a reason for hiding this comment

oleks-rip Jan 24, 2025 • edited Loading

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

oleks-rip Jan 31, 2025 • edited Loading

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

oleks-rip commented Jan 24, 2025 • edited Loading

Bronek commented Jan 27, 2025

ximinez commented Jan 27, 2025

oleks-rip commented Jan 29, 2025

oleks-rip commented Feb 10, 2025

oleks-rip commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading

oleks-rip Jan 24, 2025 •

edited

Loading

oleks-rip Jan 31, 2025 •

edited

Loading

oleks-rip commented Jan 24, 2025 •

edited

Loading