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

pull TheBlueMatt wants to merge 1 commits into bitcoin:master from TheBlueMatt:2019-07-unsigned-tx-ver changing 4 files +5 −9

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
practicalswift commented at 10:50 PM on July 31, 2019: contributor

Concept ACK
DrahtBot added the label Consensus on Aug 1, 2019
DrahtBot added the label TX fees and policy on Aug 1, 2019
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")

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

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.
TheBlueMatt force-pushed on Aug 1, 2019
TheBlueMatt renamed this:
~~Switch CTransaction::nVersion to an unsigned integer~~
[RFC] Switch CTransaction::nVersion to an unsigned integer
on Aug 1, 2019
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.
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.
TheBlueMatt closed this on Aug 1, 2019
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

TX fees and policy Consensus