Consensus-wise we already treat it as an unsigned integer (the only rules around it are in CSV/locktime handling), but changing the underlying data type means touching consensus code for a simple cleanup change, which isn't really worth it.
See-also, https://github.com/rust-bitcoin/rust-bitcoin/pull/299
rpc_rawtransaction.py is failing with:
test/functional/test_runner.py rpc_rawtransaction.py
Temporary test directory at /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190802_081411
Remaining jobs: [rpc_rawtransaction.py]
1/1 - rpc_rawtransaction.py failed, Duration: 28 s
stdout:
2019-08-02T00:14:11.781000Z TestFramework (INFO): Initializing test directory /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190802_081411/rpc_rawtransaction_0
2019-08-02T00:14:13.145000Z TestFramework (INFO): prepare some coins for multiple *rawtransaction commands
2019-08-02T00:14:20.763000Z TestFramework (INFO): Test getrawtransaction on genesis block coinbase returns an error
2019-08-02T00:14:20.765000Z TestFramework (INFO): Check parameter types and required parameters of createrawtransaction
2019-08-02T00:14:20.906000Z TestFramework (INFO): Check that createrawtransaction accepts an array and object as outputs
2019-08-02T00:14:20.975000Z TestFramework (INFO): sendrawtransaction with missing prevtx info (bech32)
2019-08-02T00:14:21.107000Z TestFramework (INFO): sendrawtransaction with missing prevtx info (p2sh-segwit)
2019-08-02T00:14:21.228000Z TestFramework (INFO): sendrawtransaction with missing prevtx info (legacy)
2019-08-02T00:14:21.294000Z TestFramework (INFO): sendrawtransaction with missing input
2019-08-02T00:14:39.081000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/michael/github/bitcoin/test/functional/test_framework/test_framework.py", line 193, in main
self.run_test()
File "/Users/michael/github/bitcoin/test/functional/rpc_rawtransaction.py", line 424, in run_test
assert_equal(decrawtx['version'], -0x80000000)
File "/Users/michael/github/bitcoin/test/functional/test_framework/util.py", line 40, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(2147483648 == -2147483648)
2019-08-02T00:14:39.135000Z TestFramework (INFO): Stopping nodes
2019-08-02T00:14:39.499000Z TestFramework (WARNING): Not cleaning up dir /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190802_081411/rpc_rawtransaction_0
2019-08-02T00:14:39.499000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190802_081411/rpc_rawtransaction_0/test_framework.log
2019-08-02T00:14:39.499000Z TestFramework (ERROR): Hint: Call /Users/michael/github/bitcoin/test/functional/combine_logs.py '/var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_runner_₿_🏃_20190802_081411/rpc_rawtransaction_0' to consolidate all logs
rpc_rawtransaction.py | ✖ Failed | 28 s
This needs to update the rpc help texts and add release notes
Does it? Negative transaction versions are already non-standard so you can't materially use them.
Because they are non-standard doesn't mean they are not in the main chain. There are many databases or explorers out there that use the rpc interface to get tx info. See for example https://www.smartbit.com.au/tx/35e79ee733fad376e76d16d1f10088273c2f4c2eaba1374a837378a88e530005
Changing the return type will break their deployment.
Concept ACK assuming @MarcoFalke's feedback is addressed
Consensus-wise we already treat it as an unsigned integer (the
only rules around it are in CSV/locktime handling), but changing
the underlying data type means touching consensus code for a
simple cleanup change, which isn't really worth it.
See-also, https://github.com/rust-bitcoin/rust-bitcoin/pull/299
Added release notes.
0 | @@ -0,0 +1,7 @@ 1 | +RPC changes
nit, REST /rest/tx/txid.json is also affected.
Relevant code:
https://github.com/bitcoin/bitcoin/blob/e5fdda68c6d2313edb74443f0d1e6d2ce2d97f5e/src/consensus/tx_verify.cpp#L42-L46 https://github.com/bitcoin/bitcoin/blob/e5fdda68c6d2313edb74443f0d1e6d2ce2d97f5e/src/policy/policy.cpp#L78-L81 https://github.com/bitcoin/bitcoin/blob/e5fdda68c6d2313edb74443f0d1e6d2ce2d97f5e/src/script/interpreter.cpp#L1375-L1379
joinpsbts in rpc/rawtransaction.cpp looks to me like it'll currently leave nVersion as 1 rather than allowing a negative (or 2**31 or higher) value; nothing else seems like it has behaviour changes if nVersion were unsigned rather than signed.
Concept ACK
Concept ACK. Can you rebase this onto 37f236acc6de08745118ac6cb4268bb5206e67c6 so it's a clean merge to 0.18 too?
This gets its own release note callout, though doesn't appear to
violate the BIP as the BIP appears to be underspecified. We
probably want to update BIP 174 to mention how version numbers are
combined.
@ajtowns I updated in a new commit to combine by treating nVersion as unsigned, which, as far as I can tell, doesn't violate BIP 174, though as I understand it it probably makes sense to have specified this kind of thing in the BIP.
Concept ACK. The changes to joinpsbts look correct. As that RPC is part of the "creator" role, the version number to be used is up to the discretion of the creator so it isn't actually specified in the BIP.
ACK e80259f1976545e4f1ab6a420644be0c32261773
ACK e80259f1976545e4f1ab6a420644be0c32261773
ACK e80259f1976545e4f1ab6a420644be0c32261773 code review -- checked all other uses of tx.nVersion treat it as unsigned (except for policy.cpp:IsStandard anyway), so looks good.
Ready for merge? :)
ACK e80259f