test: Fix flaky wallet

fjahr commented at 6:05 pm on September 5, 2020: member

Fixes #19853

I investigated the issue in #19876 and I still intend to fix the underlying issue of a race when using wallet RPCs right after starting a node in that PR. However, since that is a bit more complicated than I initially thought it makes sense to merge the fix of the test so the intermittent test failures stop. This fix in the test is going to be needed, either way, #19876 will only provide an error where before it was reporting a false balance.

test: Fix flaky wallet_basic test

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

56b018ca7f

DrahtBot added the label Tests on Sep 5, 2020

promag commented at 7:52 pm on September 5, 2020: member

ACK

MarcoFalke commented at 5:46 am on September 6, 2020: member

Can you explain what is going on here?

Does the wallet submit txs to the mempool, which do not get marked as in the mempool? This seems like a bug, as the wallet should mark them as soon as they are submitted.
Or, it seems the issue is that the wallet waits for the mempool to be loaded, in which case the txs in the wallet are only marked as in-mempool when they went through the validationinterfacequeue? In that case it could make sense to fixup the comment wait until the wallet has submitted all transacti... to wait until the mempool is loaded. (I added the wrong comment, but this pull is somewhat related, so could fix up the comment in one go)

MarcoFalke commented at 7:32 am on September 6, 2020: member

Maybe it is a race where either the wallet or the (load)mempool may add the tx?

The syncwithvalidationinterfacequeue seems like a nice temporary workaround, but ideally the wallet would properly start up/block before responding to RPCs. Maybe the BlockUntilSynced... should also block until the wallet has imported all mempool txs on start-up/loadwallet?

fjahr commented at 10:56 am on September 6, 2020: member

@MarcoFalke Thanks for these hints! I am still working on fixing the actual issue in #19876. But since it’s taking a bit longer to investigate and it’s in dispute if this actually needs fixing, I decided to open this fix to the intermittent CI failures since they seem to occur pretty frequently. If the fix for the underlying issue is showing a better error this line will be needed anyway, if the fix makes the issue go away completely I will remove it again with #19876. So it may or may not be temporary, either way, I am just trying to mitigate the CI issue as fast as possible.

MarcoFalke commented at 11:02 am on September 6, 2020: member

Having a balance, then restarting bitcoin core and having a different balance would be a bug that needs proper fixing.

ACK either way on the temporary test workaround, but maybe it could say that it is only temporary?

MarcoFalke merged this on Sep 6, 2020

MarcoFalke closed this on Sep 6, 2020

promag commented at 3:04 pm on September 6, 2020: member

Having a balance, then restarting bitcoin core and having a different balance would be a bug

Is this what’s going on?

MarcoFalke commented at 3:15 pm on September 6, 2020: member

Yes, I believe so. At least the balance is less than it should be otherwise the amount wouldn’t be out of range (i.e. negative) later on, no?

promag commented at 3:51 pm on September 17, 2020: member

@MarcoFalke missed this comment. I’ll

PastaPastaPasta referenced this in commit 8de6694794 on Jun 27, 2021

PastaPastaPasta referenced this in commit 8dab9f488d on Jun 28, 2021

PastaPastaPasta referenced this in commit 201c606366 on Jun 29, 2021

PastaPastaPasta referenced this in commit b88d413046 on Jul 1, 2021

PastaPastaPasta referenced this in commit 843e1d573c on Jul 1, 2021

PastaPastaPasta referenced this in commit 7b796a1e1b on Jul 15, 2021

PastaPastaPasta referenced this in commit 76eb5e3a03 on Jul 16, 2021

Fabcien referenced this in commit 314bdf8203 on Sep 23, 2021

DrahtBot locked this on Feb 15, 2022

test: Fix flaky wallet_basic test #19887