rpc: Add a parameter to sendrawtransaction which sets a maximum value for unspendable outputs. #25943

pull davidgumberg wants to merge 2 commits into bitcoin:master from davidgumberg:wip-opreturn_nz changing 7 files +85 −3
  1. davidgumberg commented at 9:49 PM on August 27, 2022: contributor

    This PR adds a user configurable, zero by default parameter — maxburnamount — to sendrawtransaction. This PR makes bitcoin core reject transactions that contain unspendable outputs which exceed maxburnamount. closes #25899.

    As a result of this PR, sendrawtransaction will by default block 3 kinds of transactions:

    1. Those that begin with OP_RETURN - (datacarriers)
    2. Those whose lengths exceed the script limit.
    3. Those that contain invalid opcodes.

    The user is able to configure a maxburnamount that will override this check and allow a user to send a potentially unspendable output into the mempool.

    I see two legitimate use cases for this override:

    1. Users that deliberately use OP_RETURN for datacarrier transactions that embed data into the blockchain.
    2. Users that refuse to update, or are unable to update their bitcoin core client would be able to make use of new opcodes that their client doesn't know about.
  2. davidgumberg force-pushed on Aug 27, 2022
  3. davidgumberg force-pushed on Aug 28, 2022
  4. davidgumberg force-pushed on Aug 28, 2022
  5. ghost commented at 7:47 AM on August 28, 2022: none

    Concept ACK

    I am not sure about name of this argument: rejectreturn

  6. kristapsk commented at 8:08 AM on August 28, 2022: contributor

    Concept ACK

  7. in src/rpc/mempool.cpp:68 in dd79c5fe18 outdated 
      61 | @@ -61,12 +62,24 @@ static RPCHelpMan sendrawtransaction()
  62 |              RPCTypeCheck(request.params, {
  63 |                  UniValue::VSTR,
  64 |                  UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
  65 | +                UniValue::VBOOL
  66 |              });
  67 |  
  68 | +            bool reject_return = request.params[2].isNull() ? true : request.params[2].isTrue();
    brunoerg commented at 11:54 AM on August 28, 2022: 
                const bool reject_return = request.params[2].isNull() ? true : request.params[2].isTrue();
    davidgumberg commented at 7:19 PM on August 28, 2022:

    I have pushed this, thank you.

  8. brunoerg commented at 11:56 AM on August 28, 2022: contributor

    Concept ACK

    I agree on @1440000bytes about the name of the argument rejectreturn, I think this name is not so intuitive about what it does.

  9. davidgumberg force-pushed on Aug 28, 2022
  10. davidgumberg commented at 7:21 PM on August 28, 2022: contributor

    @1440000bytes @brunoerg You are right about the name, I am not sure if the name should be something like dontburn or something like reject_nonzero_returns. I've pushed no_burning as something in between. Some of the rpc parameters have very concise names like finalize, replaceable, or extract, and others have names like include_immature_coinbase and subtractfeefromamount.

  11. in src/rpc/mempool.cpp:45 in b829861276 outdated 
      41 | @@ -42,6 +42,7 @@ static RPCHelpMan sendrawtransaction()
  42 |              {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
  43 |               "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
  44 |                   "/kvB.\nSet to 0 to accept any fee rate.\n"},
  45 | +            {"no_burning", RPCArg::Type::BOOL, RPCArg::Default{true}, "If true, reject transactions with non-zero OP_RETURN outputs, which are unspendable and a 'burning' of coins."},
    maflcko commented at 6:39 AM on August 29, 2022:

    I wonder if this can indicate the maximum value for a data output (default: 0) instead of a true/false

    glozow commented at 8:22 AM on August 29, 2022:

    I thinkmax_burn_amount is the right name here, similar to maxfeerate, since that's what this check is most similar to.

    davidgumberg commented at 11:17 PM on August 29, 2022:

    Thanks for the suggestion, I have changed the parameter to maxburnamount (I avoided snake case in the json parameter to keep consistent with maxfeerate and used snake case for our c++ variable max_burn_amount) and ensure that our nValue is not greater.

  12. in src/rpc/mempool.cpp:65 in b829861276 outdated 
      61 | @@ -61,12 +62,24 @@ static RPCHelpMan sendrawtransaction()
  62 |              RPCTypeCheck(request.params, {
  63 |                  UniValue::VSTR,
  64 |                  UniValueType(), // VNUM or VSTR, checked inside AmountFromValue()
  65 | +                UniValue::VBOOL
    maflcko commented at 6:39 AM on August 29, 2022: 
                    UniValue::VBOOL,
    davidgumberg commented at 11:18 PM on August 29, 2022:

    Thanks, I corrected this.

  13. glozow added the label RPC/REST/ZMQ on Aug 29, 2022
  14. bitcoin deleted a comment on Aug 29, 2022
  15. bitcoin deleted a comment on Aug 29, 2022
  16. theStack commented at 11:14 AM on August 29, 2022: contributor

    Concept ACK

  17. davidgumberg force-pushed on Aug 29, 2022
  18. davidgumberg renamed this:
    [WIP] rpc: Add a check in sendrawtransaction for nonzero outputs with OP_RETURN
    [WIP] rpc: Add a parameter to sendrawtransaction which sets a maximum burned output for OP_RETURN transactions.
    on Aug 29, 2022
  19. davidgumberg force-pushed on Aug 29, 2022
  20. davidgumberg force-pushed on Aug 30, 2022
  21. davidgumberg force-pushed on Aug 30, 2022
  22. davidgumberg force-pushed on Aug 30, 2022
  23. davidgumberg commented at 2:06 AM on August 30, 2022: contributor

    I have some questions:

    1. Should the max_burn_amount parameter also be added to the testmempoolaccept call? I am not 100% sure about the use case for this call, in particular why it has the maxfeerate parameter. Is the maxfeerate parameter present because this call is a more general "would sendrawtransaction work on this tx" than "will the mempool accept this"?
    2. I am still trying to understand the BTC transaction language, so I am unsure about whether or not it is possible for OP_RETURN to appear somewhere other than at the top of the scriptPubKey in a valid tx. And if it can, then does the presence of OP_RETURN anywhere in the scriptPubKey guarantee that the value of that output is unspendable? If both are true, then we would need to check every non-data byte of the scriptPubKey for an OP_RETURN. If the former is true and the latter is false, something more complicated might have to happen.
    3. This may be outside of the scope of this pull request, and based on a foolish misunderstanding, but is there any way that we validate that the default parameter promised in the RPC description is actually the default value?
  24. davidgumberg commented at 2:21 AM on August 30, 2022: contributor

    Also, I apologize for the obscene number of force pushes when my first edit fails CI because of a silly mistake, I realize this makes review more of a pain, going forward I'll run the test suite locally before force pushing.

  25. in src/rpc/mempool.cpp:48 in 17f8542a8a outdated 
      40 | @@ -41,7 +41,10 @@ static RPCHelpMan sendrawtransaction()
  41 |              {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
  42 |              {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
  43 |               "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
  44 | -                 "/kvB.\nSet to 0 to accept any fee rate.\n"},
  45 | +                 "/kvB.\nSet to 0 to accept any fee rate."},
  46 | +            {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},
  47 | +             "Reject transactions with OP_RETURN outputs greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
  48 | +             "If set to 0, all transactions with non-zero OP_RETURN outputs, which are unspendable and a 'burning' of coins, will be rejected.\n"},
    unknown commented at 6:32 AM on August 30, 2022: 
                 "Default value is 0 which disallows non-zero OP RETURN outputs.\n"},
  26. in src/util/error.cpp:37 in 17f8542a8a outdated 
      32 | @@ -33,6 +33,8 @@ bilingual_str TransactionErrorString(const TransactionError err)
  33 |              return Untranslated("Specified sighash value does not match value stored in PSBT");
  34 |          case TransactionError::MAX_FEE_EXCEEDED:
  35 |              return Untranslated("Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)");
  36 | +        case TransactionError::MAX_BURN_EXCEEDED:
  37 | +            return Untranslated("OP_RETURN output exceeds maximum set by maxburnamount");
    unknown commented at 6:34 AM on August 30, 2022: 
                return Untranslated("OP_RETURN output exceeds maximum configured by user (-maxburnamount)");
    davidgumberg commented at 7:11 PM on August 30, 2022:

    There's no commandline option like for maxfeerate so there shouldn't be a hyphen, but I'll change to configured by user, thanks.

  27. unknown commented at 6:45 AM on August 30, 2022: none

    Tested maxburnamount parameter added in sendratransaction RPC and it works as expected:

    1. Create a transaction in electrum (signet) with OP_RETURN 74657374 in 'payto' and 0.0001 in amount. Sign and copy the transaction hex.

    2. Try to broadcast transaction in bitcoin core with default settings and custom burn amount:

    $ bitcoin-cli sendrawtransaction 020000000001019bc0177bd2816b10daecd76401ccbacffa8b4d83c3a1c8f6461fb0c88633de840000000000fdffffff021027000000000000066a047465737430f4080000000000160014d77709c50f964d6ccc668c3345a0dd9ba083e05c0247304402206a0d0cc140d9e99caccc776474669a57bac3f7e1c605b5b5ca7815d021e65ccd02200172ef8fe2693e2d4564b0aec5ebc28cc3c75f8475508a31b5a9c2271658b9c001210205e865e6c9b71d16ce7ba97b999db8a08115648abbaeca59bd462b0a8fd0054a209d0100

OP_RETURN output exceeds maximum set by maxburnamount (code -25)

$ bitcoin-cli -named sendrawtransaction hexstring=020000000001019bc0177bd2816b10daecd76401ccbacffa8b4d83c3a1c8f6461fb0c88633de840000000000fdffffff021027000000000000066a047465737430f4080000000000160014d77709c50f964d6ccc668c3345a0dd9ba083e05c0247304402206a0d0cc140d9e99caccc776474669a57bac3f7e1c605b5b5ca7815d021e65ccd02200172ef8fe2693e2d4564b0aec5ebc28cc3c75f8475508a31b5a9c2271658b9c001210205e865e6c9b71d16ce7ba97b999db8a08115648abbaeca59bd462b0a8fd0054a209d0100 maxburnamount=0.0001

76fe1211a1059ff9bcf6e2b330631c75b901e0884669ff47be82ec948d84e68c
  28. maflcko commented at 7:45 AM on August 30, 2022: member

    I am still trying to understand the BTC transaction language, so I am unsure about whether or not it is possible for OP_RETURN to appear somewhere other than at the top of the scriptPubKey in a valid tx. And if it can, then does the presence of OP_RETURN anywhere in the scriptPubKey guarantee that the value of that output is unspendable? If both are true, then we would need to check every non-data byte of the scriptPubKey for an OP_RETURN. If the former is true and the latter is false, something more complicated might have to happen.

    I think you can just use the solver to check for NULL_DATA outputs. Everything else with an OP_RETURN is nonstandard and is thus rejected anyway (regardless of amount), unless the user modified the settings or the source code.

  29. davidgumberg force-pushed on Aug 30, 2022
  30. davidgumberg renamed this:
    [WIP] rpc: Add a parameter to sendrawtransaction which sets a maximum burned output for OP_RETURN transactions.
    rpc: Add a parameter to sendrawtransaction which sets a maximum burned output for OP_RETURN transactions.
    on Aug 30, 2022
  31. davidgumberg commented at 10:51 PM on August 30, 2022: contributor

    I have changed the OP_RETURN check to use Solver and I have added the tests, so I'm removing '[WIP]' from the PR title.

  32. davidgumberg commented at 11:27 PM on August 30, 2022: contributor

    As a side note: would it make sense to have an overloaded Solver with no Solutions vector parameter for when we only need the TxoutType? I count that 5 of the 15 existing calls to Solver don't use the solutions vector data that gets returned: AreInputsStandard in policy.cpp, CBloomFilter::IsRelevantAndUpdate in bloom.cpp, ScriptToUniv in core_write.cpp, AvailableCoins in spend.cpp, and CWallet::TransactionChangeType in wallet.cpp

  33. DrahtBot commented at 4:07 AM on August 31, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK glozow, achow101
    Concept ACK kristapsk, brunoerg, theStack, MarcoFalke
    Stale ACK 1440000bytes, rajarshimaitra

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  34. w0xlt commented at 3:51 PM on September 1, 2022: contributor

    The code seems to meet the proposed objective.

    But, as mentioned in #25899 (comment), it's also unclear to me why any application needs to use non-zero op_returns, considering the money used is lost.

    It might be good to include in the PR and commit description some real cases where this feature is used.

  35. ghost commented at 3:56 PM on September 1, 2022: none

    The code seems to meet the proposed objective.

    But, as mentioned in #25899 (comment), it's also unclear to me why any application needs to use non-zero op_returns, considering the money used is lost.

    It might be good to include in the PR and commit description some real cases where this feature is used.

    We as a group of (less than 100 active developers) developers cannot predict how bitcoin core is used. There can be use cases where it seems appropriate to burn a small amount.

    Its good that this is optional and users can still decide the burn amount with a parameter.

  36. davidgumberg commented at 8:18 PM on September 1, 2022: contributor

    +1 @1440000bytes

    Given that burned utxo's don't negatively impact the network since OP_RETURN tx's are prunable, I think we should do whatever is possible to prevent users from mistakenly burning coins, but we should not try to prevent users who are dead-set on burning coins and understand the consequences from doing so.

    As mentioned in #25899, users cannot even craft raw tx's with nonzero OP_RETURN outputs using the reference client. The theoretical transactions we are rejecting would have to have been either written in hex by hand or crafted with another client (as @1440000bytes did above) and broadcast by the reference client.

    Here are a few situations I can imagine:

    1. A separate chain which leverages the integrity of the bitcoin POW network for coin creation while only having a small number of users. The only way users get spendable coins is by creating OP_RETURN outputs and embedding a message with their alternate chain address in the OP_RETURN data field.

    2. Possibly combined with 1, a religious or political group that asks its members to destroy all or some of their money, including crypto.

    3. Gift giving: Maybe not dissimilar from spending thousands of dollars on a fireworks show that spells someone's name in the sky, someone might want to burn 1.0 BTC and embed the message "Alice loves Bob."

  37. maflcko approved
  38. maflcko commented at 10:27 AM on September 2, 2022: member

    ACK, lgtm

  39. glozow commented at 11:27 AM on September 2, 2022: member

    Thanks for working on this, I think it is a good improvement to help prevent accidental money loss. Might want a release note?

  40. unknown approved
  42. in src/rpc/mempool.cpp:49 in 83882e65b1 outdated 
      41 | @@ -41,7 +42,10 @@ static RPCHelpMan sendrawtransaction()
  42 |              {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
  43 |              {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
  44 |               "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
  45 | -                 "/kvB.\nSet to 0 to accept any fee rate.\n"},
  46 | +                 "/kvB.\nSet to 0 to accept any fee rate."},
  47 | +            {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},
    luke-jr commented at 12:12 AM on September 9, 2022:

    Probably should be an options object instead (can replace maxfeerate).

    davidgumberg commented at 5:13 AM on September 16, 2022:

    Conceptually I agree. I don't have much practical knowledge of how the RPC interface is used in the wild but wouldn't this break existing code that sets a maxfeerate? I also see that there are a few other calls that pass multiple options as separate arguments like convertopsbt and gettxoutsetinfo so maybe there is a case for making a broader change outside of the scope of this PR.

    But, if in practice this would not meaningfully break existing use cases then I will change it to an options object.

    davidgumberg commented at 2:56 AM on October 19, 2022:

    Marking as resolved, but feel free to reopen.

  43. in src/rpc/mempool.cpp:47 in 83882e65b1 outdated 
      41 | @@ -41,7 +42,10 @@ static RPCHelpMan sendrawtransaction()
  42 |              {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
  43 |              {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
  44 |               "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
  45 | -                 "/kvB.\nSet to 0 to accept any fee rate.\n"},
  46 | +                 "/kvB.\nSet to 0 to accept any fee rate."},
  47 | +            {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},
  48 | +             "Reject transactions with OP_RETURN outputs greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
    luke-jr commented at 12:14 AM on September 9, 2022:

    "datacarrier" is typically the term used for this rather than "OP_RETURN" (which is an opcode)

    davidgumberg commented at 3:03 AM on October 19, 2022:

    I corrected this, thanks

  44. luke-jr changes_requested
  45. in src/util/error.cpp:37 in 83882e65b1 outdated 
      32 | @@ -33,6 +33,8 @@ bilingual_str TransactionErrorString(const TransactionError err)
  33 |              return Untranslated("Specified sighash value does not match value stored in PSBT");
  34 |          case TransactionError::MAX_FEE_EXCEEDED:
  35 |              return Untranslated("Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)");
  36 | +        case TransactionError::MAX_BURN_EXCEEDED:
  37 | +            return Untranslated("OP_RETURN output exceeds maximum configured by user (maxburnamount)");
    rajarshimaitra commented at 1:57 PM on October 13, 2022: 
                return Untranslated("OP_RETURN output exceeds maximum amount configured by user (maxburnamount)");
    davidgumberg commented at 2:56 AM on October 19, 2022:

    Thanks for the review, but I disagree with the nit,

    Maximum - n., The highest possible magnitude or quantity of something which is attained, attainable, or customary; an upper limit of magnitude or quantity

    OED Online

    rajarshimaitra commented at 4:28 AM on October 19, 2022:

    It is clear what "maximum" means, but isn't clear what "quantity" it is talking about.. But not a big deal..

  46. rajarshimaitra commented at 2:27 PM on October 13, 2022: contributor

    Concept + tACK 83882e65b12ebd4e0a72967dbd8e54520eeecaa5

    Just a minor nit..

  47. davidgumberg force-pushed on Oct 19, 2022
  48. davidgumberg force-pushed on Oct 19, 2022
  49. davidgumberg commented at 3:05 AM on October 19, 2022: contributor

    Rebased to master and changed text in rpc description from OP_RETURN outputs to datacarrier outputs as suggested by @luke-jr .

  50. maflcko commented at 7:04 AM on October 19, 2022: member

    I think you want to replace all occurrences of OP_RETURN output in your patch with datacarrier output, if you replace one?

  51. achow101 commented at 8:28 PM on October 24, 2022: member

    I think this should be extended to also include any provably unspendable outputs. Currently that would be anything with an OP_RETURN, and anything larger than the maximum script size. Using the Solver and checking for NULL_DATA only finds those things which match the standardness rules for datacarrier, not all OP_RETURN scripts nor anything larger than the max script size. However we have CScript::IsUnspendable which does check for those things.

    The help text would need to be updated to say "unspendable outputs" rather than "datacarrier" or "OP_RETURN", although it could mention those as examples of unspendable outputs.

  52. maflcko commented at 7:17 AM on October 25, 2022: member

    Nonstandard (spendable and unspendable) transactions are already rejected, so I am not sure how much difference this will make in practice, but no objection to adding more checks either here or in a follow-up. Also, standard transaction outputs might be unprovably unspendable, so if this is changed, it should be clear that the unspendable check is a heuristic at best.

  53. maflcko commented at 2:52 PM on October 27, 2022: member

    It may also check for HasValidOps, but I think all of this can be done in a follow-up.

  54. DrahtBot added the label Needs rebase on Jan 17, 2023
  55. bitcoin deleted a comment on Jan 17, 2023
  56. davidgumberg force-pushed on Jan 18, 2023
  57. DrahtBot removed the label Needs rebase on Jan 19, 2023
  58. davidgumberg force-pushed on Jan 19, 2023
  59. davidgumberg force-pushed on Jan 19, 2023
  60. davidgumberg commented at 8:01 AM on January 19, 2023: contributor

    Rebased, I have adjusted the language to indicate that the check is for provable unspendability, instead of just datacarrier txn's and changed the check to use both IsUnspendable as suggested by @achow101 and HasValidOps as suggested by @MarcoFalke. I have added new functional tests that check that transactions containing invalid opcodes, or exceeding maximum script size are rejected. Since this change exposes a new RPC option, I have added a release note as suggested by @glozow.

    It seems to me that care should be taken that there are never any previously spendable and legitimate transactions that sendrawtransaction refuses to send as a result of this check. If needed, what should be done to better ensure this change will not cause any transactions that are expected to be spendable to be rejected?

  61. davidgumberg renamed this:
    rpc: Add a parameter to sendrawtransaction which sets a maximum burned output for OP_RETURN transactions.
    rpc: Add a parameter to sendrawtransaction which sets a maximum output for unspendable transactions.
    on Jan 19, 2023
  62. achow101 commented at 5:46 PM on January 19, 2023: member

    ACK ab8c380a7370e29dfedce1399ad1b2a2d234915e

    It seems to me that care should be taken that there are never any previously spendable and legitimate transactions that sendrawtransaction refuses to send as a result of this check. If needed, what should be done to better ensure this change will not cause any transactions that are expected to be spendable to be rejected?

    Even if there are, a suitable workaround is to just set maxburnamount to a high value. However I believe the check is specific enough that we won't have any issues other than with people who are explicitly trying to burn money. IsUnspendable is used by mempool policy and validation, and HasValidOps is used by the transaction hex parser (so actually it might be redundant here). These are things that, if they were not working correctly, would cause issues in other parts of the codebase.

  63. achow101 requested review from glozow on Feb 17, 2023
  64. achow101 requested review from maflcko on Feb 17, 2023
  65. in doc/release-notes-25943.md:3 in ab8c380a73 outdated 
       0 | @@ -0,0 +1,4 @@
   1 | +New RPC Argument
   2 | +--------
   3 | +- `sendrawtransaction` has received a new, optional argument, `maxburnamount` with a default value of `0`. All transactions which are heuristically determined to be unspendable with an output greater than `maxburnamount` will be rejected. At present, the transactions deemed unspendable are those with scripts that begin with an `OP_RETURN` code (known as 'datacarriers'), scripts that exceed the maximum script size, and scripts that contain invalid opcodes.
    glozow commented at 11:57 AM on February 20, 2023:

    nit: (un)spendable should refer to outputs, not transactions. Same issue with the PR description.

    - `sendrawtransaction` has a new, optional argument, `maxburnamount` with a default value of `0`. Any transaction containing an unspendable output with value greater than `maxburnamount` will not be submitted. At present, the outputs deemed unspendable are those with scripts that begin with an `OP_RETURN` code (known as 'datacarriers'), scripts that exceed the maximum script size, and scripts that contain invalid opcodes.
  66. in src/rpc/mempool.cpp:51 in ab8c380a73 outdated 
      44 | @@ -44,7 +45,11 @@ static RPCHelpMan sendrawtransaction()
  45 |              {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
  46 |              {"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())},
  47 |               "Reject transactions whose fee rate is higher than the specified value, expressed in " + CURRENCY_UNIT +
  48 | -                 "/kvB.\nSet to 0 to accept any fee rate.\n"},
  49 | +                 "/kvB.\nSet to 0 to accept any fee rate."},
  50 | +            {"maxburnamount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)},
  51 | +             "Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
  52 | +             "If set to 0, all transactions with provably unspendable outputs will be rejected.\n"
    glozow commented at 12:12 PM on February 20, 2023:

    This isn't true? If set to 0, transactions with provably unspendable outputs are fine as long as the output values are 0. Perhaps remove this line, as we wouldn't want a user to mistakenly think they're filtering out all datacarrier transactions by setting this (also, the same thing happens if this option is left blank since 0 is default).

    glozow commented at 12:12 PM on February 20, 2023:

    Perhaps add "If burning funds through unspendable outputs is desired, increase this value." If a downstream app is (hypothetically) regularly burning money and upgrades their bitcoind, it should be easy to understand what to do to make their error go away.

  67. glozow commented at 12:17 PM on February 20, 2023: member

    ACK ab8c380a73 but I think one of the docs is a bit misleading

  68. DrahtBot requested review from ghost on Feb 20, 2023
  69. DrahtBot requested review from rajarshimaitra on Feb 20, 2023
  70. Add test for unspendable transactions and parameter 'maxburnamount' to sendrawtransaction.
    'maxburnamount' sets a maximum value for outputs heuristically deemed unspendable including datacarrier scripts that begin with `OP_RETURN`.
    04f270b435
  71. Add release note for PR#25943
    Co-authored-by: glozow <gloriajzhao@gmail.com>
    7013da07fb
  72. davidgumberg force-pushed on Feb 20, 2023
  73. davidgumberg renamed this:
    rpc: Add a parameter to sendrawtransaction which sets a maximum output for unspendable transactions.
    rpc: Add a parameter to sendrawtransaction which sets a maximum value for unspendable outputs.
    on Feb 20, 2023
  74. davidgumberg commented at 8:05 PM on February 20, 2023: contributor

    Force-pushed to address @glozow review.

  75. glozow commented at 10:10 AM on February 21, 2023: member

    reACK 7013da07fbcddb04abae9759767a9419ab90444c

  76. fanquake removed review request from rajarshimaitra on Feb 23, 2023
  77. fanquake removed review request from ghost on Feb 23, 2023
  78. fanquake requested review from achow101 on Feb 23, 2023
  79. achow101 commented at 4:48 PM on February 23, 2023: member

    re-ACK 7013da07fbcddb04abae9759767a9419ab90444c

  80. DrahtBot requested review from ghost on Feb 23, 2023
  81. DrahtBot requested review from rajarshimaitra on Feb 23, 2023
  82. achow101 merged this on Feb 23, 2023
  83. achow101 closed this on Feb 23, 2023
  84. sidhujag referenced this in commit cc8ec5f466 on Feb 25, 2023
  85. bitcoin locked this on Feb 23, 2024
Contributors
davidgumbergghostkristapskbrunoergmaflckoglozowtheStackDrahtBotw0xltluke-jrrajarshimaitraachow101


achow101maflckorajarshimaitra

Labels
RPC/REST/ZMQ
Linked (view graph)
#25899 rpc: prevent non-zero value OP_RETURN outputs in sendrawtransaction#26384 .
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me