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

pull mryandao wants to merge 1 commits into bitcoin:master from mryandao:fix-getrawmempool-fee-representation changing 3 files +28 −4
  1. 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.

  2. fanquake added the label RPC/REST/ZMQ on Jan 22, 2018
  3. 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?
  4. 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.

  5. promag commented at 11:17 AM on January 22, 2018: member

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

    {
  ...,
  "fees": {
    "base": ...,
    "modified": ...,
    "ancestor": ...,
    "descendant": ...
  },
  ...
}

    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.
  6. 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.

  7. 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.

  8. 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.

  9. 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...

  10. promag commented at 12:01 PM on January 22, 2018: member

    Missing fees.modified?

  11. 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

  13. mryandao commented at 12:22 AM on January 23, 2018: contributor

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

  14. MarcoFalke commented at 12:37 AM on January 23, 2018: member

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

  15. 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 12: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.

  17. mryandao force-pushed on Jan 23, 2018
  18. promag commented at 11:26 AM on January 23, 2018: member

    Care to update doc/release-notes.md?

  19. 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"

  21. mryandao force-pushed on Jan 23, 2018
  22. 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
  23. mryandao force-pushed on Jan 23, 2018
  24. promag commented at 12:04 AM on January 24, 2018: member

    utACK ac168a6.

    Nit, please cleanup the commit body.

  25. 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 ....

  26. mryandao force-pushed on Jan 24, 2018
  27. mryandao commented at 12:55 PM on January 24, 2018: contributor

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

  28. 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

  29. 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
  30. 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
  31. mryandao commented at 6:57 PM on January 31, 2018: contributor

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

  32. 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.

  33. 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

    git checkout fix-getrawmempool-fee-representation
git reset --soft 895fbd768f0c89cea3f78acac58b233d4e3a145e
git commit -m "Add new fee structure with all sub-fields denominated in BTC"
git push origin checkout fix-getrawmempool-fee-representation -f
  34. laanwj commented at 3:53 PM on February 8, 2018: member

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

  35. mryandao force-pushed on Feb 20, 2018
  36. mryandao force-pushed on Feb 20, 2018
  37. mryandao force-pushed on Feb 20, 2018
  38. mryandao force-pushed on Feb 20, 2018
  39. mryandao commented at 12:10 AM on February 21, 2018: contributor

    rebased, pending review.

  40. 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 12:36 AM on February 21, 2018:

    Nit, bad alignment?

  41. 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 12:38 AM on February 21, 2018:

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

    promag commented at 12: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.

  42. 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 12:44 AM on February 21, 2018:

    Should be fees, not fee.

  43. 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 12: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.

  44. 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 12:48 AM on February 21, 2018:

    fees.base = fee

    promag commented at 12: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:

    $ test/functional/mempool_packages.py
...
2018-02-21 01:00:44.877000 TestFramework (ERROR): Key error
Traceback (most recent call last):
  File "/Users/promag/bitcoin/test/functional/test_framework/test_framework.py", line 125, in main
    self.run_test()
  File "test/functional/mempool_packages.py", line 69, in run_test
    assert_equal(mempool[x]['fees']['modified'], mempool[x]['fee'])
KeyError: 'fees'
  45. promag commented at 12: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.

  46. mryandao force-pushed on Feb 21, 2018
  47. 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.

  48. 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.

  49. 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.

  50. mryandao force-pushed on Feb 21, 2018
  51. mryandao force-pushed on Feb 21, 2018
  52. 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
  53. 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
  54. mryandao commented at 9:35 PM on February 21, 2018: contributor

    @promag, I've updated the release note.

  55. 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)"?

  56. jnewbery commented at 9:48 PM on April 3, 2018: member

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

  57. mryandao force-pushed on Apr 5, 2018
  58. mryandao force-pushed on Apr 5, 2018
  59. mryandao force-pushed on Apr 5, 2018
  60. mryandao force-pushed on Apr 5, 2018
  61. mryandao commented at 8:46 AM on April 5, 2018: contributor

    @jnewbery done :)

  62. promag commented at 9:56 AM on April 5, 2018: member

    utACK 1426d930.

  63. mryandao force-pushed on Apr 12, 2018
  64. mryandao force-pushed on Apr 18, 2018
  65. Add new fee structure with all sub-fields denominated in BTC 7de1de7da4
  66. promag commented at 10:29 AM on April 26, 2018: member

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

  67. laanwj merged this on Apr 26, 2018
  68. laanwj closed this on Apr 26, 2018
  69. laanwj referenced this in commit 6f8b3453f8 on Apr 26, 2018
  70. 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

  71. mryandao deleted the branch on Apr 26, 2018
  72. mryandao restored the branch on Apr 26, 2018
  73. mryandao deleted the branch on Apr 26, 2018
  74. mryandao restored the branch on Apr 26, 2018
  75. laanwj referenced this in commit 219970524d on Apr 30, 2018
  76. jasonbcox referenced this in commit a6851819dd on Oct 11, 2019
  77. jonspock referenced this in commit 703ea346f6 on Dec 27, 2019
  78. jonspock referenced this in commit 27aa16c638 on Dec 29, 2019
  79. PastaPastaPasta referenced this in commit 666f6459cf on Dec 16, 2020
  80. PastaPastaPasta referenced this in commit be992718f8 on Dec 18, 2020
  81. UdjinM6 referenced this in commit 616549fd02 on May 21, 2021
  82. UdjinM6 referenced this in commit 9a6855a050 on May 25, 2021
  83. TheArbitrator referenced this in commit 957457f8a7 on Jun 4, 2021
  84. MarcoFalke locked this on Sep 8, 2021
Contributors
mryandaolaanwjpromagmeshcolliderMarcoFalkerandolfjnewbery
Labels
RPC/REST/ZMQ
Linked (view graph)
#13109 [rpc] nit: fix typo for entry description string#22682 properly deprecate mempool entry RPC fee fields
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 00:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me