Always create the FeeSettings object in genesis ledger #4319

ximinez · 2022-10-11T19:01:34Z

High Level Overview of Change

This is a follow-up to #4247. Reviewers do not need to concern themselves with those commits, and can start at "Always create the FeeSettings object in genesis ledger".

Inspired by this comment (and previous discussions), this refactors the genesis ledger creation process to always create and initialize a FeeSettings object. This simplifies some of the downstream initialization functions, and removes the need to pass a Config object in as many places.

This change does not require an amendment because it will have no meaningful effects processing transactions, syncing and staying up-to-date on any existing network, nor loading historical ledgers. In practical terms, it's more of a refactor that will slightly change the process for starting up a new network.

Type of Change

[X ] New feature (non-breaking change which adds functionality)
[X ] Refactor (non-breaking change that only restructures code)
[X ] Tests (You added tests for code that already exists, or your new feature included in this PR)

Test Plan

Ensure that servers running this version can continue to connect and function properly with existing networks.
Because this adds a new object to the genesis / start up ledger, any tests that examine the ledger after a fresh start will probably need to be modified to expect the FeeSettings object, initialized with the default fee values (10 drop base fee, 10 XRP account reserve, 2 XRP incremental reserve).

mDuo13 · 2022-12-12T23:33:16Z

I think this fixes #3734

ximinez · 2022-12-13T16:36:51Z

I think this fixes #3734

Not quite. This PR does create the FeeSettings object, but it still uses the "poorly documented" config values instead of the [voting] section. I think I can reasonably convert it over and get rid of the other values entirely. I'll do that in a separate commit sometime soon.

Reviewers: If you haven't started reviewing yet, please hold off until I can do that.

a-noni-mousse · 2022-12-23T16:02:58Z

src/ripple/protocol/impl/SField.cpp

@@ -234,6 +234,11 @@ CONSTRUCT_TYPED_SFIELD(sfRippleEscrow,          "RippleEscrow",         AMOUNT,
 CONSTRUCT_TYPED_SFIELD(sfDeliveredAmount,       "DeliveredAmount",      AMOUNT,    18);
 CONSTRUCT_TYPED_SFIELD(sfNFTokenBrokerFee,      "NFTokenBrokerFee",     AMOUNT,    19);

+// currency amount (fees)
+CONSTRUCT_TYPED_SFIELD(sfBaseFeeDrops,            "BaseFeeDrops",           AMOUNT,    20);


The number values here make a conflict with @RichardAH pull for Hooks values here: https://github.com/XRPLF/rippled/pull/4225/files#diff-24b5cf98b88491e906e2e365bfdbba35fffee45b36c1a52a1fb6dc539dd72023R239-R240

You should change the these values.

I updated the values over in the base PR, #4247

* Introduces amendment `featureXRPFees` * Includes Validations, Change transactions, the "Fees" ledger object, and subscription messages.

* For use with CDBC and sidechain projects 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.

* Break featureXRPCheck logic into multiple blocks. * Change default network reserves to match mainnet. * Add a isLegalAmountSigned check

* Initialize with default values from the config. Removes the need to pass a Config down into the Ledger initialization functions, including setup(). * Because old mainnet ledgers (prior to 562177 - yes, I looked it up), don't have FeeSettings, some of the other ctors will default them to the config values before setup() tries to load the object. * Update default Config fee values to match mainnet. * Fix unit tests: * Updated fees: Some tests are converted to use computed values of fee object, but the default Env config was also updated to fix the rest. * Unit tests that check the structure of the ledger have updated hashes and counts.

* upstream/develop: Add a unit test for invalid memos (XRPLF#4287)

* upstream/develop: Make NodeToShardRPC a manual test (XRPLF#4379) Update build instructions (XRPLF#4376)

* upstream/develop: Update BUILD.md (XRPLF#4383)

ximinez · 2023-03-01T20:34:12Z

what's the status of this PR? Is it ready for review/merge?

@intelliot I'm going to run a quick test of the changes I made to get rid of the undocumented config values while I grab some food, and I'll push if it looks good.

ximinez · 2023-03-01T23:53:38Z

Ok. I think it's really ready. @thejohnfreeman @nbougalis @intelliot

The last commit finally gets rid of the "poorly documented" config settings for reserves, and uses the [voting] section to initialize the configuration for new networks / standalone tests.

* upstream/develop: Set version to 1.10.0-rc4 Rename 'NFT' to 'NFToken' in DisallowIncoming flags (XRPLF#4442) Update Docker.md (XRPLF#4432) Update package building scripts and images to use Conan (XRPLF#4435) Disable duplicate detector: (XRPLF#4438)

* upstream/develop: Set version to 1.10.0

* upstream/develop: docs: security bug bounty acknowledgements (XRPLF#4460)

* upstream/develop: Rectify the import paths of boost/iterator: (XRPLF#4293) Allow port numbers be be specified with a colon: (XRPLF#4328) Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411) Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)

* upstream/develop: docs: update protocol README (4457) `fixNFTokenRemint`: prevent NFT re-mint: (4406) fix: support RPC markers for any ledger object: (4361) refactor: optimize NodeStore object conversion: (4353) fix(ValidatorSite): handle rare null pointer dereference in timeout: (4420) fix(gateway_balances): handle overflow exception: (4355) Set version to 1.10.1-b1

intelliot · 2023-03-22T23:02:20Z

@thejohnfreeman could you review this? (If not, please suggest 1-2 other potential reviewers.)

gregtatcam · 2023-03-23T19:34:25Z

src/test/app/RCLValidations_test.cpp

@@ -108,7 +108,8 @@ class RCLValidations_test : public beast::unit_test::suite
            next->updateSkipList();
            if (forceHash)
            {
-                next->setImmutable(config);
+                BEAST_EXPECT(next->read(keylet::fees()));


Why is this expected only when forceHash is set? I didn't go into the details of this test but the fees is available regardless of forceHash.

I think I just put it there because I was changing the call to setImmutable. I'll move it.

gregtatcam · 2023-03-23T19:39:00Z

src/test/app/TxQ_test.cpp

@@ -2944,7 +2944,7 @@ class TxQ1_test : public beast::unit_test::suite
        // we only see a reduction by 5.


Should this comment be updated?

Sure. I'll make it more vague. 😄

gregtatcam

LGTM 👍 I have two minor comments, which I assume would be addressed.

** Do not include this message in the squashed commit message * Move one of the tests and update a comment.

intelliot · 2023-03-27T16:55:49Z

@ximinez please suggest a commit message for this PR (to use for the entire squashed changeset)

ximinez · 2023-03-27T18:27:02Z

please suggest a commit message for this PR (to use for the entire squashed changeset)

Refactor fee initialization and configuration (#4319):

* Create the FeeSettings object in genesis ledger.
* Initialize with default values from the config. Removes the need to
  pass a Config down into the Ledger initialization functions,
  including setup().
* Drop the undocumented fee config settings in favor of the [voting]
  section.
* Because old mainnet ledgers (prior to 562177 - yes, I looked it up),
  don't have FeeSettings, some of the other ctors will default them to
  the config values before setup() tries to load the object.
* Update default Config fee values to match mainnet.
* Fix unit tests:
  * Updated fees: Some tests are converted to use computed
    values of fee object, but the default Env config was
    also updated to fix the rest.
  * Unit tests that check the structure of the ledger have updated
    hashes and counts.

* Create the FeeSettings object in genesis ledger. * Initialize with default values from the config. Removes the need to pass a Config down into the Ledger initialization functions, including setup(). * Drop the undocumented fee config settings in favor of the [voting] section. * Because old mainnet ledgers (prior to 562177 - yes, I looked it up), don't have FeeSettings, some of the other ctors will default them to the config values before setup() tries to load the object. * Update default Config fee values to match mainnet. * Fix unit tests: * Updated fees: Some tests are converted to use computed values of fee object, but the default Env config was also updated to fix the rest. * Unit tests that check the structure of the ledger have updated hashes and counts.

This assert was put in the wrong place, but it only triggers if shards are configured. This change moves the assert to the right place and updates it to ensure correctness. The assert could be hit after the server downloads some shards. It may be necessary to restart after the shards are downloaded. Note that asserts are normally checked only in debug builds, so release packages should not be affected. Introduced in: #4319 (66627b2)

ximinez added the Tech Debt Non-urgent improvements label Oct 11, 2022

ximinez requested review from thejohnfreeman and nbougalis October 11, 2022 19:01

ximinez assigned thejohnfreeman and nbougalis Oct 11, 2022

ximinez mentioned this pull request Oct 11, 2022

XRPFees: Fee setting and handling improvements #4247

Merged

ximinez changed the title ~~Ledgerinit~~ Always create the FeeSettings object in genesis ledger Oct 12, 2022

ximinez force-pushed the ledgerinit branch from dcc3edb to f3ad876 Compare October 17, 2022 17:05

ximinez force-pushed the ledgerinit branch from f3ad876 to 8e532c5 Compare November 30, 2022 20:18

intelliot added Low Priority Needs Review labels Dec 9, 2022

mDuo13 mentioned this pull request Dec 12, 2022

Document config settings for fee settings on starting a new network XRPLF/xrpl-dev-portal#1632

Open

ximinez force-pushed the ledgerinit branch from 8e532c5 to ec07ea8 Compare December 14, 2022 19:20

intelliot unassigned nbougalis Dec 14, 2022

ximinez force-pushed the ledgerinit branch from ec07ea8 to cde0ce5 Compare December 16, 2022 21:42

a-noni-mousse reviewed Dec 23, 2022

View reviewed changes

ximinez force-pushed the ledgerinit branch from cde0ce5 to 3835c9d Compare January 3, 2023 22:15

ximinez added 6 commits January 4, 2023 12:12

Convert fee voting and protocol messages to use XRPAmounts

1365039

* Introduces amendment `featureXRPFees` * Includes Validations, Change transactions, the "Fees" ledger object, and subscription messages.

[FOLD] Review feedback from Nik:

8d15137

* Break featureXRPCheck logic into multiple blocks. * Change default network reserves to match mainnet. * Add a isLegalAmountSigned check

[FOLD, maybe] Rename the new SFields from *XRP to *Drops

ebe3a1c

[FOLD] Reserve SField IDs for Hooks, renumber fee fields

fdff84d

ximinez force-pushed the ledgerinit branch from 3835c9d to a92f947 Compare January 4, 2023 17:54

ximinez added 3 commits January 5, 2023 12:05

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

41df657

* upstream/develop: Add a unit test for invalid memos (XRPLF#4287)

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

cfd30fa

* upstream/develop: Make NodeToShardRPC a manual test (XRPLF#4379) Update build instructions (XRPLF#4376)

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

c72cf84

* upstream/develop: Update BUILD.md (XRPLF#4383)

intelliot removed the Low Priority label Jan 20, 2023

intelliot assigned nbougalis and drlongle and unassigned ximinez Mar 2, 2023

intelliot requested review from mtrippled, mDuo13 and drlongle March 2, 2023 19:36

drlongle approved these changes Mar 14, 2023

View reviewed changes

ximinez added 3 commits March 14, 2023 14:27

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

137d124

* upstream/develop: Set version to 1.10.0

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

492e3d2

* upstream/develop: docs: security bug bounty acknowledgements (XRPLF#4460)

intelliot requested a review from gregtatcam March 22, 2023 17:44

gregtatcam reviewed Mar 23, 2023

View reviewed changes

gregtatcam approved these changes Mar 23, 2023

View reviewed changes

[FOLD] Review feedback from @gregtatcam:

f2742cf

** Do not include this message in the squashed commit message * Move one of the tests and update a comment.

ximinez force-pushed the ledgerinit branch from 32c9bf6 to f2742cf Compare March 24, 2023 16:44

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

intelliot merged commit 66627b2 into XRPLF:develop Mar 28, 2023
4 checks passed

ximinez deleted the ledgerinit branch March 28, 2023 22:11

drlongle mentioned this pull request Mar 31, 2023

Fix unit test ripple.app.LedgerData #4484

Merged

7 tasks

ximinez mentioned this pull request May 22, 2023

Move faulty assert #4533

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always create the FeeSettings object in genesis ledger #4319

Always create the FeeSettings object in genesis ledger #4319

ximinez commented Oct 11, 2022

mDuo13 commented Dec 12, 2022

ximinez commented Dec 13, 2022

a-noni-mousse Dec 23, 2022

ximinez Jan 4, 2023

ximinez commented Mar 1, 2023

ximinez commented Mar 1, 2023

intelliot commented Mar 22, 2023

gregtatcam Mar 23, 2023

ximinez Mar 24, 2023

gregtatcam Mar 23, 2023

ximinez Mar 24, 2023

gregtatcam left a comment

intelliot commented Mar 27, 2023

ximinez commented Mar 27, 2023 •

edited

		@@ -2944,7 +2944,7 @@ class TxQ1_test : public beast::unit_test::suite
		// we only see a reduction by 5.

Always create the FeeSettings object in genesis ledger #4319

Always create the FeeSettings object in genesis ledger #4319

Conversation

ximinez commented Oct 11, 2022

High Level Overview of Change

Type of Change

Test Plan

mDuo13 commented Dec 12, 2022

ximinez commented Dec 13, 2022

a-noni-mousse Dec 23, 2022

Choose a reason for hiding this comment

ximinez Jan 4, 2023

Choose a reason for hiding this comment

ximinez commented Mar 1, 2023

ximinez commented Mar 1, 2023

intelliot commented Mar 22, 2023

gregtatcam Mar 23, 2023

Choose a reason for hiding this comment

ximinez Mar 24, 2023

Choose a reason for hiding this comment

gregtatcam Mar 23, 2023

Choose a reason for hiding this comment

ximinez Mar 24, 2023

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

intelliot commented Mar 27, 2023

ximinez commented Mar 27, 2023 • edited

ximinez commented Mar 27, 2023 •

edited