seen in #29898 too - https://cirrus-ci.com/task/5457627730149376?logs=ci#L3718

The relevant part of the test in wallet_backwards_compatibility.py creates tx3, calls bump fee to replace it in the wallet (creating tx4), and calls abandontransaction on tx3. Later on there's an assertion to check if tx3 is marked as abandoned, but this is the call that sporadically fails.

The failure seems to occur because the TransactionAddedToMempool signal from the creation of tx3, and the TransactionRemovedFromMempool signal from the replacement of tx3 are handled after abandontransaction is called and overwrites the wallet's version of the abandoned tx3.

In the logs posted in the first comment, we can see this in the timestamps (Enqueuing ... happens before abandontransaction is called, while the handling of the enqueued task occurs after):

node1 2024-04-03T22:38:25.781773Z [httpworker.0] [validationinterface.cpp:191] [TransactionAddedToMempool] [validation] Enqueuing TransactionAddedToMempool: txid=41ac814a7e27d942d5f8bd212d83090114dd3d28a2d264eeaa1a53873e727de6 wtxid=dafaa05b20990d2dbdb6488e2a9d405e5bcc4ae32b3c012557fb6c1ef9dd2c8c 

 node1 2024-04-03T22:38:25.783955Z [httpworker.1] [validation.cpp:1165] [Finalize] [mempool] replacing tx 41ac814a7e27d942d5f8bd212d83090114dd3d28a2d264eeaa1a53873e727de6 (wtxid=dafaa05b20990d2dbdb6488e2a9d405e5bcc4ae32b3c012557fb6c1ef9dd2c8c) with 190fd4bd5fab876d2d6258952d487a98965d2092810f964b44af2ea02c5512e9 (wtxid=74725b97d0668fc3aea49d3322a9f4ed4daf7809c42cf06740b3ef8a12d7c9fa) for 0.00000706 additional fees, 0 delta bytes 

node1 2024-04-03T22:38:25.783969Z [httpworker.1] [validationinterface.cpp:200] [TransactionRemovedFromMempool] [validation] Enqueuing TransactionRemovedFromMempool: txid=41ac814a7e27d942d5f8bd212d83090114dd3d28a2d264eeaa1a53873e727de6 wtxid=dafaa05b20990d2dbdb6488e2a9d405e5bcc4ae32b3c012557fb6c1ef9dd2c8c reason=replaced 

node1 2024-04-03T22:38:25.785044Z [httpworker.2] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=abandontransaction user=__cookie__ 

node1 2024-04-03T22:38:25.785369Z [scheduler] [validationinterface.cpp:191] [operator()] [validation] TransactionAddedToMempool: txid=41ac814a7e27d942d5f8bd212d83090114dd3d28a2d264eeaa1a53873e727de6 wtxid=dafaa05b20990d2dbdb6488e2a9d405e5bcc4ae32b3c012557fb6c1ef9dd2c8c 

node1 2024-04-03T22:38:25.794933Z [scheduler] [wallet/wallet.h:933] [WalletLogPrintf] [w1] AddToWallet 41ac814a7e27d942d5f8bd212d83090114dd3d28a2d264eeaa1a53873e727de6  update InMempool

node1 2024-04-03T22:38:25.795430Z [scheduler] [validationinterface.cpp:200] [operator()] [validation] TransactionRemovedFromMempool: txid=41ac814a7e27d942d5f8bd212d83090114dd3d28a2d264eeaa1a53873e727de6 wtxid=dafaa05b20990d2dbdb6488e2a9d405e5bcc4ae32b3c012557fb6c1ef9dd2c8c reason=replaced 

This can be fixed by calling self.sync_mempools() before node_master.abandontransaction(tx3_id), but I wanted to confirm whether overriding the abandoned status of the wallet transaction is expected behavior before tackling this.

The failure can be reproduced locally by wrapping callbacks.TransactionAddedToMempool and callbacks.TransactionRemovedFromMempool (in validationinterface.cpp) with a lambda introducing an UninterruptibleSleep(40ms), and calling self.sync_mempools() after node_master.abandontransaction(tx3_id) in wallet_backwards_compatibility.py

If the transaction is eventually considered abandoned in all cases, then adding the sync_mempools (or I guess syncwithvalidationinterface would be enough) seems fine.

Sounds good, I'll add sync_mempools() to mitigate the immediate inconsistency observed with the abandontransaction call. I'll mention the potential race conditions between the signals and wallet operations, but I'm thinking that could be treated as a separate issue.

The failure can be reproduced locally by wrapping callbacks.TransactionAddedToMempool and callbacks.TransactionRemovedFromMempool (in validationinterface.cpp) with a lambda introducing an UninterruptibleSleep(40ms), and calling self.sync_mempools() after node_master.abandontransaction(tx3_id) in wallet_backwards_compatibility.py

Can you produce a diff for this please, so that it can be applied locally by reviewers?

Here's a commit with the referenced changes applied:
Randy808@f39144d

Repro instructions after branch is built:

# Pick a directory to put the previous releases in
export PREVIOUS_RELEASES_DIR=<prev_releases_dir>
test/get_previous_releases.py -b -t "$PREVIOUS_RELEASES_DIR"
test/functional/wallet_backwards_compatibility.py