[REST] Add memory pool API by paveljanik · Pull Request #6013 · bitcoin/bitcoin

paveljanik · 2015-04-14T19:21:18Z

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 · 2015-04-15T08:29:57Z

Concept ACK

jgarzik · 2015-04-15T14:50:05Z

ACK

jonasschnelli · 2015-04-15T15:36:24Z

Concept ACK.
Misses RPC test.

paveljanik · 2015-04-15T17:06:22Z

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

jonasschnelli · 2015-04-15T18:25:44Z

src/rest.cpp

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.

paveljanik · 2015-04-15T19:12:18Z

nit addressed, squashed, ready.

jonasschnelli · 2015-04-21T18:14:36Z

Tested ACK.

paveljanik · 2015-04-28T20:40:18Z

Other reviews, please?

jgarzik · 2015-06-11T19:37:10Z

tested ACK

jonasschnelli · 2015-06-11T19:38:29Z

Needs rebase.

paveljanik · 2015-06-11T20:08:28Z

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

paveljanik · 2015-06-15T20:56:41Z

Needs to be adapted to UniValue. Tomorrow.

paveljanik · 2015-06-16T06:29:13Z

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

jonasschnelli · 2015-06-16T07:03:34Z

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 · 2015-06-16T11:49:14Z

@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 · 2015-06-16T11:53:30Z

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

jonasschnelli · 2015-06-16T14:57:21Z

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 · 2015-06-17T07:17:26Z

@jonasschnelli Please review. Thanks.

jonasschnelli · 2015-06-17T07:24:13Z

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

jonasschnelli · 2015-07-02T19:59:18Z

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 · 2015-07-02T20:27:43Z

@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 · 2015-07-02T20:32:16Z

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 · 2015-07-02T20:38:59Z

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

paveljanik · 2015-07-23T19:28:46Z

Rebased.
Added "usage" to the documentation.

jgarzik · 2015-07-23T19:31:39Z

re-ACK

jonasschnelli · 2015-08-05T12:53:59Z

Needs rebase.
Anything holding this back?

paveljanik · 2015-08-06T17:47:49Z

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

btcdrak · 2015-08-07T17:21:11Z

ACK

TheBlueMatt · 2015-08-08T14:03:19Z

doc/REST-interface.md

Nit: informations->information

Fixed in the SQUASHME commit.

TheBlueMatt · 2015-08-08T15:27:59Z

utACK

paveljanik · 2015-08-13T21:36:21Z

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

jgarzik · 2015-08-13T23:43:50Z

I can merge, after this latest commit is squashed.

paveljanik · 2015-08-14T05:02:01Z

Squashed.

Diapolo · 2015-08-14T05:51:01Z

src/rest.cpp

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; :).

jonasschnelli · 2015-08-14T06:01:14Z

Re ACK

jonasschnelli · 2015-08-14T06:52:05Z

@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 · 2015-08-14T06:53:39Z

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

dcousens · 2015-08-14T09:11:10Z

utACK

Bitcoin 0.12 mempool memory usage PRs Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6410 - bitcoin/bitcoin#6453 - bitcoin/bitcoin#6013 (excludes changes to docs we deleted) Part of #2074.

laanwj added the REST label Apr 15, 2015

jonasschnelli reviewed Apr 15, 2015
View reviewed changes

paveljanik force-pushed the rest_mempool branch from 3779c93 to d1f90f8 Compare April 15, 2015 19:11

paveljanik force-pushed the rest_mempool branch from d1f90f8 to e3ca66e Compare June 11, 2015 20:07

paveljanik force-pushed the rest_mempool branch 2 times, most recently from 0188efb to da94f0d Compare June 16, 2015 05:14

paveljanik force-pushed the rest_mempool branch from 62c5470 to d848fe6 Compare July 23, 2015 19:28

paveljanik force-pushed the rest_mempool branch from d848fe6 to 42db8ae Compare August 6, 2015 17:44

TheBlueMatt reviewed Aug 8, 2015
View reviewed changes

Implement REST mempool API, add test and documentation.

70180b2

paveljanik force-pushed the rest_mempool branch from 5b114eb to 70180b2 Compare August 14, 2015 05:02

Diapolo reviewed Aug 14, 2015
View reviewed changes

jgarzik merged commit 70180b2 into bitcoin:master Aug 15, 2015

jgarzik pushed a commit that referenced this pull request Aug 15, 2015

Merge pull request #6013

6feeec1

paveljanik mentioned this pull request Aug 17, 2015

Mempool -> REST Api #5844

Closed

str4d mentioned this pull request Mar 14, 2017

Bitcoin 0.12 mempool memory usage PRs zcash/zcash#2175

Merged

dagurval mentioned this pull request Jul 12, 2017

libevent-based http server bitcoinxt/bitcoinxt#172

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

paveljanik commented Apr 14, 2015

Uh oh!

laanwj commented Apr 15, 2015

Uh oh!

jgarzik commented Apr 15, 2015

Uh oh!

jonasschnelli commented Apr 15, 2015

Uh oh!

paveljanik commented Apr 15, 2015

Uh oh!

jonasschnelli Apr 15, 2015

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Apr 15, 2015

Uh oh!

jonasschnelli commented Apr 21, 2015

Uh oh!

paveljanik commented Apr 28, 2015

Uh oh!

jgarzik commented Jun 11, 2015

Uh oh!

jonasschnelli commented Jun 11, 2015

Uh oh!

paveljanik commented Jun 11, 2015

Uh oh!

paveljanik commented Jun 15, 2015

Uh oh!

paveljanik commented Jun 16, 2015

Uh oh!

jonasschnelli commented Jun 16, 2015

Uh oh!

paveljanik commented Jun 16, 2015

Uh oh!

sipa commented Jun 16, 2015

Uh oh!

jonasschnelli commented Jun 16, 2015

Uh oh!

paveljanik commented Jun 17, 2015

Uh oh!

jonasschnelli commented Jun 17, 2015

Uh oh!

jonasschnelli commented Jul 2, 2015

Uh oh!

paveljanik commented Jul 2, 2015

Uh oh!

jonasschnelli commented Jul 2, 2015

Uh oh!

paveljanik commented Jul 2, 2015

Uh oh!

paveljanik commented Jul 23, 2015

Uh oh!

jgarzik commented Jul 23, 2015

Uh oh!

jonasschnelli commented Aug 5, 2015

Uh oh!

paveljanik commented Aug 6, 2015

Uh oh!

btcdrak commented Aug 7, 2015

Uh oh!

TheBlueMatt Aug 8, 2015

Choose a reason for hiding this comment

Uh oh!

paveljanik Aug 8, 2015

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Aug 8, 2015

Uh oh!

paveljanik commented Aug 13, 2015

Uh oh!

jgarzik commented Aug 13, 2015

Uh oh!

paveljanik commented Aug 14, 2015

Uh oh!

Diapolo Aug 14, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 14, 2015

Uh oh!

jonasschnelli commented Aug 14, 2015