qa: Add further tests to wallet

MarcoFalke commented at 7:06 pm on April 5, 2019: member

Second commit can be reviewed with --ignore-all-space

DrahtBot added the label Tests on Apr 5, 2019

MarcoFalke force-pushed on Apr 5, 2019

in test/functional/wallet_balance.py:78 in faa26db8b3 outdated

64@@ -64,8 +65,10 @@ def run_test(self):
65         self.log.info("Test getbalance with different arguments")
66         assert_equal(self.nodes[0].getbalance("*"), 50)
67         assert_equal(self.nodes[0].getbalance("*", 1), 50)
68-        assert_equal(self.nodes[0].getbalance("*", 1, True), 50)
69+        assert_equal(self.nodes[0].getbalance("*", 1, True), 100)
70         assert_equal(self.nodes[0].getbalance(minconf=1), 50)
71+        assert_equal(self.nodes[0].getbalance(minconf=0, include_watchonly=True), 100)

promag commented at 4:19 pm on April 7, 2019:

Just include_watchonly=True, if #15729 is the way to go? Same below.

MarcoFalke commented at 2:45 pm on April 8, 2019:

The two pulls will conflict either way, so I’d rather keep it

DrahtBot commented at 2:02 pm on April 8, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15729 (rpc: Raise error in getbalance if minconf is not zero by promag)

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.

MarcoFalke added the label Wallet on Apr 10, 2019

laanwj added this to the "Blockers" column in a project

in test/functional/wallet_balance.py:67 in fa8d8eb2bd outdated

62@@ -55,7 +63,7 @@ def run_test(self):
63         self.nodes[0].generate(1)
64         self.sync_all()
65         self.nodes[1].generate(1)
66-        self.nodes[1].generatetoaddress(100, RANDOM_COINBASE_ADDRESS)
67+        self.nodes[1].generatetoaddress(101, ADDRESS_WATCHONLY)

jnewbery commented at 3:07 pm on April 30, 2019:

The log above “Mining one block for each node” is now incorrect (or at least misleading). Can you change it?

MarcoFalke commented at 3:57 pm on April 30, 2019:

Huh, there are two calls to generate(1) for each node? How is it incorrect?

jnewbery commented at 4:17 pm on April 30, 2019:

Because there is then a call to generate to a watch-only address for node 1

in test/functional/wallet_balance.py:161 in fa8d8eb2bd outdated

156+        # mempool because it is the third descendant of the tx above
157+        for _ in range(3):
158+            # Set amount high enough such that all coins are spent by each tx
159+            txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 99)
160+        assert txid not in self.nodes[0].getrawmempool()
161+        assert_equal(self.nodes[0].getbalance(minconf=0), 0)  # wallet txs not in the mempool are untrusted

jnewbery commented at 3:22 pm on April 30, 2019:

A few additional tests you could do here:

check that gettransaction(txid) returns something but getrawtransaction(txid) doesn’t.
check that getrawtransaction() returns something for the second transaction that was sent (to sanity-check your testing methodology)
check that gettransaction(txid)['trusted'] == False

MarcoFalke commented at 4:03 pm on April 30, 2019:

Good suggestions

MarcoFalke commented at 3:25 pm on April 30, 2019: member

Unless there are objections, this will be merged tomorrow

in test/functional/wallet_balance.py:171 in fa8d8eb2bd outdated

162+
163+        self.log.info("Test replacement and reorg of non-mempool tx")
164+        tx_orig = self.nodes[0].gettransaction(txid)['hex']
165+        # Increase fee by 1 coin
166+        tx_replace = tx_orig.replace(
167+            struct.pack("<q", 99 * 10**8).hex(),

jnewbery commented at 3:31 pm on April 30, 2019:

😨 is it possible to do this by pre-constructing multiple versions of the same transaction with different fees? My preference would be to avoid doing raw construction byte wrangling if possible.

MarcoFalke commented at 4:03 pm on April 30, 2019:

Maybe I should use bumpfee?

jnewbery commented at 4:20 pm on April 30, 2019:

Perhaps, or the same createrawtransaction/signrawtransaction method used in create_transactions() above.

Just a suggestion. If this adds a ton of additional lines, then it’s not worth it. I just find that anything that directly builds/edits transactions can become quite cumbersome to update if the test needs to be changed in future.

MarcoFalke commented at 7:13 pm on April 30, 2019:

I couldn’t get create_transactions to work for this use case after minutes of trying, so I gave up and kept my two-line hack. I think I found two issues with create_transactions in the process, so I fixed them up.

Feel free to try it yourself and send me the commit if you succeed …

jnewbery commented at 8:12 pm on April 30, 2019:

No, I agree with you that it’s not worth the additional effort. Thanks for trying though!

in test/functional/wallet_balance.py:181 in fa8d8eb2bd outdated

176+        # Now confirm tx_replace
177+        block_reorg = self.nodes[1].generatetoaddress(1, ADDRESS_WATCHONLY)[0]
178+        self.sync_all()
179+        assert_equal(self.nodes[0].getbalance(minconf=0), total_amount)
180+
181+        # Put txs back in mempool

jnewbery commented at 3:33 pm on April 30, 2019:

Presumably the txs don’t get back into node 0’s mempool because of the descendant limit, which is why the getbalance values are 0 below? If that’s true, can you update this comment to explain what’s going on?

MarcoFalke commented at 7:14 pm on April 30, 2019:

Added logprint with comment

jnewbery commented at 3:33 pm on April 30, 2019: member

looks good. A few comments inline.

test: Add wallet_balance test for watchonly fa464e8211

test: Add getunconfirmedbalance test with conflicts fa195315e6

test: Check that wallet txs not in the mempool are untrusted fad03cd046

test: Add reorg test to wallet_balance fa79a783d6

MarcoFalke force-pushed on Apr 30, 2019

jnewbery commented at 8:14 pm on April 30, 2019: member

utACK fa79a783d63060dc6a8521c1de58b158979a59e9

MarcoFalke referenced this in commit 2eb8c5d7a2 on May 1, 2019

MarcoFalke merged this on May 1, 2019

MarcoFalke closed this on May 1, 2019

MarcoFalke deleted the branch on May 1, 2019

MarcoFalke removed this from the "Blockers" column in a project

luke-jr referenced this in commit beb282b205 on Aug 23, 2019

luke-jr referenced this in commit 40e809710a on Aug 23, 2019

deadalnix referenced this in commit 6d30026d43 on Jun 9, 2020

deadalnix referenced this in commit 7ac7090ca0 on Jun 9, 2020

deadalnix referenced this in commit 2982610b69 on Jun 9, 2020

deadalnix referenced this in commit 906a4e6c15 on Jun 9, 2020

DrahtBot locked this on Dec 16, 2021

qa: Add further tests to wallet_balance #15758

Conflicts