[REST] Add memory pool API #6013

paveljanik commented at 7:21 PM on April 14, 2015: contributor

As requested in #5844, memory pool API added.

Get TX mempool info:

$ curl localhost:8332/rest/mempool/info.json 2>/dev/null | json_pp
{
   "bytes" : 877,
   "size" : 2
}
$

Get TX mempool contents:

$ curl localhost:8332/rest/mempool/contents.json 2>/dev/null | json_pp
{
   "d84fb825b54442d737f9026ac52c4208f48eae8e1aaf9c942acede7ee8457f35" : {
      "currentpriority" : 8060141.48717949,
      "time" : 1429038596,
      "fee" : 0.0001,
      "height" : 352141,
      "depends" : [],
      "startingpriority" : 8060141.48717949,
      "size" : 225
   },
...
$

laanwj commented at 8:29 AM on April 15, 2015: member

Concept ACK

laanwj added the label REST on Apr 15, 2015

jgarzik commented at 2:50 PM on April 15, 2015: contributor

ACK

jonasschnelli commented at 3:36 PM on April 15, 2015: contributor

Concept ACK. Misses RPC test.

paveljanik commented at 5:06 PM on April 15, 2015: contributor

Yup, I plan to add them in the evening or tomorrow.

in src/rest.cpp:None in acbdf22b2c outdated

 289 | +    switch (rf) {
 290 | +    case RF_JSON: {
 291 | +        Array rpcParams;
 292 | +	rpcParams.push_back(true);
 293 | +
 294 | +        Value mempoolObject = getrawmempool(rpcParams, false);

jonasschnelli commented at 6:25 PM on April 15, 2015:

I'm not sure if we should just loop through RPC commands. I know that i did the same when i added the /chaininfos REST command. This will tie the REST interface to the current RPC implementation. But duplicating code is also something we should avoid. I don't have a solution here i just wanted to bring up this concern.

in src/rest.cpp:None in acbdf22b2c outdated

 287 | +    const RetFormat rf = ParseDataFormat(params, strReq);
 288 | +
 289 | +    switch (rf) {
 290 | +    case RF_JSON: {
 291 | +        Array rpcParams;
 292 | +	rpcParams.push_back(true);

jonasschnelli commented at 6:27 PM on April 15, 2015:

very little nit: indent.

paveljanik force-pushed on Apr 15, 2015

paveljanik commented at 7:12 PM on April 15, 2015: contributor

nit addressed, squashed, ready.

jonasschnelli commented at 6:14 PM on April 21, 2015: contributor

Tested ACK.

paveljanik commented at 8:40 PM on April 28, 2015: contributor

Other reviews, please?

jgarzik commented at 7:37 PM on June 11, 2015: contributor

tested ACK

jonasschnelli commented at 7:38 PM on June 11, 2015: contributor

Needs rebase.

paveljanik force-pushed on Jun 11, 2015

paveljanik commented at 8:08 PM on June 11, 2015: contributor

Rebased (changed the header style in README to match the other entries).

paveljanik commented at 8:56 PM on June 15, 2015: contributor

Needs to be adapted to UniValue. Tomorrow.

paveljanik force-pushed on Jun 15, 2015

paveljanik force-pushed on Jun 16, 2015

paveljanik commented at 6:29 AM on June 16, 2015: contributor

Travis CI build fails only in one environment because of the comparison tool. Ready for merge.

jonasschnelli commented at 7:03 AM on June 16, 2015: contributor

I like this PR.

But, am i alone with the feeling that calling a RPC method (getrawmempool, getmempoolinfo) will end up in REST/RPC dependency and smells after a unideal design? Duplicating code would also be kinda stupid. Maybe factoring out some code to a independent helper class/file?

But however reACK.

paveljanik commented at 11:49 AM on June 16, 2015: contributor

@jonasschnelli Something like CTxMemPool::toJSON factored out from get*mempool*? Or do you prefer interface similar to TxToJSON (defined in rpc, used in rest)?

I do not have a problem to refactor once agreed, but in the next PR please.

sipa commented at 11:53 AM on June 16, 2015: member

No JSON conversion code inside core classes, please. That's something that belongs in the RPC glue layer.

jonasschnelli commented at 2:57 PM on June 16, 2015: contributor

For sure we don't want json code within core classes. But i think calling a RPC function within REST should be avoided. Maybe something like blockToJSON or similar. Just factor out the json encoding and for now – place it within the rpcblockchain.cpp.

paveljanik commented at 7:17 AM on June 17, 2015: contributor

@jonasschnelli Please review. Thanks.

jonasschnelli commented at 7:24 AM on June 17, 2015: contributor

design wise this looks much better. utACK. Will give it a real testshot soon.

jonasschnelli commented at 7:59 PM on July 2, 2015: contributor

Tested ACK.

Two nits: a) missing support for HEX/BIN (would make it consistence). aa) could be serialized vector of transactions for mempool/content and two serialized int64_t for mempool/info b) your diff looks somehow strange (look at UniValue getrawmempool(const UniValue& params, bool fHelp) remove L150-185, re-add at L169-232). Maybe shuffle some lines to avoid this?

paveljanik commented at 8:27 PM on July 2, 2015: contributor

@jonasschnelli Thanks for review.

hex/bin: Does this make sense at all here? I do not think so. I modelled info it similar to getchaininfo.
does git diff --diff-algorithm=minimal help you?

jonasschnelli commented at 8:32 PM on July 2, 2015: contributor

hex/bin: Does this make sense at all here? I do not think so. I modelled info it similar to getchaininfo.

If someone actively polls the mempool over REST, he could probably save CPU time and bandwidth if there would be a BIN/HEX support. But as said: low prio.

does git diff --diff-algorithm=minimal help you?

Thanks. Yes: https://github.com/bitcoin/bitcoin/pull/6013/files?1 (the 1 at the end helps)

paveljanik commented at 8:38 PM on July 2, 2015: contributor

Hmm, 1 at the end seems to not have any effect here (compared with two curl calls and diff on the output).

paveljanik force-pushed on Jul 23, 2015

paveljanik commented at 7:28 PM on July 23, 2015: contributor

Rebased. Added "usage" to the documentation.

jgarzik commented at 7:31 PM on July 23, 2015: contributor

re-ACK

jonasschnelli commented at 12:53 PM on August 5, 2015: contributor

Needs rebase. Anything holding this back?

paveljanik force-pushed on Aug 6, 2015

paveljanik commented at 5:47 PM on August 6, 2015: contributor

Rebased (to accomodate #6504 changes). Ready for merge IMO.

btcdrak commented at 5:21 PM on August 7, 2015: contributor

ACK

in doc/REST-interface.md:None in 42db8ae802 outdated

  76 | @@ -77,6 +77,20 @@ $ curl localhost:18332/rest/getutxos/checkmempool/b2cdfd7b89def827ff8af7cd9bff76
  77 |  }
  78 |  ```
  79 |  
  80 | +####Memory pool
  81 | +`GET /rest/mempool/info.json`
  82 | +
  83 | +Returns various informations about the TX mempool.

TheBlueMatt commented at 2:03 PM on August 8, 2015:

Nit: informations->information

paveljanik commented at 3:30 PM on August 8, 2015:

Fixed in the SQUASHME commit.

in doc/REST-interface.md:None in 42db8ae802 outdated

  87 | +* usage : (numeric) total TX mempool memory usage
  88 | +
  89 | +`GET /rest/mempool/contents.json`
  90 | +
  91 | +Returns transactions in the TX mempool.
  92 | +Only supports JSON as output format.

TheBlueMatt commented at 2:06 PM on August 8, 2015:

Its not clear you mean that both info and contents only support JSON.

paveljanik commented at 3:26 PM on August 8, 2015:

This is why I wrote the same sentence in both parts...

TheBlueMatt commented at 3:31 PM on August 8, 2015:

Oh, missed that :)

in src/rest.cpp:None in 42db8ae802 outdated

  64 | @@ -65,6 +65,8 @@ class RestErr
  65 |  
  66 |  extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry);
  67 |  extern UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool txDetails = false);
  68 | +extern UniValue mempoolInfoToJSON();
  69 | +extern UniValue mempoolToJSON(bool fVerbose = false);

TheBlueMatt commented at 3:25 PM on August 8, 2015:

Nit: rest_chaininfo just aclls the getblockchaininfo RPC function directly. Though your change makes the code a bit cleaner, maybe split the commits up to just call the RPC first, then move the code around in a separate commit.

paveljanik commented at 4:07 PM on August 8, 2015:

If I should split it into more commits, I'd prefer the other way round:

separate mempoolInfoToJSON and mempoolToJSON and call the from RPC functions (ie. prepare the current code for REST)
add REST functions

Should I really split it?

TheBlueMatt commented at 4:11 PM on August 8, 2015:

Order them either way. I dont care too much, I just always prefer easier-to-read diffs over fewer commits, but its really up to you. I ACKed it as-is, was more of a suggestion than anything.

TheBlueMatt commented at 3:27 PM on August 8, 2015: member

utACK

paveljanik commented at 9:36 PM on August 13, 2015: contributor

@laanwj Can you please have a look. Thank you.

jgarzik commented at 11:43 PM on August 13, 2015: contributor

I can merge, after this latest commit is squashed.

Implement REST mempool API, add test and documentation. 70180b2e57

paveljanik commented at 5:02 AM on August 14, 2015: contributor

Squashed.

paveljanik force-pushed on Aug 14, 2015

in src/rest.cpp:None in 70180b2e57

 299 | +                              const std::string& strURIPart,
 300 | +                              const std::string& strRequest,
 301 | +                              const std::map<std::string, std::string>& mapHeaders,
 302 | +                              bool fRun)
 303 | +{
 304 | +    vector<string> params;

Diapolo commented at 5:51 AM on August 14, 2015:

You could (in the future) start adding std:: in front of such code, as that means we can more easily get rid of using namespace std; :).

in src/rpcblockchain.cpp:None in 70180b2e57

 760 | @@ -757,6 +761,16 @@ UniValue getchaintips(const UniValue& params, bool fHelp)
 761 |      return res;
 762 |  }
 763 |  
 764 | +UniValue mempoolInfoToJSON()
 765 | +{
 766 | +    UniValue ret(UniValue::VOBJ);
 767 | +    ret.push_back(Pair("size", (int64_t) mempool.size()));

Diapolo commented at 5:51 AM on August 14, 2015:

Minor nit: You also could remove the spaces before the casts here.

paveljanik commented at 6:17 AM on August 14, 2015:

Eh? Please read the whole diff (I did not change that code at all).

Space before the cast? I hope we do not have a rule do not add a space after comma...

Diapolo commented at 6:40 AM on August 14, 2015:

Don't be aggressive, I wrote minor nit and I'm well aware you just copied the code. But if there was something left to do you could've just change to (int64_t)mempool.size(), what's the problem. No there is no such rule!

paveljanik commented at 6:48 AM on August 14, 2015:

So you mean after the cast? I'd write it the same way it is written anyway.

jonasschnelli commented at 6:01 AM on August 14, 2015: contributor

Re ACK

jonasschnelli commented at 6:52 AM on August 14, 2015: contributor

@Diapolo: i think it's unfair to start nitpicking at this stage of the PR.

Let's get this PR merged.

Anyone can open a cleanup PR over @trivial.

Diapolo commented at 6:53 AM on August 14, 2015: none

ACK and merge is fine with me, I just comment when I've got time or start reading a pull, nothing more ;).

dcousens commented at 9:11 AM on August 14, 2015: contributor

utACK

jgarzik merged this on Aug 15, 2015

jgarzik closed this on Aug 15, 2015

jgarzik referenced this in commit 6feeec1ec5 on Aug 15, 2015

zkbot referenced this in commit 00e59e5935 on Mar 23, 2017

DrahtBot locked this on Feb 15, 2022