The existing wallet_resendwallettransactions.py test only tests the resendwallettransactions RPC. It does not test whether transactions are actually rebroadcast, or whether the rebroadcast logic is called on a timer.
Update the test to not use the resendwallettransactions RPC and test that transactions are resent on a timer.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
| Coverage | Change (pull 15646, 9e11c99e56402afa5e9a323307491132bbf87dde) | Reference (master, 68520597ccf8ff3f6e8a7ad6869b06bf2012ae8a) |
|---|---|---|
| Lines | +0.0063 % | 87.4567 % |
| Functions | -0.0145 % | 84.7870 % |
| Branches | +0.0052 % | 51.5361 % |
Updated at: 2019-03-22T20:15:25.016978.
I’m seeing this locally on x86_64 Linux Debian 4.19.16-1 (2019-01-17):
0$ test/functional/test_runner.py wallet_resendwallettransactions.py
1Temporary test directory at /tmp/test_runner_₿_🏃_20190323_124023
2Remaining jobs: [wallet_resendwallettransactions.py]
3..........................................................................................................
41/1 - wallet_resendwallettransactions.py failed, Duration: 69 s
5
6stdout:
72019-03-23T11:40:23.995000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190323_124023/wallet_resendwallettransactions_0
82019-03-23T11:41:32.807000Z TestFramework.utils (ERROR): wait_until() failed.
9 Predicate: (['wait_until(lambda: self.nodes[0].p2ps[1].tx_invs_received[txid] >= 1)\n'], 67)
102019-03-23T11:41:32.808000Z TestFramework (ERROR): Assertion failed
11Traceback (most recent call last):
12 File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 175, in main
13 self.run_test()
14 File "/home/jon/projects/bitcoin/bitcoin/test/functional/wallet_resendwallettransactions.py", line 67, in run_test
15 wait_until(lambda: self.nodes[0].p2ps[1].tx_invs_received[txid] >= 1)
16 File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 224, in wait_until
17 raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
18AssertionError: Predicate (['wait_until(lambda: self.nodes[0].p2ps[1].tx_invs_received[txid] >= 1)\n'], 67) not true after 60 seconds
35 self.skip_if_no_wallet()
36
37 def run_test(self):
38- # Should raise RPC_WALLET_ERROR (-4) if walletbroadcast is disabled.
39- assert_raises_rpc_error(-4, "Error: Wallet transaction broadcasting is disabled with -walletbroadcast", self.nodes[0].resendwallettransactions)
40+ self.nodes[0].add_p2p_connection(P2PStoreTxInvs())
self.nodes[0] could be assigned at the start of the test to a local variable like node for readability, since it is called 14 times in run_test.
self.nodes[0] 14 times or move the new test to a function with the node arg.
node as a local alias for self.nodes[0] in tests where there’s only one node is a common technique in our tests. I’ll add this.
If helpful, this version of the test passes for me locally: https://github.com/jonatack/bitcoin/commit/0608a16. Main change is the last setmocktime value. I’m unsure if more time invalidates the test but it appears to become unreliable with lower values:
0 # Transaction should be broadcast after 30 minutes. Give it ~60 (31 + 29) to be sure.
1 node.setmocktime(time_now + 60 * 29)
IIUC doing the following forces CWallet::ResendWalletTransactions at the begin of the test, which sets the correct value for nNextResend.
0--- a/test/functional/wallet_resendwallettransactions.py
1+++ b/test/functional/wallet_resendwallettransactions.py
2@@ -37,6 +37,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
3 # Create a new transaction. Set time to 31 minutes in the past.
4 time_now = int(time.time())
5 self.nodes[0].setmocktime(time_now - 60 * 31)
6+ txid = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
7 txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
8
9 # Can take a few seconds due to transaction trickling
Thanks for the review @jonatack and @promag . I’ve pushed a fixup commit that:
P2PStoreTxInvs (it was previously storing as hex and would drop leading zeros, which meant it wouldn’t match with the txid in the case where the txid had a leading zero.It should pass reliably now. Are you able to rereview/retest?
resendwalletransactions is no longer (smoke) tested. E.g: “It is called in other tests already”
24+ for i in message.inv:
25+ if i.type == 1:
26+ # save as hex
27+ self.tx_invs_received[format(i.hash, 'x')] += 1
28
29 class ResendWalletTransactionsTest(BitcoinTestFramework):
In commit “[tests] Add test for wallet rebroadcasts” (f2faae27895927bb8fefca3d066bf8972e3bdaa7)
It would be nice to move some of the comments from your commit message into the test itself to convey what the test currently covers and how it can be improved in the future (“It does not test whether transactions are actually rebroadcast, or whether the rebroadcast logic is called on a timer”).
I think you’ve misunderstood the commit message
Oh, yes I did misunderstand. I thought it was saying the test didn’t check whether transactions were rebroadcast on an ongoing basis. Anyway, no need to change anything.
Commits squashed. I’ve also made a small update to the commit log to make it clearer that this commit fixes the deficiencies with the previous test.
would be good to explain why resendwalletransactions is no longer (smoke) tested
I intend to remove the resendwallettransactions RPC method in a follow-up PR. It was added as a hidden, test-only RPC to test wallet transaction rebroadcast (https://github.com/bitcoin/bitcoin/pull/5940), which it doesn’t do effectively. Removing it would allow further simplification of the rebroadcast code.
54+ # Create and submit a block without the transaction.
55+ # Transactions are only rebroadcast if there has been a block at least five minutes
56+ # after the last time we tried to broadcast. Use mocktime and give an extra minute to be sure.
57+ block_time = int(time.time()) + 6 * 60
58+ node.setmocktime(block_time)
59+ block = create_block(int(node.getbestblockhash(), 16), create_coinbase(node.getblockchaininfo()['blocks']), block_time)
node.setmocktime and the block time in create_block a key fix? Good catch.
35 self.skip_if_no_wallet()
36
37 def run_test(self):
38- # Should raise RPC_WALLET_ERROR (-4) if walletbroadcast is disabled.
39- assert_raises_rpc_error(-4, "Error: Wallet transaction broadcasting is disabled with -walletbroadcast", self.nodes[0].resendwallettransactions)
40+ node = self.nodes[0] # alias
Just an idea: It might be nice to hoist all the local variables for the test setup together at the top, or in a setup function, to see the setup as a whole. Doing this also made it more apparent that we can potentially reduce the number of system time calls.
0 node = self.nodes[0] # alias
1 block_time = int(time.time()) + 6 * 60
2 rebroadcast_time = block_time + 31 * 60
I tried this locally as above, simplified to one single call to int(time.time()) and 31 minutes for rebroadcast instead of 35, and the test appears equally robust this way since an extra minute is given for the block.
Edit: I initially thought that defining rebroadcast_time relative to block_time, rather than the current time, might be a more rigorous test of CWallet::ResendWalletTransactions in wallet.cpp. On re-reading it this morning, that was perhaps incorrect, though I am still looking at how all the state is handled. On my system it does appear we can tighten the extra 5 minutes down to 1 minute in the test.
It might be nice to hoist all the local variables for the test setup together at the top
Generally we declare variables at the point of first use (both in the C++ and Python code). You could argue that these are more like constants so declaring them at the top makes sense. I don’t have a strong feeling either way, so I’ll leave it as is.
39- assert_raises_rpc_error(-4, "Error: Wallet transaction broadcasting is disabled with -walletbroadcast", self.nodes[0].resendwallettransactions)
40+ node = self.nodes[0] # alias
41+
42+ node.add_p2p_connection(P2PStoreTxInvs())
43+
44+ self.log.info("Create a new transaction and wait until it's broadcast.")
The logs are a great enhancement:
0$ test/functional/wallet_resendwallettransactions.py
12019-03-26T21:05:37.079000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_eb5_rvg_
22019-03-26T21:05:37.874000Z TestFramework (INFO): Create a new transaction and wait until it's broadcast.
32019-03-26T21:05:38.807000Z TestFramework (INFO): Create a block.
42019-03-26T21:05:38.864000Z TestFramework (INFO): Transaction should be rebroadcast after 30 minutes.
52019-03-26T21:05:40.021000Z TestFramework (INFO): Stopping nodes
62019-03-26T21:05:40.329000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_eb5_rvg_ on exit
72019-03-26T21:05:40.329000Z TestFramework (INFO): Tests successful
Note that the auto logging does not appear to terminate with periods, if you wish to do the same.
The existing wallet_resendwallettransactions.py test only tests the
resendwallettransactions RPC. It does not test whether transactions are
actually rebroadcast, or whether the rebroadcast logic is called on a
timer.
This commit updates the test to not use the resendwallettransactions RPC and
test that transactions are rebroadcast on a timer.
re-utACK 529c1ae4a0
Only change is punctuation
I intend to remove the resendwallettransactions RPC method in a follow-up PR.
Concept ACK on this, I think it can be useful to send individual transactions, but this can be done by getting the transaction hex from getwallettransaction then sendrawtransaction, this RPC seems to be redundant.