-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
clarify bip68 and bip112 description of required transaction versions #1542
base: master
Are you sure you want to change the base?
Conversation
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.
ACK 6e8a11e
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 seems to me that being explicit about how to interpret the version bytes is preferable to implicitly treating it as a signed integer and then having to worry about negative versions.
@@ -25,7 +25,7 @@ The transaction nLockTime is used to prevent the mining of a transaction until a | |||
|
|||
==Specification== | |||
|
|||
This specification defines the meaning of sequence numbers for transactions with an nVersion greater than or equal to 2 for which the rest of this specification relies on. | |||
This specification defines the meaning of sequence numbers for transactions with an nVersion greater than or equal to 2, or any negative version, for which the rest of this specification relies on. |
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.
This specification defines the meaning of sequence numbers for transactions with an nVersion greater than or equal to 2, or any negative version, for which the rest of this specification relies on. | |
This specification defines the meaning of sequence numbers for transactions with an nVersion greater than or equal to 2 when interpreted as an unsigned little-endian integer, for which the rest of this specification relies on. |
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.
leaving as-is unless people feel strongly
@@ -29,7 +29,7 @@ When executed, if any of the following conditions are true, the script interpret | |||
* the stack is empty; or | |||
* the top item on the stack is less than 0; or | |||
* the top item on the stack has the disable flag (1 << 31) unset; and | |||
** the transaction version is less than 2; or | |||
** the transaction version is non-negative and less than 2, i.e. 0 or 1; or |
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.
** the transaction version is non-negative and less than 2, i.e. 0 or 1; or | |
** the transaction version is less than 2 when interpreted as an unsigned little-endian integer; or |
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.
leaving as-is unless people feel strongly
@instagibbs, this PR has had unaddressed review comments for three months. Are you still working on this? |
I'm not planning on changing anything further unless people feel strongly about @vostrnad 's suggestions(which seem fine to me as well) |
FWIW, i prefer @instagibbs 's current wording. |
To elaborate: as is, the BIP implicitly treats the version as an unsigned integer. This PR switches to (still implicitly) treating it as a signed integer. My proposed changes would explicitly treat it as an unsigned integer. Given that Bitcoin Core will likely switch from interpreting the version as signed to unsigned (bitcoin/bitcoin#29325), having it unsigned here as well seems like the best way forward (and being explicit about it is a strict improvement in any case). |
@NicolasDorier mind weighing in here? |
https://delvingbitcoin.org/t/disclosure-btcd-consensus-bugs-due-to-usage-of-signed-transaction-version/455
Seems like a good idea to be clear up front on the nversion requirements even if we're relying on the implementation-as-spec aspects of the bips.