[tests] Add test for wallet rebroadcasts #15646

jnewbery · 2019-03-22T19:15:09Z

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.

DrahtBot · 2019-03-22T20:15:25Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15632 (Remove ResendWalletTransactions from the Validation Interface by jnewbery)

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

Coverage	Change (pull 15646, 9e11c99e56402afa5e9a323307491132bbf87dde)	Reference (master, `6852059`)
Lines	+0.0063 %	87.4567 %
Functions	-0.0145 %	84.7870 %
Branches	+0.0052 %	51.5361 %

^{Updated at: 2019-03-22T20:15:25.016978.}

jonatack · 2019-03-23T11:45:17Z

I'm seeing this locally on x86_64 Linux Debian 4.19.16-1 (2019-01-17):

$ test/functional/test_runner.py wallet_resendwallettransactions.py
Temporary test directory at /tmp/test_runner_₿_🏃_20190323_124023
Remaining jobs: [wallet_resendwallettransactions.py]
..........................................................................................................                                                                                                          
1/1 - wallet_resendwallettransactions.py failed, Duration: 69 s

stdout:
2019-03-23T11:40:23.995000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190323_124023/wallet_resendwallettransactions_0
2019-03-23T11:41:32.807000Z TestFramework.utils (ERROR): wait_until() failed.
  Predicate: (['wait_until(lambda: self.nodes[0].p2ps[1].tx_invs_received[txid] >= 1)\n'], 67)
2019-03-23T11:41:32.808000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 175, in main
    self.run_test()
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/wallet_resendwallettransactions.py", line 67, in run_test
    wait_until(lambda: self.nodes[0].p2ps[1].tx_invs_received[txid] >= 1)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 224, in wait_until
    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate (['wait_until(lambda: self.nodes[0].p2ps[1].tx_invs_received[txid] >= 1)\n'], 67) not true after 60 seconds

test/functional/wallet_resendwallettransactions.py

jonatack · 2019-03-23T13:53:03Z

If helpful, this version of the test passes for me locally: jonatack@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:

    # Transaction should be broadcast after 30 minutes. Give it ~60 (31 + 29) to be sure.
    node.setmocktime(time_now + 60 * 29)

promag

IIUC doing the following forces CWallet::ResendWalletTransactions at the begin of the test, which sets the correct value for nNextResend.

--- a/test/functional/wallet_resendwallettransactions.py
+++ b/test/functional/wallet_resendwallettransactions.py
@@ -37,6 +37,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
         # Create a new transaction. Set time to 31 minutes in the past.
         time_now = int(time.time())
         self.nodes[0].setmocktime(time_now - 60 * 31)
+        txid = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
         txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)

         # Can take a few seconds due to transaction trickling

jnewbery · 2019-03-26T15:20:09Z

Thanks for the review @jonatack and @promag . I've pushed a fixup commit that:

fixes a bug in the way we were storing txids in 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.
changes the mocktimes slightly
adds logging
a few other small changes.

It should pass reliably now. Are you able to rereview/retest?

practicalswift · 2019-03-26T15:58:23Z

Concept ACK

maflcko · 2019-03-26T18:05:02Z

@practicalswift No need to "Concept ACK" someone adding tests. This is already encouraged by the contribution guidelines and adding a comment adds no value. It would help if you ran the test or gave feedback about the code, however.

maflcko · 2019-03-26T18:12:18Z

Should be squashed to make ready for review. Also, would be good to explain why resendwalletransactions is no longer (smoke) tested. E.g: "It is called in other tests already"

ryanofsky

utACK b5eb0ea42fe32426dd22b77d4146df3a56e2f006

test/functional/wallet_resendwallettransactions.py

jnewbery · 2019-03-26T19:31:55Z

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 (#5940), which it doesn't do effectively. Removing it would allow further simplification of the rebroadcast code.

jonatack

Thanks @jnewbery. The test now passes repeatedly for me on Linux Debian 4.19 x86/64 👍

jonatack · 2019-03-26T21:07:28Z

test/functional/wallet_resendwallettransactions.py

Was using the same value for node.setmocktime and the block time in create_block a key fix? Good catch.

Yes, sometimes the block was being rejected because of the bad timestamp. The node logs were pretty helpful in identifying the problem.

jonatack · 2019-03-26T21:17:09Z

test/functional/wallet_resendwallettransactions.py

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.

node = self.nodes[0] # alias block_time = int(time.time()) + 6 * 60 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.

jonatack · 2019-03-26T21:20:19Z

test/functional/wallet_resendwallettransactions.py

The logs are a great enhancement:

$ test/functional/wallet_resendwallettransactions.py 2019-03-26T21:05:37.079000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_eb5_rvg_ 2019-03-26T21:05:37.874000Z TestFramework (INFO): Create a new transaction and wait until it's broadcast. 2019-03-26T21:05:38.807000Z TestFramework (INFO): Create a block. 2019-03-26T21:05:38.864000Z TestFramework (INFO): Transaction should be rebroadcast after 30 minutes. 2019-03-26T21:05:40.021000Z TestFramework (INFO): Stopping nodes 2019-03-26T21:05:40.329000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_eb5_rvg_ on exit 2019-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.

maflcko · 2019-03-27T13:45:35Z

utACK c1a96f0f339987d48cdaba75e628030cbf6391b8

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.

jnewbery · 2019-03-27T15:24:08Z

Thanks for the review/test @jonatack . I've changed the log messages, but kept the time variables as they were.

maflcko · 2019-03-27T18:31:04Z

re-utACK 529c1ae

Only change is punctuation

529c1ae [tests] Add test for wallet rebroadcasts (John Newbery) Pull request description: 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. ACKs for commit 529c1a: MarcoFalke: re-utACK 529c1ae Tree-SHA512: 7341e7dd07cdc8ecbc08b1949121824148d2b58133a8e298ecdc5b7555713df3cecffb49854443cef9f033ef847cbf329e879a3bf57ab4e1fc733be432e9f718

maflcko · 2019-04-01T19:47:33Z

This will be backported to 0.18.0rc3

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. Github-Pull: bitcoin#15646 Rebased-From: 529c1ae

laanwj · 2019-04-02T12:01:20Z

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.

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. Github-Pull: bitcoin#15646 Rebased-From: 529c1ae

Summary: 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. --- This is a backport of Core [[bitcoin/bitcoin#15646 | PR15646]] Test Plan: cmake .. -GNinja ./test/functional/test_runner.py wallet_resendwallettransactions.py Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D5947

529c1ae [tests] Add test for wallet rebroadcasts (John Newbery) Pull request description: 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. ACKs for commit 529c1a: MarcoFalke: re-utACK 529c1ae Tree-SHA512: 7341e7dd07cdc8ecbc08b1949121824148d2b58133a8e298ecdc5b7555713df3cecffb49854443cef9f033ef847cbf329e879a3bf57ab4e1fc733be432e9f718

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

jnewbery mentioned this pull request Mar 22, 2019

Remove ResendWalletTransactions from the Validation Interface #15632

Merged

fanquake added the Tests label Mar 22, 2019

jonatack reviewed Mar 23, 2019

View reviewed changes

test/functional/wallet_resendwallettransactions.py Outdated Show resolved Hide resolved

promag reviewed Mar 24, 2019

View reviewed changes

ryanofsky approved these changes Mar 26, 2019

View reviewed changes

test/functional/wallet_resendwallettransactions.py Show resolved Hide resolved

jnewbery force-pushed the 2019_03_wallet_rebroadcasat_test branch 2 times, most recently from 6fc3c40 to c1a96f0 Compare March 26, 2019 19:28

jonatack reviewed Mar 26, 2019

View reviewed changes

jnewbery force-pushed the 2019_03_wallet_rebroadcasat_test branch from c1a96f0 to 529c1ae Compare March 27, 2019 15:19

maflcko merged commit 529c1ae into bitcoin:master Mar 27, 2019

fanquake added the Needs backport label Apr 1, 2019

maflcko removed the Needs backport label Apr 8, 2019

luke-jr mentioned this pull request Mar 4, 2020

test: Add missing syncwithvalidationinterfacequeue #18228

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021

merge bitcoin#15646: Add test for wallet rebroadcasts

11fc0f5

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

[tests] Add test for wallet rebroadcasts #15646

[tests] Add test for wallet rebroadcasts #15646

Uh oh!

Conversation

jnewbery commented Mar 22, 2019

Uh oh!

DrahtBot commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Coverage

Uh oh!

jonatack commented Mar 23, 2019

Uh oh!

Uh oh!

jonatack commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 26, 2019

Uh oh!

practicalswift commented Mar 26, 2019

Uh oh!

maflcko commented Mar 26, 2019

Uh oh!

maflcko commented Mar 26, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Mar 26, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

jonatack Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

jonatack Mar 26, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 27, 2019

Uh oh!

jnewbery commented Mar 27, 2019

Uh oh!

maflcko commented Mar 27, 2019

Uh oh!

maflcko commented Apr 1, 2019

Uh oh!

laanwj commented Apr 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

DrahtBot commented Mar 22, 2019 •

edited

Loading

jonatack commented Mar 23, 2019 •

edited

Loading

jonatack Mar 26, 2019 •

edited

Loading

jonatack Mar 26, 2019 •

edited

Loading