clarify bip68 and bip112 description of required transaction versions #1542
Conversation
| @@ -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.
| 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.
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.
| ** 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.
leaving as-is unless people feel strongly
murchandamus
added
Proposed BIP modification
Pending acceptance
|
@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). |
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.