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: Add child transactions to getrawmempool verbose output #12479

Merged
merged 2 commits into from Mar 6, 2018

Conversation

Projects
None yet
10 participants
@conscott
Contributor

conscott commented Feb 18, 2018

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"
    ]
  },
@esotericnonsense

This comment has been minimized.

Contributor

esotericnonsense commented Feb 18, 2018

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.

std::set<std::string> setSpent;
for (unsigned int i = 0; i < tx.vout.size(); i++) {
auto it = mempool.mapNextTx.find(COutPoint(tx.GetHash(), i));

This comment has been minimized.

@esotericnonsense

esotericnonsense Feb 18, 2018

Contributor

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)

This comment has been minimized.

@promag

promag Feb 19, 2018

Member

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

@dcousens

This comment has been minimized.

Contributor

dcousens commented Feb 18, 2018

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

This comment has been minimized.

Contributor

conscott commented Feb 19, 2018

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

This comment has been minimized.

Member

sipa commented Feb 19, 2018

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

This comment has been minimized.

Contributor

dcousens commented Feb 19, 2018

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.

@promag

Concept ACK.

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

spent.push_back(dep);
}
info.pushKV("spentby", spent);

This comment has been minimized.

@promag

promag Feb 19, 2018

Member

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.

This comment has been minimized.

@conscott

conscott Feb 19, 2018

Contributor

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

std::set<std::string> setSpent;
for (unsigned int i = 0; i < tx.vout.size(); i++) {
auto it = mempool.mapNextTx.find(COutPoint(tx.GetHash(), i));

This comment has been minimized.

@promag

promag Feb 19, 2018

Member

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

@@ -372,6 +372,9 @@ std::string EntryDescriptionString()
" \"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"
" \"spentby\" : [ (array) unconfirmed transactions spending outputs from this transaction\n"

This comment has been minimized.

@promag

promag Feb 19, 2018

Member

Looks like alignment is bad.

@conscott

This comment has been minimized.

Contributor

conscott commented Feb 20, 2018

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?

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

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

std::set<uint256>?

std::set<std::string> set_spent;
for (unsigned int i = 0; i < tx.vout.size(); i++) {
auto it = mempool.mapNextTx.find(COutPoint(tx.GetHash(), i));

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

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

}
const uint256& tx_hash = it->second->GetHash();
if (mempool.exists(tx_hash)) {
set_spent.insert(tx_hash.ToString());

This comment has been minimized.

@promag

promag Feb 21, 2018

Member

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

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

utACK 8ad782d

@promag

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

BTW 2nd commit message is very long.

continue;
}
const uint256& tx_hash = it->second->GetHash();
if (mempool.exists(tx_hash)) {

This comment has been minimized.

@promag

promag Feb 23, 2018

Member

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

This comment has been minimized.

@conscott

conscott Feb 24, 2018

Contributor

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

std::set<uint256> set_spent;
UniValue spent(UniValue::VARR);
for (unsigned int i = 0; i < tx.vout.size(); i++) {

This comment has been minimized.

@promag

promag Feb 23, 2018

Member

Nit, ++i.

std::set<uint256> set_spent;
UniValue spent(UniValue::VARR);
for (unsigned int i = 0; i < tx.vout.size(); i++) {

This comment has been minimized.

@junfx

junfx Feb 24, 2018

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

This comment has been minimized.

@promag

promag Feb 24, 2018

Member

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

This comment has been minimized.

@conscott

conscott Feb 26, 2018

Contributor

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

This comment has been minimized.

@promag

promag Feb 26, 2018

Member

Right, I've overlooked @junfx suggestion.

@morcos

This comment has been minimized.

Member

morcos commented Mar 5, 2018

ACK 1dfb4e7

@dcousens

This comment has been minimized.

Contributor

dcousens commented Mar 5, 2018

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

This comment has been minimized.

Member

sipa commented Mar 6, 2018

utACK 1dfb4e7

@laanwj laanwj merged commit 1dfb4e7 into bitcoin:master Mar 6, 2018

1 check passed

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

laanwj added a commit that referenced this pull request Mar 6, 2018

Merge #12479: RPC: Add child transactions to getrawmempool verbose ou…
…tput

1dfb4e7 [Tests] Check output of parent/child tx list from getrawmempool, getmempooldescendants, getmempoolancestors, and REST interface (Conor Scott)
fc44cb1 [RPC] Add list of child transactions to verbose output of getrawmempool (Conor Scott)

Pull request description:

  `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"
      ]
    },
  ```

Tree-SHA512: 83da7d421c9799a40ef65af3b7fdb586d6d87385f3f2ede3afd2c311725444b858f9d91cc110422a0fa31905779934fee07211ca6fe6b746792b83692c94b3ce

@conscott conscott deleted the conscott:getrawmempool_descendent_pointers branch Mar 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment