Fix createrawtransaction multi op return - issue #14868

[isghe]

fix issue #14868

[gmaxwell]

NAK. Network IsStandard rules prohibit multiple op_returns (https://github.com/bitcoin/bitcoin/blob/master/src/policy/policy.cpp#L135), additional bloat on the blockchain is also not desired. The inputs to this RPC are also an array, so duplicating inputs is also pretty broken.

[isghe]

@gmaxwell so maybe we should raise an error? Because actually it's producing a strange thing

bitcoin-cli createrawtransaction "[]" "{\"data\":\"ee\", \"data\":\"ff\"}"
0200000000020000000000000000036a01ee0000000000000000036a01ee00000000

[Empact]

@isghe you can help review by making your PR minimal:

Patchsets should always be focused. For example, a pull request could add a feature, fix a bug, or refactor code; but not a mixture.
https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#pull-request-philosophy

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #14890 (rpc: Avoid creating non-standard raw transactions by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[isghe]

https://github.com/isghe/bitcoin/commits/raise-error-createrawtransaction-multi-OP_RETURN
just adding on this PR

    const CTransaction rawTxCopy (rawTx);
    std::string error;
    if (!IsStandardTx (rawTxCopy, error)) {
        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter combination: " + error);
    }

obtaining:

$ ./src/bitcoin-cli createrawtransaction "[]" "[{\"mguLQ6eGCHpnmRrDwAjbPpYAHjxY6GneeQ\":\"0.1\"}, {\"data\":\"ee\"}, {\"muLYgM1L3gCzLs1bPX5cVzx3ax1f4vBcpN\":\"0.2\"},{\"data\":\"ff\"}]" | xargs ./src/bitcoin-cli decoderawtransaction
error code: -8
error message:
Invalid parameter combination: multi-op-return

[Empact]

5987b7c makes sense to me, independent of this issue. Calls for another PR IMO.

[isghe]

@Empact my idea, is that createrawtransaction should check if a transaction is standard or not, just delegating to IsStandardTx function. If not, we are spreading the rules in the source code, and doesn't sound good. At this point I think it would be better to create a PR removing hard-coded rules written in createrawtransaction, delegating the all things to IsStandardTx. I'll work on it, in the weekend.

[isghe]

anyway, createrawtransaction could be created as an external tool, without affecting the consensus rules

[Empact]

@isghe to get this done safely and most easily, I recommend approaching this incrementally, e.g.:

1. fixing the immediate issue
2. applying the IsStandardTx check to createrawtransaction
3. removing the then-redundant checks

[gmaxwell]

Rejecting duplicate data fields instead of mangling them is the right thing to do regardless of standardness rules: as I pointed out, the input is a dictionary... and also even if standardness permitted multiple op_returns we probably would not support it.

IsStandard probably shouldn't be run on non-final/signed transactions so it isn't a solution here. (It might actually work, but if so that's largely an accident.) The standardness behaviour of the wallet and the network are disjoint in any case: lets imagine that the network standardness rule changed, we still wouldn't want the wallet creating things the old behaviour would reject until the new behaviour were widely deployed. Sometimes code/behavior that looks the same isn't really the same and isn't a duplicate due to how the code needs to be maintained or modified.

[promag]

NACK, this changes nothing.

[DrahtBot]

Needs rebase

[isghe]

@gmaxwell thanks for the explanation. I think we can close this PR.