RPC: Add child transactions to getrawmempool verbose output #12479

conscott commented at 7:44 PM on February 18, 2018: contributor

bitcoin-cli getrawmempool true only lists a transaction's parents in the depends field. This change adds a spentby field to the json response, which lists the transaction's children in the mempool.

Currently the only way to find child transactions is to use getrawmempool or make another call to getmempooldescendants and search the response for transactions that list the parent_txid in the depends list, which is inefficient.

This change allows direct lookup of children.

Example Output

  "9a9b5733c0d89f207908cfa3fe17809bee71f629aa095c9f8754524e29e98ba4": {
    ...other geterawmempool data...
    "wtxid": "9a9b5733c0d89f207908cfa3fe17809bee71f629aa095c9f8754524e29e98ba4",
    "depends": [
      "bdd92851d5766a42aeb62af667bb422a116cab4e032bba5e3dd6efe5b4b40aa0"
    ],
    "spentby": [
      "dc5d3ec388a9121421208738a041ac30a22163bc2e17758f2275b6c51a15ba7b"
    ]
  },

MarcoFalke added the label RPC/REST/ZMQ on Feb 18, 2018

esotericnonsense commented at 10:50 PM on February 18, 2018: contributor

This is cool. I'd thought about adding a similar concept to getblocktemplate (or some sort of package number or something).

Have you given any thought to performance / done any benchmarking? I'm particularly thinking of what happens when the mempool is in the hundreds of megs as it was not too long ago.

in src/rpc/blockchain.cpp:415 in 46b89a2fe6 outdated

 408 | @@ -406,6 +409,24 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
 409 |      }
 410 |  
 411 |      info.pushKV("depends", depends);
 412 | +
 413 | +    std::set<std::string> setSpent;
 414 | +    for (unsigned int i = 0; i < tx.vout.size(); i++) {
 415 | +        auto it = mempool.mapNextTx.find(COutPoint(tx.GetHash(), i));

esotericnonsense commented at 10:50 PM on February 18, 2018:

Does this repeat tx.GetHash() for every vout? Possibly makes sense to do it outside of the loop? (Maybe the compiler is more intelligent than I'm imagining)

promag commented at 10:02 AM on February 19, 2018:

It's fine as is, see CTransaction::GetHash().

dcousens commented at 11:33 PM on February 18, 2018: contributor

NACK, atleast, not by default, this looks like it might increase the cost of this RPC call. (happy to be proven wrong though)

edit: maybe, I initially mis-read this as adding the entirety of each spendby TX, not simply the txid, still, numbers are nice

conscott commented at 12:04 AM on February 19, 2018: contributor

Per suggestion of @dcousens and @esotericnonsense I will perform some timing tests and pull this out into an optional parameter if it's hampering performance significantly.

sipa commented at 12:08 AM on February 19, 2018: member

I looks like it's a single map lookup per reported transaction output, which I don't expect to be all that expensive.

Numbers are better, of course.

dcousens commented at 1:10 AM on February 19, 2018: contributor

revoking my NACK, I initially mis-interpreted the extent of the added data. I'd instead suggest to additionally add the vout which spends the TX, not only the txid.

in src/rpc/blockchain.cpp:420 in 58aa4dc144 outdated

 424 | +    for (const std::string& dep : setSpent)
 425 | +    {
 426 | +        spent.push_back(dep);
 427 | +    }
 428 | +
 429 | +    info.pushKV("spentby", spent);

promag commented at 9:59 AM on February 19, 2018:

In addition to RPC getrawmempool, this also affects getmempoolentry, getmempooldescendants, getmempoolancestors and also REST /rest/mempool/contents. If this is intended then update respective tests to test the new field.

conscott commented at 7:55 PM on February 19, 2018:

Agreed. I have updated the tests to coverage all these cases.

in src/rpc/blockchain.cpp:376 in 58aa4dc144 outdated

 371 | @@ -372,6 +372,9 @@ std::string EntryDescriptionString()
 372 |             "    \"wtxid\" : hash,         (string) hash of serialized transaction, including witness data\n"
 373 |             "    \"depends\" : [           (array) unconfirmed transactions used as inputs for this transaction\n"
 374 |             "        \"transactionid\",    (string) parent transaction id\n"
 375 | +           "       ... ]\n"
 376 | +           "    \"spentby\" : [          (array) unconfirmed transactions spending outputs from this transaction\n"

promag commented at 10:16 AM on February 19, 2018:

Looks like alignment is bad.

promag commented at 10:17 AM on February 19, 2018: member

Concept ACK.

See doc/developer-notes.md regarding style for new code.

conscott force-pushed on Feb 19, 2018

conscott commented at 10:04 PM on February 20, 2018: contributor

Thanks for review @esotericnonsense @promag and @dcousens

I have updated commits from your comments.

It is correct that this just adds a map lookup per transactions, but I went ahead and tested performance as well. This seems to add about 3% in execution time in my particular test.

The details are that I hacked together a test to generate a mempool of 18k transactions with long chains of unconfirmed transactions that excercise child/parent relations for getrawmempool output. Once the mempool is generated there is a 5 minute sleep, so in a loop I could call time bitcoin-cli -datadir=<test_dir> getrawmempool with / without descendant pointers. I recorded the timing and arrived at an average increase in exec time of 3%, with fairly low deviation (in range 1%-8%). I realize this is pretty kludgy, but just wanted to get a rough idea if something really bad was happening. Seems okay. @dcousens I am not sure we want to add the vout of Tx in the spentby list. A child transaction can spend multiple outputs of the same parent, so representing this will add a bit more complexity to the output. This also applies to the depends list, in that the current transaction could be spending multiple outputs for a single parent, and I think we want to keep the spends / depends format about the same. What are your thoughts?

in src/rpc/blockchain.cpp:413 in 465cd73ed9 outdated

 408 | @@ -406,6 +409,26 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
 409 |      }
 410 |  
 411 |      info.pushKV("depends", depends);
 412 | +
 413 | +    std::set<std::string> set_spent;

promag commented at 9:36 PM on February 21, 2018:

std::set<uint256>?

in src/rpc/blockchain.cpp:415 in 465cd73ed9 outdated

 408 | @@ -406,6 +409,26 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
 409 |      }
 410 |  
 411 |      info.pushKV("depends", depends);
 412 | +
 413 | +    std::set<std::string> set_spent;
 414 | +    for (unsigned int i = 0; i < tx.vout.size(); i++) {
 415 | +        auto it = mempool.mapNextTx.find(COutPoint(tx.GetHash(), i));

promag commented at 9:43 PM on February 21, 2018:

Not sure if it matters, but how about const auto& it = ?

in src/rpc/blockchain.cpp:421 in 465cd73ed9 outdated

 416 | +        if (it == mempool.mapNextTx.end()) {
 417 | +            continue;
 418 | +        }
 419 | +        const uint256& tx_hash = it->second->GetHash();
 420 | +        if (mempool.exists(tx_hash)) {
 421 | +            set_spent.insert(tx_hash.ToString());

promag commented at 9:47 PM on February 21, 2018:

Avoid loop below by taking advantage of insert() return value:

if (set_spent.insert(tx_hash).second) {
    spent.push_back(tx_hash.ToString());
}

conscott force-pushed on Feb 22, 2018

jamesob approved

jamesob commented at 8:44 PM on February 22, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12479/commits/8ad782d7071b8e1042e27a9c147e935b457c46b6

in src/rpc/blockchain.cpp:421 in 8ad782d707 outdated

 416 | +        const auto& it = mempool.mapNextTx.find(COutPoint(tx.GetHash(), i));
 417 | +        if (it == mempool.mapNextTx.end()) {
 418 | +            continue;
 419 | +        }
 420 | +        const uint256& tx_hash = it->second->GetHash();
 421 | +        if (mempool.exists(tx_hash)) {

promag commented at 10:31 PM on February 23, 2018:

Is this test really necessary? If mapNextTx contains the outpoint, then the spending transaction should be in the mempool? See CTxMemPool::addUnchecked().

Therefore, maybe:

replace with assert(mempool.exists(tx_hash))
or test and throw an error if not in the mempool (or log something)
or even remove the check.

IMO not being in mempool.mapTx means that something is broken so I would choose the assert.

cc @morcos @sdaftuar

conscott commented at 6:48 PM on February 24, 2018:

I agree. This was a paranoia check and I debated whether this should be an assert. I think I will update as such.

in src/rpc/blockchain.cpp:415 in 8ad782d707 outdated

 408 | @@ -406,6 +409,23 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
 409 |      }
 410 |  
 411 |      info.pushKV("depends", depends);
 412 | +
 413 | +    std::set<uint256> set_spent;
 414 | +    UniValue spent(UniValue::VARR);
 415 | +    for (unsigned int i = 0; i < tx.vout.size(); i++) {

promag commented at 10:31 PM on February 23, 2018:

Nit, ++i.

junfx commented at 7:31 AM on February 24, 2018:

This for loop iterates tx.vout, does it equal to: auto entryit = mempool.mapTx.find(tx.GetHash()); auto children = mempool.GetMemPoolChildren(entryit); ?

promag commented at 9:11 PM on February 24, 2018:

The index is needed below, to construct the outpoint from the output.

conscott commented at 6:36 PM on February 26, 2018:

I believe @junfx is right. This looks like the more efficient path and I have tested it out.

promag commented at 8:37 PM on February 26, 2018:

Right, I've overlooked @junfx suggestion.

promag commented at 10:50 PM on February 23, 2018: member

utACK 8ad782d. I would like to get feedback from my comment below though.

BTW 2nd commit message is very long.

[RPC] Add list of child transactions to verbose output of getrawmempool fc44cb108b

[Tests] Check output of parent/child tx list from getrawmempool, getmempooldescendants, getmempoolancestors, and REST interface 1dfb4e7d75

conscott force-pushed on Feb 26, 2018

morcos commented at 4:27 PM on March 5, 2018: member

ACK 1dfb4e7

dcousens commented at 11:38 PM on March 5, 2018: contributor

This also applies to the depends list, in that the current transaction could be spending multiple outputs for a single parent, and I think we want to keep the spends / depends format about the same. What are your thoughts? @conscott I think that is fine, I guess it depends on the verbosity required for your application. The few places I can see this being useful, I would appreciate the vout information (as is, I simply download everything).

sipa commented at 3:28 PM on March 6, 2018: member

utACK 1dfb4e7d753e9282c89d55bde358b8ad96d3bfc2

laanwj merged this on Mar 6, 2018

laanwj closed this on Mar 6, 2018

laanwj referenced this in commit cd5e4381d4 on Mar 6, 2018

conscott deleted the branch on Mar 23, 2018

PastaPastaPasta referenced this in commit 320dff5582 on Jun 10, 2020

PastaPastaPasta referenced this in commit e2fd868ea2 on Jun 13, 2020

PastaPastaPasta referenced this in commit 6815c235af on Jun 13, 2020

PastaPastaPasta referenced this in commit e3341ba245 on Jun 13, 2020

PastaPastaPasta referenced this in commit c93ec8a0e9 on Jun 14, 2020

PastaPastaPasta referenced this in commit 01602a9ee7 on Jun 17, 2020

MarcoFalke locked this on Sep 8, 2021