Add OP_RETURN support in createrawtransaction RPC call, add tests.

paveljanik commented at 2:41 pm on June 27, 2015: contributor

This change adds support for specifying OP_RETURN transaction output in createrawtransaction RPC call and automatic tests of this functionality to the testsuite.

Usage:

generate the data you want to put in the OP_RETURN output:

0$ echo -n "This OP_RETURN transaction output was created by modified createrawtransaction." | xxd -p -c 200
154686973204f505f52455455524e207472616e73616374696f6e206f7574707574207761732063726561746564206279206d6f646966696564206372656174657261777472616e73616374696f6e2e
2$

call createrawtransaction with it:

0$ bitcoin-cli -testnet createrawtransaction "[{\"txid\":\"f7640e528c4ecbcf437adc22e1a02634aefcd286b0affc8a0edde34dcfaef60b\",\"vout\":1}]" "{\"data\":\"54686973204f505f52455455524e207472616e73616374696f6e206f7574707574207761732063726561746564206279206d6f646966696564206372656174657261777472616e73616374696f6e2e\", \"msj42CCGruhRsFrGATiUuh25dtxYtnpbTx\": 1.89999 }"
101000000010bf6aecf4de3dd0e8afcafb086d2fcae3426a0e122dc7a43cfcb4e8c520e64f70100000000ffffffff020000000000000000526a4c4f54686973204f505f52455455524e207472616e73616374696f6e206f7574707574207761732063726561746564206279206d6f646966696564206372656174657261777472616e73616374696f6e2e9827530b000000001976a91485eb47fe98f349065d6f044e27a4ac541af79ee288ac00000000
2$

The resulting transaction can be seen here: https://www.blocktrail.com/tBTC/tx/519cb9ac3541a2589c909750b7749048ed52df3e8345bb22510c76d14145cca6

~~This modification follows the current rules, i.e.:

only one OP_RETURN transaction output in the transaction
no more than 80 bytes of data~~

As OP_RETURN outputs are not spendable and can be pruned from UTXO sets, we should encourage its usage. This PR adds this possibility to more Bitcoin Core users.

TODO/need help: rewrite createrawtransaction help. I only did a minimal modification of its help text.

paveljanik commented at 3:02 pm on June 27, 2015: contributor

~~FIXME: allow more than one OP_RETURN on testnet and regtest according to chain params.~~

lapp0 commented at 4:24 pm on June 27, 2015: none

OP_RETURN transactions are relayed in order to minimize harm to the network. Do you think this change will convert people from UTXO spam to the less-bad OP_RETURN spam, or will it just make it easier for non-spammers to start spamming? I have a feeling it’s the latter unfortunately.

in src/rpcrawtransaction.cpp: in 8f7a99b598 outdated

333@@ -334,12 +334,18 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
334             "      ,...\n"
335             "    }\n"
336 
337+            "\n"
338+            "    If you want to create OP_RETURN output instead of an address output, entries should be in the following format:\n"
339+            "\n"
340+            "      \"OP_RETURN\": \"hex\" (string, required) The key is \"OP_RETURN\", the value is hex encoded data\n"

luke-jr commented at 5:51 pm on June 27, 2015:

Key probably should be named “commitment” or “data”, not “OP_RETURN” (which is just a technical implementation detail).

paveljanik commented at 9:38 am on June 28, 2015:

Done.

luke-jr commented at 5:02 pm on June 28, 2015:

Don’t forget to update docs here.

in src/rpcrawtransaction.cpp: in 8f7a99b598 outdated

333@@ -334,12 +334,18 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
334             "      ,...\n"
335             "    }\n"
336 
337+            "\n"
338+            "    If you want to create OP_RETURN output instead of an address output, entries should be in the following format:\n"

luke-jr commented at 5:51 pm on June 27, 2015:

Prefer expounding on the proper use cases here to avoid people getting the impression it should be used incorrectly.

in src/rpcrawtransaction.cpp: in 8f7a99b598 outdated

384-        if (setAddress.count(address))
385-            throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address: ")+name_);
386-        setAddress.insert(address);
387+        if (name_ == "OP_RETURN") {
388+	    if (fOpReturnFound)
389+                throw JSONRPCError(RPC_INVALID_PARAMETER, string("Only one OP_RETURN transaction output permitted"));

luke-jr commented at 5:52 pm on June 27, 2015:

createrawtransaction shouldn’t enforce policy.

paveljanik commented at 9:38 am on June 28, 2015:

Policy check removed.

in src/rpcrawtransaction.cpp: in 8f7a99b598 outdated

394+            std::vector<unsigned char> data = ParseHexV(sendTo[name_].getValStr(),"OP_RETURN data");
395 
396-        CTxOut out(nAmount, scriptPubKey);
397-        rawTx.vout.push_back(out);
398+            if (data.size() > MAX_OP_RETURN_RELAY)
399+                throw JSONRPCError(RPC_INVALID_PARAMETER, string("OP_RETURN data too long"));

luke-jr commented at 5:53 pm on June 27, 2015:

Again, policy. Although maybe okay if you use nMaxDatacarrierBytes instead.

paveljanik commented at 9:38 am on June 28, 2015:

I have removed policy checks completely.

luke-jr commented at 5:55 pm on June 27, 2015: member

To address @lapp0 ’s concern, I’d recommend instead focussing on doing this in bitcoin-tx, or even more ideal, beginning work on a tx utility library (that bitcoin-tx would wrap).

jonasschnelli commented at 6:45 pm on June 27, 2015: contributor

Nice work! But i also agree with @luke-jr. Best would be to add OP_RETURN support to bitcoin-tx. Together with now merged fundrawtransaction this would make sense. There it would be good to avoid policy and support multiple OP_RETURN outputs.

paveljanik commented at 7:58 pm on June 27, 2015: contributor

bitcoin-tx support added (to make it much easier for me - read: I’m lazy - I allow to specify the value for OP_RETURN transaction output. This is worth a few moments to rethink carefully - opinions? Maybe we should only allow zero as the output value… but this way, it is more generic).

The above createrawtransaction call rewritten for bitcoin-tx looks much better and is easier to red/write:

0$ ./bitcoin-tx -testnet -create in=f7640e528c4ecbcf437adc22e1a02634aefcd286b0affc8a0edde34dcfaef60b:1 outdata=0:54686973204f505f52455455524e207472616e73616374696f6e206f7574707574207761732063726561746564206279206d6f646966696564206372656174657261777472616e73616374696f6e2e outaddr=1.89999:msj42CCGruhRsFrGATiUuh25dtxYtnpbTx
101000000010bf6aecf4de3dd0e8afcafb086d2fcae3426a0e122dc7a43cfcb4e8c520e64f70100000000ffffffff020000000000000000526a4c4f54686973204f505f52455455524e207472616e73616374696f6e206f7574707574207761732063726561746564206279206d6f646966696564206372656174657261777472616e73616374696f6e2e9827530b000000001976a91485eb47fe98f349065d6f044e27a4ac541af79ee288ac00000000
2$

@luke-jr I have to sort in my head first why we should allow to create transaction with two OP_RETURN txouts and mainnet inputs/outputs when the resulting TX is unusable there. The same applies to the size of the data. When we do not check against policy, the output may be unusable and users will ask… When we check against current policy rules, the transactions will be usable. This is the reason why I have chosen to check against policy rules. @lapp0 No opinion on your concern (I do not care enough - there are zillions of other ways to create OP_RETURN outputs and people already use them anyway). I also do not understand @luke-jr ’s answer to your concern 8) Did it addressed your concern at all?

luke-jr commented at 8:14 pm on June 27, 2015: member

@paveljanik Users should not be doing anything with OP_RETURN in the first place. Policy is node-specific and cannot be expected to be consistent. Nothing should be designed around specific policies.

paveljanik commented at 8:24 pm on June 27, 2015: contributor

@luke-jr There are several types of users… :-)

Ad policy: got it. I’ll delete every policy rules checks (even MAX_OP_RETURN_RELAY/nMaxDatacarrierBytes because the transaction can be created on this system but sent via another where there are different rules).

jgarzik commented at 9:53 pm on June 27, 2015: contributor

mostly ACK. Comments:

The long term direction is to move “pure function” stuff out of bitcoind and into bitcoin-tx and similar utilities. IRC discussion has proposed similar tools “bitcoin-key”, etc.
Thus, not implementing it at all in bitcoind is weakly preferred. (“weak” preference = may be useful in the short term, even if long term it is certain the code will be deleted)
Missing bitcoin-tx test

paveljanik commented at 9:19 am on June 28, 2015: contributor

Travis builds fail on some systems - can’t find the file txcreatedata1.hex. Strange. Any ideas?

paveljanik commented at 9:41 am on June 28, 2015: contributor

I’ll keep both createrawtransaction and bitcoin-tx parts here in this PR. We can later decide to remove it. Still have to rewrite the createrawtransaction help text and squash into nice commits.

in src/bitcoin-tx.cpp: in f35154f426 outdated

68@@ -69,6 +69,7 @@ static bool AppInitRawTx(int argc, char* argv[])
69         strUsage += HelpMessageOpt("locktime=N", _("Set TX lock time to N"));
70         strUsage += HelpMessageOpt("nversion=N", _("Set TX version to N"));
71         strUsage += HelpMessageOpt("outaddr=VALUE:ADDRESS", _("Add address-based output to TX"));
72+        strUsage += HelpMessageOpt("outdata=VALUE:DATA", _("Add data-based output to TX"));

luke-jr commented at 5:00 pm on June 28, 2015:

Is there a use case for burning a value?

jgarzik commented at 5:31 pm on June 28, 2015:

It is easy to make the “VALUE:” piece optional, in the colon search/parsing code.

That way the most common use case is outdata=DATA.

However, it is technically possible to put a value in the OP_RETURN output. This is a raw transaction interface, so setting that as an option seems reasonable, even if the use cases are rare.

paveljanik commented at 6:05 pm on June 28, 2015:

This is the best solution, yes.

paveljanik commented at 5:28 pm on June 28, 2015: contributor

@luke-jr I commented on non-zero output value above. I prefer to be able to construct the transaction with non-zero output value/burning the value. I do not personally have a use case for it though. I only want to be more generic.

Yes, I still have to rewrite the docs of createrawtransaction. As a OP_RETURN is a “stranger” in createrawtransaction, I have to come up with a good way to describe it. Not found it yet.

paveljanik commented at 6:31 pm on June 28, 2015: contributor

Implemented @jgarzik ’s solution - the value is now optional, default is 0.

paveljanik commented at 6:58 pm on June 28, 2015: contributor

Help text updated. I do not want to use []/{} notation for optional or required parts of input because it can’t be combined with JSON text in an elegant way. Two spaces after “txid”:“id” added on purpose to fix the formatting. I hope it is clear (but getting too long).

jgarzik commented at 7:45 pm on June 28, 2015: contributor

ut ACK

theuni commented at 8:22 pm on June 28, 2015: member

@paveljanik You need to list the .hex files in Makefile.test.in, otherwise they’re missing from the source tarball.

paveljanik commented at 8:27 pm on June 28, 2015: contributor

@theuni Thanks! But what is the reason for the first and the last Travis builds to be OK and only the rest of builds failing?

theuni commented at 8:30 pm on June 28, 2015: member

@paveljanik Those two just compile, they don’t run the tests. The others failed to open the files for tests, but those two never tried to open them at all.

jonasschnelli commented at 7:05 am on June 29, 2015: contributor

Slightly tested. But i was assuming that this steps should work:

0Jonass-MacBook-Pro:bitcoin jonasschnelli$ ./src/bitcoin-cli -regtest createrawtransaction [] '{"data":"6a6f6e61737363686e656c6c69"}'
101000000000100000000000000000f6a0d6a6f6e61737363686e656c6c6900000000
2Jonass-MacBook-Pro:bitcoin jonasschnelli$ ./src/bitcoin-cli -regtest fundrawtransaction 01000000000100000000000000000f6a0d6a6f6e61737363686e656c6c6900000000
3error: {"code":-32603,"message":"Transaction amount too small"}

I was expecting it would add a vin and a vout for paying the estimated transaction fee.

0Jonass-MacBook-Pro:bitcoin jonasschnelli$ ./src/bitcoin-tx -create outdata=0.0001:6a6f6e61737363686e656c6c69
1
201000000000110270000000000000f6a0d6a6f6e61737363686e656c6c6900000000
3
4./src/bitcoin-cli -regtest createrawtransaction [] '{"data":"6a6f6e61737363686e656c6c69"}'
5
601000000000100000000000000000f6a0d6a6f6e61737363686e656c6c6900000000

The decoded bitcoin-tx transaction looks like:

 0  "txid": "3f37bbc46d2171c20d2d68a7d7d687fc556bd8b9d1e964f95b7954d9681d5d23",
 1  "version": 1,
 2  "locktime": 0,
 3  "vin": [
 4  ],
 5  "vout": [
 6    {
 7      "value": 0.00010000,
 8      "n": 0,
 9      "scriptPubKey": {
10        "asm": "OP_RETURN 6a6f6e61737363686e656c6c69",
11        "hex": "6a0d6a6f6e61737363686e656c6c69",
12        "type": "nulldata"
13      }
14    }
15  ]

I’m not sure, but does it make sense to support a non 0.0 value for the OP_RETURN vout?

jonasschnelli commented at 8:18 am on June 29, 2015: contributor

Tested ACK (nits see previous comment).

paveljanik commented at 9:20 am on June 29, 2015: contributor

@jonasschnelli Current master (without OP_RETURN changes):

0$ bitcoin-cli -testnet createrawtransaction "[{\"txid\":\"f7640e528c4ecbcf437adc22e1a02634aefcd286b0affc8a0edde34dcfaef60b\",\"vout\":1}]" "{\"msj42CCGruhRsFrGATiUuh25dtxYtnpbTx\": 0 }"
101000000010bf6aecf4de3dd0e8afcafb086d2fcae3426a0e122dc7a43cfcb4e8c520e64f70100000000ffffffff0100000000000000001976a91485eb47fe98f349065d6f044e27a4ac541af79ee288ac00000000
2$ bitcoin-cli -testnet fundrawtransaction 01000000010bf6aecf4de3dd0e8afcafb086d2fcae3426a0e122dc7a43cfcb4e8c520e64f70100000000ffffffff0100000000000000001976a91485eb47fe98f349065d6f044e27a4ac541af79ee288ac00000000
3error: {"code":-32603,"message":"Transaction amount too small"}
4$

What else can you expect from a fundrawtransaction when you submit zero value output to it? Maybe it could be more verbose and say something like: “No output needs funding.” or something similar.

Your last question is answered above by jgarzik. We are creating raw transactions. Maybe there is a use case for it. E.g. burning bitcoin value provably instead of sending it to some random addresses.

bitcoin-tx supports specifying the transaction output value for data transaction, but createrawtransaction does not.

jonasschnelli commented at 9:24 am on June 29, 2015: contributor

@paveljanik: Yes. This makes sense.

dexX7 commented at 4:03 pm on June 29, 2015: contributor

Maybe it could be more verbose and say something like: “No output needs funding.” or something similar.

It’s not strictly because there is no output to fund, but because OP_RETURN outputs don’t pass the IsDust check in CWallet::CreateTransaction.

paveljanik commented at 5:01 pm on June 29, 2015: contributor

@dexX7 Yes. And this applies to address outputs as well (not specific to this particular OP_RETURN/data transaction). Hmm, this looks like applying policy checks at the wrong place. Funded dust transaction can be delivered to friendly miner to be mined.

paveljanik force-pushed on Jun 29, 2015

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

Squashed, separated both changes and their tests.

luke-jr commented at 9:52 pm on June 29, 2015: member

Have you given any thought to integrating contracthashtool so people don’t misuse this unnecessarily?

jonasschnelli commented at 7:12 pm on July 2, 2015: contributor

While testing i was stumbled over this issue…

0jonasschnelli$ ./src/bitcoin-cli --regtest sendrawtransaction 010000000156a1172a1cfeede2f1c868f737b5c10c9d39bb85c05e45fae01bffbf7cc9194a0000000049483045022100b84e6381224773ffc3a5f1040e1bc675046b48bfad0655e947032dacd946b7370220166ebc760ea78c18ea49a4686fd751f206859b9f3d8670c3ec6d8400ce232f7e01ffffffff0100000000000000000c6a0a68656c6c6f776f726c6400000000
1error: {"code":-25,"message":""}

This fails because the fee is around 49.99 BTC. The fRejectAbsurdFee check in AcceptToMemoryPool() detect this but the returned error() never gets evaluated and sent back to the user.

Not directly related to this PR.

laanwj commented at 7:21 pm on July 2, 2015: member

Concept ACK. Especially the bitcoin-tx part.

paveljanik commented at 7:25 pm on July 2, 2015: contributor

@jonasschnelli Yes, this is #5913. Almost ready.

laanwj commented at 7:27 pm on July 2, 2015: member

Although I also agree we should deprecate pure utility functions from the RPC at some point, and extending them may raise the wrong expectations. We rejected a similar change by @dgenr8 in #5936 to add a field to locktime to createrawtransaction recently, so this is not entirely conistent.

paveljanik commented at 7:31 pm on July 2, 2015: contributor

This is why I separated both commits. I also think that the first one can be omitted…

jonasschnelli commented at 7:44 pm on July 2, 2015: contributor

I think this is useful to have in the RPC createrawtransaction call. Would it be a bad design to add support for OP_RETURN to fundrawtransaction? Something like that: If there is a OP_RETURN vout, always add a change output with the minimum fee and fund it as usual.

jonasschnelli commented at 7:49 pm on July 2, 2015: contributor

Tested ACK. Wrote a simple rpc test for rawtransaction.py https://github.com/jonasschnelli/bitcoin/commit/5aa299ff2c40b4b5504ab99ecf19887842d7cf39

laanwj added the label RPC on Jul 3, 2015

btcdrak commented at 10:48 am on July 6, 2015: contributor

Concept ACK

paveljanik commented at 7:33 pm on July 12, 2015: contributor

More reviews please, thank you.

dexX7 commented at 8:21 pm on July 15, 2015: contributor

Tested, works as expected. @jonasschnelli: I proposed #6444, and with this PR "fundrawtransaction" actually works out of the box:

0$ ./src/bitcoin-cli createrawtransaction '[]' '{"data":"74657374"}'
10100000000010000000000000000066a047465737400000000
2$ ./src/bitcoin-cli fundrawtransaction '0100000000010000000000000000066a047465737400000000'
3{
4  "hex": "0100000001197c6aafa1edb66059ca6adc21e441c3aef90cddede30f903298a919b6e4b8a70000000000feffffff020000000000000000066a047465737453f1052a010000001976a914a7abbb3caf9ad0632a61a2a0c8c52cd6f680a55688ac00000000",
5  "changepos": 1,
6  "fee": 0.00000173
7}

btcdrak commented at 10:49 pm on August 4, 2015: contributor

needs rebase

Add OP_RETURN support in createrawtransaction RPC call, add tests. d7078533eb

Add support for data-based outputs (OP_RETURN) to bitcoin-tx. 627468d2ea

paveljanik force-pushed on Aug 6, 2015

paveljanik commented at 6:54 pm on August 6, 2015: contributor

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

laanwj merged this on Aug 10, 2015

laanwj closed this on Aug 10, 2015

laanwj referenced this in commit 6bb28058d3 on Aug 10, 2015

dgenr8 commented at 5:30 pm on August 10, 2015: contributor

@laanwj Thank you for noticing the parallel to #5936, rebasing it … :-)

in src/rpcrawtransaction.cpp: in 627468d2ea

386-            throw JSONRPCError(RPC_INVALID_PARAMETER, string("Invalid parameter, duplicated address: ")+name_);
387-        setAddress.insert(address);
388+        if (name_ == "data") {
389+            std::vector<unsigned char> data = ParseHexV(sendTo[name_].getValStr(),"Data");
390+
391+            CTxOut out(0, CScript() << OP_RETURN << data);

jtimon commented at 8:21 pm on July 21, 2016:

FIX Why did this OP_RETURN got here?

paveljanik commented at 5:15 am on July 22, 2016:

Because it is part of the “data” transaction?

MarcoFalke locked this on Sep 8, 2021

Add OP_RETURN support in createrawtransaction RPC call, add tests. #6346