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

davidgumberg · 2022-08-27T21:49:56Z

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:

Those that begin with OP_RETURN - (datacarriers)
Those whose lengths exceed the script limit.
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:

Users that deliberately use OP_RETURN for datacarrier transactions that embed data into the blockchain.
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.

ghost · 2022-08-28T07:47:54Z

Concept ACK

I am not sure about name of this argument: rejectreturn

kristapsk · 2022-08-28T08:08:30Z

Concept ACK

src/rpc/mempool.cpp

brunoerg · 2022-08-28T11:56:19Z

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.

davidgumberg · 2022-08-28T19:21:51Z

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

src/rpc/mempool.cpp

theStack · 2022-08-29T11:14:09Z

Concept ACK

davidgumberg · 2022-08-30T02:06:27Z

I have some questions:

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

davidgumberg · 2022-08-30T02:21:26Z

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.

ghost

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

Create a transaction in electrum (signet) with OP_RETURN 74657374 in 'payto' and 0.0001 in amount. Sign and copy the transaction hex.
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

src/rpc/mempool.cpp

ghost · 2022-08-30T06:34:40Z

src/util/error.cpp

Suggested change

return Untranslated("OP_RETURN output exceeds maximum set by maxburnamount");

return Untranslated("OP_RETURN output exceeds maximum configured by user (-maxburnamount)");

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

maflcko · 2022-08-30T07:45:00Z

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.

davidgumberg · 2022-08-30T22:51:55Z

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.

davidgumberg · 2022-08-30T23:27:34Z

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

DrahtBot · 2022-08-31T04:07:04Z

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

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.

Conflicts

No conflicts as of last run.

maflcko · 2022-10-27T14:52:40Z

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

davidgumberg · 2023-01-19T08:01:18Z

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?

achow101 · 2023-01-19T17:46:58Z

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.

glozow

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

doc/release-notes-25943.md

src/rpc/mempool.cpp

…o sendrawtransaction. 'maxburnamount' sets a maximum value for outputs heuristically deemed unspendable including datacarrier scripts that begin with `OP_RETURN`.

Co-authored-by: glozow <[email protected]>

davidgumberg · 2023-02-20T20:05:00Z

Force-pushed to address @glozow review.

glozow · 2023-02-21T10:10:49Z

reACK 7013da0

achow101 · 2023-02-23T16:48:41Z

re-ACK 7013da0

… sets a…

davidgumberg force-pushed the wip-opreturn_nz branch 3 times, most recently from 10ec3ca to dd79c5f Compare August 28, 2022 02:29

brunoerg reviewed Aug 28, 2022

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

davidgumberg force-pushed the wip-opreturn_nz branch from dd79c5f to b829861 Compare August 28, 2022 19:18

maflcko reviewed Aug 29, 2022

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

glozow added the RPC/REST/ZMQ label Aug 29, 2022

bitcoin deleted a comment from Meru852 Aug 29, 2022

bitcoin deleted a comment from Ellajoke Aug 29, 2022

davidgumberg force-pushed the wip-opreturn_nz branch from b829861 to 1e047b1 Compare August 29, 2022 23:10

davidgumberg changed the title ~~[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. Aug 29, 2022

davidgumberg force-pushed the wip-opreturn_nz branch 4 times, most recently from 7e486fc to 17f8542 Compare August 30, 2022 02:05

ghost reviewed Aug 30, 2022

View reviewed changes

davidgumberg force-pushed the wip-opreturn_nz branch from 17f8542 to 83882e6 Compare August 30, 2022 22:38

davidgumberg changed the title ~~[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. Aug 30, 2022

DrahtBot mentioned this pull request Aug 31, 2022

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

DrahtBot added the Needs rebase label Jan 17, 2023

bitcoin deleted a comment from Ellajoke Jan 17, 2023

davidgumberg force-pushed the wip-opreturn_nz branch from ce5f09e to d4eb4c8 Compare January 18, 2023 23:07

DrahtBot removed the Needs rebase label Jan 19, 2023

davidgumberg force-pushed the wip-opreturn_nz branch 2 times, most recently from 7f40c73 to ab8c380 Compare January 19, 2023 07:55

davidgumberg changed the title ~~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. Jan 19, 2023

achow101 requested review from glozow and maflcko February 17, 2023 22:30

glozow reviewed Feb 20, 2023

View reviewed changes

doc/release-notes-25943.md Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from a user and rajarshimaitra February 20, 2023 12:17

davidgumberg and others added 2 commits February 20, 2023 11:38

Add test for unspendable transactions and parameter 'maxburnamount' t…

04f270b

…o sendrawtransaction. 'maxburnamount' sets a maximum value for outputs heuristically deemed unspendable including datacarrier scripts that begin with `OP_RETURN`.

Add release note for PR#25943

7013da0

Co-authored-by: glozow <[email protected]>

davidgumberg force-pushed the wip-opreturn_nz branch from ab8c380 to 7013da0 Compare February 20, 2023 18:48

davidgumberg changed the title ~~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. Feb 20, 2023

fanquake requested review from achow101 and removed request for a user and rajarshimaitra February 23, 2023 10:01

DrahtBot requested review from a user and rajarshimaitra February 23, 2023 16:48

achow101 merged commit b7702bd into bitcoin:master Feb 23, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 25, 2023

Merge bitcoin#25943: rpc: Add a parameter to sendrawtransaction which…

cc8ec5f

… sets a…

bitcoin locked and limited conversation to collaborators Feb 23, 2024

	return Untranslated("OP_RETURN output exceeds maximum set by maxburnamount");
	return Untranslated("OP_RETURN output exceeds maximum configured by user (-maxburnamount)");

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

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

Uh oh!

Conversation

davidgumberg commented Aug 27, 2022 • edited by glozow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 28, 2022

Uh oh!

kristapsk commented Aug 28, 2022

Uh oh!

Uh oh!

brunoerg commented Aug 28, 2022

Uh oh!

davidgumberg commented Aug 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theStack commented Aug 29, 2022

Uh oh!

davidgumberg commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidgumberg commented Aug 30, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

davidgumberg Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 30, 2022

Uh oh!

davidgumberg commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidgumberg commented Aug 30, 2022

Uh oh!

DrahtBot commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko commented Oct 27, 2022

Uh oh!

davidgumberg commented Jan 19, 2023

Uh oh!

achow101 commented Jan 19, 2023

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidgumberg commented Feb 20, 2023

Uh oh!

glozow commented Feb 21, 2023

Uh oh!

achow101 commented Feb 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

davidgumberg commented Aug 27, 2022 •

edited by glozow

Loading

davidgumberg commented Aug 28, 2022 •

edited

Loading

davidgumberg commented Aug 30, 2022 •

edited

Loading

davidgumberg commented Aug 30, 2022 •

edited

Loading

DrahtBot commented Aug 31, 2022 •

edited

Loading