clarify bip68 and bip112 description of required transaction versions #1542

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:bip68_112_nversion changing 2 files +2 −2
  1. instagibbs commented at 5:58 pm on January 22, 2024: member

    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.

  2. clarify bip68 and bip112 description of required transaction versions 6e8a11e8a1
  3. dergoegge approved
  4. dergoegge commented at 6:03 pm on January 22, 2024: member
    ACK 6e8a11e8a16113e4c17b3309875f08696b46bfb0
  5. Christewart approved
  6. in bip-0068.mediawiki:28 in 6e8a11e8a1 
    24@@ -25,7 +25,7 @@ The transaction nLockTime is used to prevent the mining of a transaction until a
25 
26 ==Specification==
27 
28-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.
29+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.
    vostrnad commented at 10:03 pm on January 22, 2024: 
    0This 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.
    instagibbs commented at 1:22 pm on April 29, 2024:
    leaving as-is unless people feel strongly
  7. in bip-0112.mediawiki:32 in 6e8a11e8a1 
    28@@ -29,7 +29,7 @@ When executed, if any of the following conditions are true, the script interpret
29 * the stack is empty; or
30 * the top item on the stack is less than 0; or
31 * the top item on the stack has the disable flag (1 << 31) unset; and
32-** the transaction version is less than 2; or
33+** the transaction version is non-negative and less than 2, i.e. 0 or 1; or
    vostrnad commented at 10:03 pm on January 22, 2024: 
    0** the transaction version is less than 2 when interpreted as an unsigned little-endian integer; or
    instagibbs commented at 1:22 pm on April 29, 2024:
    leaving as-is unless people feel strongly
  8. vostrnad commented at 10:03 pm on January 22, 2024: contributor
    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.
  9. Roasbeef commented at 1:01 am on April 23, 2024: contributor
    @NicolasDorier
  10. murchandamus added the label Proposed BIP modification on Apr 29, 2024
  11. murchandamus added the label Pending acceptance on Apr 29, 2024
  12. murchandamus commented at 1:12 pm on April 29, 2024: contributor
    @instagibbs, this PR has had unaddressed review comments for three months. Are you still working on this?
  13. instagibbs commented at 1:21 pm on April 29, 2024: member
    I’m not planning on changing anything further unless people feel strongly about @vostrnad ’s suggestions(which seem fine to me as well)
  14. Christewart commented at 1:24 pm on April 29, 2024: contributor
    FWIW, i prefer @instagibbs ’s current wording.
  15. vostrnad commented at 1:48 pm on April 29, 2024: contributor
    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 (https://github.com/bitcoin/bitcoin/pull/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).
  16. jonatack commented at 6:39 pm on May 16, 2024: member
    @NicolasDorier mind weighing in here?
  17. Aldocapurro approved
  18. instagibbs commented at 2:02 pm on October 29, 2024: member
    closing, I think @vostrnad has right way forward, anyone can grab it
  19. instagibbs closed this on Oct 29, 2024
  20. jonatack added the label Up for grabs on Oct 29, 2024


instagibbs dergoegge Christewart vostrnad Roasbeef murchandamus jonatack Aldocapurro

Labels
Proposed BIP modification Pending acceptance Up for grabs

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bips. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 01:10 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me