Fix intermittent failure in wallet_send.py and rpc_fundrawtransaction.py #23200

pull meshcollider wants to merge 2 commits into bitcoin:master from meshcollider:202110_fix_external_tests changing 5 files +8 −5

meshcollider commented at 10:48 PM on October 5, 2021: contributor

After #17211 was merged, there have been a few intermittent test failures in wallet_send.py, rpc_fundrawtransaction.py, and rpc_psbt.py

I believe all these failures are due to these missing sync_all()s
meshcollider added the label Tests on Oct 5, 2021

in test/functional/rpc_fundrawtransaction.py:1013 in b0b935122c outdated

1010 | @@ -1011,6 +1011,7 @@ def test_external_inputs(self):
1011 |          self.nodes[0].sendtoaddress(addr, 10)
1012 |          self.nodes[0].sendtoaddress(wallet.getnewaddress(), 10)
1013 |          self.nodes[0].generate(6)

jonatack commented at 10:58 PM on October 5, 2021:

ISTM both of these lines should be this instead (see #22788 and #22567):

        self.generate(self.nodes[0], 6)

jonatack commented at 11:10 PM on October 5, 2021:

$ git grep "].generate"
test/functional/interface_zmq.py:586:        self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)
test/functional/p2p_compactblocks_blocksonly.py:36:        blockhash = self.nodes[2].generate(1)[0]
test/functional/rpc_fundrawtransaction.py:1013:        self.nodes[0].generate(6)
test/functional/rpc_psbt.py:630:        self.nodes[0].generate(6)
test/functional/wallet_send.py:505:        self.nodes[0].generate(6)

achow101 commented at 11:03 PM on October 5, 2021: member

rpc_psbt.py has a similar potential issue.
meshcollider commented at 11:08 PM on October 5, 2021: contributor

Added rpc_psbt.py and addressed @jonatack's suggestion
achow101 commented at 11:18 PM on October 5, 2021: member

ACK 6b1b7157c15620f560ac237cbef08cdb13fc346e

The lack of sync_all has bitten us in the past in similar situations, so it's likely to be the problem here.
Use self.generate not node.generate throughout tests eb02dbba3c
Fix intermittent test failures due to missing sync_all 75a9305d45
meshcollider commented at 11:20 PM on October 5, 2021: contributor

Sorry to invalidate your ACK immediately @achow101, just included the two other instances of node.generate that @jonatack pointed out so they're all gone.
meshcollider requested review from instagibbs on Oct 5, 2021
achow101 commented at 11:25 PM on October 5, 2021: member

ACK 75a9305d455e234c6b63635d80b0f2aef902342e
instagibbs commented at 11:37 PM on October 5, 2021: member

the idea being block processing hasn't finished before the wallet calls are used, even though it's on the same node?
meshcollider commented at 11:52 PM on October 5, 2021: contributor

the idea being block processing hasn't finished before the wallet calls are used, even though it's on the same node?

It is for the benefit of the other node, using the input externally. For example, in rpc_fundrawtransaction.py, the node called wallet is the one calling fundrawtransaction, after nodes[0] generates the transaction and blocks. Similarly, in wallet_send.py, ext_wallet needs to be synced after nodes[0] generates the transaction and blocks.
in test/functional/rpc_fundrawtransaction.py:1014 in 75a9305d45
```
1009 | @@ -1010,7 +1010,8 @@ def test_external_inputs(self):
1010 |  
1011 |          self.nodes[0].sendtoaddress(addr, 10)
1012 |          self.nodes[0].sendtoaddress(wallet.getnewaddress(), 10)
1013 | -        self.nodes[0].generate(6)
1014 | +        self.generate(self.nodes[0], 6)
1015 | +        self.sync_all()
```
instagibbs commented at 12:40 AM on October 6, 2021:

slight preference for syncing right before the other wallet call rather than sandwiched between nodes[0] calls. This makes spotting gaps easier in the future.

meshcollider commented at 1:34 AM on October 6, 2021:

@instagibbs #22567 will merge these sync_all into the generate calls, so it makes sense to put them together for clarity at the moment IMO

lsilva01 commented at 2:00 AM on October 6, 2021:

@instagibbs PR #22741 has more information about the generate* calls
instagibbs commented at 1:49 AM on October 6, 2021: member
Got it.

On Wed, Oct 6, 2021, 9:34 AM Samuel Dobson @.***> wrote:
@.**** commented on this pull request.

In test/functional/rpc_fundrawtransaction.py https://github.com/bitcoin/bitcoin/pull/23200#discussion_r722821159:

@@ -1010,7 +1010,8 @@ def test_external_inputs(self):

self.nodes[0].sendtoaddress(addr, 10) self.nodes[0].sendtoaddress(wallet.getnewaddress(), 10)

self.nodes[0].generate(6)

self.generate(self.nodes[0], 6)

self.sync_all()

@instagibbs https://github.com/instagibbs #22567 https://github.com/bitcoin/bitcoin/pull/22567 will merge these sync_all into the generate calls, so it makes sense to put them together for clarity at the moment IMO

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/23200#discussion_r722821159, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFUZQBXYMSLETRSZI6RDUFORS3ANCNFSM5FM4JY4Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
lsilva01 approved
lsilva01 commented at 1:53 AM on October 6, 2021: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/23200/commits/75a9305d455e234c6b63635d80b0f2aef902342e on Ubuntu 20.04

sync_all() ensures that blocks and mempool will be synchronized between nodes. Tests were run several times without errors.
MarcoFalke merged this on Oct 6, 2021
MarcoFalke closed this on Oct 6, 2021
DrahtBot commented at 11:27 AM on October 6, 2021: 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:
- #23202 (wallet: allow psbtbumpfee to work with txs with external inputs by achow101)
- #23201 (wallet: Allow users to specify input weights when funding a transaction by achow101)
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.
meshcollider deleted the branch on Oct 6, 2021
sidhujag referenced this in commit 98fe03bf77 on Oct 6, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#17211 Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs

Fix intermittent failure in wallet_send.py and rpc_fundrawtransaction.py #23200

@.**** commented on this pull request.

Conflicts