The first PR is the feature, the following ones are fixes for intermittent failures introduced by the new test (and maybe for an intermittent failure we currently see on master as well?)
Our version of the test will test the "child before parent" scenario only 50% of the time. I ran the test with an assert txids == [child_txid, txid] to confirm that this happens half the time (50% failure rate with this patch)
Both of these functions do almost the exact same thing. They can be
deduplicated so that their behavior matches except for the filtering
aspect. As this function will now always be called on wallet loading,
nNextResend will also always be initialized, so
wallet_resendwallettransactions.py is updated to account for that.This also resolves a bug where ResendWalletTransactions would fail to
rebroadcast txs in insertion order thereby potentially rebroadcasting a
child transaction before its parent and causing the child to not
actually get rebroadcast.Also names the combined function to ResubmitWalletTransactions as the
function just submits the transactions to the mempool rather than doing
any sending by itself.
test: Test that an unconfirmed not-in-mempool chain is rebroadcast
The test checks that parent txs are broadcast before child txs.
The previous behavior is that the rebroadcasting would simply iterate mapWallet. As
mapWallet is a std::unsorted_map, the child can sometimes come before the parent and thus
be rebroadcast in the wrong order and fail the test.
test/doc: Remove unused syncwithvalidationinterfacequeue
See https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958562071
Also fix doc typo from https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958571943
test: Add missing syncwithvalidationinterfacequeue
test: fix intermittent failure in wallet_resendwallettransactions.py
Before, it was possible that a resend was triggered right between
eviction the txns from the mempool and the check that they were evicted.
tests: Use manual bumps instead of bumpfee for resendwallettransactions
Bumpfee will try to increase the entire package to the target feerate,
which causes repeated bumpfees to quickly shoot up in fees, causing
intermittent failures when the fee is too large. We don't care about
this property, just that the child is continuously replaced until we
observe it's position in mapWallet is before its parent. Instead of
using bumpfee, we can create raw transactions which have only pay the
additional incremental relay fee, thus avoiding this problem.
This PR addresses the outstanding comments/issues from #25768:
- capitalization typo in docstring
- remove unused locks that we previously needed for ReacceptWalletTransactions()
- before #25768, only ResendWalletTransactions() would reset m_next_resend (formerly called nNextResend). By unifying it with ReacceptWalletTransactions() into ResubmitWalletTransactions(), the number of callsites that would reset the m_next_resend timer increased
- since m_next_resend is only used in case of relay=true (formerly ResendWalletTransactions()), this is unintuitive
- it leads to unexpected behaviour such as transactions potentially never being rebroadcasted.
- it makes the ResubmitWalletTransactions()` logic more complicated than strictly necessary
- since #25768, we relied on an earlier call of ResubmitWalletTransactions(relay=false, force=true) to initialize m_next_resend(), I think we can more elegantly do that by just providing m_next_resend with a default value
This is a backport of core#25768, core#26091 core#28108, core#28540 and core#26205
Bypasses core#26132 (make m_next_resend a std::atomic) as the last commit of #26205 reverts this:
Since m_next_resend is now only called from MaybeResendWalletTxs()
we don't have any potential race conditions anymore, so the usage
of std::atomic can be reverted.
Depends on D20617