Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FeeVote] Voting for fee change and its implications are unclear #2451

Closed
rrozek opened this issue Mar 23, 2018 · 8 comments · Fixed by #2910
Closed

[FeeVote] Voting for fee change and its implications are unclear #2451

rrozek opened this issue Mar 23, 2018 · 8 comments · Fixed by #2910
Assignees
Labels

Comments

@rrozek
Copy link

rrozek commented Mar 23, 2018

Hello!
On my rippled fork i'm trying to figure out how are all Fee-related things working. I think it might even be a bug in how certain fee-related variables are used in code.
There are several places from which we can obtain fee stats including:

  1. server_state (raw values (in drops???))
  2. server_info (human readable server_state output - with potential bug?)
  3. fee
  4. subscription to closed ledgers where we can see 'fee_ref' value (which is actually not votable 'reference_fee_units' == 10, not the votable 'reference_fee').

at first i set up recommended values on my standalone server. and then advance 256 ledgers to first flag ledger and query server_state, server_info, fee

rozek@***:~$ tail /etc/rippled/rippled.cfg
[features]
MultiSign

[voting]
reference_fee = 10
account_reserve = 20000000
owner_reserve = 5000000

rozek@***:~$ /usr/bin/rippled --conf=/etc/rippled/rippled.cfg server_info
Loading: "/etc/rippled/rippled.cfg"
2018-Mar-23 12:38:54 HTTPClient:NFO Connecting to 127.0.0.1:5005

{
"result" : {
"info" : {
"build_version" : "1.0.0-b2",
"complete_ledgers" : "2-261",
"hostid" : "",
"io_latency_ms" : 1,
"jq_trans_overflow" : "0",
"last_close" : {
"converge_time_s" : 0.1,
"proposers" : 0
},
--- editors cut ---
"validated_ledger" : {
"base_fee_xrp" : 1e-05,
"hash" : "94B62A82705D7B241AA8B2BA4C0EBF772905E9A3F8CF76B4F971C1BCDA82D233",
"reserve_base_xrp" : 20,
"reserve_inc_xrp" : 5,
"seq" : 261
},
"validation_quorum" : 0,
"validator_list_expires" : "unknown"
},
"status" : "success"
}
}
rozek@
:~$ /usr/bin/rippled --conf=/etc/rippled/rippled.cfg server_state
Loading: "/etc/rippled/rippled.cfg"
2018-Mar-23 12:38:57 HTTPClient:NFO Connecting to 127.0.0.1:5005

{
"result" : {
"state" : {
"build_version" : "1.0.0-b2",
"complete_ledgers" : "2-261",
"io_latency_ms" : 1,
"jq_trans_overflow" : "0",
"last_close" : {
"converge_time" : 100,
"proposers" : 0
},
"load" : {
"job_types" : [
{
"in_progress" : 1,
"job_type" : "clientCommand"
}
],
"threads" : 1
},
"load_base" : 256,
"load_factor" : 256,
"load_factor_fee_escalation" : 256,
"load_factor_fee_queue" : 256,
"load_factor_fee_reference" : 256,
"load_factor_server" : 256,
--- editors cut ---
"validated_ledger" : {
"base_fee" : 10,
"close_time" : 575124139,
"hash" : "94B62A82705D7B241AA8B2BA4C0EBF772905E9A3F8CF76B4F971C1BCDA82D233",
"reserve_base" : 20000000,
"reserve_inc" : 5000000,

"seq" : 261
},
"validation_quorum" : 0,
"validator_list_expires" : 0
},
"status" : "success"
}
}

rozek@***:~$ /usr/bin/rippled --conf=/etc/rippled/rippled.cfg fee
Loading: "/etc/rippled/rippled.cfg"
2018-Mar-23 12:39:02 HTTPClient:NFO Connecting to 127.0.0.1:5005

{
   "result" : {
      "current_ledger_size" : "1",
      "current_queue_size" : "0",
      "drops" : {
         "base_fee" : "10",
         "median_fee" : "5000",
         "minimum_fee" : "10",
         "open_ledger_fee" : "10"
      },
      "expected_ledger_size" : "1000",
      "ledger_current_index" : 262,
      "levels" : {
         "median_level" : "128000",
         "minimum_level" : "256",
         "open_ledger_level" : "256",
         "reference_level" : "256"
      },
      "max_queue_size" : "20000",
      "status" : "success"
   }
}

so far so good, everything is allright.
now modify rippled.cfg to reference_fee = 100 and hit next flag ledger
rozek@***:~$ tail /etc/rippled/rippled.cfg
[features]
MultiSign
[voting]
reference_fee = 100
account_reserve = 20000000
owner_reserve = 5000000

in code reference_fee from conf file is mapped to FeeVote::Setup::reference_fee which is loaded to Fees::base (Ledger class member). there is also an Fees::units member which is not voteable (it is added to SetFee tx though) and hardcoded to 10.
Now what seems not correct to me is calculation of human-readable response to server_info command in NetworkOPs.cpp:2286

l[jss::reserve_base_xrp]   = static_cast<double> (Json::UInt (
                    lpClosed->fees().accountReserve(0).drops() * baseFee / baseRef))
/ SYSTEM_CURRENCY_PARTS;

baseFee / baseRef gives fine result when both are equal to 10, but if baseFee (which is votable) changes, this also has impact on how values are reported in server_info.
similar construction * baseFee / baseRef is used in a few more places in code and i'm wondering if that has any logical impact on applied fees if fee is changed via voting?
proof malformed server_info below:

rozek@***:~$ /usr/bin/rippled --conf=/etc/rippled/rippled.cfg server_state
Loading: "/etc/rippled/rippled.cfg"
2018-Mar-23 12:44:57 HTTPClient:NFO Connecting to 127.0.0.1:5005

{
"result" : {
"state" : {
"build_version" : "1.0.0-b2",
"complete_ledgers" : "2-572",
"io_latency_ms" : 1,
"jq_trans_overflow" : "0",
"last_close" : {
"converge_time" : 100,
"proposers" : 0
},
"load" : {
"job_types" : [
{
"avg_time" : 4,
"in_progress" : 1,
"job_type" : "clientCommand",
"peak_time" : 29
},
{
"job_type" : "WriteNode",
"per_second" : 1
}
],
"threads" : 1
},
"load_base" : 256,
"load_factor" : 256,
"load_factor_fee_escalation" : 256,
"load_factor_fee_queue" : 256,
"load_factor_fee_reference" : 256,
"load_factor_server" : 256,
--- editors cut ---
"validated_ledger" : {
"base_fee" : 100,
"close_time" : 575124750,
"hash" : "E4773984777DCDBFEE7CF84077FB88FFE0AC920A58F9096E03D599D287F4B3B1",
"reserve_base" : 20000000,
"reserve_inc" : 5000000,

"seq" : 572
},
"validation_quorum" : 0,
"validator_list_expires" : 0
},
"status" : "success"
}
}
rozek@***:~$ /usr/bin/rippled --conf=/etc/rippled/rippled.cfg server_info
Loading: "/etc/rippled/rippled.cfg"
2018-Mar-23 12:45:13 HTTPClient:NFO Connecting to 127.0.0.1:5005

{
"result" : {
"info" : {
"build_version" : "1.0.0-b2",
"complete_ledgers" : "2-572",
"hostid" : "***",
"io_latency_ms" : 1,
"jq_trans_overflow" : "0",
--- editors cut ---
"validated_ledger" : {
"base_fee_xrp" : 0.0001,
"hash" : "E4773984777DCDBFEE7CF84077FB88FFE0AC920A58F9096E03D599D287F4B3B1",
"reserve_base_xrp" : 200,
"reserve_inc_xrp" : 50,

"seq" : 572
},
"validation_quorum" : 0,
"validator_list_expires" : "unknown"
},
"status" : "success"
}
}

rozek@***:~$ /usr/bin/rippled --conf=/etc/rippled/rippled.cfg fee
Loading: "/etc/rippled/rippled.cfg"
2018-Mar-23 12:45:26 HTTPClient:NFO Connecting to 127.0.0.1:5005

{
   "result" : {
      "current_ledger_size" : "1",
      "current_queue_size" : "0",
      "drops" : {
         "base_fee" : "100",
         "median_fee" : "50000",
         "minimum_fee" : "100",
         "open_ledger_fee" : "100"
      },
      "expected_ledger_size" : "1000",
      "ledger_current_index" : 573,
      "levels" : {
         "median_level" : "128000",
         "minimum_level" : "256",
         "open_ledger_level" : "256",
         "reference_level" : "256"
      },
      "max_queue_size" : "20000",
      "status" : "success"
   }
}

Now my questions...
is it a bug in displaying human-readable values? ( i guess so?)
is it working as expected? if so, why fee increase results in account_reserve increase?
and most important: what does 'fee in fee units' or 'fee units' or FeeVote::Setup::reference_fee_units mean? could anyone enlighten me please?
(it is for instance referenced in TransactionSign.cpp in checkFee function)

Thank you very much in advance!

btw. Great work!

@nbougalis
Copy link
Contributor

Thanks for the report. We'll look closely at this and get back to you.

@rrozek
Copy link
Author

rrozek commented Mar 28, 2018

Thank you, can't wait your opinion ;)

@mDuo13
Copy link
Collaborator

mDuo13 commented Apr 3, 2018

"Fee units" is an arbitrary unit for representing fees. Something like, you translate things into fee units and back so that the calculations are irrelevant to the scale of the actual monetary values involved? I'm sure it makes sense to @JoelKatz.

So for example, the SetFee pseudo-tx has the BaseFee parameter (drops of XRP) and the ReferenceFeeUnits parameter (fee units), which represent the same thing in different units. I think that establishes how many drops represent how many fee units.

That makes me wonder if the docs on SetFee are wrong about the ReserveBase and ReserveIncrement fields being in drops, because UInt32 seems like a poor choice to represent drops. (The docs for the FeeSettings ledger object type are at least consistent with them being drops in 32 bits, which a previous conversation noted limits the maximum reserve to a kind of small amount of XRP...)

Given that it's so confusing, I would not be surprised if some calculations did the wrong conversion...

@rrozek
Copy link
Author

rrozek commented Apr 4, 2018

Docs on SetFee are describing actual situation (all votable parameters are in drops).

I personally think that "Fee units" are meant to be arbitrary unit, but in fact they are not. They are fixed to value of '10' in code and calculations made in 'Fee units' actually work in current setup because BaseFee == ReferenceFeeUnits. But since all 'votable' parameters are described in drops this kind of discards the idea of "Fee unit".
In my opinion Fees::units are used not inline with the idea in a few places, as for instance in Transactor::calculateBaseFee where Fees::units are used to calculate baseFee. Its like you determine that one dollar equals 100 pennies and call it 'unit', and then you use 'unit' instead of 'value' for calculations. Calculations will be const no matter how you change 'value'.

Really would love to hear some wise and confident opinion on that :)

ximinez added a commit to ximinez/rippled that referenced this issue Feb 22, 2019
* Proof of concept
* Uses tagged_integers with tags for drops, fee units (LoadFeeTrack),
  and fee levels (TxQ).
* Resolves XRPLF#2451
@JoelKatz
Copy link
Collaborator

There was really never any specific intention to support a base reference other than 10 fee units for a minimum transaction (a simple transfer of XRP between two accounts that already exist).

The reserves are voted on directly, so they are expressed in drops. The cost of a minimal transaction is also voted on directly, so it is expressed in drops.

However, we can't express, for example, the cost of an extra signature in a multisigned transaction in drops because if we did, that would have to be voted on separately and be changed any time the cost of the reference transaction changed. So we express those fees in fee units (tenths of a reference transaction).

ximinez added a commit to ximinez/rippled that referenced this issue Mar 25, 2019
* Proof of concept
* Uses tagged_integers with tags for drops, fee units (LoadFeeTrack),
  and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Mar 27, 2019
* Proof of concept
* Uses tagged_integers with tags for drops, fee units (LoadFeeTrack),
  and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Mar 28, 2019
* Proof of concept
* Uses tagged_integers with tags for drops, fee units (LoadFeeTrack),
  and fee levels (TxQ).
* Resolves XRPLF#2451
@ximinez
Copy link
Collaborator

ximinez commented Mar 28, 2019

There was indeed an error in some of the calculations. I have submitted #2884 to resolve it and hopefully keep it from recurring.

This issue should automatically close when that code is merged to develop

@rrozek
Copy link
Author

rrozek commented Mar 29, 2019

yay! Now it will definitely be easier to read through fee tangles :)
thank you! 👍

ximinez added a commit to ximinez/rippled that referenced this issue Mar 29, 2019
* Uses tagged_integers with tags for drops, fee units (LoadFeeTrack),
  and fee levels (TxQ).
* Resolves XRPLF#2451
@ximinez
Copy link
Collaborator

ximinez commented Apr 1, 2019

You're welcome!

ximinez added a commit to ximinez/rippled that referenced this issue Apr 23, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue May 3, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue May 24, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jun 14, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jun 14, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jun 14, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jun 26, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jun 26, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jul 11, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jul 11, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jul 11, 2019
* Uses the boost::units library with units for drops,
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jul 16, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jul 16, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
@mDuo13 mDuo13 added the Bug label Aug 2, 2019
ximinez added a commit to ximinez/rippled that referenced this issue Aug 19, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Aug 26, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Sep 9, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
@rrozek rrozek closed this as completed Sep 23, 2019
ximinez added a commit to ximinez/rippled that referenced this issue Oct 2, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Nov 20, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Dec 18, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Dec 18, 2019
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
ximinez added a commit to ximinez/rippled that referenced this issue Jan 8, 2020
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
cjcobb23 pushed a commit to cjcobb23/rippled that referenced this issue Jan 10, 2020
* Uses existing XRPAmount with units for drops, and a new TaggedFee for
  fee units (LoadFeeTrack), and fee levels (TxQ).
* Resolves XRPLF#2451
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants