-
Notifications
You must be signed in to change notification settings - Fork 38.8k
test, refactor: add GetTransaction() coverage, improve rpc_rawtransaction #22437
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
test, refactor: add GetTransaction() coverage, improve rpc_rawtransaction #22437
Conversation
|
Test output after these changes. The slow legacy multisig tests are placed at the end. |
241ef3a to
27ed21d
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
27ed21d to
729518f
Compare
729518f to
993189b
Compare
|
ACK 993189b1fe39cfc29e960ea3a20092309001fa8f very nicely done! |
993189b to
d27edf1
Compare
lsilva01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK d27edf1 on Ubuntu 20.04
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Concept ACK |
d27edf1 to
afb4eab
Compare
|
Rebased due to #22510 and updated with the excellent review feedback from @kiminuo and @rajarshimaitra (thanks!) Commit-by-commit changes (re-pushed a second time for #22437 (comment)):
Thank you @mjdietzx, @lsilva01, and @rajarshimaitra for the ACKs. Would you mind re-ACKing? |
afb4eab to
7f7e64e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jonatack for considering the comments.
Re tested ACK afb4eab
It might not be a good place to discuss test approaches here, but I have the following observations
- I am finding that
getrawtransaction_testssetup can be simplified a bit. Instead of 3 transactions (all in the chain) we can do the test with just one. All we need is a tx in a block and its id. So if we simply start the test like this
test_tx = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
test_tx_hex = self.nodes[2].getrawtransaction(test_tx)
block1, block2 = self.nodes[2].generate(2)
self.sync_peers()this will give us everything we want to do the assertions. This I feel will simplify the testing logic and would make the test easy
to reason about.
- The #22383 changes behaviour that if
txindexis on, theblockhashsearching won't take place even if it's provided. It seems to me that this particular behaviour is not being tested. I am also not sure what can be a possible approach to test this. But I feel this needs to be covered, as it's a performance boost, and we don't want future PR to accidentally change this.
Would like to have your thoughts on the above. It's not necessary to have those changes in this PR itself.
Below is a minor redundancy I found.
|
@rajarshimaitra I agree, bringing together the various related tests shows that we can simplify them. I'll look at integrating the following diff based on your suggestion, which works for me, into the changes. code diff
diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 84210d3a03..cc6324d9fc 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -101,19 +101,9 @@ class RawTransactionsTest(BitcoinTestFramework):
self.raw_multisig_transaction_legacy_tests()
def getrawtransaction_tests(self):
- addr = self.nodes[1].getnewaddress()
- txid = self.nodes[0].sendtoaddress(addr, 10)
- self.generate_and_sync(node=0, blocks=1)
- vout = find_vout_for_address(self.nodes[1], txid, addr)
- rawTx = self.nodes[1].createrawtransaction([{'txid': txid, 'vout': vout}], {self.nodes[1].getnewaddress(): 9.999})
- rawTxSigned = self.nodes[1].signrawtransactionwithwallet(rawTx)
- txid2 = self.nodes[1].sendrawtransaction(rawTxSigned['hex'])
- self.generate_and_sync(node=0, blocks=1)
-
# Make a tx by sending, then generate 2 blocks; block1 has the tx in it
- txid3 = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
+ tx = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
+ hex = self.nodes[2].getrawtransaction(tx)
block1, block2 = self.nodes[2].generate(2)
- self.sync_peers()
err_msg = (
"No such mempool transaction. Use -txindex or provide a block hash to enable"
@@ -134,47 +124,44 @@ class RawTransactionsTest(BitcoinTestFramework):
if n == 0 or n == 5:
# with -txindex
for verbose in [None, 0, False]:
- assert_equal(self.nodes[n].getrawtransaction(txid2, verbose), rawTxSigned['hex'])
+ assert_equal(self.nodes[n].getrawtransaction(tx, verbose), hex)
for verbose in [1, True]:
- gottx1 = self.nodes[n].getrawtransaction(txid2, verbose)
- assert_equal(gottx1['hex'], rawTxSigned['hex'])
- assert 'in_active_chain' not in gottx1.keys()
- gottx2 = self.nodes[n].getrawtransaction(txid=txid3, verbose=verbose)
- assert_equal(gottx2['txid'], txid3)
- assert 'in_active_chain' not in gottx2.keys()
+ gottx = self.nodes[n].getrawtransaction(tx, verbose)
+ assert_equal(gottx['hex'], hex)
+ assert 'in_active_chain' not in gottx.keys()
else:
# without -txindex
for verbose in [None, 0, False, 1, True]:
- assert_raises_rpc_error(-5, err_msg, self.nodes[n].getrawtransaction, txid2, verbose)
+ assert_raises_rpc_error(-5, err_msg, self.nodes[n].getrawtransaction, tx, verbose)
# 2. invalid parameters - supply txid and invalid boolean values (strings) for verbose
for value in ["True", "False"]:
- assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid=txid2, verbose=value)
+ assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid=tx, verbose=value)
# 3. invalid parameters - supply txid and empty array
- assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid2, [])
+ assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, tx, [])
# 4. invalid parameters - supply txid and empty dict
- assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, txid2, {})
+ assert_raises_rpc_error(-1, "not a boolean", self.nodes[n].getrawtransaction, tx, {})
# 5. with block hash
# We should be able to get the raw transaction by providing the correct block
- gottx = self.nodes[n].getrawtransaction(txid=txid3, verbose=True, blockhash=block1)
- assert_equal(gottx['txid'], txid3)
+ gottx = self.nodes[n].getrawtransaction(txid=tx, verbose=True, blockhash=block1)
+ assert_equal(gottx['txid'], tx)
assert_equal(gottx['in_active_chain'], True)
# We should not get the tx if we provide an unrelated block
- assert_raises_rpc_error(-5, "No such transaction found", self.nodes[n].getrawtransaction, txid=txid3, blockhash=block2)
+ assert_raises_rpc_error(-5, "No such transaction found", self.nodes[n].getrawtransaction, txid=tx, blockhash=block2)
# An invalid block hash should raise the correct errors
- assert_raises_rpc_error(-1, "JSON value is not a string as expected", self.nodes[n].getrawtransaction, txid=txid3, blockhash=True)
- assert_raises_rpc_error(-8, "parameter 3 must be of length 64 (not 6, for 'foobar')", self.nodes[n].getrawtransaction, txid=txid3, blockhash="foobar")
- assert_raises_rpc_error(-8, "parameter 3 must be of length 64 (not 8, for 'abcd1234')", self.nodes[n].getrawtransaction, txid=txid3, blockhash="abcd1234")
+ assert_raises_rpc_error(-1, "JSON value is not a string as expected", self.nodes[n].getrawtransaction, txid=tx, blockhash=True)
+ assert_raises_rpc_error(-8, "parameter 3 must be of length 64 (not 6, for 'foobar')", self.nodes[n].getrawtransaction, txid=tx, blockhash="foobar")
+ assert_raises_rpc_error(-8, "parameter 3 must be of length 64 (not 8, for 'abcd1234')", self.nodes[n].getrawtransaction, txid=tx, blockhash="abcd1234")
foo = "ZZZ0000000000000000000000000000000000000000000000000000000000000"
- assert_raises_rpc_error(-8, f"parameter 3 must be hexadecimal string (not '{foo}')", self.nodes[n].getrawtransaction, txid=txid3, blockhash=foo)
+ assert_raises_rpc_error(-8, f"parameter 3 must be hexadecimal string (not '{foo}')", self.nodes[n].getrawtransaction, txid=tx, blockhash=foo)
bar = "0000000000000000000000000000000000000000000000000000000000000000"
- assert_raises_rpc_error(-5, "Block hash not found", self.nodes[n].getrawtransaction, txid=txid3, blockhash=bar)
+ assert_raises_rpc_error(-5, "Block hash not found", self.nodes[n].getrawtransaction, txid=tx, blockhash=bar)
# Undo the blocks and verify that "in_active_chain" is false.
self.nodes[n].invalidateblock(block1)
- gottx = self.nodes[n].getrawtransaction(txid=txid3, verbose=True, blockhash=block1)
+ gottx = self.nodes[n].getrawtransaction(txid=tx, verbose=True, blockhash=block1)
assert_equal(gottx['in_active_chain'], False)
self.nodes[n].reconsiderblock(block1)
assert_equal(self.nodes[n].getbestblockhash(), block2)
At first glance I don't see a straightforward way to test which code path is taken in that case with the current code, as there is no observable difference in behavior other than hopefully in performance, for which a benchmark could be added. |
|
@rajarshimaitra I've appended a commit with you as the author. Let me know if the name and email |
3bd7ead to
c2d7995
Compare
|
It could be a good to reorder some of these commits so we're not making one change, then changing the same lines again straight after. For example, in 10a3db0 you rename variables i.e |
|
Thanks everyone for the reviews! It would be nice for this to be merged before any other change touching this file invalidates all the review. One can hope :) |
f0aacf0 to
d426cab
Compare
|
Dropped the commits after d426cab86c1b, no other change to not invalidate review. Can continue with the other improvements (test de-duplication, in-mempool txn tests, setup simplification, variable naming cleanup, and others) in a follow-up. |
(and make the 'string "Flase"' test clearer as requested by reviewers)
d426cab to
adfade6
Compare
|
Squashed the first two commits per review feedback and made some minor improvements per |
adfade6 to
387355b
Compare
|
reACK 387355b |
|
reACK 387355b |
maflcko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. Thanks.
Approach ACK 387355b 🔆
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Approach ACK 387355bb9482a09c1fc9b137bea56745a93b7dfd 🔆
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjpnQv/d+jQxCI/GaFHTfHIOmYvfjLAIVoaPnCfJYPoRavcXA92Ov2DiktzzN2j
j8s0Y7Jy86T33aBsbFnGBxM5hWG/WpaceaR7Ga3ixRMQce8TRB9Hng+ab9SuhL7I
eQUdaCAYCDr8pRdY3lmcOOJGzSPNKdiuf2cfFwyDL287VCXrn4JNCTOHZBfjknBu
8aTzGoJXkbbCtxQ26+aNFn4otQrQES9x8VkfrCUbGrUG9wrq64lbhqeoKq3rg6cd
ZbcxL6B8RLVBGiGGVyK4pn0P/sQmoCuND+1fEpUOT7TR7EFmI54Ah3xg3ycsvwx6
WZv+iDh+Zgv/TqTmMvklCHzdxanzxSbnjZ8hu8xiOs3ZVC6r98yYW0Z0QBSx+PN6
WizL0l1tCna5qnRl3ySta74GTOrtFUSihQq9jl+3AWV3Mo3c3Twuz8BCrCXoY+/J
xZEtkIwscpRNKC9nlGzKwItI59+s+B7E3AgmF9gDmOddQ6ZDSwrwrVOrMwuhO9aw
lSJFPNLT
=NEYX
-----END PGP SIGNATURE-----
Timestamp of file with hash 2d816e7fc4d23afbf4d13cf22ed4e17e67a0d483e53eeb2099251d3af22c67e7 -
| vout = find_vout_for_address(self.nodes[1], txid, addr) | ||
| rawTx = self.nodes[1].createrawtransaction([{'txid': txid, 'vout': vout}], {self.nodes[1].getnewaddress(): 9.999}) | ||
| rawTxSigned = self.nodes[1].signrawtransactionwithwallet(rawTx) | ||
| txId = self.nodes[1].sendrawtransaction(rawTxSigned['hex']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traceback (most recent call last):
File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/home/marco/workspace/btc_bitcoin_core/test/functional/rpc_rawtransaction.py", line 84, in run_test
self.getrawtransaction_tests()
File "/home/marco/workspace/btc_bitcoin_core/test/functional/rpc_rawtransaction.py", line 107, in getrawtransaction_tests
assert_equal(self.nodes[n].getrawtransaction(txId), rawTxSigned['hex'])
File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/coverage.py", line 49, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/authproxy.py", line 144, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: No such mempool transaction. Use -txindex or provide a block hash to enable blockchain transaction queries. Use gettransaction for wallet transactions. (-5)
not sure how this could happen, though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the test isn't testing anything, as the tx is never included in a block.
If the tx is included, the test will fail:
diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 96691b2686..b648012413 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -99,7 +99,7 @@ class RawTransactionsTest(BitcoinTestFramework):
rawTx = self.nodes[1].createrawtransaction([{'txid': txid, 'vout': vout}], {self.nodes[1].getnewaddress(): 9.999})
rawTxSigned = self.nodes[1].signrawtransactionwithwallet(rawTx)
txId = self.nodes[1].sendrawtransaction(rawTxSigned['hex'])
- self.generate(self.nodes[0], 1)
+ self.generateblock(self.nodes[0], output=self.nodes[0].getnewaddress(), transactions=[rawTxSigned['hex']])
for n in [0, 3]:
self.log.info(f"Test getrawtransaction {'with' if n == 0 else 'without'} -txindex")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, was working on the part 2 follow-up to this, will have a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on #22383 (review), this pull adds missing
src/node/transaction::GetTransaction()test coverage for combinations of-txindexandblockhashand does some refactoring of the test file.