consensus: Store transaction nVersion as uint32_t #29325

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:tx-nversion-uint changing 45 files +157 βˆ’149
  1. achow101 commented at 9:53 pm on January 25, 2024: member

    Given that the use of a transaction’s nVersion is always as an unsigned int, it doesn’t make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.

    Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.

    I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.

    Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.

  2. DrahtBot commented at 9:53 pm on January 25, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, glozow, shaavan
    Concept ACK theStack, sipa
    Stale ACK naumenkogs, vostrnad

    If your review is incorrectly listed, please react with πŸ‘Ž to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30050 (refactor, wallet: get serialized size of CRecipients directly by josibake)
    • #29564 (wallet: clarify FundTransaction signature by S3RK)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)
    • #21283 (Implement BIP 370 PSBTv2 by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Consensus on Jan 25, 2024
  4. achow101 force-pushed on Jan 25, 2024
  5. achow101 renamed this:
    consensus: Store transaction nVersion as uin32_t
    consensus: Store transaction nVersion as uint32_t
    on Jan 25, 2024
  6. DrahtBot added the label CI failed on Jan 25, 2024
  7. DrahtBot commented at 10:53 pm on January 25, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/20879211240

  8. theStack commented at 11:06 pm on January 25, 2024: contributor

    Concept ACK

    Another instance in the tests which could be adapted to 0xffffffff: https://github.com/bitcoin/bitcoin/blob/717103bccec8d271f61f9cd6481b334bd9889146/src/test/transaction_tests.cpp#L783 Slightly related to this PR: since this currently doesn’t produce a warning, was it ever considered to enable -Wsign-conversion?

  9. sipa commented at 11:09 pm on January 25, 2024: member

    Concept ACK on making transaction’s nVersion unsigned, as that is in practice how it already behaves.

    Would it make sense to also rename the field to e.g. unsigned_version or so, so that:

    • Reviewing the diff makes it obvious whether any references to the field exist that aren’t modified.
    • Any future contributor who missed this change won’t accidentally assume the field still has signed semantics?

    For normal code I’d consider this measure overkill, but this is pretty fundamental consensus-critical code.

  10. in src/core_write.cpp:179 in 59d0b7e2d5 outdated
    175@@ -176,7 +176,7 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
    176     entry.pushKV("hash", tx.GetWitnessHash().GetHex());
    177     // Transaction version is actually unsigned in consensus checks, just signed in memory,
    178     // so cast to unsigned before giving it to the user.
    179-    entry.pushKV("version", static_cast<int64_t>(static_cast<uint32_t>(tx.nVersion)));
    180+    entry.pushKV("version", static_cast<int64_t>(tx.nVersion));
    


    maflcko commented at 7:46 am on January 26, 2024:
    0    entry.pushKV("version", tx.nVersion);
    

    No need for a cast here. Also, the comment above is wrong.


    achow101 commented at 8:29 pm on January 26, 2024:
    Done, removed the comment.
  11. in test/functional/feature_taproot.py:1413 in 59d0b7e2d5 outdated
    1409@@ -1410,7 +1410,7 @@ def test_spenders(self, node, spenders, input_counts):
    1410         while left:
    1411             # Construct CTransaction with random nVersion, nLocktime
    1412             tx = CTransaction()
    1413-            tx.nVersion = random.choice([1, 2, random.randint(-0x80000000, 0x7fffffff)])
    1414+            tx.nVersion = random.choice([1, 2, random.randrange(0xffffffff)])
    


    maflcko commented at 7:48 am on January 26, 2024:
    0            tx.nVersion = random.choice([1, 2, random.randbits(32)])
    

    nit: No need to call the randbits wrapper


    achow101 commented at 8:29 pm on January 26, 2024:
    Done
  12. maflcko approved
  13. maflcko commented at 7:54 am on January 26, 2024: member
    Agree that a rename of the field should be considered, but my preference would be just version, as opposed to unsigned_version. Otherwise, the code might be annoyingly verbose, even when it is clear to everyone that the transaction version is unsigned. (Maybe in a separate commit, so that if the changes are touching too many lines, or reviewers don’t like it, it can be dropped again)
  14. maflcko commented at 8:02 am on January 26, 2024: member

    Slightly related to this PR: since this currently doesn’t produce a warning, was it ever considered to enable -Wsign-conversion?

    I think it is a common convention to write -1 as an alias for std::numeric_limits<unsigned ...>::max(). Also, I expect there will be many places where positive signed integers are sign-preserving converted to unsigned (and vice-versa). So enabling that compiler warning may not be possible without changing a lot more code.

    Luckily, if a sign isn’t preserved at runtime, during the tests, the sanitizers will catch it. (See the CI failure)

  15. vostrnad commented at 8:07 am on January 26, 2024: none

    Concept ACK

    As someone quite familiar with Bitcoin’s consensus rules but largely unfamiliar with Bitcoin Core’s internals, I had no idea until recently that the version was stored as a signed integer. This should bring it in line with expectations of most future contributors.

    Also negative versions are just weird.

  16. in src/consensus/tx_verify.cpp:54 in 59d0b7e2d5 outdated
    50@@ -51,8 +51,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
    51     // tx.nVersion is signed integer so requires cast to unsigned otherwise
    52     // we would be doing a signed comparison and half the range of nVersion
    53     // wouldn't support BIP 68.
    54-    bool fEnforceBIP68 = static_cast<uint32_t>(tx.nVersion) >= 2
    55-                      && flags & LOCKTIME_VERIFY_SEQUENCE;
    56+    bool fEnforceBIP68 = tx.nVersion >= 2 && flags & LOCKTIME_VERIFY_SEQUENCE;
    


    vostrnad commented at 8:09 am on January 26, 2024:
    Comment above needs updating.

    achow101 commented at 8:29 pm on January 26, 2024:
    Removed it.
  17. fanquake commented at 10:02 am on January 26, 2024: member

    So enabling that compiler warning may not be possible without changing a lot more code.

    Yea. See here for example output compiling master with GCC 13.2.0 + -Wsign-conversion: https://gist.github.com/fanquake/8e7a49dc1968afd07d89e822ffb4adac.

  18. in test/functional/rpc_rawtransaction.py:479 in 59d0b7e2d5 outdated
    477@@ -478,6 +478,20 @@ def transaction_version_number_tests(self):
    478         decrawtx = self.nodes[0].decoderawtransaction(rawtx)
    479         assert_equal(decrawtx['version'], 0x7fffffff)
    480 
    


    shaavan commented at 12:36 pm on January 26, 2024:
    Question: If we are removing the “signedness” of the tx.nVersion, should we think about removing the test cases that made sense for the signed nVersion?

    achow101 commented at 8:31 pm on January 26, 2024:
    I think it makes sense to leave them in to make sure that everything still matches previous versions.
  19. shaavan commented at 12:37 pm on January 26, 2024: contributor

    Concept ACK

    1. Change the type of nVersion for transactions from int32_t β†’ uint32_t
    2. Appropriately remove casts, from the codebase
    3. Introduce test cases to properly address the change in behavior
    4. Appropriately change the format form <i to <I indicating the change from signed little-endian to unsigned little-endian when serializing and deserializing tx.nVersion.

    Suggestions:

    1. In void static RandomTransaction(CMutableTransaction& tx, bool fSingle) in sighash_tests.cpp file:

      tx.nVersion = int(InsecureRand32()); β†’ tx.nVersion = InsecureRand32(); since the return type of the function is uint32_t

  20. achow101 force-pushed on Jan 26, 2024
  21. achow101 commented at 8:32 pm on January 26, 2024: member
    I’ve added a commit that renames nVersion to just version. Note that it is not a scripted diff since a bunch of different variables share the same name.
  22. achow101 force-pushed on Jan 26, 2024
  23. achow101 force-pushed on Jan 26, 2024
  24. achow101 force-pushed on Jan 26, 2024
  25. achow101 force-pushed on Jan 26, 2024
  26. dergoegge commented at 1:38 pm on January 29, 2024: member
    • CURRENT_VERSION should probably also change to uint32_t

    • CI failes here due to UB, can be fixed by changing to ConsumeIntegral<uint32_t>()

  27. in src/bitcoin-tx.cpp:213 in 3a41d8ca84 outdated
    209@@ -210,7 +210,7 @@ static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)
    210         throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");
    211     }
    212 
    213-    tx.nVersion = (int) newVersion;
    214+    tx.version = (int) newVersion;
    


    maflcko commented at 1:48 pm on January 29, 2024:
    0    tx.version =  newVersion;
    

    achow101 commented at 5:45 pm on January 29, 2024:
    Since newVersion is an int64_t, I think this still needs to be cast to uint32_t, which I’ve done.
  28. in src/primitives/transaction.cpp:66 in 3a41d8ca84 outdated
    62@@ -63,8 +63,8 @@ std::string CTxOut::ToString() const
    63     return strprintf("CTxOut(nValue=%d.%08d, scriptPubKey=%s)", nValue / COIN, nValue % COIN, HexStr(scriptPubKey).substr(0, 30));
    64 }
    65 
    66-CMutableTransaction::CMutableTransaction() : nVersion(CTransaction::CURRENT_VERSION), nLockTime(0) {}
    67-CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime) {}
    68+CMutableTransaction::CMutableTransaction() : version(CTransaction::CURRENT_VERSION), nLockTime(0) {}
    


    maflcko commented at 1:49 pm on January 29, 2024:
    0CMutableTransaction::CMutableTransaction() : version{CTransaction::CURRENT_VERSION}, nLockTime{0} {}
    

    nit: {} cat be used to check at compile time that the integral type does not change the sign or is truncated


    achow101 commented at 5:46 pm on January 29, 2024:
    Done
  29. in src/primitives/transaction.cpp:67 in 3a41d8ca84 outdated
    62@@ -63,8 +63,8 @@ std::string CTxOut::ToString() const
    63     return strprintf("CTxOut(nValue=%d.%08d, scriptPubKey=%s)", nValue / COIN, nValue % COIN, HexStr(scriptPubKey).substr(0, 30));
    64 }
    65 
    66-CMutableTransaction::CMutableTransaction() : nVersion(CTransaction::CURRENT_VERSION), nLockTime(0) {}
    67-CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime) {}
    68+CMutableTransaction::CMutableTransaction() : version(CTransaction::CURRENT_VERSION), nLockTime(0) {}
    69+CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), version(tx.version), nLockTime(tx.nLockTime) {}
    


    maflcko commented at 1:49 pm on January 29, 2024:
    same

    achow101 commented at 5:46 pm on January 29, 2024:
    Done
  30. achow101 force-pushed on Jan 29, 2024
  31. achow101 commented at 5:46 pm on January 29, 2024: member
    • CURRENT_VERSION should probably also change to uint32_t

    • CI failes here due to UB, can be fixed by changing to ConsumeIntegral<uint32_t>()

    Updated these.

  32. DrahtBot removed the label CI failed on Jan 29, 2024
  33. naumenkogs commented at 9:08 am on January 30, 2024: member
    Concept ACK
  34. in src/bitcoin-tx.cpp:213 in af5a1fdfa6 outdated
    209@@ -210,7 +210,7 @@ static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)
    210         throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");
    211     }
    212 
    213-    tx.nVersion = (int) newVersion;
    214+    tx.nVersion = static_cast<uint32_t>(newVersion);
    


    maflcko commented at 9:43 am on January 30, 2024:
    What is the point of parsing as signed integer and then casting to unsigned? Why not use ParseUInt32 directly?

    achow101 commented at 5:20 pm on January 30, 2024:
    Ah, didn’t realize we had that. Done
  35. in src/primitives/transaction.h:299 in af5a1fdfa6 outdated
    295@@ -296,7 +296,7 @@ class CTransaction
    296 {
    297 public:
    298     // Default transaction version.
    299-    static const int32_t CURRENT_VERSION=2;
    300+    static const uint32_t CURRENT_VERSION=2;
    


    maflcko commented at 9:45 am on January 30, 2024:

    nit: Clang-format while touching this line of code?

    0static const uint32_t CURRENT_VERSION{2};
    

    achow101 commented at 5:20 pm on January 30, 2024:
    Done
  36. maflcko approved
  37. DrahtBot added the label Needs rebase on Jan 30, 2024
  38. achow101 force-pushed on Jan 30, 2024
  39. achow101 force-pushed on Jan 30, 2024
  40. DrahtBot removed the label Needs rebase on Jan 30, 2024
  41. in test/functional/test_framework/messages.py:584 in ca7090d43f outdated
    580@@ -581,7 +581,7 @@ def __init__(self, tx=None):
    581             self.wit = copy.deepcopy(tx.wit)
    582 
    583     def deserialize(self, f):
    584-        self.nVersion = int.from_bytes(f.read(4), "little", signed=True)
    585+        self.nVersion = int.from_bytes(f.read(4), "little", signed=False)
    


    maflcko commented at 12:07 pm on February 1, 2024:

    nit in ca7090d43fab2013bd396d683f4cf03062666b14: I think it would be consistent to omit the argument here, which is False by default. Otherwise, it could create the impression that other calls that have it omitted are different (signed) serializations. For example, the unsigned locktime below also has it omitted.

    One could even argue that anything that has a signed value in this file here is a bug or at a minimum a red flag. See for example: fa3886b7c69cbbe564478f30bb2c35e9e6b1cffa or https://github.com/bitcoin/bitcoin/pull/29363


    achow101 commented at 4:35 pm on February 20, 2024:
    Done as suggested.
  42. in test/functional/test_framework/script.py:742 in ca7090d43f outdated
    738@@ -739,7 +739,7 @@ def SegwitV0SignatureMsg(script, txTo, inIdx, hashtype, amount):
    739         hashOutputs = uint256_from_str(hash256(serialize_outputs))
    740 
    741     ss = bytes()
    742-    ss += struct.pack("<i", txTo.nVersion)
    743+    ss += struct.pack("<I", txTo.nVersion)
    


    maflcko commented at 12:26 pm on February 1, 2024:

    unrelated nit: Would be nice to use the python built-in int.to_bytes feature, because the struct interface is confusing and has lead to bugs in the past, see above.

    (Can be done in a follow-up, as a good-first issue)


    achow101 commented at 4:35 pm on February 20, 2024:
    Done as suggested.
  43. in src/primitives/transaction.cpp:118 in 6acfcbfe24 outdated
    116@@ -117,7 +117,7 @@ std::string CTransaction::ToString() const
    117     std::string str;
    118     str += strprintf("CTransaction(hash=%s, ver=%d, vin.size=%u, vout.size=%u, nLockTime=%u)\n",
    


    maflcko commented at 12:33 pm on February 1, 2024:
    review note: This is fine for tinyformat, but if you re-touch it could make sense to change the “signed decimal” to %s or %u.

    achow101 commented at 4:35 pm on February 20, 2024:
    Done
  44. in test/functional/p2p_segwit.py:1167 in 6acfcbfe24 outdated
    1163@@ -1164,7 +1164,7 @@ def serialize_with_witness(self):
    1164                 if not self.wit.is_null():
    1165                     flags |= 1
    1166                 r = b""
    1167-                r += struct.pack("<i", self.nVersion)
    1168+                r += struct.pack("<i", self.version)
    


    maflcko commented at 12:39 pm on February 1, 2024:
    unrelated follow-up: (Same here: Remove struct)

    achow101 commented at 4:36 pm on February 20, 2024:
    Done
  45. maflcko approved
  46. maflcko commented at 12:40 pm on February 1, 2024: member

    ACK 6acfcbfe2487f683e8f62606d195a9974bc2234f 🌡

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 6acfcbfe2487f683e8f62606d195a9974bc2234f 🌡
    3rWI9ANXeLccWSbPTbyKsnso1o6VZc+jr+YkpUEQVUs4ZzPer/ucIHhkSP4LZEz8sZYOUbXyFmxFoSAGyuvN4DA==
    
  47. DrahtBot requested review from theStack on Feb 1, 2024
  48. DrahtBot requested review from sipa on Feb 1, 2024
  49. DrahtBot requested review from naumenkogs on Feb 1, 2024
  50. DrahtBot requested review from shaavan on Feb 1, 2024
  51. DrahtBot requested review from vostrnad on Feb 1, 2024
  52. DrahtBot removed review request from vostrnad on Feb 1, 2024
  53. DrahtBot removed review request from shaavan on Feb 1, 2024
  54. DrahtBot requested review from shaavan on Feb 1, 2024
  55. DrahtBot requested review from vostrnad on Feb 1, 2024
  56. DrahtBot added the label Needs rebase on Feb 10, 2024
  57. in src/test/transaction_tests.cpp:783 in ca7090d43f outdated
    779@@ -780,7 +780,7 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
    780     CheckIsStandard(t);
    781 
    782     // Disallowed nVersion
    783-    t.nVersion = -1;
    784+    t.nVersion = static_cast<uint32_t>(-1);
    


    naumenkogs commented at 8:48 am on February 20, 2024:

    ca7090d43fab2013bd396d683f4cf03062666b14

    Does this still look meaningful? If you want to test a large number, then better check against TX_MAX_STANDARD_VERSION + 1?


    maflcko commented at 11:04 am on February 20, 2024:

    If you want to test a large number, then better check against TX_MAX_STANDARD_VERSION + 1?

    This check already exists two lines below.

    Does this still look meaningful?

    Yes, the goal is to check the edge cases (minimum and maximum possible number)


    sipa commented at 11:20 am on February 20, 2024:
    It’d look a bit more natural using = 0xffffffff or = std::numeric_limits<uint32_t>::max().

    achow101 commented at 4:36 pm on February 20, 2024:
    Changed to std::numeric_limits<uint32_t>::max()
  58. naumenkogs approved
  59. naumenkogs commented at 8:53 am on February 20, 2024: member
    ACK 6acfcbfe2487f683e8f62606d195a9974bc2234f
  60. DrahtBot removed review request from shaavan on Feb 20, 2024
  61. DrahtBot removed review request from vostrnad on Feb 20, 2024
  62. DrahtBot requested review from shaavan on Feb 20, 2024
  63. DrahtBot requested review from vostrnad on Feb 20, 2024
  64. DrahtBot removed review request from shaavan on Feb 20, 2024
  65. DrahtBot removed review request from vostrnad on Feb 20, 2024
  66. DrahtBot requested review from shaavan on Feb 20, 2024
  67. DrahtBot requested review from vostrnad on Feb 20, 2024
  68. DrahtBot removed review request from shaavan on Feb 20, 2024
  69. DrahtBot removed review request from vostrnad on Feb 20, 2024
  70. DrahtBot requested review from shaavan on Feb 20, 2024
  71. DrahtBot requested review from vostrnad on Feb 20, 2024
  72. DrahtBot removed review request from shaavan on Feb 20, 2024
  73. DrahtBot removed review request from vostrnad on Feb 20, 2024
  74. DrahtBot requested review from shaavan on Feb 20, 2024
  75. DrahtBot requested review from vostrnad on Feb 20, 2024
  76. achow101 force-pushed on Feb 20, 2024
  77. DrahtBot removed the label Needs rebase on Feb 20, 2024
  78. in src/test/transaction_tests.cpp:782 in 4542a4d76e outdated
    779@@ -780,20 +780,20 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
    780     CheckIsStandard(t);
    781 
    782     // Disallowed nVersion
    


    maflcko commented at 12:11 pm on February 21, 2024:
    :eyes:
  79. in src/test/txvalidation_tests.cpp:76 in 4542a4d76e outdated
    71@@ -72,11 +72,11 @@ static inline std::vector<CPubKey> random_keys(size_t num_keys) {
    72     return keys;
    73 }
    74 
    75-// Creates a placeholder tx (not valid) with 25 outputs. Specify the nVersion and the inputs.
    76+// Creates a placeholder tx (not valid) with 25 outputs. Specify the version and the inputs.
    77 static inline CTransactionRef make_tx(const std::vector<COutPoint>& inputs, int32_t version)
    


    maflcko commented at 12:12 pm on February 21, 2024:
    :eyes:
  80. in src/policy/v3_policy.cpp:46 in 4542a4d76e outdated
    42@@ -43,13 +43,13 @@ struct ParentInfo {
    43     const Txid& m_txid;
    44     /** Wtxid used for debug string */
    45     const Wtxid& m_wtxid;
    46-    /** nVersion used to check inheritance of v3 and non-v3 */
    47-    decltype(CTransaction::nVersion) m_version;
    48+    /** version used to check inheritance of v3 and non-v3 */
    


    maflcko commented at 12:15 pm on February 21, 2024:
    Need to change it in the comment in the header as well

    maflcko commented at 1:11 pm on February 21, 2024:
    Also doc/policy/mempool-replacements.md
  81. in test/functional/feature_bip68_sequence.py:413 in 4542a4d76e outdated
    409@@ -410,6 +410,5 @@ def test_version2_relay(self):
    410         mini_wallet = MiniWallet(self.nodes[1])
    411         mini_wallet.send_self_transfer(from_node=self.nodes[1], version=2)
    412 
    413-
    


    maflcko commented at 12:19 pm on February 21, 2024:
    :eyes:

    achow101 commented at 5:27 pm on February 21, 2024:
    Fixed
  82. maflcko approved
  83. maflcko commented at 12:19 pm on February 21, 2024: member

    Left some nits. Lgtm

    re-ACK 4542a4d76e7598ac67fa4f154b13b2fa32050dd2 🌷

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: re-ACK 4542a4d76e7598ac67fa4f154b13b2fa32050dd2  🌷
    3gZ56+jV2aJ8Pwn0w4jdGsrhbCyYQh5Syoq40v5jlZBUMztqTt5uUIu0rYOSOzJFKOchO+zQALuivCEUPn5W5AA==
    
  84. DrahtBot removed review request from shaavan on Feb 21, 2024
  85. DrahtBot removed review request from vostrnad on Feb 21, 2024
  86. DrahtBot requested review from shaavan on Feb 21, 2024
  87. DrahtBot requested review from vostrnad on Feb 21, 2024
  88. DrahtBot requested review from naumenkogs on Feb 21, 2024
  89. DrahtBot removed review request from shaavan on Feb 21, 2024
  90. DrahtBot removed review request from vostrnad on Feb 21, 2024
  91. DrahtBot requested review from shaavan on Feb 21, 2024
  92. DrahtBot requested review from vostrnad on Feb 21, 2024
  93. DrahtBot removed review request from shaavan on Feb 21, 2024
  94. DrahtBot removed review request from vostrnad on Feb 21, 2024
  95. DrahtBot requested review from shaavan on Feb 21, 2024
  96. DrahtBot requested review from vostrnad on Feb 21, 2024
  97. achow101 force-pushed on Feb 21, 2024
  98. achow101 commented at 5:27 pm on February 21, 2024: member
    Changed all of the remaining tx.nVersion that I could find.
  99. naumenkogs commented at 7:27 am on February 22, 2024: member
    ACK 4a45b28a3f258bf156eca980aace69e0a90554cd
  100. DrahtBot removed review request from naumenkogs on Feb 22, 2024
  101. DrahtBot removed review request from shaavan on Feb 22, 2024
  102. DrahtBot removed review request from vostrnad on Feb 22, 2024
  103. DrahtBot requested review from vostrnad on Feb 22, 2024
  104. DrahtBot requested review from shaavan on Feb 22, 2024
  105. DrahtBot requested review from maflcko on Feb 22, 2024
  106. maflcko commented at 8:31 am on February 22, 2024: member

    ACK 4a45b28a3f258bf156eca980aace69e0a90554cd πŸ›Œ

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 4a45b28a3f258bf156eca980aace69e0a90554cd πŸ›Œ
    3eLEC1ffengRZXyHf0wui+3jRLFqHwl4jpGYk5AkJfGzQW0lq2IBhVeMaIVQzAEZ0EuVkRS4s4A3Gq85yvE26Bg==
    
  107. DrahtBot removed review request from vostrnad on Feb 22, 2024
  108. DrahtBot removed review request from shaavan on Feb 22, 2024
  109. DrahtBot removed review request from maflcko on Feb 22, 2024
  110. DrahtBot requested review from vostrnad on Feb 22, 2024
  111. DrahtBot requested review from shaavan on Feb 22, 2024
  112. maflcko added this to the milestone 28.0 on Feb 22, 2024
  113. vostrnad commented at 10:18 pm on February 22, 2024: none

    Since DrahtBot is incessantly pinging me for review, here it is:

    ACK 4a45b28a3f258bf156eca980aace69e0a90554cd

    I reviewed the code with my somewhat limited knowledge of C++ and didn’t find any problems. I also checked that all instances of nVersion that referred to the transaction version have been renamed, except for the command line argument in bitcoin-tx where it would be a breaking change.

  114. DrahtBot removed review request from vostrnad on Feb 22, 2024
  115. DrahtBot removed review request from shaavan on Feb 22, 2024
  116. DrahtBot requested review from shaavan on Feb 22, 2024
  117. maflcko commented at 8:47 am on February 23, 2024: member

    Since DrahtBot is incessantly pinging me for review, here it is:

    This is a GitHub bug. I believe a combination of the following:

    • A non-member leaves a review comment (The GitHub review thing, not a pull/issue comment)
    • A non-member indicates a code review (something with A C K), which goes stale and makes DrahtBot to re-request review

    This is fine, as the re-request API call should either succeed (for members) or fail (for non-members). However, as the non-member left a previous review, the API call both succeeds and fails at the same time for some reason (this is a GitHub bug).

    Possible solutions are:

    • Report a bug to GitHub, ideally with steps to reproduce
    • Removing the intent to review this pull request, by editing your comment, which says A C K
    • Exclude non-members in the DrahtBot code explicitly (It is open-source, pull requests welcome)
    • Do not leave a review comment as a non-member, only pull/issue comments
    • Ask to become a member, if you are an active reviewer or contributor
  118. DrahtBot removed review request from shaavan on Feb 23, 2024
  119. DrahtBot requested review from shaavan on Feb 23, 2024
  120. DrahtBot removed review request from shaavan on Feb 23, 2024
  121. DrahtBot requested review from shaavan on Feb 23, 2024
  122. DrahtBot removed review request from shaavan on Feb 23, 2024
  123. DrahtBot requested review from shaavan on Feb 23, 2024
  124. DrahtBot removed review request from shaavan on Feb 23, 2024
  125. DrahtBot requested review from shaavan on Feb 23, 2024
  126. shaavan approved
  127. shaavan commented at 12:56 pm on February 23, 2024: contributor

    ACK 4a45b28a3f258bf156eca980aace69e0a90554cd

    As far as the internal tx nVersion variables go, all the instances of it are rightly converted to uint32_t. All the typecasting is appropriately modified (or removed) for clarity. And we moved to using int.to_bytes instead of struct.pack in the test which allows for a cleaner, more readable code by using the function more suitable for integer conversion.

    The only question I have is I notice that though the instances of internal tx nVersions are modified, the command line call of it still uses nVersion as the variable:

    https://github.com/bitcoin/bitcoin/blob/1ac627c485a43e50a9a49baddce186ee3ad4daad/src/bitcoin-tx.cpp#L57

    https://github.com/bitcoin/bitcoin/blob/1ac627c485a43e50a9a49baddce186ee3ad4daad/src/bitcoin-tx.cpp#L713-L714

    This makes sense, as we don’t want to break compatibility with code that already utilizes this variable in their code and scripts. What I am curious about is whether we will ever be moving the using version as the name of the command line variable as well.

    P.S.: On a side, I must say that DrahtBot is a persistent boi! :)

  128. maflcko commented at 1:11 pm on February 23, 2024: member

    P.S.: On a side, I must say that DrahtBot is a persistent boi! :)

    I’ve reported the bug to GitHub and will fix it on the bot side, otherwise.

    0Hi maflcko,
    1 
    2Thanks for reaching out and providing clear reproducible steps!
    3 
    4I have filed an internal issue with the engineering team on this report. While I cannot a guarantee if or when a fix would be implemented, we appreciate you reaching out on how we can make things better.
    
  129. DrahtBot added the label CI failed on Mar 1, 2024
  130. achow101 force-pushed on Mar 11, 2024
  131. achow101 commented at 3:01 pm on March 11, 2024: member
    Rebased
  132. maflcko commented at 3:07 pm on March 11, 2024: member

    ACK 9b929ed4e741ff8571f799e7342829ff44017da8 πŸ“²

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 9b929ed4e741ff8571f799e7342829ff44017da8 πŸ“²
    3g9VfcoL7yrSDf66FEG5fxokFIeIYYdvGFeTDPnUyhfiTejntiI2/h9Ej0FDu+FWXWSCdiHfczUJCqHpuODfcDg==
    
  133. DrahtBot requested review from vostrnad on Mar 11, 2024
  134. DrahtBot requested review from shaavan on Mar 11, 2024
  135. DrahtBot requested review from naumenkogs on Mar 11, 2024
  136. DrahtBot removed the label CI failed on Mar 12, 2024
  137. DrahtBot added the label Needs rebase on Mar 12, 2024
  138. achow101 force-pushed on Mar 12, 2024
  139. DrahtBot removed the label Needs rebase on Mar 12, 2024
  140. maflcko commented at 2:04 pm on March 15, 2024: member

    ACK cc0d6bd73e67abde657cb8fb38c0e18de316d337 🎺

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK cc0d6bd73e67abde657cb8fb38c0e18de316d337 🎺
    35b+h5HG/odh4+xd3o7x2kwyIrbffvZ/ZDbF/Ri9Xb2iKeqMzIh3gli0mIjYmhBtED44GFSdmNPexnkc6Xjz6DA==
    
  141. vostrnad commented at 7:40 pm on March 15, 2024: none
    re-ACK cc0d6bd73e67abde657cb8fb38c0e18de316d337
  142. DrahtBot added the label CI failed on Apr 20, 2024
  143. DrahtBot removed the label CI failed on Apr 23, 2024
  144. DrahtBot added the label Needs rebase on May 15, 2024
  145. achow101 force-pushed on May 15, 2024
  146. DrahtBot removed the label Needs rebase on May 15, 2024
  147. DrahtBot added the label Needs rebase on May 23, 2024
  148. achow101 force-pushed on May 23, 2024
  149. DrahtBot removed the label Needs rebase on May 23, 2024
  150. DrahtBot added the label Needs rebase on May 24, 2024
  151. achow101 force-pushed on May 29, 2024
  152. DrahtBot removed the label Needs rebase on May 29, 2024
  153. achow101 commented at 11:15 pm on June 4, 2024: member
    @maflcko @vostrnad @shaavan @naumenkogs reACKs would be appreciated
  154. maflcko commented at 9:44 am on June 5, 2024: member

    ACK 2431ecff2fb6f4b68d3de0ea36e3bcc4403e94ba πŸ”³

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 2431ecff2fb6f4b68d3de0ea36e3bcc4403e94ba πŸ”³
    3xur9ZGGma5tikLjekGHRJ+CttEpJwRqGw/j5ke/6G6hfpK7N5+z+NP9z7nBzFBXKeGVFyAdjHuk/SQSxXJKrDQ==
    
  155. shaavan approved
  156. shaavan commented at 11:39 am on June 5, 2024: contributor
    ACK 2431ecff2fb6f4b68d3de0ea36e3bcc4403e94ba πŸš€
  157. achow101 force-pushed on Jun 6, 2024
  158. maflcko commented at 7:39 am on June 7, 2024: member

    ACK 659663af32f02c570e334e8f375fd5f5737258d7 πŸš‹

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 659663af32f02c570e334e8f375fd5f5737258d7 πŸš‹
    3d3lhhWY8KPIufcgBMSH5+n/TTcdj5RXJZN1O0Qj2O4NCepHVXB3nlSdnbSxKM7sufpYbbH2UrxxgSsPFbEO2Cw==
    
  159. DrahtBot requested review from shaavan on Jun 7, 2024
  160. shaavan commented at 10:52 am on June 7, 2024: contributor
    ACK 659663af32f02c570e334e8f375fd5f5737258d7 ⚑️
  161. glozow commented at 3:26 pm on June 7, 2024: member

    concept ACK + lgtm 659663af32

    Seems like this is a remaining mention of CTransaction::nVersion? https://github.com/bitcoin/bitcoin/blob/659663af32f02c570e334e8f375fd5f5737258d7/src/compressor.h#L57-L60

    Also noting that #29496 unfortunately conflicts and I would advocate for merging that first… but I’m happy to re-review

  162. consensus: Store transaction nVersion as uint32_t
    Given that the use of a transaction's nVersion is always as an unsigned
    int, it doesn't make sense to store it as signed and then cast it to
    unsigned.
    27e70f1f5b
  163. achow101 force-pushed on Jun 7, 2024
  164. achow101 commented at 4:46 pm on June 7, 2024: member

    Rebased

    Seems like this is a remaining mention of CTransaction::nVersion?

    Good catch, fixed.

  165. refactor: Rename CTransaction::nVersion to version
    In order to ensure that the change of nVersion to a uint32_t in the
    previous commit has no effect, rename nVersion to version in this commit
    so that reviewers can easily spot if a spot was missed or if there is a
    check somewhere whose semantics have changed.
    429ec1aaaa
  166. achow101 force-pushed on Jun 7, 2024
  167. maflcko commented at 4:44 pm on June 9, 2024: member

    ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 🐿

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 🐿
    3V0zhbzKiZuDg5Jhgt1TaDJ+/E32EhiOwnzfhBu5ZhuK2A8ZNVjfYRFlP0dTywWM3YcW0T2QOm47s8ncGNm7nAw==
    
  168. DrahtBot requested review from glozow on Jun 9, 2024
  169. DrahtBot requested review from shaavan on Jun 9, 2024
  170. glozow commented at 8:41 am on June 10, 2024: member

    ACK 429ec1aaaa

    Thank you for rebasing!

  171. glozow approved
  172. shaavan commented at 11:58 am on June 10, 2024: contributor
    ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 πŸ’―
  173. fanquake merged this on Jun 12, 2024
  174. fanquake closed this on Jun 12, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me