Skip to content
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

[REST] Add memory pool API #6013

Merged
merged 1 commit into from Aug 15, 2015
Merged

[REST] Add memory pool API #6013

merged 1 commit into from Aug 15, 2015

Conversation

@paveljanik
Copy link
Contributor

paveljanik commented Apr 14, 2015

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
Copy link
Member

laanwj commented Apr 15, 2015

Concept ACK

@laanwj laanwj added the REST label Apr 15, 2015
@jgarzik
Copy link
Contributor

jgarzik commented Apr 15, 2015

ACK

@jonasschnelli
Copy link
Member

jonasschnelli commented Apr 15, 2015

Concept ACK.
Misses RPC test.

@paveljanik
Copy link
Contributor Author

paveljanik commented Apr 15, 2015

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

Array rpcParams;
rpcParams.push_back(true);

Value mempoolObject = getrawmempool(rpcParams, false);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Apr 15, 2015

Member

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.

switch (rf) {
case RF_JSON: {
Array rpcParams;
rpcParams.push_back(true);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Apr 15, 2015

Member

very little nit: indent.

@paveljanik paveljanik force-pushed the paveljanik:rest_mempool branch from 3779c93 to d1f90f8 Apr 15, 2015
@paveljanik
Copy link
Contributor Author

paveljanik commented Apr 15, 2015

nit addressed, squashed, ready.

@jonasschnelli
Copy link
Member

jonasschnelli commented Apr 21, 2015

Tested ACK.

@paveljanik
Copy link
Contributor Author

paveljanik commented Apr 28, 2015

Other reviews, please?

@jgarzik
Copy link
Contributor

jgarzik commented Jun 11, 2015

tested ACK

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 11, 2015

Needs rebase.

@paveljanik paveljanik force-pushed the paveljanik:rest_mempool branch from d1f90f8 Jun 11, 2015
@paveljanik
Copy link
Contributor Author

paveljanik commented Jun 11, 2015

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

@paveljanik
Copy link
Contributor Author

paveljanik commented Jun 15, 2015

Needs to be adapted to UniValue. Tomorrow.

@paveljanik paveljanik force-pushed the paveljanik:rest_mempool branch 2 times, most recently Jun 15, 2015
@paveljanik
Copy link
Contributor Author

paveljanik commented Jun 16, 2015

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

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 16, 2015

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
Copy link
Contributor Author

paveljanik commented Jun 16, 2015

@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
Copy link
Member

sipa commented Jun 16, 2015

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

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 16, 2015

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
Copy link
Contributor Author

paveljanik commented Jun 17, 2015

@jonasschnelli Please review. Thanks.

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 17, 2015

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

@jonasschnelli
Copy link
Member

jonasschnelli commented Jul 2, 2015

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
Copy link
Contributor Author

paveljanik commented Jul 2, 2015

@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
Copy link
Member

jonasschnelli commented Jul 2, 2015

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
Copy link
Contributor Author

paveljanik commented Jul 2, 2015

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

@paveljanik paveljanik force-pushed the paveljanik:rest_mempool branch Jul 23, 2015
@paveljanik
Copy link
Contributor Author

paveljanik commented Jul 23, 2015

Rebased.
Added "usage" to the documentation.

@jgarzik
Copy link
Contributor

jgarzik commented Jul 23, 2015

re-ACK

@jonasschnelli
Copy link
Member

jonasschnelli commented Aug 5, 2015

Needs rebase.
Anything holding this back?

@paveljanik paveljanik force-pushed the paveljanik:rest_mempool branch Aug 6, 2015
@paveljanik
Copy link
Contributor Author

paveljanik commented Aug 6, 2015

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

@btcdrak
Copy link
Contributor

btcdrak commented Aug 7, 2015

ACK

@TheBlueMatt
TheBlueMatt reviewed Aug 8, 2015
View changes
doc/REST-interface.md Outdated
####Memory pool
`GET /rest/mempool/info.json`

Returns various informations about the TX mempool.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Aug 8, 2015

Contributor

Nit: informations->information

This comment has been minimized.

Copy link
@paveljanik

paveljanik Aug 8, 2015

Author Contributor

Fixed in the SQUASHME commit.

`GET /rest/mempool/contents.json`

Returns transactions in the TX mempool.
Only supports JSON as output format.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Aug 8, 2015

Contributor

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

This comment has been minimized.

Copy link
@paveljanik

paveljanik Aug 8, 2015

Author Contributor

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

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Aug 8, 2015

Contributor

Oh, missed that :)

@@ -65,6 +65,8 @@ class RestErr

extern void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry);
extern UniValue blockToJSON(const CBlock& block, const CBlockIndex* blockindex, bool txDetails = false);
extern UniValue mempoolInfoToJSON();
extern UniValue mempoolToJSON(bool fVerbose = false);

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Aug 8, 2015

Contributor

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.

This comment has been minimized.

Copy link
@paveljanik

paveljanik Aug 8, 2015

Author Contributor

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?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Aug 8, 2015

Contributor

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
Copy link
Contributor

TheBlueMatt commented Aug 8, 2015

utACK

@paveljanik
Copy link
Contributor Author

paveljanik commented Aug 13, 2015

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

@jgarzik
Copy link
Contributor

jgarzik commented Aug 13, 2015

I can merge, after this latest commit is squashed.

@paveljanik
Copy link
Contributor Author

paveljanik commented Aug 14, 2015

Squashed.

@paveljanik paveljanik force-pushed the paveljanik:rest_mempool branch to 70180b2 Aug 14, 2015
const std::map<std::string, std::string>& mapHeaders,
bool fRun)
{
vector<string> params;

This comment has been minimized.

Copy link
@Diapolo

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

UniValue mempoolInfoToJSON()
{
UniValue ret(UniValue::VOBJ);
ret.push_back(Pair("size", (int64_t) mempool.size()));

This comment has been minimized.

Copy link
@Diapolo

Diapolo Aug 14, 2015

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

This comment has been minimized.

Copy link
@paveljanik

paveljanik Aug 14, 2015

Author Contributor

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...

This comment has been minimized.

Copy link
@Diapolo

Diapolo Aug 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!

This comment has been minimized.

Copy link
@paveljanik

paveljanik Aug 14, 2015

Author Contributor

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

@jonasschnelli
Copy link
Member

jonasschnelli commented Aug 14, 2015

Re ACK

@jonasschnelli
Copy link
Member

jonasschnelli commented Aug 14, 2015

@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
Copy link

Diapolo commented Aug 14, 2015

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

@dcousens
Copy link
Contributor

dcousens commented Aug 14, 2015

utACK

@jgarzik jgarzik merged commit 70180b2 into bitcoin:master Aug 15, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jgarzik pushed a commit that referenced this pull request Aug 15, 2015
Jeff Garzik
@paveljanik paveljanik mentioned this pull request Aug 17, 2015
zkbot added a commit to zcash/zcash that referenced this pull request Mar 23, 2017
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.