test: Fix flaky wallet_basic test #19887
Conversation
Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
|
ACK |
|
Can you explain what is going on here?
|
|
Maybe it is a race where either the wallet or the (load)mempool may add the tx? The |
|
@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. |
|
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? |
Is this what's going on? |
|
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? |
|
@MarcoFalke missed this comment. I'll |
56b018c test: Fix flaky wallet_basic test (Fabian Jahr) Pull request description: Fixes bitcoin#19853 I investigated the issue in bitcoin#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, bitcoin#19876 will only provide an error where before it was reporting a false balance. Top commit has no ACKs. Tree-SHA512: 52bb2388a3e77aa20d26ab0fd45796bc1781483b1cffe49cbb44e2488a72e76998edfb1198495373f9c6fd2ec26064d4176bd1a64dd59806622d5e50a4f4e870
56b018c test: Fix flaky wallet_basic test (Fabian Jahr) Pull request description: Fixes bitcoin#19853 I investigated the issue in bitcoin#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, bitcoin#19876 will only provide an error where before it was reporting a false balance. Top commit has no ACKs. Tree-SHA512: 52bb2388a3e77aa20d26ab0fd45796bc1781483b1cffe49cbb44e2488a72e76998edfb1198495373f9c6fd2ec26064d4176bd1a64dd59806622d5e50a4f4e870
Summary: > I investigated the issue in [[bitcoin/bitcoin#19876 | core#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 Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com> This is a backport of [[bitcoin/bitcoin#19887 | core#19887]] Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10183
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.