RPC: Add ancestor{count,size,fees} to listunspent output #12677

luke-jr commented at 6:01 PM on March 12, 2018: member

Requested by a user

fanquake added the label RPC/REST/ZMQ on Mar 12, 2018

in src/wallet/rpcwallet.cpp:2911 in 3815e1bc5c outdated

3059 | @@ -3056,6 +3060,15 @@ UniValue listunspent(const JSONRPCRequest& request)
3060 |          entry.pushKV("scriptPubKey", HexStr(scriptPubKey.begin(), scriptPubKey.end()));
3061 |          entry.pushKV("amount", ValueFromAmount(out.tx->tx->vout[out.i].nValue));
3062 |          entry.pushKV("confirmations", out.nDepth);
3063 | +        if (out.nDepth) {
3064 | +            LOCK(mempool.cs);
3065 | +            const auto it = mempool.mapTx.find(out.tx->GetHash());

promag commented at 11:47 PM on March 12, 2018:

Not sure if this is correct, if out.nDepth > 0 then out.tx should not be in the mempool right? Or should the condition be out.nDepth == 0?

promag commented at 11:49 PM on March 12, 2018: member

Could add tests for new fields. Also, help message could state when these are in the response.

in src/wallet/rpcwallet.cpp:3061 in 3815e1bc5c outdated

3066 | +            if (it != mempool.mapTx.end()) {
3067 | +                entry.pushKV("ancestorcount", it->GetCountWithAncestors());
3068 | +                entry.pushKV("ancestorsize", it->GetSizeWithAncestors());
3069 | +                entry.pushKV("ancestorfees", it->GetModFeesWithAncestors());
3070 | +            }
3071 | +        }

conscott commented at 5:13 AM on March 13, 2018:

Should this also add the no ancestor case?

{
    ...
    "ancestorcount": 0,
    "ancestorsize": 0,
    "ancestorfees": 0,
    ....
}

If the aim is to avoid adding unnecessary size to responses, then the help message above might mention these fields will only print if ancestors exist.

luke-jr commented at 2:54 PM on October 18, 2019:

ancestor* fields always include the transaction itself

adrianh87 commented at 2:00 PM on March 13, 2018: none

Thank you Luke for taking it into consideration and adding it !

meshcollider commented at 8:53 PM on March 13, 2018: contributor

Concept ACK, perhaps it'd be cleaner to sub-object them like you put in the title of the PR

sipa commented at 12:39 AM on March 17, 2018: member

Concept ACK

luke-jr force-pushed on Mar 31, 2018

promag commented at 8:15 AM on May 3, 2018: member

@luke-jr is this still relevant? There are a couple of suggestions above and it's missing test(s) update.

achow101 commented at 10:21 PM on July 9, 2018: member

utACK daeb431011eefb35b9c76c0b1072d44ac40fe2a6

MarcoFalke commented at 5:30 PM on July 10, 2018: member

@promag Indeed, needs a trivial test where they are all different from 0

DrahtBot closed this on Jul 21, 2018

DrahtBot reopened this on Jul 21, 2018

DrahtBot commented at 11:07 AM on October 20, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22798 (doc: Fix RPC result documentation by MarcoFalke)
#22689 (rpc: deprecate top-level fee fields in getmempool RPCs by josibake)

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.

DrahtBot added the label Needs rebase on Nov 5, 2018

MarcoFalke deleted a comment on Feb 7, 2019

luke-jr commented at 1:48 PM on February 12, 2019: member

Rebased and added tests

luke-jr force-pushed on Feb 12, 2019

DrahtBot removed the label Needs rebase on Feb 12, 2019

DrahtBot added the label Needs rebase on Feb 14, 2019

luke-jr commented at 4:29 PM on April 6, 2019: member

Rebased again

luke-jr force-pushed on Apr 6, 2019

DrahtBot added the label Up for grabs on Sep 30, 2019

DrahtBot commented at 1:02 PM on September 30, 2019: member

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.

DrahtBot closed this on Sep 30, 2019

MarcoFalke added the label good first issue on Sep 30, 2019

sipa commented at 3:32 AM on October 18, 2019: member

Concept ACK

meshcollider reopened this on Oct 18, 2019

luke-jr force-pushed on Oct 18, 2019

DrahtBot removed the label Needs rebase on Oct 18, 2019

MarcoFalke removed the label Up for grabs on Oct 18, 2019

MarcoFalke removed the label good first issue on Oct 18, 2019

in src/wallet/rpcwallet.cpp:2829 in 1174b8e54b outdated

2823 | @@ -2823,6 +2824,9 @@ static UniValue listunspent(const JSONRPCRequest& request)
2824 |              "    \"scriptPubKey\" : \"key\",   (string) the script key\n"
2825 |              "    \"amount\" : x.xxx,         (numeric) the transaction output amount in " + CURRENCY_UNIT + "\n"
2826 |              "    \"confirmations\" : n,      (numeric) The number of confirmations\n"
2827 | +            "    \"ancestorcount\" : n,      (numeric) The number of unconfirmed ancestors if spent\n"
2828 | +            "    \"ancestorsize\" : n,       (numeric) The virtual transaction size of all unconfirmed ancestors\n"
2829 | +            "    \"ancestorfees\" : n,       (numeric) The modified fees of all unconfirmed ancestors\n"

MarcoFalke commented at 12:35 PM on October 18, 2019:

They are only present (optional) when unconfirmed? Should mention that, or make then unconditional.

luke-jr commented at 9:12 PM on October 18, 2019:

Fixed

in test/functional/mempool_packages.py:96 in 1174b8e54b outdated

  74 | @@ -64,9 +75,9 @@ def run_test(self):
  75 |          descendant_fees = 0
  76 |          descendant_vsize = 0
  77 |  
  78 | -        ancestor_vsize = sum([mempool[tx]['vsize'] for tx in mempool])
  79 | +        assert_equal(ancestor_vsize, sum([mempool[tx]['vsize'] for tx in mempool]))
  80 |          ancestor_count = MAX_ANCESTORS
  81 | -        ancestor_fees = sum([mempool[tx]['fee'] for tx in mempool])
  82 | +        assert_equal(ancestor_fees, sum([mempool[tx]['fee'] for tx in mempool]))

MarcoFalke commented at 12:37 PM on October 18, 2019:

I don't think you can replace an assignment with assert_equal

MarcoFalke commented at 12:38 PM on October 18, 2019:

This will result in a run time error

luke-jr commented at 2:54 PM on October 18, 2019:

It's calculated above: https://github.com/bitcoin/bitcoin/pull/12677/files#diff-a01d25e7f69f4199a300a53bb4ff9957R56

(but yes, I need to update the size one to vsize...)

luke-jr commented at 9:05 PM on October 18, 2019:

Fixed

luke-jr force-pushed on Oct 18, 2019

luke-jr force-pushed on Oct 19, 2019

DrahtBot added the label Needs rebase on Mar 4, 2020

luke-jr force-pushed on Mar 31, 2020

DrahtBot removed the label Needs rebase on Mar 31, 2020

MarcoFalke commented at 5:07 PM on May 7, 2020: member

ACK 911d940f14b3e05decf8bb8001fb133a0647b782

DrahtBot added the label Needs rebase on May 22, 2020

instagibbs commented at 12:50 AM on May 22, 2020: member

Promise to review this if it gets rebased

MarcoFalke commented at 1:08 AM on May 22, 2020: member

Promise to review this if it gets rebased

Same

MarcoFalke commented at 2:33 PM on June 8, 2020: member

@luke-jr You still working on this or is this up for grabs?

luke-jr force-pushed on Jun 18, 2020

DrahtBot removed the label Needs rebase on Jun 18, 2020

luke-jr commented at 9:17 PM on June 18, 2020: member

Rebased

in test/functional/mempool_packages.py:86 in 2299b1fb78 outdated

  81 | +            this_unspent = next(i for i in wallet_unspent if i['txid'] == txid)
  82 | +            assert_equal(this_unspent['ancestorcount'], i + 1)
  83 | +            ancestor_vsize += self.nodes[0].getrawtransaction(txid=txid, verbose=True)['vsize']
  84 | +            assert_equal(this_unspent['ancestorsize'], ancestor_vsize)
  85 | +            ancestor_fees -= self.nodes[0].gettransaction(txid=txid)['fee']
  86 | +            assert_equal(this_unspent['ancestorfees'], ancestor_fees * 100000000)

jonatack commented at 12:44 PM on June 19, 2020:

2299b1f

            assert_equal(this_unspent['ancestorfees'], ancestor_fees * COIN)

in src/wallet/rpcwallet.cpp:2750 in 2299b1fb78 outdated

2744 | @@ -2744,6 +2745,9 @@ static UniValue listunspent(const JSONRPCRequest& request)
2745 |                              {RPCResult::Type::STR, "scriptPubKey", "the script key"},
2746 |                              {RPCResult::Type::STR_AMOUNT, "amount", "the transaction output amount in " + CURRENCY_UNIT},
2747 |                              {RPCResult::Type::NUM, "confirmations", "The number of confirmations"},
2748 | +                            {RPCResult::Type::NUM, "ancestorcount", "If transaction is in the mempool, the number of in-mempool ancestor transactions (including this one)"},
2749 | +                            {RPCResult::Type::NUM, "ancestorsize", "If transaction is in the mempool, the virtual transaction size of in-mempool ancestors (including this one)"},
2750 | +                            {RPCResult::Type::STR_AMOUNT, "ancestorfees", "If transaction is in the mempool, total fees of in-mempool ancestors (including this one) with fee deltas used for mining priority"},

jonatack commented at 12:49 PM on June 19, 2020:

f65a341 nit: s/total fees/the total fees/

while touching this help, maybe s/the/The/ for the listunspent txid, vout, address, and scriptPubKey fields

in src/interfaces/chain.h:177 in 2299b1fb78 outdated

 179 | @@ -180,7 +180,7 @@ class Chain
 180 |          std::string& err_string) = 0;
 181 |  
 182 |      //! Calculate mempool ancestor and descendant counts for the given transaction.
 183 | -    virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) = 0;
 184 | +    virtual void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize = nullptr, CAmount* ancestorfees = nullptr) = 0;

jonatack commented at 12:58 PM on June 19, 2020:

Is using Optional preferred nowadays for params that can be {}?

instagibbs commented at 1:01 PM on June 19, 2020:

might be a good meeting topic?

MarcoFalke commented at 1:07 PM on June 19, 2020:

I think for optional return values it is preferable to pass by pointer. Otherwise the caller code will read clumsy like

Optional<int> foo;
GetInt(foo);
std::cout << * foo << std::endl;

As opposed to a simple

int foo;
GetInt(&foo);
std::cout << foo << std::endl;

kiminuo commented at 8:55 AM on September 19, 2021:

I wonder whether we should update documentation on L176 too to reflect the change here.

jonatack commented at 12:59 PM on June 19, 2020: member

Light slightly-tested ACK 2299b1fb786bb692aa1374a0c86d4e5b0fee9361. Naming request: in the code and the RPC output, could ancestorsize, ancestorfees, and ancestorcount be snakecased?

in src/wallet/rpcwallet.cpp:3052 in f65a341c96 outdated

2904 | @@ -2901,6 +2905,16 @@ static UniValue listunspent(const JSONRPCRequest& request)
2905 |          entry.pushKV("scriptPubKey", HexStr(scriptPubKey.begin(), scriptPubKey.end()));
2906 |          entry.pushKV("amount", ValueFromAmount(out.tx->tx->vout[out.i].nValue));
2907 |          entry.pushKV("confirmations", out.nDepth);
2908 | +        if (!out.nDepth) {

instagibbs commented at 1:06 PM on June 19, 2020:

AvailableCoins filters negative values but that takes a little code snooping to assure the reader. Can we make it easier on the reader and make this out.nDepth == 0?

luke-jr commented at 7:00 PM on August 9, 2020:

That doesn't change it at all... Negative values would be handled the same.

MarcoFalke commented at 1:11 PM on June 19, 2020: member

re-ACK 2299b1fb78 only change was solved conflict in test due to diff in an adjacent line

in test/functional/mempool_packages.py:72 in 2299b1fb78 outdated

  68 | @@ -69,11 +69,22 @@ def run_test(self):
  69 |          fee = Decimal("0.0001")
  70 |          # MAX_ANCESTORS transactions off a confirmed tx should be fine
  71 |          chain = []
  72 | +        wallet_unspent = self.nodes[0].listunspent(minconf=0)

instagibbs commented at 1:12 PM on June 19, 2020:

add a quick test making sure these fields don't exist when transaction confirmed/not in mempool?

jonatack commented at 1:27 PM on June 19, 2020:

add a quick test making sure these fields don't exist when transaction confirmed/not in mempool?

Good idea

instagibbs approved

instagibbs commented at 1:12 PM on June 19, 2020: member

utACK 2299b1fb786bb692aa1374a0c86d4e5b0fee9361

snake_case the return values would be nice

jonatack commented at 1:29 PM on June 19, 2020: member

Would also need a release note here or as a follow-up.

in src/wallet/rpcwallet.cpp:20 in f65a341c96 outdated

  16 | @@ -17,6 +17,7 @@
  17 |  #include <rpc/util.h>
  18 |  #include <script/descriptor.h>
  19 |  #include <script/sign.h>
  20 | +#include <txmempool.h>

sipa commented at 1:15 AM on July 2, 2020:

Is this include still needed?

promag commented at 11:35 PM on July 5, 2020:

Indeed not needed.

promag commented at 11:43 PM on July 5, 2020: member

Code review ACK, could remove unnecessary include and follow @instagibbs suggestion too. Also tag needs release note.

MarcoFalke added the label Waiting for author on Jul 20, 2020

MarcoFalke commented at 6:19 AM on July 20, 2020: member

@luke-jr Are you still working on this?

DrahtBot added the label Needs rebase on Jul 22, 2020

luke-jr commented at 7:45 PM on August 9, 2020: member

Naming request: in the code and the RPC output, could ancestorsize, ancestorfees, and ancestorcount be snakecased? @jonatack No, we already have these names in the RPC interface as-is (see MempoolEntryDescription).

luke-jr force-pushed on Aug 9, 2020

DrahtBot removed the label Needs rebase on Aug 9, 2020

MarcoFalke removed the label Waiting for author on Aug 10, 2020

DrahtBot added the label Needs rebase on Aug 11, 2020

MarcoFalke commented at 5:00 AM on August 11, 2020: member

Should be good to go in after another rebase

JeremyRubin commented at 5:17 AM on August 11, 2020: contributor

To be annoying -- slight concept nack without more info.

The field should be named mempool_ancestors, as there is a distinct concept of the ancestry of a coin (e.g., max distance to a coinbase txn).
If the desired thing to know is if a transaction can support more descendants, we should return the field as n_descendants_placeable_in_mempool or something. The ancestor stats themselves are insufficient to figure this out.
We can compute 2 against both our own local settings and against the network defaults...

Without a concrete user story in mind, it's hard to see how this information helps a user complete a task. For what purpose would a user use this? @BitcoinGeek

luke-jr force-pushed on Aug 20, 2020

luke-jr commented at 2:45 AM on August 20, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Aug 20, 2020

DrahtBot added the label Needs rebase on Sep 5, 2020

luke-jr force-pushed on Oct 30, 2020

DrahtBot removed the label Needs rebase on Oct 30, 2020

kristapsk approved

kristapsk commented at 10:16 PM on October 30, 2020: contributor

ACK 0db275f2b51169c60c7fecf069a81928c2647678. Looked at code, did automated tests and also manually tested on a testnet.

DrahtBot added the label Needs rebase on Dec 1, 2020

Expose ancestorsize and ancestorfees via getTransactionAncestry 3f77dfdaf0

luke-jr force-pushed on Aug 2, 2021

DrahtBot removed the label Needs rebase on Aug 2, 2021

luke-jr commented at 4:51 PM on August 2, 2021: member

rebased

kristapsk approved

kristapsk commented at 6:09 PM on August 2, 2021: contributor

re-ACK 7c0b6d6919e88d6f9e49cde70dc0713a1e4bdc74

laanwj added this to the "Blockers" column in a project

in src/wallet/rpcwallet.cpp:3057 in 47fdd185f1 outdated

3048 | @@ -3046,6 +3049,16 @@ static RPCHelpMan listunspent()
3049 |          entry.pushKV("scriptPubKey", HexStr(scriptPubKey));
3050 |          entry.pushKV("amount", ValueFromAmount(out.tx->tx->vout[out.i].nValue));
3051 |          entry.pushKV("confirmations", out.nDepth);
3052 | +        if (!out.nDepth) {
3053 | +            size_t ancestor_count, descendant_count, ancestor_size;
3054 | +            CAmount ancestor_fees;
3055 | +            pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees);
3056 | +            if (ancestor_count) {
3057 | +                entry.pushKV("ancestorcount", (uint64_t)ancestor_count);

fjahr commented at 10:54 PM on August 24, 2021:

nit: static_cast<uint64_t>(ancestor_count) would be preferred

in src/wallet/rpcwallet.cpp:2883 in 47fdd185f1 outdated

2879 | @@ -2880,6 +2880,9 @@ static RPCHelpMan listunspent()
2880 |                              {RPCResult::Type::STR, "scriptPubKey", "the script key"},
2881 |                              {RPCResult::Type::STR_AMOUNT, "amount", "the transaction output amount in " + CURRENCY_UNIT},
2882 |                              {RPCResult::Type::NUM, "confirmations", "The number of confirmations"},
2883 | +                            {RPCResult::Type::NUM, "ancestorcount", "If transaction is in the mempool, the number of in-mempool ancestor transactions (including this one)"},

fjahr commented at 11:02 PM on August 24, 2021:

nit: I think there is a slight convention in the existing rpc help to append the conditional at the end, like: "This thing shows XYZ (if available)" (I grepped for available). So I would slightly prefer this and the following to be rewritten something like this:

"The number of in-mempool ancestor transactions, including this one (if the transaction is in the mempool)"

MarcoFalke commented at 8:39 AM on August 26, 2021:

nit: I think there is a slight convention in the existing rpc help

This part of the help is now auto-generated:

                            {RPCResult::Type::NUM, "ancestorcount", /* optional */ true, "If transaction is in the mempool, the number of in-mempool ancestor transactions (including this one)"},

in test/functional/mempool_packages.py:74 in 19cfa2f1ce outdated

  69 | @@ -65,6 +70,14 @@ def run_test(self):
  70 |              witnesstx = self.nodes[0].decoderawtransaction(fulltx, True)
  71 |              witness_chain.append(witnesstx['hash'])
  72 |  
  73 | +            wallet_unspent = self.nodes[0].listunspent(minconf=0)
  74 | +            this_unspent = next(i for i in wallet_unspent if i['txid'] == txid)

fjahr commented at 11:29 PM on August 24, 2021:

This i shadows the i from the loop above.

I think also this could be a bit more efficient by using the address filter of listunspent but it's also not super readable so feel free to ignore:

            filter_addr = witnesstx['vout'][0]['scriptPubKey']['address']
            this_unspent = self.nodes[0].listunspent(minconf=0, addresses=[filter_addr])[0]

Maybe also add a small comment above this block, like "Check that listunspent ancestor{count, size, fees} yield the correct results"

in doc/release-notes-12677.md:7 in 7c0b6d6919 outdated

   0 | @@ -0,0 +1,7 @@
   1 | +Notable changes
   2 | +===============
   3 | +
   4 | +Updated RPCs
   5 | +------------
   6 | +
   7 | +- `listunspent` now includes `ancestorcount`, `ancestorsize`, and `ancestorfees` in some cases.

fjahr commented at 11:32 PM on August 24, 2021:

suggestion: s/in some cases/for each transaction output that is still in the mempool/

fjahr commented at 11:48 PM on August 24, 2021: member

ACK 7c0b6d6919e88d6f9e49cde70dc0713a1e4bdc74

None of my comments is a blocker for me but I would try to re-review as quickly as possible if you decide to address any of them.

in src/txmempool.cpp:1125 in 3f77dfdaf0 outdated

1121 |      LOCK(cs);
1122 |      auto it = mapTx.find(txid);
1123 |      ancestors = descendants = 0;
1124 |      if (it != mapTx.end()) {
1125 |          ancestors = it->GetCountWithAncestors();
1126 | +        if (ancestorsize) *ancestorsize = it->GetSizeWithAncestors();

naumenkogs commented at 12:36 PM on September 3, 2021:

Perhaps a more general question: I was always confused that "ancestors" includes +1 for the transaction itself. Here we're continuing this pattern by including the parent. Perhaps at least we could name new variables something like txAndAncestorsSize?

kiminuo commented at 9:25 AM on September 19, 2021:

(Good point)

naumenkogs commented at 12:50 PM on September 3, 2021: member

utACK 7c0b6d6919e88d6f9e49cde70dc0713a1e4bdc74

in src/wallet/rpcwallet.cpp:3059 in 47fdd185f1 outdated

3054 | +            CAmount ancestor_fees;
3055 | +            pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees);
3056 | +            if (ancestor_count) {
3057 | +                entry.pushKV("ancestorcount", (uint64_t)ancestor_count);
3058 | +                entry.pushKV("ancestorsize", (uint64_t)ancestor_size);
3059 | +                entry.pushKV("ancestorfees", (uint64_t)ancestor_fees);

achow101 commented at 7:12 PM on September 13, 2021:

In 47fdd185f1dca3f0a92b1b67c4523053ed2fc1a3 " RPC: Add ancestor{count,size,fees} to listunspent output"

This will output the fees in satoshis which is highly unintuitive, especially since the documentation does not say so. Everywhere else we have amounts in the RPCs, they are formatted for output with ValueFromAmount which converts them to the correct decimal form for output.

luke-jr commented at 8:03 PM on September 13, 2021:

Not everywhere, no, and the only other location of ancestorfees specifically also uses satoshis.

achow101 commented at 7:10 PM on September 16, 2021:

The docs should at least indicate the units then. In most places the term "fee" is used, the expected unit is BTC, not satoshis. Additionally, the one place that ancestorfees is used is deprecated, and it's replacement uses BTC rather than satoshis. Also just because we do that somewhere else doesn't make it good or not unintuitive.

unknown approved

unknown commented at 3:59 AM on September 14, 2021: none

tACK https://github.com/bitcoin/bitcoin/pull/12677/commits/7c0b6d6919e88d6f9e49cde70dc0713a1e4bdc74

$ 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,

RPC: Add ancestor{count,size,fees} to listunspent output 6966e80f45

QA: Add tests for listunspent ancestor{count,size,fees} to mempool_packages 0be2f17ef5

doc/release-notes: Add new listunspent fields 6cb60f3e6d

luke-jr force-pushed on Sep 16, 2021

luke-jr commented at 8:36 PM on September 16, 2021: member

Addressed various nits, needs re-ACKs

luke-jr requested review from fjahr on Sep 16, 2021

luke-jr requested review from jonatack on Sep 16, 2021

luke-jr requested review from promag on Sep 16, 2021

luke-jr requested review from naumenkogs on Sep 16, 2021

luke-jr requested review from achow101 on Sep 16, 2021

luke-jr requested review from MarcoFalke on Sep 16, 2021

laanwj commented at 9:12 PM on September 16, 2021: member

Diff LGTM https://github.com/bitcoin/bitcoin/compare/7c0b6d6..6cb60f3

achow101 commented at 9:20 PM on September 16, 2021: member

Code Review ACK 6cb60f3e6d652ffa4cf570426a7cf1f690d15c45

naumenkogs approved

naumenkogs commented at 7:41 AM on September 17, 2021: member

ACK 6cb60f3e6d652ffa4cf570426a7cf1f690d15c45

ghost commented at 1:37 PM on September 17, 2021: none

reACK https://github.com/bitcoin/bitcoin/commit/6cb60f3e6d652ffa4cf570426a7cf1f690d15c45

fjahr commented at 9:54 AM on September 18, 2021: member

Code review re-ACK 6cb60f3e6d652ffa4cf570426a7cf1f690d15c45

in src/wallet/rpcwallet.cpp:2885 in 6cb60f3e6d

2879 | @@ -2880,6 +2880,9 @@ static RPCHelpMan listunspent()
2880 |                              {RPCResult::Type::STR, "scriptPubKey", "the script key"},
2881 |                              {RPCResult::Type::STR_AMOUNT, "amount", "the transaction output amount in " + CURRENCY_UNIT},
2882 |                              {RPCResult::Type::NUM, "confirmations", "The number of confirmations"},
2883 | +                            {RPCResult::Type::NUM, "ancestorcount", /* optional */ true, "The number of in-mempool ancestor transactions, including this one (if transaction is in the mempool)"},
2884 | +                            {RPCResult::Type::NUM, "ancestorsize", /* optional */ true, "The virtual transaction size of in-mempool ancestors, including this one (if transaction is in the mempool)"},
2885 | +                            {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)"},

kiminuo commented at 9:14 AM on September 19, 2021:

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
  }
]

in src/wallet/rpcwallet.cpp:2883 in 6cb60f3e6d

2879 | @@ -2880,6 +2880,9 @@ static RPCHelpMan listunspent()
2880 |                              {RPCResult::Type::STR, "scriptPubKey", "the script key"},
2881 |                              {RPCResult::Type::STR_AMOUNT, "amount", "the transaction output amount in " + CURRENCY_UNIT},
2882 |                              {RPCResult::Type::NUM, "confirmations", "The number of confirmations"},
2883 | +                            {RPCResult::Type::NUM, "ancestorcount", /* optional */ true, "The number of in-mempool ancestor transactions, including this one (if transaction is in the mempool)"},

kiminuo commented at 9:23 AM on September 19, 2021:

#22903 makes me wonder whether one should change:

/* optional */ true -> /* optional= */ true

kiminuo commented at 9:45 AM on September 19, 2021: contributor

ACK 6cb60f3

laanwj referenced this in commit 681755350b on Sep 20, 2021

darosior commented at 1:03 PM on September 20, 2021: member

utACK 6cb60f3e6d652ffa4cf570426a7cf1f690d15c45

laanwj merged this on Sep 20, 2021

laanwj closed this on Sep 20, 2021

laanwj removed this from the "Blockers" column in a project

sipa commented at 7:56 PM on September 20, 2021: member

Shouldn't "ancestorfees" use ValueFromAmount here?

in doc/release-notes-12677.md:8 in 6cb60f3e6d

   0 | @@ -0,0 +1,8 @@
   1 | +Notable changes
   2 | +===============
   3 | +
   4 | +Updated RPCs
   5 | +------------
   6 | +
   7 | +- `listunspent` now includes `ancestorcount`, `ancestorsize`, and
   8 | +`ancestorfees` for each transaction output that is still in the mempool.

jonatack commented at 8:00 PM on September 20, 2021:

Missing PR number at the end: s/mempool./mempool. (#12677)

in src/node/interfaces.cpp:577 in 6cb60f3e6d

 573 | @@ -574,11 +574,11 @@ class ChainImpl : public Chain
 574 |          // that Chain clients do not need to know about.
 575 |          return TransactionError::OK == err;
 576 |      }
 577 | -    void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants) override
 578 | +    void getTransactionAncestry(const uint256& txid, size_t& ancestors, size_t& descendants, size_t* ancestorsize, CAmount* ancestorfees) override

jonatack commented at 8:02 PM on September 20, 2021:

function params naming: s/ancestorsize/ancestor_size/, idem for ancestorfees

in src/wallet/rpcwallet.cpp:3059 in 6cb60f3e6d

3054 | +            CAmount ancestor_fees;
3055 | +            pwallet->chain().getTransactionAncestry(out.tx->GetHash(), ancestor_count, descendant_count, &ancestor_size, &ancestor_fees);
3056 | +            if (ancestor_count) {
3057 | +                entry.pushKV("ancestorcount", uint64_t(ancestor_count));
3058 | +                entry.pushKV("ancestorsize", uint64_t(ancestor_size));
3059 | +                entry.pushKV("ancestorfees", uint64_t(ancestor_fees));

jonatack commented at 8:03 PM on September 20, 2021:

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.

jonatack commented at 8:06 PM on September 20, 2021: member

A few quick comments for a follow-up.

sidhujag referenced this in commit ec141043fe on Sep 21, 2021

Fabcien referenced this in commit e3406c8098 on Oct 5, 2022

DrahtBot locked this on Oct 30, 2022