rpc: Avoid creating non-standard raw transactions #14890

1. 
   MarcoFalke commented at 9:06 PM on December 6, 2018: member

   Multiple OP_RETURN outputs in a transaction are not standard and unlikely to be relayed, so avoid creating them.

   Apart from that, the logic was broken in that it duplicated the same hex-data for each data output: Closes #14868.

2. MarcoFalke added the label Needs backport on Dec 6, 2018
3. MarcoFalke added this to the milestone 0.17.2 on Dec 6, 2018
4. in src/rpc/rawtransaction.cpp:423 in fa0f488dff outdated

    418 | +    bool has_data{false};
    419 | +
    420 |      for (const std::string& name_ : outputs.getKeys()) {
    421 |          if (name_ == "data") {
    422 | +            if (has_data) {
    423 | +                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, duplicate data");
   

   ---

   ---

   

   Empact commented at 9:10 PM on December 6, 2018:

   nit: "duplicate data" could be misleading, as it could refer to the key or the value.

   ---

   

   MarcoFalke commented at 9:11 PM on December 6, 2018:

   They are key-value pairs and neither the data key, nor the data value are allowed to be duplicate?

   ---

   

   Empact commented at 9:24 PM on December 6, 2018:

   True, but the error triggered is irrespective of the data itself, "Invalid parameter, duplicate key: data" would be more clear IMO. Def a nit, though.

   ---

   

   MarcoFalke commented at 9:35 PM on December 6, 2018:

   Ok, fixed

5. 
   gmaxwell commented at 9:14 PM on December 6, 2018: contributor

   utACK

6. MarcoFalke force-pushed on Dec 6, 2018
7. meshcollider added the label RPC/REST/ZMQ on Dec 6, 2018
8. 
   DrahtBot commented at 9:34 PM on December 6, 2018: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #14888 (Fix createrawtransaction multi op return - issue #14868 by isghe)
   • #14877 (rpc: Document default values for optional arguments 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.

9. MarcoFalke force-pushed on Dec 6, 2018
10. MarcoFalke added the label Needs gitian build on Dec 6, 2018
11. 
    Empact commented at 9:43 PM on December 6, 2018: member

    rpcwallet.cpp also calls ConstructTransaction, has docs to update.

12. rpc: Avoid creating non-standard raw transactions fa4c8679ed
13. MarcoFalke force-pushed on Dec 6, 2018
14. MarcoFalke removed the label Needs gitian build on Dec 6, 2018
15. 
    isghe commented at 10:55 PM on December 6, 2018: contributor

    wouldn't be better to delegate to IsStandardTx function, in "src/ policy/policy.cpp", to check if the tx is standard or not? I made a test in https://github.com/isghe/bitcoin/commits/raise-error-createrawtransaction-multi-OP_RETURN and it looks is working good.

    I just added on top of #14888

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

    with this result:

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

    without spreading the source code, on how to decide if a transaction is standard or not

16. 
    Empact commented at 11:20 PM on December 6, 2018: member

    utACK fa4c8679ed94f215ce895938f7c3c169a2ce101e

17. 
    promag commented at 11:40 PM on December 6, 2018: member

    @isghe note that this PR ensures the RPC is correct, rejects duplicate data objects, and the error is clear for the client.

    utACK fa4c867.

18. 
    isghe commented at 11:52 PM on December 6, 2018: contributor

    before merging this PR I think we should consider also the discussion on #14888

19. 
    isghe commented at 11:54 PM on December 6, 2018: contributor

    who decide if a transaction is standard or not? createrawtransaction or IsStandardTx? In the first case, I think it will be a security problem.

20. 
    laanwj commented at 4:19 PM on December 7, 2018: member

    utACK fa4c8679ed94f215ce895938f7c3c169a2ce101e

21. laanwj merged this on Dec 7, 2018
22. laanwj closed this on Dec 7, 2018

    ---

23. laanwj referenced this in commit d38a2c1416 on Dec 7, 2018
24. MarcoFalke deleted the branch on Dec 7, 2018
25. MarcoFalke referenced this in commit 46c162df47 on Dec 7, 2018
26. fanquake removed the label Needs backport on Dec 13, 2018
27. 
    fanquake commented at 2:13 PM on December 13, 2018: member

    Removing "Needs Backport" as this is being done in #14893.

28. deadalnix referenced this in commit f1534f8274 on Sep 2, 2020
29. MarcoFalke locked this on Sep 8, 2021

Contributors

MarcoFalkeEmpactgmaxwellDrahtBotisghepromaglaanwjfanquake

Milestone
0.17.2