Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dump transaction version as an unsigned integer in RPC/TxToUniv #16525

Open
wants to merge 2 commits into
base: master
from

Conversation

@TheBlueMatt
Copy link
Contributor

commented Aug 1, 2019

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, rust-bitcoin/rust-bitcoin#299

@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

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
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-unsigned-tx-ver branch from 12f33b7 to 980d490 Aug 2, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

This needs to update the rpc help texts and add release notes

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Does it? Negative transaction versions are already non-standard so you can't materially use them.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

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.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

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, rust-bitcoin/rust-bitcoin#299
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-unsigned-tx-ver branch from 980d490 to 970de70 Aug 2, 2019
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Added release notes.

Copy link
Member

left a comment

Relevant code:

// tx.nVersion is signed integer so requires cast to unsigned otherwise
// we would be doing a signed comparison and half the range of nVersion
// wouldn't support BIP 68.
bool fEnforceBIP68 = static_cast<uint32_t>(tx.nVersion) >= 2
&& flags & LOCKTIME_VERIFY_SEQUENCE;

if (tx.nVersion > CTransaction::MAX_STANDARD_VERSION || tx.nVersion < 1) {
reason = "version";
return false;
}

// Fail if the transaction's version number is not set high
// enough to trigger BIP 68 rules.
if (static_cast<uint32_t>(txTo->nVersion) < 2)
return false;

@@ -0,0 +1,7 @@
RPC changes

This comment has been minimized.

Copy link
@promag

promag Aug 7, 2019

Member

nit, REST /rest/tx/txid.json is also affected.

@fanquake fanquake added this to Chasing Concept ACK in High-priority for review Aug 14, 2019
@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

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

@luke-jr

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Concept ACK. Can you rebase this onto 37f236a 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.
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@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.

@achow101

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
High-priority for review
Chasing Concept ACK
8 participants
You can’t perform that action at this time.