[REST] Add send raw transaction #6844

lclc · 2015-10-17T20:16:57Z

Adding sendrawtransaction to the REST API enables web-clients like https://coinb.in/ (and many others) to work with Bitcoin Core nodes instead of using a centralized wallet API service.

So far the REST API was read-only but I don't know if this was intentionally or not.

Python tests will follow ones it's clear when there are some Concept ACK for this PR.

Or if someone likes to write some Python code for this PR let me know. I don't have much Python experience yet so it might take a while :)

Thanks Jonas Schnelli for the pre-PR feedback.

sipa · 2015-10-17T20:33:14Z

This makes sense to me. It was already possible to do over RPC, but there is no need for authentication. Something similar could be done for submitting blocks?

jgarzik · 2015-10-17T21:42:26Z

ut ACK.

Nits:

No need to adopt over-long RPC naming system. "sendtx" or "pushtx" works.
Agree that sending blocks should be added also (note - not a blocker for this PR)

dcousens · 2015-10-17T23:19:22Z

No need to adopt over-long RPC naming system. "sendtx" or "pushtx" works.

@jgarzik why not just: POST /rest/tx
If you want to be able to post in the different formats... POST /rest/tx/[format]

Also, concept ACK, this would be great

dcousens · 2015-10-17T23:21:09Z

doc/REST-interface.md

It should not be a GET... if this is intended to be a REST-ful interface.
Some proxies won't allow URL's longer than 2000 chars anyway, so using this hacky [in-the-url] approach will be prone to issues.

dcousens · 2015-10-17T23:25:12Z

utACK, except the nits posted are blockers to the merge IMHO.

jgarzik · 2015-10-18T03:00:18Z

agree w/ @dcousens

jonasschnelli · 2015-10-18T06:35:34Z

+1 for POST /rest/tx

I would also think supporting BIN (and JSON) as input format could make sense and would be consistent.

utACK.

lclc · 2015-10-18T13:18:38Z

Thanks for the fast feedback.

I've changed it to POST /rest/tx

Regarding the format: So far there is support for JSON / HEX / BIN for some of the REST methods. IMHO it should be either all methods support all formats or there is only one until there is a demand / request for other formats. I think the REST API was mostly created for webclients, which are probably most confortable with JSON or plaintext, or what was the intention when REST was added?

If we want to have support for several formats, I would vote for using the HTTP header files instead of a file-ending for the request:

5 Use HTTP headers for serialization formats

Both, client and server, need to know which format is used for the communication. The format has to be specified in the HTTP-Header.

Content-Type defines the request format.
Accept defines a list of acceptable response formats.

(From: http://blog.mwaysolutions.com/2014/06/05/10-best-practices-for-better-restful-api/)

SubmitBlock over REST is a good idea but I'll leave it out for another PR :)

dcousens · 2015-10-18T23:02:54Z

doc/REST-interface.md

You should specify:

"application/octet-stream" for plain-old-data

"text/plain" for hex

"application/json" for JSON

I will ones we agreed on it :)

In the code in this version it's like in the other methods, determined by the ending, e.g.
POST 127.0.0.1:18332/rest/tx.hex

dcousens · 2015-10-18T23:05:00Z

@lclc agreed we should use the Content-Type header to specify in the request what response we are giving.

dcousens · 2015-10-18T23:06:02Z

@lclc I have no doubt the file name extensions were made for easily viewing the data via a browser URL. Not for API usage.

jgarzik · 2015-10-19T03:29:06Z

As background, the discussion in IRC generally concluded that Content-type as the only method of specifying file type was not good. Command line users (curl, wget) and web browser URL bars generally make it easy to alter a URL, yet difficult to specify Content-type.

dcousens · 2015-10-19T04:52:23Z

@jgarzik then lets take the approach of /rest/tx/[format]? A file type seems odd here, these aren't files.
But this argument is also relevant to all the other routes, so, may be for this pull request, we just use file types (such as .hex) for consistency, then discuss that elsewhere?

jgarzik · 2015-10-19T05:30:39Z

It is fair to open the topic - not disputing that.

What is the best for the most likely users? If most users are automated, content-type would seem reasonable.

lclc · 2015-10-19T06:55:49Z

If we want to have it easy to alter the URL I would vote for using GET for everything ;)

With the RPC interface we already have a easy to use API for command line usage.
I see the REST API as a self-hosted, potentially decentralize alternative to REST APIs from Blockchain.info, Blocktrail, etc.

Does anyone know a project that is using the REST API?

jonasschnelli · 2015-10-19T06:55:53Z

If I think about it without looking at other REST API designs, I like the file extension to determine the output format. Adding a file extension to the URL is much easier as adding a content-type request header.

I could see:

URL file _extension_ = desired output forma
request header _content-type_ = POST input format

Non post commands (currently everything expect rest/getutxo and this new command) would ignore the content-type request header because they have no input data (except to the URL itself).

If this makes sense, i think this PR would only need to evaluate the content-type request header and parse/deserialize the input POST data in the given format.

jonasschnelli · 2015-10-19T07:04:16Z

Using GET has some length limitations (not in RFC [there it is uint32max IIRC] but in most implementations). Using POST for posting data makes sense IMO.

Another nice addition would be to support an array of transactions.
Either content-type: application/json with ["<hex>", ["<hex>", ...]] or by binary stream (content-type: application/octet-stream): <varint><vector:transactions>.

I think the later, allowing tx submission without doing the JSON enc-/decoding, would be desirable.

lclc · 2015-10-19T07:37:12Z

I don't think Bitcoin Core should do it different then everyone else because we think it's easier.
But with this PR I just wanted to add the functionality to send raw transactions, not changing the general API format. Maybe we could discuss that in a new Github issue?
And keep this PR for now the same like it is for the other methods.

Another topic to discuss there is adding a REST API version method.

Regarding an array of transaction:
I'd keep it simple & stupid until there is a demand for more functionality. Since there wasn't such a demand for the sendrawtransaction RPC call I don't expect there is for the REST API.

jonasschnelli · 2015-10-19T08:06:45Z

@lclc: fair enough.
Supporting input formats – although – should be done to keep constancy. rest/getutxo also supports posting JSON/HEX/BIN as input format. But i agree – can also be done later.

lclc · 2015-10-19T08:20:13Z

Thanks. I'll add JSON & BIN as input formats next, determining them from the requested output format based on the 'file' ending in the URL (I expect if someone sends binary he wants binary back. Other scenarios can be covered ones we agree or disagree on Content-Type & Accept for data formats).

jonasschnelli · 2015-10-19T08:25:22Z

[...] I expect if someone sends binary he wants binary back [...]

Right. That exactly how rest/getutxo works.

jgarzik · 2015-10-19T12:52:25Z

In terms of HTTP "verbs", the method can be GET but should be POST or PUT.

laanwj · 2015-10-19T14:18:09Z

Please don't add a JSON input format. For security reasons (to reduce attack surface as there is no authentication), we don't want to parse complex input in REST (HEX and BIN are fine).
The verb should be POST or PUT. GET is not acceptable for submitting new data in REST, it should be reserved for operations without side effects.

jgarzik · 2015-10-27T19:34:13Z

Re-reviewing latest version:

Send different URI/behavior to a different dispatch function - avoid rest_tx() - and you don't need to reformat huge amounts of untouched code, vastly shrinking diff.
Agree w/ @laanwj comments: Verb should be PUT or POST (done) and avoid JSON input.

ACK with those updates

lclc · 2015-10-27T20:31:53Z

Thanks for the review. It's strange that Travis says the checks passed even when they actually didn't (https://travis-ci.org/bitcoin/bitcoin/builds/87533414).

Makes sense, moved the POST /rest/tx (without /) code to rest_tx_sendrawtx (although the naming isn't consistent anymore now, anyone has a better idea for a function name?).

My binary tests (see latest part of rest.py) are still failing. Maybe someone has an idea whats wrong there or in the code? Thanks.

sipa · 2015-10-28T01:43:16Z

src/rest.cpp

The 6th argument is not fRejectAbsurdFee currently.

Thanks, set both to false now.

maflcko · 2015-10-28T09:33:32Z

src/rest.cpp

Why would you set it to false? A better (but not perfect) solution would be something like maflcko@18f5df4#diff-52257e4c1cedbf5e302f183065cab880L812

But #6726 is not yet merged and needs more work anyway. So let's just consider this as NIT and I will clean it up in a later PR if this gets merged before #6726.

Also travis fails with "absurdly-high-fee" so you'd have to pass it on, I guess. (Haven't looked closely, yet)

But then this is more like a blocking NIT.

I think it's job of the API user / wallet to check if the fee is too high or not, instead of trusting the node to check this and care about it.

The Travis error regarding "absurdly-high-fee" comes later, not because of this but because of the RPC-call to cross-check the result I do there. I'm aware of that, but it already fails earlier.

There is no wallet when you use raw transactions...

The rpc interface checks for high fee by default and you can disable it with an additional argument. Any chance you could implement something like that?

Somewhere there is a wallet to create that raw transaction.

IMO it should be checked on the sender side (the user of the REST API) if
the fee is too high and not trust the node with this.

E.g. someone could implement a web wallet that only relies on some nodes it
doesn't control so you trust that node that it actually cares if you added
a too high fee or not. False security.
E.g. Nodes operated by miners have no incentive to tell the user of their
REST API the truth.
Am 28.10.2015 11:23 schrieb "MarcoFalke" [email protected]:

In src/rest.cpp
#6844 (comment):

else

{

if (!DecodeHexTx(tx, strTx))

return RESTERR(req, HTTP_BAD_REQUEST, strprintf("TX decode failed %s",strTx));

}

uint256 hashTx = tx.GetHash();

CCoinsViewCache &view = *pcoinsTip;

const CCoins* existingCoins = view.AccessCoins(hashTx);

bool fHaveMempool = mempool.exists(hashTx);

bool fHaveChain = existingCoins && existingCoins->nHeight < 1000000000;

if (!fHaveMempool && !fHaveChain) {

// push to local node and sync with wallets

CValidationState state;

bool fMissingInputs;

if (!AcceptToMemoryPool(mempool, state, tx, false, &fMissingInputs, false, false)) {

There is no wallet when you use raw transactions...

The rpc interface checks for high fee by default and you can disable it
with an additional argument. Any chance you could implement something like
that?

—
Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/6844/files#r43237619.

lclc · 2015-11-01T12:28:10Z

Ok.
What do you prefer @sipa , fRejectAbsurdFee always to be true or as an extra parameter to pass?

@MarcoFalke that's not the problem. You can run the same tx with hex (currently commented out) and it works. I'll look at this again when I've changed the Fee thing

dcousens · 2015-11-18T13:11:18Z

src/rest.cpp

why is this parameter still here?
Isn't the POST expecting a pure octet stream?

It seems you're expecting: transaction=<binary>, that seems very odd.
It should just be <binary>

Agreed. This needs to be removed. strBody should treated as equivalent to strTx.

maflcko · 2015-11-18T15:54:51Z

@lclc So why would travis fail if that's not the problem? You can try to set it to false and the see if qa/pull-tester/rpc-tests.py rest fails.

dcousens · 2015-11-19T03:01:19Z

@lclc I'm happy to take over this if you're busy

lclc · 2015-11-19T19:50:38Z

Sorry didn't have time for this, started at a new job (in Bitcoin), been abroad the last 3 weeks :)

@dcousens If you like to finish it that would be great, so maybe this makes it into 0.12. I can give you merge rights on my Github fork so we don't lose the discussions in here?

Otherwise I'll work on it again at the last weekend before December.

dcousens · 2015-11-19T20:51:41Z

@lclc sounds good, add me when you can.

paveljanik · 2016-01-20T20:48:47Z

Needs rebase.

laanwj · 2016-05-09T10:49:50Z

I'm still not entirely convinced that submitting anything belongs on this interface.

I've always felt that the REST interface was read-only on purpose - a way to query data that is public anyway without authentication. Making is possible to submit something changes the security expectations.

lclc · 2016-05-22T09:50:48Z

Closing this PR for now, to keep this interface read-only.

dcousens · 2016-05-22T10:07:41Z

to keep this interface read-only.

I think that is a valid reason 👍

lclc mentioned this pull request Oct 17, 2015

Retreive Unspent outputs from Bitcoin Core REST API OutCast3k/coinbin#27

Open

dcousens reviewed Oct 17, 2015
View reviewed changes

lclc force-pushed the sendrawtransactionREST branch from 01b1536 to ff5f52c Compare October 18, 2015 13:17

lclc changed the title ~~[REST] Add sendrawtransaction~~ [REST] Add send raw transaction Oct 18, 2015

dcousens reviewed Oct 18, 2015
View reviewed changes

laanwj added the REST label Oct 19, 2015

dcousens mentioned this pull request Oct 20, 2015

http: Restrict maximum size of http + headers #6859

Merged

lclc force-pushed the sendrawtransactionREST branch from ff5f52c to c818404 Compare October 26, 2015 20:09

lclc force-pushed the sendrawtransactionREST branch from c818404 to 91687ba Compare October 27, 2015 21:34

sipa reviewed Oct 28, 2015
View reviewed changes

lclc force-pushed the sendrawtransactionREST branch from 91687ba to d28e974 Compare October 28, 2015 07:20

maflcko reviewed Oct 28, 2015
View reviewed changes

dcousens reviewed Nov 18, 2015
View reviewed changes

lclc force-pushed the sendrawtransactionREST branch from d28e974 to 8c28d71 Compare December 3, 2015 14:29

[REST] Add send raw transaction

7f63a2e

lclc force-pushed the sendrawtransactionREST branch from 8c28d71 to 7f63a2e Compare December 3, 2015 15:39

laanwj added RPC/REST/ZMQ and removed REST labels Mar 29, 2016

lclc closed this May 22, 2016

dcousens deleted the sendrawtransactionREST branch May 22, 2016 10:06

dcousens restored the sendrawtransactionREST branch May 22, 2016 10:06

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[REST] Add send raw transaction #6844

[REST] Add send raw transaction #6844

Uh oh!

Conversation

lclc commented Oct 17, 2015

Uh oh!

sipa commented Oct 17, 2015 via email

Uh oh!

jgarzik commented Oct 17, 2015

Uh oh!

dcousens commented Oct 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens commented Oct 17, 2015

Uh oh!

jgarzik commented Oct 18, 2015

Uh oh!

jonasschnelli commented Oct 18, 2015

Uh oh!

lclc commented Oct 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens commented Oct 18, 2015

Uh oh!

dcousens commented Oct 18, 2015

Uh oh!

jgarzik commented Oct 19, 2015

Uh oh!

dcousens commented Oct 19, 2015

Uh oh!

jgarzik commented Oct 19, 2015

Uh oh!

lclc commented Oct 19, 2015

Uh oh!

jonasschnelli commented Oct 19, 2015

Uh oh!

jonasschnelli commented Oct 19, 2015

Uh oh!

lclc commented Oct 19, 2015

Uh oh!

jonasschnelli commented Oct 19, 2015

Uh oh!

lclc commented Oct 19, 2015

Uh oh!

jonasschnelli commented Oct 19, 2015

Uh oh!

jgarzik commented Oct 19, 2015

Uh oh!

laanwj commented Oct 19, 2015

Uh oh!

jgarzik commented Oct 27, 2015

Uh oh!

lclc commented Oct 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lclc commented Nov 1, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!