[rpc] Introduced a new `fees` structure that aggregates all sub-field fee types denominated in BTC

mryandao commented at 8:28 am on January 22, 2018: contributor

the denomination for fee is current in btc while the other such as decendentFee and ancestorFee are in satoshis.

fanquake added the label RPC/REST/ZMQ on Jan 22, 2018

laanwj commented at 10:52 am on January 22, 2018: member

The convention on the RPC interface is to use ValueFromAmount and AmountFromValue for monetary values unless there is a really good reason (e.g. a BIP) that says otherwise.
This is an interface-breaking change. Current software assumes the values are in a certain unit and can blow up dangerously when it suddenly changes from one version to the other. How long has it been like this?

promag commented at 11:00 am on January 22, 2018: member

NACK as it is. Call it bug, or lack of consistency, but the truth is changing may/will break existing software. Instead it should follow the deprecation mechanism.

Something like adding a flag to control the resulting unit or adding a new field.

promag commented at 11:17 am on January 22, 2018: member

After IRC discussion, the suggestion is to add a new field:

0{
1  ...,
2  "fees": {
3    "base": ...,
4    "modified": ...,
5    "ancestor": ...,
6    "descendant": ...
7  },
8  ...
9}

IMO this should be done here too:

all values must have the same unit and the documentation must mention it;
a test should be added to compare old fields with new fields;
bonus, existing tests could be updated to use new fields.

in src/rpc/blockchain.cpp:395 in ce16e79e27 outdated

385+    fees.push_back(Pair("descendant", ValueFromAmount(e.GetModFeesWithDescendants())));
386+
387     info.push_back(Pair("size", (int)e.GetTxSize()));
388-    info.push_back(Pair("fee", e.GetFee()));
389-    info.push_back(Pair("modifiedfee", e.GetModifiedFee()));
390+    info.push_back(Pair("fee", ValueFromAmount(e.GetFee())));

promag commented at 11:59 am on January 22, 2018:

Revert.

in src/rpc/blockchain.cpp:396 in ce16e79e27 outdated

386+
387     info.push_back(Pair("size", (int)e.GetTxSize()));
388-    info.push_back(Pair("fee", e.GetFee()));
389-    info.push_back(Pair("modifiedfee", e.GetModifiedFee()));
390+    info.push_back(Pair("fee", ValueFromAmount(e.GetFee())));
391+    info.push_back(Pair("modifiedfee", ValueFromAmount(e.GetModifiedFee())));

promag commented at 11:59 am on January 22, 2018:

Revert.

in src/rpc/blockchain.cpp:394 in ce16e79e27 outdated

392     info.push_back(Pair("time", e.GetTime()));
393     info.push_back(Pair("height", (int)e.GetHeight()));
394     info.push_back(Pair("descendantcount", e.GetCountWithDescendants()));
395     info.push_back(Pair("descendantsize", e.GetSizeWithDescendants()));
396-    info.push_back(Pair("descendantfees", e.GetModFeesWithDescendants()));
397+    info.push_back(Pair("descendantfees", ValueFromAmount(e.GetModFeesWithDescendants())));

promag commented at 11:59 am on January 22, 2018:

Revert.

in src/rpc/blockchain.cpp:383 in ce16e79e27 outdated

378@@ -379,18 +379,24 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
379 {
380     AssertLockHeld(mempool.cs);
381 
382+    UniValue fees(UniValue::VOBJ);
383+    fees.push_back(Pair("current", ValueFromAmount(e.GetFee())));

promag commented at 12:00 pm on January 22, 2018:

Not sure about current. Other options are base, raw, original, self…

promag commented at 12:01 pm on January 22, 2018: member

Missing fees.modified?

meshcollider commented at 1:13 pm on January 22, 2018: contributor

Looking better, thanks :) note that you need to update the help text too including a note that the old fields are deprecated

promag commented at 1:35 pm on January 22, 2018: member

FYI https://github.com/bitcoin/bitcoin/blob/b987ca4ee495a7fff82f0ac14ef0753bfb7586e2/src/rpc/blockchain.cpp#L359-L376

mryandao commented at 0:22 am on January 23, 2018: contributor

alright, i’ve addressed all of @MeshCollider and @promag’s comments and applied the necessary changes.

MarcoFalke commented at 0:37 am on January 23, 2018: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

in src/rpc/blockchain.cpp:375 in c8a6eed86a outdated

374-           "    \"ancestorfees\" : n,     (numeric) modified fees (see above) of in-mempool ancestors (including this one)\n"
375+           "    \"ancestorfees\" : n,     (numeric) modified fees (see above) of in-mempool ancestors (including this one) (DEPRECATED) \n"
376            "    \"wtxid\" : hash,         (string) hash of serialized transaction, including witness data\n"
377+           "    \"fees\" : {\n"
378+           "        \"base\" : n,         (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
379+           "        \"modifiedfee\" : n,  (numeric) transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT + "\n"

promag commented at 0:41 am on January 23, 2018:

Just modified?

mryandao commented at 1:47 am on January 23, 2018:

for the proposed updated test above, do I still leave the existing test in there if there are plans to deprecate the old interface?

promag commented at 11:25 am on January 23, 2018:

IMO current tests should stay. Just add more.

promag commented at 0:47 am on January 23, 2018: member

Please see my comment above regarding tests. For instance, these (and others) could be updated: https://github.com/bitcoin/bitcoin/blob/b5e4b9b5100ec15217d43edb5f4149439f4b20a5/test/functional/mempool_packages.py#L68 https://github.com/bitcoin/bitcoin/blob/b5e4b9b5100ec15217d43edb5f4149439f4b20a5/test/functional/mempool_packages.py#L102 https://github.com/bitcoin/bitcoin/blob/b5e4b9b5100ec15217d43edb5f4149439f4b20a5/test/functional/mempool_packages.py#L139

mryandao force-pushed on Jan 23, 2018

promag commented at 11:26 am on January 23, 2018: member

Care to update doc/release-notes.md?

in src/rpc/blockchain.cpp:377 in ac168a6b3b outdated

376            "    \"wtxid\" : hash,         (string) hash of serialized transaction, including witness data\n"
377+           "    \"fees\" : {\n"
378+           "        \"base\" : n,         (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
379+           "        \"modified\" : n,     (numeric) transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT + "\n"
380+           "        \"ancestor\" : n,     (numeric) modified fees (see above) of in-mempool ancestors (including this one) in " + CURRENCY_UNIT + "\n"
381+           "        \"decendent\" : n,    (numeric) number of in-mempool ancestor transactions (including this one) in " + CURRENCY_UNIT + "\n"

meshcollider commented at 2:32 pm on January 23, 2018:

“decendent” -> “descendant”

meshcollider commented at 2:33 pm on January 23, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/12240/commits/ac168a6b3b3393c24a7b42f3113483c543ec9ceb

mryandao force-pushed on Jan 23, 2018

mryandao renamed this:
~~changed fee to be in sats instead of btc~~
changed fee to be in btcs instead of sats
on Jan 23, 2018

mryandao force-pushed on Jan 23, 2018

promag commented at 0:04 am on January 24, 2018: member

utACK ac168a6.

Nit, please cleanup the commit body.

promag commented at 8:49 am on January 24, 2018: member

The commit contains some fixups leftovers from the squash. Edit the commit message git commit --amend and then git push -f ....

mryandao force-pushed on Jan 24, 2018

mryandao commented at 12:55 pm on January 24, 2018: contributor

@promag, thanks for clarifying. I’ve updated the commit message.

randolf commented at 4:29 pm on January 31, 2018: contributor

I suggest changing the title of this Pull Request from “changed fee to be in btcs instead of sats” to:

[Docs] For consistency, changed Satoshi fee denominations to BTC

mryandao renamed this:
~~changed fee to be in btcs instead of sats~~
[Docs] For consistency, changed Satoshi fee denominations to BTC
on Jan 31, 2018

mryandao renamed this:
~~[Docs] For consistency, changed Satoshi fee denominations to BTC~~
[rpc] For consistency, changed Satoshi fee denominations to BTC
on Jan 31, 2018

mryandao commented at 6:57 pm on January 31, 2018: contributor

@randolf actually, [rpc] would be more fitting.

MarcoFalke commented at 10:04 pm on January 31, 2018: member

Jup, seems correctly tagged. But needs rebase, since the release notes were cleared on master.

MarcoFalke commented at 10:41 am on February 2, 2018: member

Usually we solve conflicts by rebase (this avoids a merge commit). You can do something along the lines of

0git checkout fix-getrawmempool-fee-representation
1git reset --soft 895fbd768f0c89cea3f78acac58b233d4e3a145e
2git commit -m "Add new fee structure with all sub-fields denominated in BTC"
3git push origin checkout fix-getrawmempool-fee-representation -f

laanwj commented at 3:53 pm on February 8, 2018: member

utACK, after getting rid of the merge commit in the branch.

mryandao force-pushed on Feb 20, 2018

mryandao commented at 0:10 am on February 21, 2018: contributor

rebased, pending review.

in src/rpc/blockchain.cpp:377 in 5763828cc2 outdated

376            "    \"wtxid\" : hash,         (string) hash of serialized transaction, including witness data\n"
377+           "    \"fees\" : {\n"
378+           "        \"base\" : n,         (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
379+           "        \"modified\" : n,     (numeric) transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT + "\n"
380+           "        \"ancestor\" : n,     (numeric) modified fees (see above) of in-mempool ancestors (including this one) in " + CURRENCY_UNIT + "\n"
381+           "        \"descendent\" : n,    (numeric) number of in-mempool ancestor transactions (including this one) in " + CURRENCY_UNIT + "\n"

promag commented at 0:36 am on February 21, 2018:

Nit, bad alignment?

in src/rpc/blockchain.cpp:362 in 5763828cc2 outdated

358@@ -359,17 +359,23 @@ UniValue getdifficulty(const JSONRPCRequest& request)
359 std::string EntryDescriptionString()
360 {
361     return "    \"size\" : n,             (numeric) virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.\n"
362-           "    \"fee\" : n,              (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
363-           "    \"modifiedfee\" : n,      (numeric) transaction fee with fee deltas used for mining priority\n"
364+           "    \"fee\" : n,              (numeric) transaction fee in " + CURRENCY_UNIT + " (DEPRECATED) \n"

promag commented at 0:38 am on February 21, 2018:

Nit, remove space between ) and \n. Same below.

promag commented at 0:47 am on February 21, 2018:

This also affects getmempoolancestors, getmempooldescendants and getmempoolentry. Update respective tests and release notes?

mryandao commented at 10:47 am on February 21, 2018:

I’m not sure what needs to be updated in the tests and release notes for the above RPC commands as there’s an assert_equal check against mempool dict for all 3 RPC commands.

in doc/release-notes.md:65 in 5763828cc2 outdated

61@@ -62,6 +62,9 @@ RPC changes
62 ### Low-level changes
63 
64 - The `fundrawtransaction` rpc will reject the previously deprecated `reserveChangeKey` option.
65+- New `fee` field introduced in `getrawmempool` when verbosity is set to `true` with sub-fields

promag commented at 0:44 am on February 21, 2018:

Should be fees, not fee.

in src/rpc/blockchain.cpp:392 in 5763828cc2 outdated

384@@ -379,6 +385,12 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
385 {
386     AssertLockHeld(mempool.cs);
387 
388+    UniValue fees(UniValue::VOBJ);

promag commented at 0:45 am on February 21, 2018:

Not used?

mryandao commented at 10:49 am on February 21, 2018:

Sorry, its now being used in info Patched incorrectly when I had to rebase the 2nd time.

in test/functional/mempool_packages.py:69 in 5763828cc2 outdated

65@@ -66,7 +66,10 @@ def run_test(self):
66             assert_equal(mempool[x]['descendantcount'], descendant_count)
67             descendant_fees += mempool[x]['fee']
68             assert_equal(mempool[x]['modifiedfee'], mempool[x]['fee'])
69+            assert_equal(mempool[x]['fees']['modified'], mempool[x]['fee'])

promag commented at 0:48 am on February 21, 2018:

fees.base = fee

promag commented at 0:53 am on February 21, 2018:

How did this passed in travis since fees is not in the response?

promag commented at 1:05 am on February 21, 2018:

Ah I think for push/pr events travis doesn’t run --extended tests. Currently running this test gives:

0$ test/functional/mempool_packages.py
1...
22018-02-21 01:00:44.877000 TestFramework (ERROR): Key error
3Traceback (most recent call last):
4  File "/Users/promag/bitcoin/test/functional/test_framework/test_framework.py", line 125, in main
5    self.run_test()
6  File "test/functional/mempool_packages.py", line 69, in run_test
7    assert_equal(mempool[x]['fees']['modified'], mempool[x]['fee'])
8KeyError: 'fees'

promag commented at 0:49 am on February 21, 2018: member

PR title should be “fixed”, nothing is changed. PR description could be improved, something similar to the new release note.

mryandao force-pushed on Feb 21, 2018

in src/rpc/blockchain.cpp:368 in b6ceb091bf outdated

366            "    \"time\" : n,             (numeric) local time transaction entered pool in seconds since 1 Jan 1970 GMT\n"
367            "    \"height\" : n,           (numeric) block height when transaction entered pool\n"
368            "    \"descendantcount\" : n,  (numeric) number of in-mempool descendant transactions (including this one)\n"
369            "    \"descendantsize\" : n,   (numeric) virtual transaction size of in-mempool descendants (including this one)\n"
370-           "    \"descendantfees\" : n,   (numeric) modified fees (see above) of in-mempool descendants (including this one)\n"
371+           "    \"descendantfees\" : n,   (numeric) modified fees (see above) of in-mempool descendants (including this one) (DEPRECATED) \n"

promag commented at 11:01 am on February 21, 2018:

Remove last space.

in src/rpc/blockchain.cpp:371 in b6ceb091bf outdated

370-           "    \"descendantfees\" : n,   (numeric) modified fees (see above) of in-mempool descendants (including this one)\n"
371+           "    \"descendantfees\" : n,   (numeric) modified fees (see above) of in-mempool descendants (including this one) (DEPRECATED) \n"
372            "    \"ancestorcount\" : n,    (numeric) number of in-mempool ancestor transactions (including this one)\n"
373            "    \"ancestorsize\" : n,     (numeric) virtual transaction size of in-mempool ancestors (including this one)\n"
374-           "    \"ancestorfees\" : n,     (numeric) modified fees (see above) of in-mempool ancestors (including this one)\n"
375+           "    \"ancestorfees\" : n,     (numeric) modified fees (see above) of in-mempool ancestors (including this one) (DEPRECATED) \n"

promag commented at 11:01 am on February 21, 2018:

Remove last space.

promag commented at 11:05 am on February 21, 2018: member

Thread #12240 (review)

I’ll have to look those tests then, at least the release note must be updated.

mryandao force-pushed on Feb 21, 2018

mryandao renamed this:
~~[rpc] For consistency, changed Satoshi fee denominations to BTC~~
[rpc] Introduced a new `fee` structure that aggregates all sub-field fee types denominated in BTC
on Feb 21, 2018

mryandao renamed this:
~~[rpc] Introduced a new `fee` structure that aggregates all sub-field fee types denominated in BTC~~
[rpc] Introduced a new `fees` structure that aggregates all sub-field fee types denominated in BTC
on Feb 21, 2018

mryandao commented at 9:35 pm on February 21, 2018: contributor

@promag, I’ve updated the release note.

in doc/release-notes.md:74 in 450ec6eed9 outdated

61@@ -62,6 +62,10 @@ RPC changes
62 ### Low-level changes
63 
64 - The `fundrawtransaction` rpc will reject the previously deprecated `reserveChangeKey` option.
65+- New `fees` field introduced in `getrawmempool`, `getmempoolancestors`, `getmempooldescendants` and
66+   `getmempoolentry` when verbosity is set to `true` with sub-fields `ancestor`, `base`, `modified`

promag commented at 9:50 pm on February 21, 2018:

Not sure what when verbosity is set to `true` means.

mryandao commented at 9:56 pm on February 21, 2018:

how about if I change the wording to “(when verbosity=true argument is supplied)”?

jnewbery commented at 9:48 pm on April 3, 2018: member

another release-notes.md conflict. Sorry - needs rebase again :(

mryandao force-pushed on Apr 5, 2018

mryandao commented at 8:46 am on April 5, 2018: contributor

@jnewbery done :)

promag commented at 9:56 am on April 5, 2018: member

utACK 1426d930.

mryandao force-pushed on Apr 12, 2018

mryandao force-pushed on Apr 18, 2018

Add new fee structure with all sub-fields denominated in BTC 7de1de7da4

promag commented at 10:29 am on April 26, 2018: member

utACK 7de1de7. Only change is rebase due to release notes conflicts.

laanwj merged this on Apr 26, 2018

laanwj closed this on Apr 26, 2018

laanwj referenced this in commit 6f8b3453f8 on Apr 26, 2018

in src/rpc/blockchain.cpp:378 in 7de1de7da4

377            "    \"wtxid\" : hash,         (string) hash of serialized transaction, including witness data\n"
378+           "    \"fees\" : {\n"
379+           "        \"base\" : n,         (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
380+           "        \"modified\" : n,     (numeric) transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT + "\n"
381+           "        \"ancestor\" : n,     (numeric) modified fees (see above) of in-mempool ancestors (including this one) in " + CURRENCY_UNIT + "\n"
382+           "        \"descendent\" : n,   (numeric) number of in-mempool ancestor transactions (including this one) in " + CURRENCY_UNIT + "\n"

MarcoFalke commented at 11:50 am on April 26, 2018:

Wrong description?

mryandao commented at 10:35 pm on April 26, 2018:

good pick up. for the descendent field, should do a s/ancestor/descendent/. Do I make another PR to fix the commit merged into Master?

MarcoFalke commented at 12:22 pm on April 27, 2018:

@mryandao Yes, it is only possible through a new pr.

mryandao commented at 7:02 am on April 28, 2018:

#13109 fixes the typo

mryandao deleted the branch on Apr 26, 2018

mryandao restored the branch on Apr 26, 2018

mryandao deleted the branch on Apr 26, 2018

mryandao restored the branch on Apr 26, 2018

laanwj referenced this in commit 219970524d on Apr 30, 2018

jasonbcox referenced this in commit a6851819dd on Oct 11, 2019

jonspock referenced this in commit 703ea346f6 on Dec 27, 2019

jonspock referenced this in commit 27aa16c638 on Dec 29, 2019

PastaPastaPasta referenced this in commit 666f6459cf on Dec 16, 2020

PastaPastaPasta referenced this in commit be992718f8 on Dec 18, 2020

UdjinM6 referenced this in commit 616549fd02 on May 21, 2021

UdjinM6 referenced this in commit 9a6855a050 on May 25, 2021

TheArbitrator referenced this in commit 957457f8a7 on Jun 4, 2021

MarcoFalke locked this on Sep 8, 2021

[rpc] Introduced a new fees structure that aggregates all sub-field fee types denominated in BTC #12240

[rpc] Introduced a new `fees` structure that aggregates all sub-field fee types denominated in BTC #12240