fix: deprecate wrong and unused max script num #3276
Conversation
Pull Request Test Coverage Report for Build 10654351438Details
💛 - Coveralls |
There was a problem hiding this comment.
Honestly, this looks like unclear documentation/API to me. I'd could argue that both representations make sense and both changing it and not changing it are wrong.
If this constant is kept then we should instead create two other constants: MAX_SCRIPT_INT and MAX_SCRIPT_LOCK_TIME and deprecate MAX_SCRIPT_INT in favor of those because of unclear semantics.
bitcoin/src/blockdata/constants.rs
Outdated
There was a problem hiding this comment.
It's weird that the constant is just thrown here with no code using it. While it's pub it's such an internal detail that probably nobody should use it and use the methods on Script instead.
There was a problem hiding this comment.
I think the history is from when rust-miniscript implemented locktime.
rust-bitcoin/rust-miniscript#408 (comment)
There was a problem hiding this comment.
I think we should deprecate this constant (and not change it to reduce breakage). It's such a weird thing. Elsewhere I have advocated having push_locktime method(s) to push locktimes, which as you note, have a different maximum value than every other scriptnum.
Another possible, though unlikely, interpretation of this constant is that it could be the maximum output of an arithmetic opcode. This, I think, is 2^31 - 1.
Also it's weird that we have a maximum value represented as a u32, and no minimum value.
My feeling is that if you are doing something that cares about this constant, you should define your own constants that clearly represent whatever it is you mean. Good find @ChrisCho-H that this constant originates in Miniscript doing something fairly specific, propagating poorly-documented constants into this library, and apparently getting it wrong.
There was a problem hiding this comment.
I agree with @apoelstra. It'd be better not to expose this weird constant. Overflow can be checked internally.
There was a problem hiding this comment.
I've just force pushed 9620cdf, removing this constant rather than changing its value.
ChrisCho-H
force-pushed
the
fix/max-script-num
branch
from
0b41e7a to
9620cdf
Compare
|
🚨 API BREAKING CHANGE DETECTED To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section. |
github-actions
bot
added
the
API break
ChrisCho-H
changed the title
ChrisCho-H
force-pushed
the
fix/max-script-num
branch
from
9620cdf to
9baf46f
Compare
bitcoin/src/blockdata/constants.rs
Outdated
There was a problem hiding this comment.
Please don't hide it, that's just confusing. The deprecation message will be shown in the doc so there's no risk of people using it accidentally.
There was a problem hiding this comment.
Also I think we should add since = "TBD"
There was a problem hiding this comment.
It makes warning if remove original comment without hidden doc. Should I leave the comment as it was?
There was a problem hiding this comment.
Yes, please do.
Also, I would change the deprecation message to `This constant has ambiguous semantics. Please carefully check your intended use-case and define a new constant reflecting that.'
ChrisCho-H
force-pushed
the
fix/max-script-num
branch
from
9baf46f to
345d3da
Compare
4 participants
Script number can be up to 2^39 - 1 to encode locktime.If it's only for the integer operation besides locktime, it must be 2^31 - 1, not 2^31.I agree with apoelstra opinion to deprecate this value.