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

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

Merged
merged 1 commit into from Apr 26, 2018

Conversation

Projects
None yet
8 participants
@mryandao
Contributor

mryandao commented Jan 22, 2018

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

@laanwj

This comment has been minimized.

Member

laanwj commented Jan 22, 2018

  • 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

This comment has been minimized.

Member

promag commented Jan 22, 2018

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

This comment has been minimized.

Member

promag commented Jan 22, 2018

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

Missing fees.modified?

info.push_back(Pair("size", (int)e.GetTxSize()));
info.push_back(Pair("fee", e.GetFee()));
info.push_back(Pair("modifiedfee", e.GetModifiedFee()));
info.push_back(Pair("fee", ValueFromAmount(e.GetFee())));

This comment has been minimized.

@promag

promag Jan 22, 2018

Member

Revert.

info.push_back(Pair("fee", e.GetFee()));
info.push_back(Pair("modifiedfee", e.GetModifiedFee()));
info.push_back(Pair("fee", ValueFromAmount(e.GetFee())));
info.push_back(Pair("modifiedfee", ValueFromAmount(e.GetModifiedFee())));

This comment has been minimized.

@promag

promag Jan 22, 2018

Member

Revert.

info.push_back(Pair("time", e.GetTime()));
info.push_back(Pair("height", (int)e.GetHeight()));
info.push_back(Pair("descendantcount", e.GetCountWithDescendants()));
info.push_back(Pair("descendantsize", e.GetSizeWithDescendants()));
info.push_back(Pair("descendantfees", e.GetModFeesWithDescendants()));
info.push_back(Pair("descendantfees", ValueFromAmount(e.GetModFeesWithDescendants())));

This comment has been minimized.

@promag

promag Jan 22, 2018

Member

Revert.

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

This comment has been minimized.

@promag

promag Jan 22, 2018

Member

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

@MeshCollider

This comment has been minimized.

Member

MeshCollider commented Jan 22, 2018

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

@promag

This comment has been minimized.

Member

promag commented Jan 22, 2018

FYI

std::string EntryDescriptionString()
{
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"
" \"fee\" : n, (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
" \"modifiedfee\" : n, (numeric) transaction fee with fee deltas used for mining priority\n"
" \"time\" : n, (numeric) local time transaction entered pool in seconds since 1 Jan 1970 GMT\n"
" \"height\" : n, (numeric) block height when transaction entered pool\n"
" \"descendantcount\" : n, (numeric) number of in-mempool descendant transactions (including this one)\n"
" \"descendantsize\" : n, (numeric) virtual transaction size of in-mempool descendants (including this one)\n"
" \"descendantfees\" : n, (numeric) modified fees (see above) of in-mempool descendants (including this one)\n"
" \"ancestorcount\" : n, (numeric) number of in-mempool ancestor transactions (including this one)\n"
" \"ancestorsize\" : n, (numeric) virtual transaction size of in-mempool ancestors (including this one)\n"
" \"ancestorfees\" : n, (numeric) modified fees (see above) of in-mempool ancestors (including this one)\n"
" \"wtxid\" : hash, (string) hash of serialized transaction, including witness data\n"
" \"depends\" : [ (array) unconfirmed transactions used as inputs for this transaction\n"
" \"transactionid\", (string) parent transaction id\n"
" ... ]\n";
}

@mryandao

This comment has been minimized.

Contributor

mryandao commented Jan 23, 2018

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

@MarcoFalke

This comment has been minimized.

Member

MarcoFalke commented Jan 23, 2018

@promag

Please see my comment above regarding tests. For instance, these (and others) could be updated:

assert_equal(mempool[x]['modifiedfee'], mempool[x]['fee'])

assert_equal(mempool[x]['ancestorfees'], ancestor_fees * COIN + 1000)

assert_equal(mempool[x]['modifiedfee'], mempool[x]['fee']+satoshi_round(0.00002))

" \"wtxid\" : hash, (string) hash of serialized transaction, including witness data\n"
" \"fees\" : {\n"
" \"base\" : n, (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
" \"modifiedfee\" : n, (numeric) transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT + "\n"

This comment has been minimized.

@promag

promag Jan 23, 2018

Member

Just modified?

This comment has been minimized.

@mryandao

mryandao Jan 23, 2018

Contributor

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

This comment has been minimized.

@promag

promag Jan 23, 2018

Member

IMO current tests should stay. Just add more.

@promag

This comment has been minimized.

Member

promag commented Jan 23, 2018

Care to update doc/release-notes.md?

" \"base\" : n, (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
" \"modified\" : n, (numeric) transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT + "\n"
" \"ancestor\" : n, (numeric) modified fees (see above) of in-mempool ancestors (including this one) in " + CURRENCY_UNIT + "\n"
" \"decendent\" : n, (numeric) number of in-mempool ancestor transactions (including this one) in " + CURRENCY_UNIT + "\n"

This comment has been minimized.

@MeshCollider

MeshCollider Jan 23, 2018

Member

"decendent" -> "descendant"

@MeshCollider

This comment has been minimized.

Member

MeshCollider commented Jan 23, 2018

utACK ac168a6

@mryandao mryandao changed the title from changed fee to be in sats instead of btc to changed fee to be in btcs instead of sats Jan 23, 2018

@promag

This comment has been minimized.

Member

promag commented Jan 24, 2018

utACK ac168a6.

Nit, please cleanup the commit body.

@promag

This comment has been minimized.

Member

promag commented Jan 24, 2018

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

@mryandao

This comment has been minimized.

Contributor

mryandao commented Jan 24, 2018

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

@randolf

This comment has been minimized.

Contributor

randolf commented Jan 31, 2018

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 mryandao changed the title from changed fee to be in btcs instead of sats to [Docs] For consistency, changed Satoshi fee denominations to BTC Jan 31, 2018

@mryandao mryandao changed the title from [Docs] For consistency, changed Satoshi fee denominations to BTC to [rpc] For consistency, changed Satoshi fee denominations to BTC Jan 31, 2018

@mryandao

This comment has been minimized.

Contributor

mryandao commented Jan 31, 2018

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

@MarcoFalke

This comment has been minimized.

Member

MarcoFalke commented Jan 31, 2018

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

@MarcoFalke

This comment has been minimized.

Member

MarcoFalke commented Feb 2, 2018

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
@laanwj

This comment has been minimized.

Member

laanwj commented Feb 8, 2018

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

@mryandao

This comment has been minimized.

Contributor

mryandao commented Feb 21, 2018

rebased, pending review.

@promag

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

" \"base\" : n, (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
" \"modified\" : n, (numeric) transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT + "\n"
" \"ancestor\" : n, (numeric) modified fees (see above) of in-mempool ancestors (including this one) in " + CURRENCY_UNIT + "\n"
" \"descendent\" : n, (numeric) number of in-mempool ancestor transactions (including this one) in " + CURRENCY_UNIT + "\n"

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

Nit, bad alignment?

@@ -359,17 +359,23 @@ UniValue getdifficulty(const JSONRPCRequest& request)
std::string EntryDescriptionString()
{
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"
" \"fee\" : n, (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
" \"modifiedfee\" : n, (numeric) transaction fee with fee deltas used for mining priority\n"
" \"fee\" : n, (numeric) transaction fee in " + CURRENCY_UNIT + " (DEPRECATED) \n"

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

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

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

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

This comment has been minimized.

@mryandao

mryandao Feb 21, 2018

Contributor

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.

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

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

Should be fees, not fee.

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

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

Not used?

This comment has been minimized.

@mryandao

mryandao Feb 21, 2018

Contributor

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

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

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

fees.base = fee

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

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

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

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'
@promag

Thread #12240 (comment)

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

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

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

Remove last space.

" \"ancestorcount\" : n, (numeric) number of in-mempool ancestor transactions (including this one)\n"
" \"ancestorsize\" : n, (numeric) virtual transaction size of in-mempool ancestors (including this one)\n"
" \"ancestorfees\" : n, (numeric) modified fees (see above) of in-mempool ancestors (including this one)\n"
" \"ancestorfees\" : n, (numeric) modified fees (see above) of in-mempool ancestors (including this one) (DEPRECATED) \n"

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

Remove last space.

@mryandao mryandao changed the title from [rpc] For consistency, changed Satoshi fee denominations to BTC to [rpc] Introduced a new `fee` structure that aggregates all sub-field fee types denominated in BTC Feb 21, 2018

@mryandao mryandao changed the title from [rpc] Introduced a new `fee` structure that aggregates all sub-field fee types denominated in BTC to [rpc] Introduced a new `fees` structure that aggregates all sub-field fee types denominated in BTC Feb 21, 2018

@mryandao

This comment has been minimized.

Contributor

mryandao commented Feb 21, 2018

@promag, I've updated the release note.

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

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

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

This comment has been minimized.

@mryandao

mryandao Feb 21, 2018

Contributor

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

@jnewbery

This comment has been minimized.

Member

jnewbery commented Apr 3, 2018

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

@mryandao

This comment has been minimized.

Contributor

mryandao commented Apr 5, 2018

@jnewbery done :)

@promag

This comment has been minimized.

Member

promag commented Apr 26, 2018

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

@laanwj laanwj merged commit 7de1de7 into bitcoin:master Apr 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 26, 2018

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

7de1de7 Add new fee structure with all sub-fields denominated in BTC (mryandao)

Pull request description:

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

Tree-SHA512: e428f6dca1d339f89ab73e38ce5903f5465c46b159069d9bcc3f8b1140fe6657fa49a11abe0088e9f7ba9999f64af72a349a4735bf5eaa61b8e4a185b23543f3
" \"base\" : n, (numeric) transaction fee in " + CURRENCY_UNIT + "\n"
" \"modified\" : n, (numeric) transaction fee with fee deltas used for mining priority in " + CURRENCY_UNIT + "\n"
" \"ancestor\" : n, (numeric) modified fees (see above) of in-mempool ancestors (including this one) in " + CURRENCY_UNIT + "\n"
" \"descendent\" : n, (numeric) number of in-mempool ancestor transactions (including this one) in " + CURRENCY_UNIT + "\n"

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 26, 2018

Member

Wrong description?

This comment has been minimized.

@mryandao

mryandao Apr 26, 2018

Contributor

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?

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 27, 2018

Member

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

This comment has been minimized.

@mryandao

mryandao Apr 28, 2018

Contributor

#13109 fixes the typo

@mryandao mryandao deleted the mryandao:fix-getrawmempool-fee-representation branch Apr 26, 2018

@mryandao mryandao restored the mryandao:fix-getrawmempool-fee-representation branch Apr 26, 2018

@mryandao mryandao deleted the mryandao:fix-getrawmempool-fee-representation branch Apr 26, 2018

@mryandao mryandao restored the mryandao:fix-getrawmempool-fee-representation branch Apr 26, 2018

laanwj added a commit that referenced this pull request Apr 30, 2018

Merge #13109: [rpc] nit: fix typo for entry description string
f7c414d nit: fix typo for entry description string (mryandao)

Pull request description:

  #12240

Tree-SHA512: 2940c383069912b04d6fdbc1f0834970cae8ed725beb606916ee27501d8f6e1b3938647babb26137440166de3a9ac745048e306696d13817775eb406adb31f4a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment