-
Notifications
You must be signed in to change notification settings - Fork 37.1k
RPC: Add ancestor{count,size,fees} to listunspent output #12677
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add tests for new fields. Also, help message could state when these are in the response.
Thank you Luke for taking it into consideration and adding it ! |
Concept ACK, perhaps it'd be cleaner to sub-object them like you put in the title of the PR |
Concept ACK |
3815e1b
to
daeb431
Compare
@luke-jr is this still relevant? There are a couple of suggestions above and it's missing test(s) update. |
utACK daeb431 |
@promag Indeed, needs a trivial test where they are all different from 0 |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Rebased and added tests |
c401d85
to
18922f8
Compare
Rebased again |
18922f8
to
305ab80
Compare
There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened. |
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 7c0b6d6
$ bitcoin-cli -rpcwallet=W1 listunspent 0
[
{
"txid": "f3c7ab973f3f1857135ae9e9b10da8e85f6482a2a5d45d22d3aa20609e408ef7",
"vout": 1,
"address": "tb1qea2wjf035dm55zmvpwzaagfjvhhg44dycj4dvh",
"label": "",
"scriptPubKey": "0014cf54e925f1a3774a0b6c0b85dea13265ee8ad5a4",
"amount": 0.00100000,
"confirmations": 0,
"ancestorcount": 2,
"ancestorsize": 349,
"ancestorfees": 350,
"spendable": true,
"solvable": true,
"desc": "wpkh([6e03b4ba/0'/0'/4']02d24349d6b5a4eb456d623c998ca3af8f9f60277373e6e59bb7292188c681dc35)#kgxt3tcu",
"safe": false
}
]
"ancestorcount": 2,
"ancestorsize": 349,
"ancestorfees": 350,
7c0b6d6
to
6cb60f3
Compare
Addressed various nits, needs re-ACKs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review ACK 6cb60f3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 6cb60f3
reACK 6cb60f3 |
Code review re-ACK 6cb60f3 |
@@ -2880,6 +2880,9 @@ static RPCHelpMan listunspent() | |||
{RPCResult::Type::STR, "scriptPubKey", "the script key"}, | |||
{RPCResult::Type::STR_AMOUNT, "amount", "the transaction output amount in " + CURRENCY_UNIT}, | |||
{RPCResult::Type::NUM, "confirmations", "The number of confirmations"}, | |||
{RPCResult::Type::NUM, "ancestorcount", /* optional */ true, "The number of in-mempool ancestor transactions, including this one (if transaction is in the mempool)"}, | |||
{RPCResult::Type::NUM, "ancestorsize", /* optional */ true, "The virtual transaction size of in-mempool ancestors, including this one (if transaction is in the mempool)"}, | |||
{RPCResult::Type::STR_AMOUNT, "ancestorfees", /* optional */ true, "The total fees of in-mempool ancestors (including this one) with fee deltas used for mining priority in " + CURRENCY_ATOM + " (if transaction is in the mempool)"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing this, I can see "ancestorfees": 164
in my output. Is it correct to mark the result type as RPCResult::Type::STR_AMOUNT
here? I would probably expect RPCResult::Type::NUM
given that the amount is in satoshis.
More complete output on my machine:
[
{
"txid": "<txid>",
"vout": 1,
"address": "<address>",
"label": "",
"scriptPubKey": "<spk>",
"amount": 0.00100000,
"confirmations": 0,
"ancestorcount": 1,
"ancestorsize": 164,
"ancestorfees": 164,
"spendable": true,
"solvable": true,
"desc": "<desc>",
"safe": false
}
]
@@ -2880,6 +2880,9 @@ static RPCHelpMan listunspent() | |||
{RPCResult::Type::STR, "scriptPubKey", "the script key"}, | |||
{RPCResult::Type::STR_AMOUNT, "amount", "the transaction output amount in " + CURRENCY_UNIT}, | |||
{RPCResult::Type::NUM, "confirmations", "The number of confirmations"}, | |||
{RPCResult::Type::NUM, "ancestorcount", /* optional */ true, "The number of in-mempool ancestor transactions, including this one (if transaction is in the mempool)"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#22903 makes me wonder whether one should change:
/* optional */ true
-> /* optional= */ true
ACK 6cb60f3 |
…t output 6cb60f3 doc/release-notes: Add new listunspent fields (Luke Dashjr) 0be2f17 QA: Add tests for listunspent ancestor{count,size,fees} to mempool_packages (Luke Dashjr) 6966e80 RPC: Add ancestor{count,size,fees} to listunspent output (Luke Dashjr) 3f77dfd Expose ancestorsize and ancestorfees via getTransactionAncestry (Luke Dashjr) Pull request description: Requested by a user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 6cb60f3
Shouldn't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few quick comments for a follow-up.
------------ | ||
|
||
- `listunspent` now includes `ancestorcount`, `ancestorsize`, and | ||
`ancestorfees` for each transaction output that is still in the mempool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing PR number at the end: s/mempool./mempool. (#12677)
@@ -574,11 +574,11 @@ class ChainImpl : public Chain | |||
// that Chain clients do not need to know about. | |||
return TransactionError::OK == err; | |||
} | |||
void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) override | |||
void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize, CAmount* ancestorfees) override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function params naming: s/ancestorsize/ancestor_size/
, idem for ancestorfees
if (ancestor_count) { | ||
entry.pushKV("ancestorcount", uint64_t(ancestor_count)); | ||
entry.pushKV("ancestorsize", uint64_t(ancestor_size)); | ||
entry.pushKV("ancestorfees", uint64_t(ancestor_fees)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three new fields should be snakecase, e.g. s/ancestorcount/ancestor_count/
.
Should they use UniValue ValueFromAmount()
for the amount to print? in which case the units would need to be listed in the help as BTC instead of sat.
…t output 6cb60f3 doc/release-notes: Add new listunspent fields (Luke Dashjr) 0be2f17 QA: Add tests for listunspent ancestor{count,size,fees} to mempool_packages (Luke Dashjr) 6966e80 RPC: Add ancestor{count,size,fees} to listunspent output (Luke Dashjr) 3f77dfd Expose ancestorsize and ancestorfees via getTransactionAncestry (Luke Dashjr) Pull request description: Requested by a user ACKs for top commit: prayank23: reACK bitcoin@6cb60f3 fjahr: Code review re-ACK 6cb60f3 kiminuo: ACK [6cb60f3](bitcoin@6cb60f3) achow101: Code Review ACK 6cb60f3 naumenkogs: ACK 6cb60f3 darosior: utACK 6cb60f3 Tree-SHA512: 5d16e5799558691e5853ab7ea2cc85514cb45da3ce69134d855c71845beef32ec6af5ab28d4462683e9800c8ea126f162773a9d3d5660edac08fd8edbfeda173
Summary: Expose ancestorsize and ancestorfees via getTransactionAncestry Add ancestor{count,size,fees} to listunspent output Add tests and release notes. Note that fees returned by listunspent are in XEC in Bitcoin ABC, not satoshis. Backport of [[bitcoin/bitcoin#12677 | core#12677]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12151
Requested by a user