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

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.

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

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.

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?

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

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

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

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

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

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

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

ut ACK

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

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

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

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?

Tested ACK (nits see previous comment).

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

@paveljanik: Yes. This makes sense.

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.

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

Squashed, separated both changes and their tests.

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

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.

Concept ACK. Especially the bitcoin-tx part.

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

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.

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

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.

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

Concept ACK

More reviews please, thank you.

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
}

needs rebase

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

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