[RFC] Switch CTransaction::nVersion to an unsigned integer #16513

1. 
   TheBlueMatt commented at 10:29 pm on July 31, 2019: member

   Consensus-wise we already treat it as an unsigned integer (the only rules around it are in CSV/locktime handling), and it keeps causing confusion here and there, so might as well just formalize it.

   See-also, https://github.com/rust-bitcoin/rust-bitcoin/pull/299

2. 
   practicalswift commented at 10:50 pm on July 31, 2019: contributor

   Concept ACK
3. DrahtBot added the label Consensus on Aug 1, 2019
4. DrahtBot added the label TX fees and policy on Aug 1, 2019
5. 
   laanwj commented at 5:48 am on August 1, 2019: member

   ~0

   Although I do think this is an improvement, I have to say that changing the consensus code like this, for a cleanup to make it more understandable, seems unnecessary risky. I’d suggest adding a documentation comment instead.

   (also while this takes away one kind of confusion it introduces CBlockHeader/CTransaction confusion “which one’s version was signed”)

6. Switch CTransaction::nVersion to an unsigned integer

   Consensus-wise we already treat it as an unsigned integer (the
   only rules around it are in CSV/locktime handling), and it keeps
   causing confusion here and there, so might as well just formalize
   it.
   
   See-also, https://github.com/rust-bitcoin/rust-bitcoin/pull/299

   7dfc4f5492
7. in src/policy/policy.cpp:78 in 20a6a50614 outdated

   74@@ -75,7 +75,9 @@ bool IsStandard(const CScript& scriptPubKey, txnouttype& whichType)
   75 
   76 bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeRate& dust_relay_fee, std::string& reason)
   77 {
   78-    if (tx.nVersion > CTransaction::MAX_STANDARD_VERSION || tx.nVersion < 1) {
   79+    if (tx.nVersion > CTransaction::MAX_STANDARD_VERSION || tx.nVersion < 1 ||
   

   ---

   ---

   

   NicolasDorier commented at 6:20 am on August 1, 2019:

   nit: tx.nVersion == 0 || tx.nVersion > CTransaction::MAX_STANDARD_VERSION is easier to read. The check on 0x80000000 is superfluous as it is always bigger than MAX_STANDARD_VERSION, and I don’t see any alternate parallel dimension where this can be different.

   ---

   

   NicolasDorier commented at 6:20 am on August 1, 2019:

   Better, you can just keep the code unchanged…

   ---

   

   TheBlueMatt commented at 3:46 pm on August 1, 2019:

   Oops, duh.
8. TheBlueMatt force-pushed on Aug 1, 2019
9. TheBlueMatt renamed this:
   Switch CTransaction::nVersion to an unsigned integer
   [RFC] Switch CTransaction::nVersion to an unsigned integer
   on Aug 1, 2019
10. 
    Sjors commented at 7:48 pm on August 1, 2019: member

    Adding a comment in consensus and casting to uint in non-consensus code (e.g. RPC) sounds good to me.
11. 
    TheBlueMatt commented at 9:35 pm on August 1, 2019: member

    Meeting conclusion: don’t touch the variable itself, but do change the RPC to cast it to unsigned everywhere (like we do in consensus). Will open a new PR for that.
12. TheBlueMatt closed this on Aug 1, 2019

    ---

13. DrahtBot locked this on Dec 16, 2021

Contributors
TheBlueMatt practicalswift laanwj NicolasDorier Sjors

Labels
TX fees and policy Consensus