-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
Policy: Decouple Solver() from nMaxDatacarrierBytes (for free) #6424
Conversation
@@ -49,7 +49,9 @@ bool IsStandard(const CScript& scriptPubKey, txnouttype& whichType) | |||
return false; | |||
if (m < 1 || m > n) | |||
return false; | |||
} | |||
} else if (whichType == TX_NULL_DATA && | |||
(!GetBoolArg("-datacarrier", true) || scriptPubKey.size() > nMaxDatacarrierBytes)) |
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.
If nMaxDatacarrierBytes
is 80, wouldn't the effective size only be 79 77 byte now, due to the op code, which is part of scriptPubKey
?
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.
It is clearer if you read commit by commit:
jtimon@dbc241d
jtimon@904d593
jtimon@984f7b4
The simplest way to maintain the current "effective size" of 80 is to simply add 3 to MAX_OP_RETURN_RELAY.
But the 3 is quite arbitrary: the number of pushes depends on the size of the data. So I think it's simpler to count the op_return and the pushes as part of the data: simply reduce the "effective size" from 80 to 77 and change the tests instead of the constant (as in the last commit).
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.
Thanks, I understand your reasoning, and even though it's a bit arbitrary, this may have an impact on applications. For example, a block header doesn't fit in nicely anymore. (I don't really care about those few bytes, but I recall reading about this use case somewhere..)
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.
Yes, I know the last commit will probably be discarded (that's why I put it the last), but I just want it to be considered as an option. I'm totally fine with 83 in the constant.
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.
It is a bit disappointing that GetBoolArg() use from prior code is continued. Need a state-and-options struct.
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.
df69832
to
984f7b4
Compare
Concept ACK - I've always thought policy doesn't belong in the Solver, which would be expected to determine what kind of script something is, then the policy to judge based on its output. |
utACK |
If you want to go ahead and rebase, that'd be great - I don't have the time/funding to do anything with this. |
984f7b4
to
0b21b27
Compare
Squashed everything but the last commit. @petertodd Thanks for the review. As long as there are reviewers with time to re-review, I'm happy to re-rebase. Thoughts on the last commit again? |
@jtimon I would strongly recommend you change the constant rather than the unit tests - people end up designing stuff based on things like "most miners will mine 80 byte op_return" so changing it can be highly disruptive. |
0b21b27
to
12f4303
Compare
12f4303
to
935a0c0
Compare
No input from anyone else besides @petertodd for a while, so I've just removed the last commit "fixup! Better yet, change the tests instead of the default constant" like he suggested. |
935a0c0
to
5ca12ee
Compare
@@ -350,12 +350,29 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) | |||
t.vout[0].scriptPubKey = CScript() << OP_1; | |||
BOOST_CHECK(!IsStandardTx(t, reason)); | |||
|
|||
// 80-byte TX_NULL_DATA (standard) | |||
// MAX_OP_RETURN_RELAY-byte TX_NULL_DATA (standard) |
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.
MAX_OP_RETURN_RELAY length bytes
, the hyphen seems odd here
concept/utACK |
ping |
Untested ACK. |
utACK, needs rebase |
Previously unlike other transaction types the TX_SCRIPTHASH would not clear vSolutionsRet, which means that unlike other transaction types if it was called twice in a row you would get the result of the previous invocation as well.
Allows IsPushOnly() to be applied to just part of the script for OP_RETURN outputs.
Previously only one PUSHDATA was allowed, needlessly limiting applications such as matching OP_RETURN contents with bloom filters that operate on a per-PUSHDATA level. Now any combination that passes IsPushOnly() is allowed, so long as the total size of the scriptPubKey is less than 42 bytes. (unchanged modulo non-minimal PUSHDATA encodings) Also, this fixes the odd bug where previously the PUSHDATA could be replaced by any single opcode, even sigops consuming opcodes such as CHECKMULTISIG. (20 sigops!)
5ca12ee
to
da894ab
Compare
Rebased |
Forgot this, but @petertodd sending a tweet remindede me of it: this needs mention in |
Mention now allowed sequence of pushdatas in OP_RETURN outputs in release notes.
Mention now allowed sequence of pushdatas in OP_RETURN outputs in release notes.
What was the rational about allowing any push data in the OP_RETURN instead of just allowing any OP ? Why not accepting any operation ? After all, since the rest is never executed, we can save bytes by allowing any OP to be used, even invalid one. (Invalid OPs are standard if they are not executed) |
@NicolasDorier That's a very good question. In the original PR #5079 @petertodd says "Previously only one PUSHDATA was allowed, needlessly limiting applications such as matching OP_RETURN contents with bloom filters that operate on a per-PUSHDATA level." |
bloom filter is a way to get notified about content in OP_RETURN by SPV wallets with configurable compromise on privacy. So this is not the reason, accepting all OP do not prevent you to use PushData in your OP_RETURN instead of garbage if you really need SPV capabilities. |
Thanks for the clarification, I'm thinking that I should have moved that check to policy::IsStandard() as well. But I don't really understand what is for, so I would like to hear an answer to your question as well. |
That's an clever observation, and I had the same idea too! But unfortunately sigops are counted in outputs, which makes it tricky to be sure we're not opening up a some kind of DoS attack by allowing anything in OP_RETURN. Easiest was just to restrict this to only pushdata's, which is pretty much just as good anyway. |
Ok, did not thought about sigops :( |
@NicolasDorier That would be hard fork. I don't think anyone here has to authority to say whether or not that will change. |
So what I mean is more, is there proposal being written to fix the issue right now ? (or even some args that the user/miner can turn on if we get spammed attacked with that so it is possible to refuse script with too much sig, we'll get some mitigation) |
Bitcoin script PRs 1 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6335 - bitcoin/bitcoin#6424 - bitcoin/bitcoin#11058 - bitcoin/bitcoin#12460 - bitcoin/bitcoin#13194 Part of #2074.
5d96c91 add missing policy.cpp file to cmake (furszy) 2d0783d Standard Solver(): Include cold staking template matching. (furszy) b7fe4c0 Remove template matching and pseudo opcodes (furszy) fe31b31 Assert CPubKey::ValidLength to the pubkey's header-relevent size (Ben Woosley) 2f6b249 Make TX_SCRIPTHASH clear vSolutionsRet first (Peter Todd) 45e5102 Policy: MOVEONLY: 3 functions to policy (furszy) 2c7da7f Policy: MOVEONLY: Create policy/policy.h with some constants (furszy) Pull request description: Coming from the following PRs (with small customizations for our features): * bitcoin#6335 * bitcoin#6424 * bitcoin#12460 * bitcoin#13194 ACKs for top commit: random-zebra: Looking good. ACK 5d96c91 Fuzzbawls: ACK 5d96c91 Tree-SHA512: b3e7479864e16942682b9bc19f432b2a657561647eeadf0ef1285651feb05875d295ff530582ebbc8edacaf3c7d3888249513b4a93d871c2d6c2e1b157ace8c8
This is a rebased and modified version of #5079. If @petertodd accepts my suggestions (specially the one about taking to opportunity of decoupling Solver() from nMaxDatacarrierBytes), I can close this PR.
If he doesn't and #5079 gets merged first, this can be rebased to do the decoupling.