Add verbose flag to getrawmempool #3239

gavinandresen commented at 1:14 AM on November 12, 2013: contributor

This is the first half of the smart fee rework.

Three commits; the first two are clean-up refactors. The third adds an optional boolean 'verbose' flag to getrawmempool, and reworks the memory pool to store CTxMemPoolEntrys instead of CTransactions.

Output of 'getrawmempool true' is an Object, with transaction id keys:

    "f5f67700586e435a8634f6235783936cb015a11115f2d379fb987e34d21d58bd" : {
        "size" : 223,
        "fee" : 0.0050000,
        "time" : 1384221652,
        "height" : 269117,
        "startingpriority" : 0.00000000,
        "currentpriority" : 313.90134529,
        "depends" : [
            "6fcad339f1a613594e9e9f877772a6c3896493ea7babf286a054421190761efa"
        ]
    },

EDITED: store/report timestamp when the transaction entered the pool.

Shed-painting on whether this should return an Array of Objects (with "txid" one of the keys) instead of an Object welcome, as are suggestions for key names (e.g. prefer 'startingpriority' or 'prioritystart').

luke-jr commented at 2:04 AM on November 12, 2013: member

The verbose flag seems needlessly redundant with getblocktemplate...

jgarzik commented at 2:05 AM on November 12, 2013: contributor

@luke-jr getblocktemplate always produces a subset of mempool

luke-jr commented at 2:11 AM on November 12, 2013: member

Not always...? Wouldn't the goal of a good mempool be to only keep transactions you'd be putting in templates?

jgarzik commented at 2:27 AM on November 12, 2013: contributor

@luke-jr Yes, always. Sometimes subset == set, but usually not. The mempool will always store more than one block's worth of transactions. getblocktemplate only returns up to 1MB, etc.

luke-jr commented at 2:58 AM on November 12, 2013: member

Could just add some non-standard extensions to bitcoind getblocktemplate to request more, but meh.

Diapolo commented at 7:21 AM on November 12, 2013: none

Nit: CTxMemPoolEntrys should be CTxMemPoolEntries

petertodd commented at 3:05 PM on November 12, 2013: contributor

Remove EraseTransaction() while we're at it; only tx replacement uses that code. I don't see any way it could have gotten much if any testing in the past.

petertodd commented at 3:11 PM on November 12, 2013: contributor

It'd be better if ComputePriority() was not a part of CTransaction - priority calculations are node policy rather than consensus critical and we'd be better off if such code was kept out of the core data structures to make code review easier in the future.

gavinandresen commented at 2:58 AM on November 14, 2013: contributor

Rebased, extended the help, and tweaked to rename verbose output 'dependson' to 'depends' to be more consistent with getblocktemplate, and to always output a 'depends' key (will be an empty array if no dependencies).

RE: getblocktemplate: I seem to remember having the same argument when getrawmempool was pulled, and the decision remains getrawmempool is valuable.

RE: removing EraseTransaction/moving ComputePriority: another clean-up pull request would be fine, I'm busy working on higher priorities.

RE: CTxMemPoolEntries: please read the code, and ideally compile and test it, before commenting.

in src/rpcblockchain.cpp:None in 767cf3f9a0 outdated

 172 |              "]\n"
 173 | +            "\nResult: (for verbose = true):\n"
 174 | +            "{                           (json object)\n"
 175 | +            "  \"transactionid\" : {       (json object)\n"
 176 | +            "    \"size\" : n,             (numeric) transaction size in bytes\n"
 177 | +            "    \"fee\" : n,              (numeric) transaction fee in satoshis\n"

laanwj commented at 2:23 PM on November 15, 2013:

Any specific reason to specify the fee in satoshis here instead of using ValueFromAmount?

laanwj commented at 3:18 PM on November 15, 2013: member

ACK apart from that, appears to work as expected, and code changes look good

gavinandresen commented at 5:19 AM on November 20, 2013: contributor

@laanwj: good catch, using ValueFromAmount to report fees now.

gavinandresen commented at 5:50 AM on November 20, 2013: contributor

Testing on my smartfee branch (which save/restores the mempool), I noticed transactions with repeated txids in the depends array because they depends on multiple outputs from previous transactions.

I decided to fix that by unique-ifying the depends array. The alternative would be to report "txid:vout" or something more complicated/verbose like [ { "txid: " .... ", "vout" : 11 }, { ... } ]

But I think just pointing to the unconfirmed parent txids is the right thing to do. You can use getrawtransaction for the child to work out exactly which txins are unconfirmed.

BitcoinPullTester commented at 6:02 AM on November 29, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/702bb264d98874e1dc4dafa2aa17c60ba416ce19 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

Remove dead transaction replacement code

This dead code can be resurrected from git history if
transaction replacement is ever implemented. Keeping
dead code in the source is a bad idea, because it implies
it was tested and worked at some point, which is not true.

98c7c8fd1d

Refactor: move GetValueIn(tx) to tx.GetValueIn()

GetValueIn makes more sense as a CTransaction member.

0733c1bde6

Add verbose boolean to getrawmempool

Also changes mempool to store CTxMemPoolEntries
to keep track of when they enter/exit the pool.

4d707d5120

gavinandresen referenced this in commit b78d1cdf82 on Nov 30, 2013

gavinandresen merged this on Nov 30, 2013

gavinandresen closed this on Nov 30, 2013

gavinandresen deleted the branch on Mar 13, 2014

Bushstar referenced this in commit dc656e3236 on Apr 8, 2020

in src/main.cpp:710 in 4d707d5120

 707 | +        int64_t nValueIn = view.GetValueIn(tx);
 708 | +        int64_t nValueOut = tx.GetValueOut();
 709 | +        int64_t nFees = nValueIn-nValueOut;
 710 | +        double dPriority = view.GetPriority(tx, chainActive.Height());
 711 | +
 712 | +        CTxMemPoolEntry entry(tx, nFees, GetTime(), dPriority, chainActive.Height());

rebroad commented at 10:18 PM on September 7, 2021:

Do we need GetTime() and height()? Just thinking to save memory, it would be better if one could be derived quickly from the other.

fanquake commented at 12:48 AM on September 8, 2021:

Stop commenting on 10 year old PRs. The code here likely doesn't even reflect the current codebase.

fanquake locked this on Sep 8, 2021