test: Fix intermittent issue in wallet_backwards_compatibility.py #29982

1. 
   Randy808 commented at 4:24 am on April 28, 2024: contributor

   When creating and replacing a transaction using bumpfee, an async update is sent in the form of the TransactionAddedToMempool and TransactionRemovedFromMempool signals. When wallet_backwards_compatibility.py creates tx3_id this way and replaces it with tx4_id, the abandontransaction rpc is called right after. In some cases the TransactionAddedToMempool and TransactionRemovedFromMempool is handled after the transaction is abandoned in the wallet, and overwrites the transaction’s abandoned flag. This PR forces the signals to get handled before abandontransaction is called by invoking self.sync_mempools which calls syncwithvalidationinterfacequeue on every node’s rpc connection.

   This will mitigate the immediate inconsistency observed with the abandontransaction call, but the potential race conditions between the signals and wallet operations may also be useful to note in a separate issue (if it’s okay to not address it in this one).

   Fixes #29806

2. test: Fix intermittent issue in wallet_backwards_compatibility.py ec777917d6
3. 
   DrahtBot commented at 4:24 am on April 28, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	tdb3

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Tests on Apr 28, 2024
5. 
   tdb3 commented at 1:58 am on April 30, 2024: contributor

   Concept ACK. I would still like to review the logic a bit more. Leaving some test activities as notes for other reviewers:

   Tested by performing the following:

   Fetched previous releases with test/get_previous_releases.py -b, ran all (non-extended) functional tests (with --previous-releases to prevent skipping). All passed.

   As a very quick/basic santity check for intermittency, executed $ test/functional/test_runner.py --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp --filter=wallet_backwards_compatibility --previous-releases five times. No failures encountered. Also did this on master (19865a8350a8181ab658bef48976a728bae6a8bd), but didn’t see failures either (I’m making the assumption that the failure is fairly intermittent and is timing dependent).

   Performed the reproduction steps described in #29806 (comment) (i.e. commit https://github.com/Randy808/bitcoin/commit/f39144d808975c016c1a94022a2c4251950afb7d) on master (i.e. adding delay to callbacks.TransactionAddedToMempool and callbacks.TransactionRemovedFromMempool in validationinterface.cpp with self.sync_mempools() added to wallet_backwards_compatibility.py after node_master.abandontransaction(tx3_id)). The test failed consistently.

   Ran without self.sync_mempools() present after node_master.abandontransaction(tx3_id). The test failed in one out five executions.

   On the PR branch, added the delays to callbacks.TransactionAddedToMempool and callbacks.TransactionRemovedFromMempool (without self.sync_mempools() added to wallet_backwards_compatibility.py after node_master.abandontransaction(tx3_id)), and the tests passed on each of six events.

6. DrahtBot added the label CI failed on Jun 18, 2024
7. DrahtBot removed the label CI failed on Jun 19, 2024
8. achow101 requested review from achow101 on Jul 9, 2024
9. 
   maflcko commented at 2:35 pm on July 19, 2024: member

   The real fix would be #18840 (see also the incoming links on that pull)

Contributors
Randy808 DrahtBot tdb3 maflcko

Review Requested
achow101

Labels
Tests