-
Notifications
You must be signed in to change notification settings - Fork 2.5k
txscript: increase OP_RETURN data limit to 100KB #2441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
txscript: increase OP_RETURN data limit to 100KB #2441
Conversation
This commit increases the MaxDataCarrierSize from 80 bytes to 100,000 bytes to align with Bitcoin Core v30, which removes the specific 80-byte OP_RETURN data limit. Rationale: 1. Policy Alignment: Maintains consistency with Bitcoin Core v30, ensuring btcd nodes accept the same set of standard transactions as the Bitcoin reference implementation. 2. Fee Estimation: Filtering large OP_RETURN transactions from the mempool prevents accurate fee estimation by hiding transactions that are competing for blockspace. Full mempool visibility is essential for informed fee rate calculation. 3. Validation Performance: Rejecting transactions that will be mined by other nodes means they must be validated when processing blocks, without cached results. This delays block validation and propagation, harming both the node operator and network health. 4. Decentralization: Slow block propagation advantages large mining operations with preferentially peered nodes, as they get a head start on the next block. Ensuring all likely-to-be-mined transactions are pre-validated helps maintain network decentralization. Key changes: - Increase MaxDataCarrierSize constant from 80 to 100000 bytes - Modify NullDataScript to use AddFullData instead of AddData to bypass MaxScriptElementSize (520 bytes) since OP_RETURN outputs are provably unspendable and never executed - Update AddFullData documentation to reflect legitimate use for OP_RETURN outputs, not just testing MaxDataCarrierSize is increased to 100,000 bytes, effectively removing the specific data carrier size limit. The existing transaction size limit (maxStandardTxWeight of 400,000 weight units ≈ 100KB total transaction) will apply before the data carrier limit is reached due to transaction overhead (inputs, outputs, signatures, etc.). Test coverage includes: - Unit tests for script creation and size validation - Policy tests for transaction standardness (75KB OP_RETURN) - Integration tests for mempool acceptance and size limit enforcement This change maintains backward compatibility while enabling larger data storage in OP_RETURN outputs for applications like inscriptions, timestamps, and data anchoring.
caae108 to
2727cf1
Compare
| // TestLargeOPReturnMempool tests that large OP_RETURN transactions (up to 100KB) | ||
| // are accepted into the mempool. This validates the Bitcoin Core v30 change that | ||
| // removes the 80-byte OP_RETURN limit. | ||
| func TestLargeOPReturnMempool(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bitcoin Core 30.0 will also allow multiple OP_RETURN outputs on transactions. I’m not sure whether BTCD ever restricted transactions to a single one, but another test that adds two small OP_RETURN outputs would demonstrate that BTCD is compatible, or reveal that additional changes would need to be made to make it compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@murchandamus It's enforced atm see
Lines 368 to 373 in b7d0706
| // A standard transaction must not have more than one output script that | |
| // only carries data. | |
| if numNullDataOutputs > 1 { | |
| str := "more than one transaction output in a nulldata script" | |
| return txRuleError(wire.RejectNonstandard, str) | |
| } |
Looking into it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@murchandamus done
This commit removes the restriction limiting transactions to a single OP_RETURN output, aligning with Bitcoin Core v30 which removed both the 80-byte data limit and the single OP_RETURN restriction. Rationale: 1. Policy Alignment: Bitcoin Core v30 removed the arbitrary one OP_RETURN restriction. Each OP_RETURN output is independently validated against MaxDataCarrierSize, and the transaction as a whole remains subject to maxStandardTxWeight. 2. Fee Estimation: Filtering transactions with multiple OP_RETURNs prevents accurate fee estimation by hiding transactions competing for blockspace. Full mempool visibility of likely-to-be-mined transactions is essential for informed fee rate calculation. 3. Validation Performance: Rejecting transactions that will be mined by other nodes means they must be validated when processing blocks, without cached results. This delays block validation and propagation, harming both node operators and network health. 4. Decentralization: Slow block propagation disproportionately advantages large mining operations with preferentially peered nodes. Pre-validating all likely-to-be-mined transactions helps maintain network decentralization. Key changes: - Remove the check restricting transactions to one OP_RETURN output - Add comment explaining that multiple OP_RETURNs are now standard - Each output remains subject to MaxDataCarrierSize (100KB) - Transaction remains subject to maxStandardTxWeight (400,000 weight) Use cases enabled by Bitcoin Core v30 policy changes: - Multiple data commitments in a single transaction (e.g., timestamp proofs, inscriptions, metadata anchoring) - More efficient blockchain space usage by batching data outputs - Reduced transaction overhead compared to separate transactions Test coverage: - Policy tests for multiple OP_RETURNs with varying sizes (~75KB total) - Policy test for combined size exceeding transaction limit (rejected) - Integration test for multiple OP_RETURNs (3 × 25KB) - Integration test verifying MaxDataCarrierSize enforcement (101KB) The natural limits remain effective: - Each OP_RETURN limited by MaxDataCarrierSize (100,000 bytes) - Total transaction limited by maxStandardTxWeight (≈100KB total tx)
|
How's this pr different from #2385 which was closed after this comment from @Roasbeef ? |
|
@saubyk it's more thorough implementation and tests, and the PR does a better job of explaining why this is a good idea for users |
|
Regardless of a better rationale I believe Laolu's comment is very clear on why the changes proposed here are not acceptable for this project. And that status quo has not been changed so far. "The only relay policies that we endeavor to keep up with are those that require interop, and tangibly affect higher layer solutions such as: RBF, TRUC, etc. These policies all have BIPs, with a clear target scope and motivation." |
|
@saubyk I read it - my point was that motives 2 3 and 4 all "tangibly affect higher layer solutions". iow they're good for users |
|
@saubyk Genuine question; what would btcd users gain from not incorporating this change given that it's proposed with additional testing to mitigate regression? How does that weigh up against the benefits listed in the motivation section? |
|
If the goal of mempool policies and standardness is to make nodes as efficient as possible, whilst not filtering out likely-to-get-mined transactions (because it harms fee estimation and block validation), then this PR should be attractive. If that's not the goal of mempool policies and standardness in btcd, then what is the goal of them? |
Let me point out again to the same comment from @Roasbeef ... "We already have a flag that enables users to accept and relay all non-standard transactions. Users that want to shed themselves of all discretionary relay policy can run with those flags." It's unfair for us to reject one pr from a contributor, while accepting the same change in a different pr just because it has better tests. There is no fundamental change in status quo after the previous pr has been rejected. |
|
A flag that enables users to accept and relay all non-standard transactions is not a reasonable alternative; that forces btcd users to pick either the status quo of over-filtering (which harms fee estimation and block validation) or a very inefficient node which is vulnerable to DoS and continually wastes resources processing and relaying transactions that even miners want nothing to do with. There was no discussion on the other thread, the author simply accepted the response and closed it. I'm asking, constructively, for more clarification on the basis for rejecting such a change. Having additional tests is a material difference. I'm getting the impression that you're reaching for reasons to reject this out of hand rather than discuss the merits. If that is the case, then this probably isn't the PR thread for you and I'll wait for someone else who is willing to discuss it properly. |
|
thanks @kmk142789 |
Change Description
This PR implements Bitcoin Core v30 OP_RETURN policy changes by:
MaxDataCarrierSizefrom 80 bytes to 100,000 bytesThese changes align btcd with Bitcoin Core v30, which removed both the 80-byte data limit and the single OP_RETURN restriction.
Motivation
Bitcoin Core v30 updated OP_RETURN policies by removing the specific
-datacarriersizedefault of 80 bytes (replacing it with 100,000 bytes) and eliminating the single OP_RETURN per transaction restriction. These changes bring btcd into alignment with Bitcoin Core v30 for several critical reasons:1. Policy Alignment
Maintains consistency with Bitcoin Core v30, ensuring btcd nodes accept the same set of standard transactions as the Bitcoin reference implementation. Both the data size limit and multiple OP_RETURN restrictions were arbitrary policy limitations with no consensus basis.
2. Accurate Fee Estimation
Filtering large OP_RETURN transactions or transactions with multiple OP_RETURNs from the mempool hides transactions competing for blockspace, leading to inaccurate fee estimation. Full mempool visibility is essential for informed fee rate calculation.
3. Validation Performance & Block Propagation
Rejecting transactions that will be mined by other nodes means they must be validated "cold" when processing blocks, without cached results. This causes slower block validation, delayed block propagation, and increased orphan risk for miners.
4. Network Decentralization
Slow block propagation disproportionately advantages large mining operations with preferentially peered nodes, giving them a head start on the next block. Pre-validating all likely-to-be-mined transactions helps maintain network decentralization.
Changes
Data Carrier Size Increase
MaxDataCarrierSizefrom 80 to 100,000 bytesNullDataScript()to useAddFullData()to bypassMaxScriptElementSize(520 bytes) since OP_RETURN outputs are never executedAddFullData()documentation to reflect its legitimate use for large OP_RETURN outputsMultiple OP_RETURN Support
Integration Tests
Technical Notes
MaxDataCarrierSize
Increased to 100,000 bytes, effectively removing the specific data carrier size limit. The existing transaction size limit (maxStandardTxWeight of 400,000 weight units ≈ 100KB total transaction) will apply before the data carrier limit is reached due to transaction overhead (inputs, outputs, signatures, etc.).
Multiple OP_RETURNs
Each OP_RETURN output is independently validated against MaxDataCarrierSize (100KB). The transaction as a whole remains subject to maxStandardTxWeight (400,000 weight units ≈ 100KB total transaction).
Script Limits
OP_RETURN outputs are exempt from
MaxScriptElementSize(520 bytes) execution limits because they are provably unspendable and never executed by the script engine.Compatibility
Steps to Test
1. Run Unit Tests
go test ./txscript/... -vExpected: All tests pass, including new tests for:
2. Run Policy Tests
go test ./mempool/... -v -run TestCheckTransactionStandardExpected: All tests pass, including:
3. Run Integration Tests
go test -tags=rpctest ./integration/... -v -run TestLargeOPReturnMempoolExpected: All test cases pass:
4. Run Full Test Suite
go test ./...Expected: All tests pass across the entire codebase.
5. Code Quality Checks
Expected: No issues found.
Pull Request Checklist
Testing
Code Style and Documentation