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 & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29982.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, 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)
10. DrahtBot added the label CI failed on Oct 23, 2024
11. 
    achow101 commented at 10:35 pm on October 23, 2024: member

    ACK ec777917d6eba0b417dbc90b9b891240a44b7ec4

    lgtm for a mitigation.

12. DrahtBot requested review from tdb3 on Oct 23, 2024
13. achow101 removed review request from achow101 on Oct 23, 2024
14. DrahtBot removed the label CI failed on Oct 25, 2024
15. tdb3 approved
16. 
    tdb3 commented at 8:13 pm on November 2, 2024: contributor

    ACK ec777917d6eba0b417dbc90b9b891240a44b7ec4

    With a similar delay (150ms) induced with https://github.com/Randy808/bitcoin/commit/f39144d808975c016c1a94022a2c4251950afb7d, Saw test failure without the sync_mempools() line added in this PR. After adding the line, saw consistent passing (20x runs).

17. 
    maflcko commented at 8:08 am on November 4, 2024: member

    lgtm as a temporary workaround, but I think the underlying issue should still be fixed
18. achow101 merged this on Dec 10, 2024
19. achow101 closed this on Dec 10, 2024

    ---

Contributors
Randy808 DrahtBot tdb3 maflcko achow101

Labels
Tests