Skip to content

test: Use MiniWallet in rpc_rawtransaction.py#25044

Merged
maflcko merged 2 commits intobitcoin:masterfrom
danielabrozzoni:test_rawtransaction_miniwallet
May 30, 2022
Merged

test: Use MiniWallet in rpc_rawtransaction.py#25044
maflcko merged 2 commits intobitcoin:masterfrom
danielabrozzoni:test_rawtransaction_miniwallet

Conversation

@danielabrozzoni
Copy link
Member

@danielabrozzoni danielabrozzoni commented Apr 30, 2022 •
edited
Loading

This PR allows rpc_rawtransaction.py to be run even without the Core wallet by using the MiniWallet instead, as proposed in #20078.
This test was previously run twice, once with --legacy-wallet and once with
--descriptors. Since this would have meant running the same test twice
if the wallet wasn't compiled, now we run it just once with the legacy
wallet.

@fanquake fanquake added the Tests label Apr 30, 2022
@fanquake
Copy link
Member

fanquake commented Apr 30, 2022

https://github.com/bitcoin/bitcoin/pull/25044/checks?check_run_id=6241399894:

85/242 - �[1mrpc_rawtransaction.py --legacy-wallet�[0m failed, Duration: 2 s

�[0m2022-04-30T17:43:46.674000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20220430_174259/rpc_rawtransaction_151
2022-04-30T17:43:47.170000Z TestFramework (INFO): Prepare some coins for multiple *rawtransaction commands
2022-04-30T17:43:48.329000Z TestFramework (INFO): Test getrawtransaction with -txindex
2022-04-30T17:43:48.333000Z TestFramework (INFO): Test getrawtransaction without -txindex
2022-04-30T17:43:48.337000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 133, in main
    self.run_test()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/rpc_rawtransaction.py", line 86, in run_test
    self.getrawtransaction_tests()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/rpc_rawtransaction.py", line 143, in getrawtransaction_tests
    tx = self.wallet.send_self_transfer(from_node=self.nodes[2])['txid']
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/wallet.py", line 171, in send_self_transfer
    tx = self.create_self_transfer(**kwargs)
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/wallet.py", line 264, in create_self_transfer
    assert_equal(mempool_valid, tx_info['allowed'])
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 51, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(True == False)
2022-04-30T17:43:48.389000Z TestFramework (INFO): Stopping nodes

@danielabrozzoni danielabrozzoni marked this pull request as draft April 30, 2022 20:09
@danielabrozzoni danielabrozzoni force-pushed the test_rawtransaction_miniwallet branch from 892f2b7 to cb1e092 Compare April 30, 2022 20:22
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 30, 2022 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)

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.

@danielabrozzoni danielabrozzoni force-pushed the test_rawtransaction_miniwallet branch from cb1e092 to f9d5b49 Compare April 30, 2022 21:32
@DrahtBot DrahtBot mentioned this pull request May 1, 2022
Merged
@danielabrozzoni danielabrozzoni marked this pull request as ready for review May 1, 2022 09:21
@danielabrozzoni
Copy link
Member Author

danielabrozzoni commented May 1, 2022 •
edited
Loading

Ops :(
Should be ok now, thanks!

@jonatack
Copy link
Member

jonatack commented May 3, 2022

Concept ACK, will review after our seminar tomorrow :)

vincenzopalazzo
vincenzopalazzo approved these changes May 3, 2022
View reviewed changes
Copy link

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f9d5b49

with a small comment!

ayush933
ayush933 approved these changes May 4, 2022
View reviewed changes
Copy link
Contributor

@ayush933 ayush933 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK f9d5b49.

maflcko
maflcko reviewed May 4, 2022
View reviewed changes
theStack
theStack reviewed May 4, 2022
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK,

and warm welcome as a new contributor!

brunoerg
brunoerg reviewed May 4, 2022
View reviewed changes
@danielabrozzoni danielabrozzoni force-pushed the test_rawtransaction_miniwallet branch 2 times, most recently from 425d7c5 to 6672ceb Compare May 5, 2022 15:53
jonatack
jonatack reviewed May 17, 2022
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with reviewing the "convert tests to use MiniWallet" pulls, but poked around a bit and this LGTM.

ACK 6672cebfb10da112eaca29b4ff0a0a4438749624 this pull also speeds up the test from 17-20 to 12-13 seconds for me

A few minor comments follow, feel free to pick/choose/ignore, happy to re-ACK if you take any.

@danielabrozzoni danielabrozzoni force-pushed the test_rawtransaction_miniwallet branch from 6672ceb to 66dfa27 Compare May 17, 2022 17:35
maflcko
maflcko approved these changes May 18, 2022
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Left some ideas/questions.

@jonatack
Copy link
Member

jonatack commented May 20, 2022

ACK 66dfa27fe8d68a423ce7e64da05090f1b410fcdc

modulo addressing the feedback here or in a follow-up

@danielabrozzoni danielabrozzoni force-pushed the test_rawtransaction_miniwallet branch 2 times, most recently from 3fb42c3 to ddf22ff Compare May 27, 2022 09:35
maflcko
maflcko reviewed May 27, 2022
View reviewed changes
@danielabrozzoni danielabrozzoni force-pushed the test_rawtransaction_miniwallet branch from ddf22ff to 5a6cd72 Compare May 27, 2022 12:50
kouloumos
kouloumos reviewed May 28, 2022
View reviewed changes
Copy link
Contributor

@kouloumos kouloumos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5a6cd726cc56ca26b7af01322d1a344df86cebf6
Tested on macOS 10.15.7 with --disable-wallet, --without-bdb and --enable-wallet and tests run/skip as expected.

I am not yet familiar with test nodes overhead, but based on my test runs, there is no apparent reason for 4 nodes and this can run a bit faster with 3 nodes (minor changes needed on L58,L62,L101,L139).

@danielabrozzoni danielabrozzoni force-pushed the test_rawtransaction_miniwallet branch from 5a6cd72 to 813b643 Compare May 30, 2022 09:52
@danielabrozzoni
Copy link
Member Author

danielabrozzoni commented May 30, 2022

Thanks for your review @kouloumos! You're right, the 4th node is useless - I updated the code to use only 3 nodes, (and also removed the useless sync_all), and it runs slightly faster.

Before:
test/functional/rpc_rawtransaction.py 2.12s user 0.64s system 27% cpu 10.121 total
After:
test/functional/rpc_rawtransaction.py 1.58s user 0.46s system 26% cpu 7.798 total

jonatack
jonatack reviewed May 30, 2022
View reviewed changes
@jonatack
Copy link
Member

jonatack commented May 30, 2022

ACK 813b643e36bd3bf2a21156ed83a30b33d7703167 per git diff 66dfa27 813b643 modulo a comment or two

@danielabrozzoni
MOVEONLY: Move signrawtransactionwithwallet test
e93046c 
Put signrawtransactionwithwallet_tests in rpc_signrawtransaction.py,
as the test is mainly testing the signrawtransaction RPC.
Review with `git show --color-moved=dimmed-zebra`
@danielabrozzoni
test: Use MiniWallet in rpc_rawtransaction.py
e895900 
This test was previously run twice, once with `--legacy-wallet` and once with
`--descriptors`.
Now we run it only with `--legacy-wallet`, as all the tests has been
ported to the MiniWallet but `raw_multisig_transaction_legacy_tests`,
which can be run only with the legacy wallet.
We also decrease the number of nodes used from 4 to 3, making the test
run slightly faster.
@danielabrozzoni danielabrozzoni force-pushed the test_rawtransaction_miniwallet branch from 813b643 to e895900 Compare May 30, 2022 14:26
@jonatack
Copy link
Member

jonatack commented May 30, 2022

ACK e895900

@maflcko maflcko merged commit 269fa66 into bitcoin:master May 30, 2022
@maflcko
Copy link
Member

maflcko commented May 30, 2022 •
edited
Loading

Some further ideas on this test: It looks like it can be speed up by removing sync_all calls, as it is not needed to sync txs when a block is mined on the node that already has the tx. Also, some unused symbols can be removed. Maybe the test should be split up to avoid the re-use of symbols?

diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index fecb8310b9..31a5002528 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -366,5 +366,4 @@ class RawTransactionsTest(BitcoinTestFramework):
         # send 1.2 BTC to msig adr
         txId = self.nodes[0].sendtoaddress(mSigObj, 1.2)
-        self.sync_all()
         self.generate(self.nodes[0], 1)
         # node2 has both keys of the 2of2 ms addr, tx should affect the balance
@@ -387,5 +386,4 @@ class RawTransactionsTest(BitcoinTestFramework):
         decTx = self.nodes[0].gettransaction(txId)
         rawTx = self.nodes[0].decoderawtransaction(decTx['hex'])
-        self.sync_all()
         self.generate(self.nodes[0], 1)
 
@@ -407,7 +405,5 @@ class RawTransactionsTest(BitcoinTestFramework):
         rawTxSigned = self.nodes[2].signrawtransactionwithwallet(rawTx, inputs)
         assert_equal(rawTxSigned['complete'], True)  # node2 can sign the tx compl., own two of three keys
-        self.nodes[2].sendrawtransaction(rawTxSigned['hex'])
-        rawTx = self.nodes[0].decoderawtransaction(rawTxSigned['hex'])
-        self.sync_all()
+        self.nodes[0].sendrawtransaction(rawTxSigned['hex'])
         self.generate(self.nodes[0], 1)
         assert_equal(self.nodes[0].getbalance(), bal + Decimal('50.00000000') + Decimal('2.19000000'))  # block reward + tx
@@ -426,7 +422,4 @@ class RawTransactionsTest(BitcoinTestFramework):
 
         txId = self.nodes[0].sendtoaddress(mSigObj, 2.2)
-        decTx = self.nodes[0].gettransaction(txId)
-        rawTx2 = self.nodes[0].decoderawtransaction(decTx['hex'])
-        self.sync_all()
         self.generate(self.nodes[0], 1)
 
@@ -450,7 +443,5 @@ class RawTransactionsTest(BitcoinTestFramework):
         rawTxComb = self.nodes[2].combinerawtransaction([rawTxPartialSigned1['hex'], rawTxPartialSigned2['hex']])
         self.log.debug(rawTxComb)
-        self.nodes[2].sendrawtransaction(rawTxComb)
-        rawTx2 = self.nodes[0].decoderawtransaction(rawTxComb)
-        self.sync_all()
+        self.nodes[0].sendrawtransaction(rawTxComb)
         self.generate(self.nodes[0], 1)
         assert_equal(self.nodes[0].getbalance(), bal + Decimal('50.00000000') + Decimal('2.19000000'))  # block reward + tx

EDIT: Leave a comment here in this thread, if you started working on the previous comment, to avoid duplicate work.

@danielabrozzoni danielabrozzoni deleted the test_rawtransaction_miniwallet branch May 30, 2022 15:34
@jonatack
Copy link
Member

jonatack commented May 30, 2022

There is the Part 2 of #22437 (initially #24113 before it was chopped to bugfix only) that I was planning to repropose after updating for the changes here. Won't do it right away so it can be used if anyone wants to: https://github.com/jonatack/bitcoin/commits/improve-getrawtransaction-tests-part-2

@bitcoin bitcoin locked and limited conversation to collaborators Jan 24, 2023
@bitcoin bitcoin deleted a comment Jan 24, 2023
@bitcoin bitcoin deleted a comment from Thankgod01 Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

7 more reviewers

@theStack theStack theStack left review comments

@jonatack jonatack jonatack left review comments

@maflcko maflcko maflcko approved these changes

@kouloumos kouloumos kouloumos left review comments

@brunoerg brunoerg brunoerg left review comments

@vincenzopalazzo vincenzopalazzo vincenzopalazzo approved these changes

@ayush933 ayush933 ayush933 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@danielabrozzoni @fanquake @DrahtBot @jonatack @maflcko @theStack @vincenzopalazzo @kouloumos @brunoerg @ayush933