[REST] Add send raw transaction

lclc commented at 8:16 pm on October 17, 2015: contributor

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 commented at 8:33 pm on October 17, 2015: member

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 commented at 9:42 pm on October 17, 2015: contributor

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 commented at 11:19 pm on October 17, 2015: contributor

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

in doc/REST-interface.md: in 01b153629e outdated

90@@ -91,6 +91,13 @@ Only supports JSON as output format.
91 Returns transactions in the TX mempool.
92 Only supports JSON as output format.
93 
94+####Send raw transaction
95+`GET /rest/sendrawtransaction/<raw-transaction>.<bin|hex|json>`

dcousens commented at 11:21 pm on October 17, 2015:

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 commented at 11:25 pm on October 17, 2015: contributor

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

jgarzik commented at 3:00 am on October 18, 2015: contributor

agree w/ @dcousens

jonasschnelli commented at 6:35 am on October 18, 2015: contributor

+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 force-pushed on Oct 18, 2015

lclc commented at 1:18 pm on October 18, 2015: contributor

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

lclc renamed this:
~~[REST] Add sendrawtransaction~~
[REST] Add send raw transaction
on Oct 18, 2015

in doc/REST-interface.md: in ff5f52cc4f outdated

19+Parameter: transaction=\<signed transaction\>
20+
21+Submits transaction (serialized, hex-encoded) to local node and network.
22+
23+Returns transaction id in binary, hex-encoded binary, or JSON formats, or an error if the transaction is invalid for any reason.
24+

dcousens commented at 11:02 pm on October 18, 2015:

You should specify:

"application/octet-stream" for plain-old-data
"text/plain" for hex
"application/json" for JSON

lclc commented at 6:39 am on October 19, 2015:

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 commented at 11:05 pm on October 18, 2015: contributor

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

dcousens commented at 11:06 pm on October 18, 2015: contributor

@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 commented at 3:29 am on October 19, 2015: contributor

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 commented at 4:52 am on October 19, 2015: contributor

@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 commented at 5:30 am on October 19, 2015: contributor

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 commented at 6:55 am on October 19, 2015: contributor

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 commented at 6:55 am on October 19, 2015: contributor

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 commented at 7:04 am on October 19, 2015: contributor

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 commented at 7:37 am on October 19, 2015: contributor

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 commented at 8:06 am on October 19, 2015: contributor

@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 commented at 8:20 am on October 19, 2015: contributor

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 commented at 8:25 am on October 19, 2015: contributor

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

Right. That exactly how rest/getutxo works.

laanwj added the label REST on Oct 19, 2015

jgarzik commented at 12:52 pm on October 19, 2015: contributor

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

laanwj commented at 2:18 pm on October 19, 2015: member

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.

lclc force-pushed on Oct 26, 2015

jgarzik commented at 7:34 pm on October 27, 2015: contributor

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 commented at 8:31 pm on October 27, 2015: contributor

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.

lclc force-pushed on Oct 27, 2015

in src/rest.cpp: in 91687badef outdated

444+    bool fHaveChain = existingCoins && existingCoins->nHeight < 1000000000;
445+    if (!fHaveMempool && !fHaveChain) {
446+        // push to local node and sync with wallets
447+        CValidationState state;
448+        bool fMissingInputs;
449+        if (!AcceptToMemoryPool(mempool, state, tx, false, &fMissingInputs, fRejectAbsurdFee)) {

sipa commented at 1:43 am on October 28, 2015:

The 6th argument is not fRejectAbsurdFee currently.

lclc commented at 7:22 am on October 28, 2015:

Thanks, set both to false now.

lclc force-pushed on Oct 28, 2015

in src/rest.cpp: in d28e974265 outdated

442+    bool fHaveChain = existingCoins && existingCoins->nHeight < 1000000000;
443+    if (!fHaveMempool && !fHaveChain) {
444+        // push to local node and sync with wallets
445+        CValidationState state;
446+        bool fMissingInputs;
447+        if (!AcceptToMemoryPool(mempool, state, tx, false, &fMissingInputs, false, false)) {

MarcoFalke commented at 9:33 am on October 28, 2015:

Why would you set it to false? A better (but not perfect) solution would be something like https://github.com/MarcoFalke/bitcoin/commit/18f5df4740c12c182667fc5c886e6605f4537a4b#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.

MarcoFalke commented at 9:34 am on October 28, 2015:

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.

lclc commented at 10:18 am on October 28, 2015:

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.

MarcoFalke commented at 10:22 am on October 28, 2015:

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?

lclc commented at 10:49 am on October 28, 2015:

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” notifications@github.com:

In src/rest.cpp #6844 (review):
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.

in qa/rpc-tests/rest.py: in d28e974265 outdated

420+
421+        #########
422+        # sendrawtransaction as binary, requesting result as binary
423+        #########
424+        txId = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 5.0)
425+        inputs  = [ {'txid' : txId, 'vout' : 1}]

MarcoFalke commented at 10:57 am on October 28, 2015:

You can’t hard code this as 1. Travis seems to fail here.

in qa/rpc-tests/rest.py: in d28e974265 outdated

439+        response_str = response.read()
440+        print response_str #lclc RAUS
441+        response_str = binascii.hexlify(response.read())
442+        print response_str #lclc RAUS
443+        print 2 #lclc RAUS
444+        print self.nodes[0].sendrawtransaction(raw_tx['hex']) #lclc RAUS

MarcoFalke commented at 10:58 am on October 28, 2015:

Fee of .6 btc is too high. Travis seems to fail here

sipa commented at 3:44 pm on October 28, 2015:

Lucas: in a raw API the creator of the transactions cannot verify the fee, because he doesn’t have the actual network’s txouts, so they don’t know the amounts being spent.

It’s a serious layer violation IMHO to make AcceptToMemoryPool perform this check, but I do agree that sendrawtransaction or its equivalent REST interface should do this validation. People do shoot themselves in the foot by spending large inputs and not setting a (correct) change, crazy as it sounds.

lclc commented at 12:28 pm on November 1, 2015: contributor

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

in src/rest.cpp: in d28e974265 outdated

408+        return false;
409+    std::string hashStr;
410+    const RetFormat rf = ParseDataFormat(hashStr, strURIPart);
411+
412+    std::string strBody = req->ReadBody();
413+    if (std::string::npos == strBody.find("transaction="))

dcousens commented at 1:11 pm on November 18, 2015:

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

dcousens commented at 1:12 pm on November 18, 2015:

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

jgarzik commented at 2:46 am on November 19, 2015:

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

MarcoFalke commented at 3:54 pm on November 18, 2015: member

@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 commented at 3:01 am on November 19, 2015: contributor

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

lclc commented at 7:50 pm on November 19, 2015: contributor

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 commented at 8:51 pm on November 19, 2015: contributor

@lclc sounds good, add me when you can.

lclc force-pushed on Dec 3, 2015

[REST] Add send raw transaction 7f63a2ec74

lclc force-pushed on Dec 3, 2015

paveljanik commented at 8:48 pm on January 20, 2016: contributor

Needs rebase.

laanwj added the label RPC/REST/ZMQ on Mar 29, 2016

laanwj removed the label REST on Mar 29, 2016

laanwj commented at 10:49 am on May 9, 2016: member

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 commented at 9:50 am on May 22, 2016: contributor

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

lclc closed this on May 22, 2016

dcousens deleted the branch on May 22, 2016

dcousens restored the branch on May 22, 2016

dcousens commented at 10:07 am on May 22, 2016: contributor

to keep this interface read-only.

I think that is a valid reason :+1:

DrahtBot locked this on Sep 8, 2021

[REST] Add send raw transaction #6844