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.
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.
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.
DrahtBot added the label
Consensus
on Jan 25, 2024
achow101 force-pushed
on Jan 25, 2024
achow101 renamed this:
consensus: Store transaction nVersion as uin32_t
consensus: Store transaction nVersion as uint32_t
on Jan 25, 2024
DrahtBot added the label
CI failed
on Jan 25, 2024
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.
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.
in
src/core_write.cpp:179
in
59d0b7e2d5outdated
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));
achow101
commented at 8:29 pm on January 26, 2024:
Done
maflcko approved
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)
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)
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.
in
src/consensus/tx_verify.cpp:54
in
59d0b7e2d5outdated
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.
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.
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.
shaavan
commented at 12:37 pm on January 26, 2024:
contributor
Concept ACK
Change the type of nVersion for transactions from int32_t β uint32_t
Appropriately remove casts, from the codebase
Introduce test cases to properly address the change in behavior
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:
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
achow101 force-pushed
on Jan 26, 2024
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.
achow101 force-pushed
on Jan 26, 2024
achow101 force-pushed
on Jan 26, 2024
achow101 force-pushed
on Jan 26, 2024
achow101 force-pushed
on Jan 26, 2024
dergoegge
commented at 1:38 pm on January 29, 2024:
member
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.
in
test/functional/test_framework/script.py:742
in
ca7090d43foutdated
738@@ -739,7 +739,7 @@ def SegwitV0SignatureMsg(script, txTo, inIdx, hashtype, amount):
739 hashOutputs = uint256_from_str(hash256(serialize_outputs))
740741 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.
in
src/primitives/transaction.cpp:118
in
6acfcbfe24outdated
maflcko
commented at 12:11 pm on February 21, 2024:
:eyes:
in
src/test/txvalidation_tests.cpp:76
in
4542a4d76eoutdated
71@@ -72,11 +72,11 @@ static inline std::vector<CPubKey> random_keys(size_t num_keys) {
72 return keys;
73 }
7475-// 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:
in
src/policy/v3_policy.cpp:46
in
4542a4d76eoutdated
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
in
test/functional/feature_bip68_sequence.py:413
in
4542a4d76eoutdated
DrahtBot removed review request from vostrnad
on Feb 22, 2024
DrahtBot removed review request from shaavan
on Feb 22, 2024
DrahtBot removed review request from maflcko
on Feb 22, 2024
DrahtBot requested review from vostrnad
on Feb 22, 2024
DrahtBot requested review from shaavan
on Feb 22, 2024
maflcko added this to the milestone 28.0
on Feb 22, 2024
vostrnad
commented at 10:18 pm on February 22, 2024:
none
Since DrahtBot is incessantly pinging me for review, here it is:
ACK4a45b28a3f258bf156eca980aace69e0a90554cd
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.
DrahtBot removed review request from vostrnad
on Feb 22, 2024
DrahtBot removed review request from shaavan
on Feb 22, 2024
DrahtBot requested review from shaavan
on Feb 22, 2024
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
DrahtBot removed review request from shaavan
on Feb 23, 2024
DrahtBot requested review from shaavan
on Feb 23, 2024
DrahtBot removed review request from shaavan
on Feb 23, 2024
DrahtBot requested review from shaavan
on Feb 23, 2024
DrahtBot removed review request from shaavan
on Feb 23, 2024
DrahtBot requested review from shaavan
on Feb 23, 2024
DrahtBot removed review request from shaavan
on Feb 23, 2024
DrahtBot requested review from shaavan
on Feb 23, 2024
shaavan approved
shaavan
commented at 12:56 pm on February 23, 2024:
contributor
ACK4a45b28a3f258bf156eca980aace69e0a90554cd
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:
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! :)
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,
12Thanks for reaching out and providing clear reproducible steps!
34I 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.
DrahtBot added the label
CI failed
on Mar 1, 2024
achow101 force-pushed
on Mar 11, 2024
achow101
commented at 3:01 pm on March 11, 2024:
member
Rebased
maflcko
commented at 3:07 pm on March 11, 2024:
member
Also noting that #29496 unfortunately conflicts and I would advocate for merging that first… but I’m happy to re-review
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
achow101 force-pushed
on Jun 7, 2024
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.
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
achow101 force-pushed
on Jun 7, 2024
maflcko
commented at 4:44 pm on June 9, 2024:
member
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