`XRPFees`: Fee setting and handling improvements #4247

ximinez · 2022-07-21T17:26:37Z

High Level Overview of Change

This branch resolves #3932. The concept of "fee units" has been (almost) completely removed. It remains in a few places to preserve backward compatibility. Additionally, a new amendment, XRPFees has been introduced which, if enabled by the validators, will convert all representations of fees in the code, ledger, and protocol that are still raw numbers into XRPAmounts.

Additionally, it fixes an issue where a sidechain / test network is able to confirm a 0 drop base fee, but is unable to accept submissions of transactions with 0 drop fees. The transaction engine appears to be perfectly fine with such fees, but fee escalation logic had an unstated assumption that the base fee would never be 0. This fix works around that assumption while still enabling fee escalation to raise required fees when the network gets busy - it effectively treats the base fee as 1 drop for purposes of getting into the open ledger ONLY when fees are escalated. No amendment is required for this fix because it only affects transaction submission, not processing. I am reasonably sure this solution is complete, but I would encourage anybody dealing with the issue to test this branch to ensure I haven't missed any edge cases.

Type of Change

[X ] Bug fix (non-breaking change which fixes an issue)
[X ] New feature (non-breaking change which adds functionality)
[X ] Tests (You added tests for code that already exists, or your new feature included in this PR)

src/ripple/app/ledger/Ledger.cpp

src/ripple/app/misc/FeeVote.h

src/ripple/app/misc/FeeVoteImpl.cpp

src/ripple/protocol/SystemParameters.h

src/ripple/protocol/impl/STValidation.cpp

mDuo13 · 2022-10-24T23:08:09Z

Does this also fix the fact that the server (in some places?) serializes BaseFee in hexadecimal for some weird reason?

ximinez · 2022-10-25T18:29:26Z

Does this also fix the fact that the server (in some places?) serializes BaseFee in hexadecimal for some weird reason?

I'm not sure. Can you point me to some places where you see it?

intelliot · 2022-11-08T05:33:01Z

(@mDuo13 whenever you have a chance - there's an open question above)

mDuo13 · 2022-11-16T21:16:03Z

Does this also fix the fact that the server (in some places?) serializes BaseFee in hexadecimal for some weird reason?

I'm not sure. Can you point me to some places where you see it?

#3165

ledger_entry for the FeeSettings singleton object

ximinez · 2022-11-16T22:57:02Z

$ rippled -q feature XRPFees

{
   "result" : {
      "93E516234E35E08CA689FA33A6D38E103881F8DCB53023F728C307AA89D515A7" : {
         "enabled" : true,
         "name" : "XRPFees",
         "supported" : true,
         "vetoed" : false
      },
      "status" : "success"
   }
}

$ rippled -q ledger_entry 4BC50C9B0D8515D3EAAE1E74B29A95804346C491EE1A95BF25E4AAB854A6A651

{
   "result" : {
      "index" : "4BC50C9B0D8515D3EAAE1E74B29A95804346C491EE1A95BF25E4AAB854A6A651",
      "ledger_current_index" : 3,
      "node" : {
         "BaseFeeDrops" : "10",
         "Flags" : 0,
         "LedgerEntryType" : "FeeSettings",
         "ReserveBaseDrops" : "10000000",
         "ReserveIncrementDrops" : "2000000",
         "index" : "4BC50C9B0D8515D3EAAE1E74B29A95804346C491EE1A95BF25E4AAB854A6A651"
      },
      "status" : "success",
      "validated" : false
   }
}

Note that I ran this test using my ledgerinit branch, which is based off of this one, but creates the fee object on genesis, and just made it simpler for me.

intelliot · 2023-01-20T19:29:44Z

@thejohnfreeman any last thoughts on this PR?

thejohnfreeman

Should we remove the section "Fee level" in src/ripple/app/misc/FeeEscalation.md and amend the rest of the documentation?

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

ximinez · 2023-01-24T21:39:50Z

Should we remove the section "Fee level" in src/ripple/app/misc/FeeEscalation.md and amend the rest of the documentation?

@thejohnfreeman I don't think so. Fee levels are not the same as fee units. I removed fee units here. Fee levels are still there and necessary for the transaction queue to work fairly. I'm hoping that after this is merged, there will be less confusion about the whole thing.

As for the rest of the documentation, I don't think "fee units" are really documented anywhere because the were a transparent implementation detail, but if there are, then yeah, that would need to be updated.

* Clarify comments explaining the ttFEE transaction field validation.

thejohnfreeman · 2023-01-25T18:53:04Z

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

+                // The ttFEE transaction format defines these fields as
+                // optional, but once the XRPFees feature is enabled, they are
+                // forbidden.


These are the required ones, right? Not forbidden? The code returns malformed if they are missing.

Yeah, woops. Got those backwards.

thejohnfreeman · 2023-01-25T18:53:26Z

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

+                // The ttFEE transaction format defines these fields as
+                // optional, but once the XRPFees feature is enabled, they are
+                // required.


Similarly, these are the forbidden ones.

Yeah, woops. Got those backwards.

* upstream/develop: `XRPFees`: Fee setting and handling improvements (XRPLF#4247)

…tpage * upstream/develop: (37 commits) Update documented pathfinding configuration defaults: (XRPLF#4409) Update dependency: grpc (XRPLF#4407) Introduce min/max observers for Number Optimize uint128_t division by 10 within Number.cpp Replace Number division algorithm Include rounding mode in XRPAmount to STAmount conversion. Remove undefined behavior * Taking the negative of a signed negative is UB, but taking the negative of an unsigned is not. Silence warnings Introduce rounding modes for Number: Use Number for IOUAmount and STAmount arithmetic Add tests Add implicit conversion from STAmount to Number Add clip Add conversions between Number, XRPAmount and int64_t AMM Add Number class and associated algorithms Revise CONTRIBUTING (XRPLF#4382) `XRPFees`: Fee setting and handling improvements (XRPLF#4247) Update BUILD.md (XRPLF#4383) Make NodeToShardRPC a manual test (XRPLF#4379) Update build instructions (XRPLF#4376) ...

* Introduces amendment `XRPFees` * Convert fee voting and protocol messages to use XRPAmounts * Includes Validations, Change transactions, the "Fees" ledger object, and subscription messages * Improve handling of 0 drop reference fee with TxQ. For use with networks that do not want to require fees * Note that fee escalation logic is still in place, which may cause the open ledger fee to rise if the network is busy. 0 drop transactions will still queue, and fee escalation can be effectively disabled by modifying the configuration on all nodes * Change default network reserves to match Mainnet * Name the new SFields *Drops (not *XRP) * Reserve SField IDs for Hooks * Clarify comments explaining the ttFEE transaction field validation

ximinez added Bug Amendment High Priority labels Jul 21, 2022

ximinez requested a review from nbougalis July 21, 2022 17:26

ximinez assigned nbougalis Jul 21, 2022

ximinez force-pushed the feesettings branch from cfbf44d to faa9a83 Compare July 27, 2022 16:15

ximinez force-pushed the feesettings branch 2 times, most recently from fc53680 to 72f0af4 Compare August 26, 2022 20:46

nbougalis reviewed Aug 26, 2022

View reviewed changes

ximinez mentioned this pull request Oct 11, 2022

Always create the FeeSettings object in genesis ledger #4319

Merged

ximinez force-pushed the feesettings branch from 7abcb31 to 71ca946 Compare October 17, 2022 15:32

intelliot assigned mDuo13 Nov 8, 2022

mDuo13 approved these changes Nov 18, 2022

View reviewed changes

ximinez force-pushed the feesettings branch from 71ca946 to 07d37da Compare November 30, 2022 20:13

intelliot requested a review from manojsdoshi December 9, 2022 00:25

intelliot added the Testable label Dec 9, 2022

intelliot requested review from nbougalis and RichardAH December 9, 2022 04:48

ximinez force-pushed the feesettings branch 2 times, most recently from 0aa77cc to fa94a73 Compare December 16, 2022 19:46

intelliot unassigned mDuo13 Dec 21, 2022

intelliot requested review from pwang200, scottschurr and seelabs December 21, 2022 20:12

drlongle approved these changes Jan 19, 2023

View reviewed changes

ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jan 19, 2023

intelliot changed the title ~~Fee setting and handling improvements~~ XRPFees: Fee setting and handling improvements Jan 20, 2023

intelliot added the Will Need Documentation label Jan 20, 2023

thejohnfreeman reviewed Jan 20, 2023

View reviewed changes

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

intelliot assigned ximinez Jan 21, 2023

Review feedback from @thejohnfreeman:

c76b700

* Clarify comments explaining the ttFEE transaction field validation.

thejohnfreeman requested changes Jan 25, 2023

View reviewed changes

fixup! Review feedback from @thejohnfreeman:

e7a9e66

thejohnfreeman approved these changes Jan 26, 2023

View reviewed changes

intelliot unassigned thejohnfreeman Feb 3, 2023

intelliot removed the Needs Review label Feb 3, 2023

intelliot merged commit e4b17d1 into XRPLF:develop Feb 3, 2023

ximinez deleted the feesettings branch February 3, 2023 16:34

ximinez added a commit to ximinez/rippled that referenced this pull request Feb 3, 2023

Merge remote-tracking branch 'upstream/develop' into github-ci

6d5dfd7

* upstream/develop: `XRPFees`: Fee setting and handling improvements (XRPLF#4247)

ximinez added a commit to ximinez/rippled that referenced this pull request Feb 3, 2023

Merge remote-tracking branch 'upstream/develop' into neverfeature

c9ce8bc

* upstream/develop: `XRPFees`: Fee setting and handling improvements (XRPLF#4247)

ximinez added a commit to ximinez/rippled that referenced this pull request Feb 3, 2023

Merge remote-tracking branch 'upstream/develop' into ledgerinit

4e93493

* upstream/develop: `XRPFees`: Fee setting and handling improvements (XRPLF#4247)

ximinez added a commit to ximinez/rippled that referenced this pull request Feb 3, 2023

Merge remote-tracking branch 'upstream/develop' into badmarker

b88e1f7

* upstream/develop: `XRPFees`: Fee setting and handling improvements (XRPLF#4247)

ckniffen mentioned this pull request Feb 17, 2023

Support featureXRPFees amendment XRPLF/xrpl.js#2219

Closed

intelliot mentioned this pull request Feb 18, 2023

Make XRPFees a non-breaking API change #4425

Closed

ckniffen mentioned this pull request Feb 21, 2023

Support featureXRPFees amendment XRPLF/xrpl-py#526

Closed

ximinez mentioned this pull request Feb 22, 2023

Undo API changes introduced in XRPFees: #4429

Merged

intelliot added the API Change label Jun 20, 2023

ximinez mentioned this pull request Oct 3, 2023

Note that fee_ref will be removed if XRPFees is enabled XRPLF/xrpl-dev-portal#2183

Merged

This was referenced Nov 2, 2023

Update API CHANGELOG for XRPFees changes #4741

Merged

Remove "fee units" from API #3164

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`XRPFees`: Fee setting and handling improvements #4247

`XRPFees`: Fee setting and handling improvements #4247

ximinez commented Jul 21, 2022 •

edited

Loading

mDuo13 commented Oct 24, 2022

ximinez commented Oct 25, 2022

intelliot commented Nov 8, 2022

mDuo13 commented Nov 16, 2022

ximinez commented Nov 16, 2022

intelliot commented Jan 20, 2023

thejohnfreeman left a comment

ximinez commented Jan 24, 2023

thejohnfreeman Jan 25, 2023

ximinez Jan 25, 2023

thejohnfreeman Jan 25, 2023

ximinez Jan 25, 2023

XRPFees: Fee setting and handling improvements #4247

XRPFees: Fee setting and handling improvements #4247

Conversation

ximinez commented Jul 21, 2022 • edited Loading

High Level Overview of Change

Type of Change

mDuo13 commented Oct 24, 2022

ximinez commented Oct 25, 2022

intelliot commented Nov 8, 2022

mDuo13 commented Nov 16, 2022

ximinez commented Nov 16, 2022

intelliot commented Jan 20, 2023

thejohnfreeman left a comment

Choose a reason for hiding this comment

ximinez commented Jan 24, 2023

thejohnfreeman Jan 25, 2023

Choose a reason for hiding this comment

ximinez Jan 25, 2023

Choose a reason for hiding this comment

thejohnfreeman Jan 25, 2023

Choose a reason for hiding this comment

ximinez Jan 25, 2023

Choose a reason for hiding this comment

`XRPFees`: Fee setting and handling improvements #4247

`XRPFees`: Fee setting and handling improvements #4247

ximinez commented Jul 21, 2022 •

edited

Loading