Intermittent failure of `import-rescan.py`

laanwj commented at 3:28 pm on February 22, 2017: member

On master, the import-rescan.py test fails intermittently. See for example: https://travis-ci.org/bitcoin/bitcoin/jobs/203878010

 0import-rescan.py:
 1Initializing test directory /tmp/testzpotg5c7/46
 2start_node: bitcoind started, waiting for RPC to come up
 3start_node: RPC successfully started
 4start_node: bitcoind started, waiting for RPC to come up
 5start_node: RPC successfully started
 6start_node: bitcoind started, waiting for RPC to come up
 7start_node: RPC successfully started
 8start_node: bitcoind started, waiting for RPC to come up
 9start_node: RPC successfully started
10start_node: bitcoind started, waiting for RPC to come up
11start_node: RPC successfully started
12Assertion failed: not(False == True)
13Stopping nodes
14Not cleaning up dir /tmp/testzpotg5c7/46
15...
16 Shutdown: done
17Failed
18stderr:
19  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/test_framework/test_framework.py", line 145, in main
20    self.run_test()
21  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/import-rescan.py", line 172, in run_test
22    variant.check(variant.sent_txid, variant.sent_amount, 1)
23  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/import-rescan.py", line 87, in check
24    assert_equal("involvesWatchonly" not in tx, True)
25  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/qa/rpc-tests/test_framework/util.py", line 529, in assert_equal
26    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
27Pass: False, Duration: 9 s

laanwj added the label Tests on Feb 22, 2017

ryanofsky commented at 4:54 pm on February 22, 2017: member

Was able to reproduce this locally (after ~150 times running in a loop). Will debug.

ryanofsky commented at 11:52 pm on February 22, 2017: member

I think I see the cause of the error. “involvesWatchonly” can be true either if source or destination address of the transaction is watch only, but the test doesn’t know anything about the source address of the transaction. The test is trying to assert that the destination is not watchonly when money is being sent to an address with an imported private key. But once in a while due to coin selection the source address will be watchonly and cause the check to fail. I can modify the test to only send from non-watchonly sources by adding a new node to the test and sending money from that node. Using sendfrom("") RPC would be another alternative since all the watchonly addresses all have non-empty labels. However the sendfrom RPC is deprecated and also appears to be broken (https://botbot.me/freenode/bitcoin-core-dev/msg/81405475/)

laanwj commented at 9:03 am on February 23, 2017: member

@ryanofsky Thanks for investigating. That makes a lot of sense.

Using sendfrom("") RPC would be another alternative since all the watchonly addresses all have non-empty labels.

That’s a common misunderstanding: sendfrom doesn’t, and never had any influence on coin selection. It just changes what internal account the send is charged to.

So yes I guess the solution would be to send from another node. Or change the sequence of the test.

ryanofsky referenced this in commit ccbc57ad7d on Feb 23, 2017

ryanofsky commented at 7:05 pm on February 23, 2017: member

Implemented rescan test fix in #9839 and sendfrom help update in #9840.

It does seem like it would be easy to add a feature to sendtoaddress/fundrawtransaction that would allow labels to be used for coin selection. This would have been nice here, and it seems the type of feature that could be touted as an advantage of the label concept over the account concept after #7729, but I don’t know if it actually would have practical uses outside of qa testing.

ryanofsky referenced this in commit 864890adf5 on Feb 23, 2017

laanwj commented at 9:20 am on February 24, 2017: member

That would be a nice feature to have, I guess, some time after account functionality has been removed and replaced with label API. When it’s added with account functionality still in place it’s going to magnify confusion even more, as people would assume account balance = amount of coins on labeled UTXOs.

sipa commented at 9:31 am on February 24, 2017: member

I think that multiwallet is a better abstraction for keeping coins separated.

If you had a send... RPC that restricted coin selection to labelled coins, but don’t prevent other RPCs that don’t have this restriction, what do you do with change? Say you have a coin C1 (0.3 BTC) with label L1, and C2 (0.2 BTC) with label L2. Someone issues a normal sendtoaddress, which spends C1 and C2, and creates C3 (0.4 BTC) to the payee, and C4 (0.1 BTC) as change back to you. What label does C4 get? 60% L1 and 40% L2?

To make this work, you need to prevent any spends that cross multiple labels, and that point you’ve just recreated multiwallet in a less understandable way.

laanwj commented at 9:46 am on February 24, 2017: member

@sipa Agreed on that, multiwallet is a better abstraction for that.

ryanofsky commented at 10:43 am on February 24, 2017: member

Also agree. With multiwallet, this whole test could run one node instead of several.

laanwj closed this on Feb 27, 2017

laanwj referenced this in commit eddaa6b35d on Feb 27, 2017

lateminer referenced this in commit 197f2e1f19 on Jan 5, 2019

DrahtBot locked this on Sep 8, 2021

Intermittent failure of import-rescan.py #9826

Intermittent failure of `import-rescan.py` #9826