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

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

Merged
merged 2 commits into from Aug 10, 2015

Conversation

Projects
None yet
@paveljanik
Contributor

paveljanik commented Jun 27, 2015

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:
$ echo -n "This OP_RETURN transaction output was created by modified createrawtransaction." | xxd -p -c 200
54686973204f505f52455455524e207472616e73616374696f6e206f7574707574207761732063726561746564206279206d6f646966696564206372656174657261777472616e73616374696f6e2e
$
  • call createrawtransaction with it:
$ bitcoin-cli -testnet createrawtransaction "[{\"txid\":\"f7640e528c4ecbcf437adc22e1a02634aefcd286b0affc8a0edde34dcfaef60b\",\"vout\":1}]" "{\"data\":\"54686973204f505f52455455524e207472616e73616374696f6e206f7574707574207761732063726561746564206279206d6f646966696564206372656174657261777472616e73616374696f6e2e\", \"msj42CCGruhRsFrGATiUuh25dtxYtnpbTx\": 1.89999 }"
01000000010bf6aecf4de3dd0e8afcafb086d2fcae3426a0e122dc7a43cfcb4e8c520e64f70100000000ffffffff020000000000000000526a4c4f54686973204f505f52455455524e207472616e73616374696f6e206f7574707574207761732063726561746564206279206d6f646966696564206372656174657261777472616e73616374696f6e2e9827530b000000001976a91485eb47fe98f349065d6f044e27a4ac541af79ee288ac00000000
$

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

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

  1. only one OP_RETURN transaction output in the transaction
  2. 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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 27, 2015

Contributor

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

Contributor

paveljanik commented Jun 27, 2015

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

@lapp0

This comment has been minimized.

Show comment
Hide comment
@lapp0

lapp0 Jun 27, 2015

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.

lapp0 commented Jun 27, 2015

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.

@luke-jr

View changes

Show outdated Hide outdated src/rpcrawtransaction.cpp
"\n"
" If you want to create OP_RETURN output instead of an address output, entries should be in the following format:\n"
"\n"
" \"OP_RETURN\": \"hex\" (string, required) The key is \"OP_RETURN\", the value is hex encoded data\n"

This comment has been minimized.

@luke-jr

luke-jr Jun 27, 2015

Member

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

@luke-jr

luke-jr Jun 27, 2015

Member

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

This comment has been minimized.

@paveljanik

paveljanik Jun 28, 2015

Contributor

Done.

@paveljanik

paveljanik Jun 28, 2015

Contributor

Done.

This comment has been minimized.

@luke-jr

luke-jr Jun 28, 2015

Member

Don't forget to update docs here.

@luke-jr

luke-jr Jun 28, 2015

Member

Don't forget to update docs here.

@luke-jr

View changes

Show outdated Hide outdated src/rpcrawtransaction.cpp
@@ -334,12 +334,18 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
" ,...\n"
" }\n"
"\n"
" If you want to create OP_RETURN output instead of an address output, entries should be in the following format:\n"

This comment has been minimized.

@luke-jr

luke-jr Jun 27, 2015

Member

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

@luke-jr

luke-jr Jun 27, 2015

Member

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

@luke-jr

View changes

Show outdated Hide outdated src/rpcrawtransaction.cpp
setAddress.insert(address);
if (name_ == "OP_RETURN") {
if (fOpReturnFound)
throw JSONRPCError(RPC_INVALID_PARAMETER, string("Only one OP_RETURN transaction output permitted"));

This comment has been minimized.

@luke-jr

luke-jr Jun 27, 2015

Member

createrawtransaction shouldn't enforce policy.

@luke-jr

luke-jr Jun 27, 2015

Member

createrawtransaction shouldn't enforce policy.

This comment has been minimized.

@paveljanik

paveljanik Jun 28, 2015

Contributor

Policy check removed.

@paveljanik

paveljanik Jun 28, 2015

Contributor

Policy check removed.

@luke-jr

View changes

Show outdated Hide outdated src/rpcrawtransaction.cpp
CTxOut out(nAmount, scriptPubKey);
rawTx.vout.push_back(out);
if (data.size() > MAX_OP_RETURN_RELAY)
throw JSONRPCError(RPC_INVALID_PARAMETER, string("OP_RETURN data too long"));

This comment has been minimized.

@luke-jr

luke-jr Jun 27, 2015

Member

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

@luke-jr

luke-jr Jun 27, 2015

Member

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

This comment has been minimized.

@paveljanik

paveljanik Jun 28, 2015

Contributor

I have removed policy checks completely.

@paveljanik

paveljanik Jun 28, 2015

Contributor

I have removed policy checks completely.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 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).

Member

luke-jr commented Jun 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 27, 2015

Member

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.

Member

jonasschnelli commented Jun 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 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:

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

@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?

Contributor

paveljanik commented Jun 27, 2015

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:

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

@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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 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.

Member

luke-jr commented Jun 27, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 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).

Contributor

paveljanik commented Jun 27, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 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
Contributor

jgarzik commented Jun 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 28, 2015

Contributor

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

Contributor

paveljanik commented Jun 28, 2015

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

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

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

Contributor

paveljanik commented Jun 28, 2015

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.

@luke-jr

View changes

Show outdated Hide outdated src/bitcoin-tx.cpp
@@ -69,6 +69,7 @@ static bool AppInitRawTx(int argc, char* argv[])
strUsage += HelpMessageOpt("locktime=N", _("Set TX lock time to N"));
strUsage += HelpMessageOpt("nversion=N", _("Set TX version to N"));
strUsage += HelpMessageOpt("outaddr=VALUE:ADDRESS", _("Add address-based output to TX"));
strUsage += HelpMessageOpt("outdata=VALUE:DATA", _("Add data-based output to TX"));

This comment has been minimized.

@luke-jr

luke-jr Jun 28, 2015

Member

Is there a use case for burning a value?

@luke-jr

luke-jr Jun 28, 2015

Member

Is there a use case for burning a value?

This comment has been minimized.

@jgarzik

jgarzik Jun 28, 2015

Contributor

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.

@jgarzik

jgarzik Jun 28, 2015

Contributor

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.

This comment has been minimized.

@paveljanik

paveljanik Jun 28, 2015

Contributor

This is the best solution, yes.

@paveljanik

paveljanik Jun 28, 2015

Contributor

This is the best solution, yes.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

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

Contributor

paveljanik commented Jun 28, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 28, 2015

Contributor

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

Contributor

paveljanik commented Jun 28, 2015

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

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 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).

Contributor

paveljanik commented Jun 28, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 28, 2015

Contributor

ut ACK

Contributor

jgarzik commented Jun 28, 2015

ut ACK

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 28, 2015

Member

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

Member

theuni commented Jun 28, 2015

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

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 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?

Contributor

paveljanik commented Jun 28, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@theuni

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

Member

theuni commented Jun 28, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 29, 2015

Member

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

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

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

Jonass-MacBook-Pro:bitcoin jonasschnelli$ ./src/bitcoin-tx -create outdata=0.0001:6a6f6e61737363686e656c6c69

01000000000110270000000000000f6a0d6a6f6e61737363686e656c6c6900000000

./src/bitcoin-cli -regtest createrawtransaction [] '{"data":"6a6f6e61737363686e656c6c69"}'

01000000000100000000000000000f6a0d6a6f6e61737363686e656c6c6900000000

The decoded bitcoin-tx transaction looks like:

  "txid": "3f37bbc46d2171c20d2d68a7d7d687fc556bd8b9d1e964f95b7954d9681d5d23",
  "version": 1,
  "locktime": 0,
  "vin": [
  ],
  "vout": [
    {
      "value": 0.00010000,
      "n": 0,
      "scriptPubKey": {
        "asm": "OP_RETURN 6a6f6e61737363686e656c6c69",
        "hex": "6a0d6a6f6e61737363686e656c6c69",
        "type": "nulldata"
      }
    }
  ]

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

Member

jonasschnelli commented Jun 29, 2015

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

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

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

Jonass-MacBook-Pro:bitcoin jonasschnelli$ ./src/bitcoin-tx -create outdata=0.0001:6a6f6e61737363686e656c6c69

01000000000110270000000000000f6a0d6a6f6e61737363686e656c6c6900000000

./src/bitcoin-cli -regtest createrawtransaction [] '{"data":"6a6f6e61737363686e656c6c69"}'

01000000000100000000000000000f6a0d6a6f6e61737363686e656c6c6900000000

The decoded bitcoin-tx transaction looks like:

  "txid": "3f37bbc46d2171c20d2d68a7d7d687fc556bd8b9d1e964f95b7954d9681d5d23",
  "version": 1,
  "locktime": 0,
  "vin": [
  ],
  "vout": [
    {
      "value": 0.00010000,
      "n": 0,
      "scriptPubKey": {
        "asm": "OP_RETURN 6a6f6e61737363686e656c6c69",
        "hex": "6a0d6a6f6e61737363686e656c6c69",
        "type": "nulldata"
      }
    }
  ]

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 29, 2015

Member

Tested ACK (nits see previous comment).

Member

jonasschnelli commented Jun 29, 2015

Tested ACK (nits see previous comment).

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 29, 2015

Contributor

@jonasschnelli Current master (without OP_RETURN changes):

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

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.

Contributor

paveljanik commented Jun 29, 2015

@jonasschnelli Current master (without OP_RETURN changes):

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

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 29, 2015

Member

@paveljanik: Yes. This makes sense.

Member

jonasschnelli commented Jun 29, 2015

@paveljanik: Yes. This makes sense.

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

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

Contributor

dexX7 commented Jun 29, 2015

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

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

Contributor

paveljanik commented Jun 29, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 29, 2015

Contributor

Squashed, separated both changes and their tests.

Contributor

paveljanik commented Jun 29, 2015

Squashed, separated both changes and their tests.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 29, 2015

Member

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

Member

luke-jr commented Jun 29, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 2, 2015

Member

While testing i was stumbled over this issue...

jonasschnelli$ ./src/bitcoin-cli --regtest sendrawtransaction 010000000156a1172a1cfeede2f1c868f737b5c10c9d39bb85c05e45fae01bffbf7cc9194a0000000049483045022100b84e6381224773ffc3a5f1040e1bc675046b48bfad0655e947032dacd946b7370220166ebc760ea78c18ea49a4686fd751f206859b9f3d8670c3ec6d8400ce232f7e01ffffffff0100000000000000000c6a0a68656c6c6f776f726c6400000000
error: {"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.

Member

jonasschnelli commented Jul 2, 2015

While testing i was stumbled over this issue...

jonasschnelli$ ./src/bitcoin-cli --regtest sendrawtransaction 010000000156a1172a1cfeede2f1c868f737b5c10c9d39bb85c05e45fae01bffbf7cc9194a0000000049483045022100b84e6381224773ffc3a5f1040e1bc675046b48bfad0655e947032dacd946b7370220166ebc760ea78c18ea49a4686fd751f206859b9f3d8670c3ec6d8400ce232f7e01ffffffff0100000000000000000c6a0a68656c6c6f776f726c6400000000
error: {"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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 2, 2015

Member

Concept ACK. Especially the bitcoin-tx part.

Member

laanwj commented Jul 2, 2015

Concept ACK. Especially the bitcoin-tx part.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jul 2, 2015

Contributor

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

Contributor

paveljanik commented Jul 2, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

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

Member

laanwj commented Jul 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jul 2, 2015

Contributor

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

Contributor

paveljanik commented Jul 2, 2015

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 2, 2015

Member

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.

Member

jonasschnelli commented Jul 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 2, 2015

Member

Tested ACK.
Wrote a simple rpc test for rawtransaction.py jonasschnelli@5aa299f

Member

jonasschnelli commented Jul 2, 2015

Tested ACK.
Wrote a simple rpc test for rawtransaction.py jonasschnelli@5aa299f

@laanwj laanwj added the RPC/REST/ZMQ label Jul 3, 2015

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jul 6, 2015

Member

Concept ACK

Member

btcdrak commented Jul 6, 2015

Concept ACK

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jul 12, 2015

Contributor

More reviews please, thank you.

Contributor

paveljanik commented Jul 12, 2015

More reviews please, thank you.

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Jul 15, 2015

Contributor

Tested, works as expected.

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

$ ./src/bitcoin-cli createrawtransaction '[]' '{"data":"74657374"}'
0100000000010000000000000000066a047465737400000000
$ ./src/bitcoin-cli fundrawtransaction '0100000000010000000000000000066a047465737400000000'
{
  "hex": "0100000001197c6aafa1edb66059ca6adc21e441c3aef90cddede30f903298a919b6e4b8a70000000000feffffff020000000000000000066a047465737453f1052a010000001976a914a7abbb3caf9ad0632a61a2a0c8c52cd6f680a55688ac00000000",
  "changepos": 1,
  "fee": 0.00000173
}
Contributor

dexX7 commented Jul 15, 2015

Tested, works as expected.

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

$ ./src/bitcoin-cli createrawtransaction '[]' '{"data":"74657374"}'
0100000000010000000000000000066a047465737400000000
$ ./src/bitcoin-cli fundrawtransaction '0100000000010000000000000000066a047465737400000000'
{
  "hex": "0100000001197c6aafa1edb66059ca6adc21e441c3aef90cddede30f903298a919b6e4b8a70000000000feffffff020000000000000000066a047465737453f1052a010000001976a914a7abbb3caf9ad0632a61a2a0c8c52cd6f680a55688ac00000000",
  "changepos": 1,
  "fee": 0.00000173
}
@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Aug 4, 2015

Member

needs rebase

Member

btcdrak commented Aug 4, 2015

needs rebase

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Aug 6, 2015

Contributor

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

Contributor

paveljanik commented Aug 6, 2015

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

@laanwj laanwj merged commit 627468d into bitcoin:master Aug 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 10, 2015

Merge pull request #6346
627468d Add support for data-based outputs (OP_RETURN) to bitcoin-tx. (Pavel Janík)
d707853 Add OP_RETURN support in createrawtransaction RPC call, add tests. (Pavel Janík)
@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Aug 10, 2015

Contributor

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

Contributor

dgenr8 commented Aug 10, 2015

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

if (name_ == "data") {
std::vector<unsigned char> data = ParseHexV(sendTo[name_].getValStr(),"Data");
CTxOut out(0, CScript() << OP_RETURN << data);

This comment has been minimized.

@jtimon

jtimon Jul 21, 2016

Member

FIX Why did this OP_RETURN got here?

@jtimon

jtimon Jul 21, 2016

Member

FIX Why did this OP_RETURN got here?

This comment has been minimized.

@paveljanik

paveljanik Jul 22, 2016

Contributor

Because it is part of the "data" transaction?

@paveljanik

paveljanik Jul 22, 2016

Contributor

Because it is part of the "data" transaction?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment