wallet: Properly rebroadcast unconfirmed transaction chains #25768

achow101 commented at 12:45 AM on August 2, 2022: member

Currently ResendWalletTransactions (used for normal rebroadcasts) will attempt to rebroadcast all of the transactions in the wallet in the order they are stored in mapWallet. This ends up being random as mapWallet is a std::unordered_map. However ReacceptWalletTransactions (used for adding to the mempool on loading) first sorts the txs by wallet insertion order, then submits them. The result is that ResendWalletTranactions will fail to rebroadcast child transactions if their txids happen to be lexicographically less than their parent's txid. This PR resolves this issue by combining ReacceptWalletTransactions and ResendWalletTransactions into a new ResubmitWalletTransactions so that the iteration code and basic checks are shared.

A test has also been added that checks that such transaction chains are rebroadcast correctly.

DrahtBot added the label Wallet on Aug 2, 2022

DrahtBot commented at 2:59 AM on August 2, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24683 (Add and use ForEachWallet by promag)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

glozow commented at 4:17 PM on August 2, 2022: member

Concept ACK

achow101 force-pushed on Aug 2, 2022

in test/functional/wallet_resendwallettransactions.py:76 in b11cceaf16 outdated

  78 | @@ -74,6 +79,41 @@ def run_test(self):
  79 |          node.setmocktime(now + 36 * 60 * 60 + 600)
  80 |          peer_second.wait_for_broadcast([txid])
  81 |  
  82 | +        self.log.info("Chain of unconfirmed not-in-mempool txs are rebroadcast")

glozow commented at 3:52 PM on August 8, 2022:

Question: Is this test meant to fail without the first commit?

achow101 commented at 4:04 PM on August 8, 2022:

Yes

glozow commented at 3:06 PM on August 18, 2022:

It's passing for me on master :thinking:

achow101 commented at 3:17 PM on August 18, 2022:

Since master has mapWallet as a std::unsorted_map, this will now only fail randomly on it. It should fail half of the time, on average. I don't know how we can guarantee that the child will come before the parent in an unsorted map.

achow101 commented at 3:26 PM on August 18, 2022:

I've removed the "make a spend that will come before" and updated the comment to explain that.

glozow commented at 3:39 PM on August 18, 2022:

Hm, I see what you're saying about std::unsorted_map and it makes sense to me, but I've run the test >20 times and haven't gotten a failure yet...

achow101 commented at 4:26 PM on August 18, 2022:

Found a bug in the test. It should fail intermittently on master now

glozow commented at 4:34 PM on August 18, 2022:

Aha, fails intermittently on master for me now :) thanks!

kevkevinpal commented at 5:48 PM on August 18, 2022:

I'm seeing the test fail intermittently on commit e40ddf3 does that sound right or should the test only fail on the master branch

achow101 commented at 6:20 PM on August 18, 2022:

It should only fail on master. It should not fail on this PR. Make sure that you've compiled this PR before running the test.

kevkevinpal commented at 9:04 PM on August 18, 2022:

hmm I think I've compiled it correctly (still new to the repo so might be doing it incorrectly), this is what I did

git fetch origin pull/25768/head:PR25768
git checkout  PR25768
./autogen.sh
./configure
make clean
make -j "$(($(sysctl -n hw.physicalcpu)+1))"
./test/functional/wallet_resendwallettransactions.py

This is the error I see after I run the test, sometimes it passes

2022-08-18T20:38:25.914000Z TestFramework (INFO): Initializing test directory /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_43xe3o56
2022-08-18T20:38:28.879000Z TestFramework (INFO): Create a new transaction and wait until it's broadcast
2022-08-18T20:38:29.627000Z TestFramework (INFO): Create a block
2022-08-18T20:38:29.634000Z TestFramework (INFO): Bump time & check that transaction is rebroadcast
2022-08-18T20:38:29.905000Z TestFramework (INFO): Chain of unconfirmed not-in-mempool txs are rebroadcast
2022-08-18T20:38:31.037000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
    self.run_test()
  File "/Users/kevkevin/DEVDIR/bitcoin/./test/functional/wallet_resendwallettransactions.py", line 107, in run_test
    assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, txid)
  File "/Users/kevkevin/DEVDIR/bitcoin/test/functional/test_framework/util.py", line 126, in assert_raises_rpc_error
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
AssertionError: No exception raised
2022-08-18T20:38:31.089000Z TestFramework (INFO): Stopping nodes
2022-08-18T20:38:31.508000Z TestFramework (WARNING): Not cleaning up dir /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_43xe3o56
2022-08-18T20:38:31.508000Z TestFramework (ERROR): Test failed. Test logging available at /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_43xe3o56/test_framework.log
2022-08-18T20:38:31.508000Z TestFramework (ERROR):
2022-08-18T20:38:31.509000Z TestFramework (ERROR): Hint: Call /Users/kevkevin/DEVDIR/bitcoin/test/functional/combine_logs.py '/var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_43xe3o56' to consolidate all logs
2022-08-18T20:38:31.509000Z TestFramework (ERROR):
2022-08-18T20:38:31.509000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2022-08-18T20:38:31.509000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2022-08-18T20:38:31.509000Z TestFramework (ERROR):

running on MacOs Monterey Version 12.3.1

achow101 commented at 9:40 PM on August 18, 2022:

I'm not seeing that error even when running on a loop.

maflcko commented at 8:10 AM on August 19, 2022:

git checkout PR25768

Can you check that git log -1 prints the correct hash, or alternatively just checkout the correct hash directly? Also, make sure that the working tree is clean.

git fetch origin e40ddf36bed81bdf28d386eb961c9ed22b69e207
git checkout e40ddf36bed81bdf28d386eb961c9ed22b69e207
git reset --hard HEAD # warning!! wipes unstaged changes in the tree if there still are some

kevkevinpal commented at 12:33 AM on August 21, 2022:

still getting the error and I am able to see I'm on the correct commit with git log -1 and tried thee commands you posted aswell, I'll try and dig deeper to see what may be the issue

kevkevinpal commented at 1:50 AM on August 21, 2022:

not exactly sure what happened but after rebuilding master then rebuilding this PR's branch twice the test passes now sorry for any confusion

in src/wallet/wallet.cpp:1831 in b11cceaf16 outdated

1826 | +    for (auto& [wtxid, wtx] : mapWallet) {
1827 | +        assert(wtx.GetHash() == wtxid);
1828 | +        sorted_txs.insert(std::make_pair(wtx.nOrderPos, &wtx));
1829 | +    }
1830 | +    std::vector<CWalletTx*> out;
1831 | +    for (auto [pos, wtx] : sorted_txs) {

kevkevinpal commented at 1:20 AM on August 17, 2022:

nit: not sure if we need pos here doesn't seem to be used

achow101 commented at 3:27 PM on August 18, 2022:

Done

kevkevinpal commented at 1:22 AM on August 17, 2022: contributor

Concept ACK was able to run the functional test on my local and it ran fine, still want to test on regtest but still new to the project so still learning

achow101 force-pushed on Aug 18, 2022

in test/functional/wallet_resendwallettransactions.py:34 in e40ddf36be outdated

  28 | @@ -27,7 +29,10 @@ def run_test(self):
  29 |          peer_first = node.add_p2p_connection(P2PTxInvStore())
  30 |  
  31 |          self.log.info("Create a new transaction and wait until it's broadcast")
  32 | -        txid = node.sendtoaddress(node.getnewaddress(), 1)
  33 | +        unspent = node.listunspent()
  34 | +        parent_utxo = unspent[0]
  35 | +        indep_utxo = unspent[1]

glozow commented at 2:18 PM on August 19, 2022:

Perhaps it's worth doing the test with, say, 10 parent-child pairs? That would catch this bug more reliably without needing to run the test multiple times.

stickies-v commented at 1:22 PM on August 24, 2022:

nit: could use a one-liner which imo is equally readable?

        parent_utxo, indep_utxo = node.listunspent()[:2]

achow101 commented at 9:56 PM on August 24, 2022:

I've reimplemented the test so that it always fails on master.

achow101 commented at 8:41 PM on August 25, 2022:

Done

in src/wallet/wallet.cpp:1827 in e40ddf36be outdated

1822 | +{
1823 | +    AssertLockHeld(cs_wallet);
1824 | +    // Sort pending transactions based on their initial wallet insertion order
1825 | +    std::map<int64_t, CWalletTx*> sorted_txs;
1826 | +    for (auto& [wtxid, wtx] : mapWallet) {
1827 | +        assert(wtx.GetHash() == wtxid);

Riahiamirreza commented at 5:37 PM on August 20, 2022:

Sorry, I don't understand the need for assert(wtx.GetHash() == wtxid);. In which case can this assert fails? Only when the data is corrupted?

sipa commented at 6:54 PM on August 20, 2022:

There is never a need for an assert. Assertions verify assumptions about the codebase, and should only fail if something you believe to be impossible in the codebase to be the case. It should never trigger, unless there is a bug in the code.

By writing this, the code author signals to readers of the code "here you can assume that wtxid is always equal to wtx.GetHash()". Further, this is verified during all tests, so we know it is also true for all scenarios any test case runs through.

glozow commented at 8:59 AM on August 22, 2022:

Wait, I see this is just moved and not new, but is this key wtxid or txid? Or does the "w" mean wallet instead of witness?

If it's wtxid, this assertion could fail when wtxid != txid since CWalletTx::GetHash() calls tx->GetHash(). But AFAICT it is a txid, so should be called txid instead? https://github.com/bitcoin/bitcoin/blob/02aefa169a9e6ed12c7bd8f3392adcd073d8d56b/src/wallet/wallet.cpp#L956-L971

achow101 commented at 4:04 PM on August 22, 2022:

The "w" refers to "wallet".

naumenkogs commented at 8:18 AM on August 24, 2022:

Is it good time to rename it to wallet_txid for clarification? Without @glozow comment, i would just assume w stands for witness without any doubt...

hernanmarino commented at 3:06 PM on August 24, 2022:

The variable name also confused me , +1 to renaming it

pablomartin4btc commented at 3:06 PM on August 24, 2022:

I got the same confusion before reading the entire thread, thought it came from witness...

achow101 commented at 3:56 PM on August 24, 2022:

The variable names wtxid and wtx are used very frequently throughout the wallet code to refer to wallet transactions and their txids. If people find this confusing, then I think it would be more appropriate to have a scripted-diff PR rename all of those rather than renaming each individual instance that occurs in a PR.

achow101 commented at 9:56 PM on August 24, 2022:

Renamed to txid.

Riahiamirreza commented at 4:27 PM on August 21, 2022: contributor

Would you explain what is the difference between ResendWalletTransactions and ReacceptWalletTransactions? As you said ResendWalletTransactions is used for normal rebroadcasting and ReacceptWalletTransactions is used for rebroadcasting on loading. I'm not sure what do you mean by "loading", you mean something like reindex or reorg?

achow101 commented at 4:36 PM on August 21, 2022: member

I'm not sure what do you mean by "loading", you mean something like reindex or reorg?

"Loading" as in when the wallet is read from disk and loaded into memory.

Riahiamirreza commented at 4:55 PM on August 21, 2022: contributor

So what is the difference between rebroadcasting when loading and in normal cases?

achow101 commented at 5:04 PM on August 21, 2022: member

There are differences in which transactions will be considered for rebroadcasting. On load, all non-abandoned transactions will be rebroadcast. The periodic rebroadcast will only rebroadcast those that are more than 5 minutes older than the most recent block.

in test/functional/wallet_resendwallettransactions.py:38 in e40ddf36be outdated

  34 | +        parent_utxo = unspent[0]
  35 | +        indep_utxo = unspent[1]
  36 | +        txid = node.send(outputs=[{node.getnewaddress(): 1}], options={"inputs": [parent_utxo]})["txid"]
  37 |  
  38 |          # Wallet rebroadcast is first scheduled 1 sec after startup (see
  39 |          # nNextResend in ResendWalletTransactions()). Tell scheduler to call

rajarshimaitra commented at 8:29 AM on August 24, 2022:

This comment on scheduling the resending logic at 1 sec after the startup seems to be visible in the StartWallets() function, rather than in ResendWalletTransactions()'s nNextResend timepoint setting.

So maybe this comment should point to this part of the code instead?

https://github.com/bitcoin/bitcoin/blob/713ea7a4181f411949a3e9bb9a358533388b1159/src/wallet/load.cpp#L154

achow101 commented at 9:53 PM on August 24, 2022:

The comment is removed now that this behavior is no longer needed.

in src/wallet/wallet.cpp:1847 in e40ddf36be outdated

1852 | +    for (auto wtx : GetSortedTxs()) {
1853 | +        int nDepth = GetTxDepthInMainChain(*wtx);
1854 |  
1855 | -        if (!wtx.IsCoinBase() && (nDepth == 0 && !wtx.isAbandoned())) {
1856 | -            mapSorted.insert(std::make_pair(wtx.nOrderPos, &wtx));
1857 | +        if (!wtx->IsCoinBase() && (nDepth == 0 && !wtx->isAbandoned())) {

stickies-v commented at 8:29 AM on August 24, 2022:

When looking at CWallet::GetTxDepthInMainChain, if nDepth == 0, isn't that equivalent to !wtx->IsConfirmed() && !wtx->IsConflicted(), which in turn in combination with !wtx->isAbandoned() is equivalent to !wtx->IsUnconfirmed() - am I missing something? (I know this is moved code)

        if (!wtx->IsCoinBase() && !wtx->IsUnconfirmed()) {

achow101 commented at 4:09 PM on August 24, 2022:

Yes, those should be equivalent. Will do if I need to re-push.

achow101 commented at 9:53 PM on August 24, 2022:

Done

stickies-v commented at 4:05 PM on August 25, 2022:

Good catch that !IsCoinBase() is redundant in combination with !IsUnconfirmed() 👍

in src/wallet/wallet.cpp:1821 in 9a556564e9 outdated

1817 | @@ -1818,32 +1818,37 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1818 |      return result;
1819 |  }
1820 |  
1821 | +std::vector<CWalletTx*> CWallet::GetSortedTxs()

naumenkogs commented at 8:33 AM on August 24, 2022:

Should we specify in variable name what kind of sorting is applied?

stickies-v commented at 8:43 AM on August 24, 2022:

I was about to add the same suggestion 👍

achow101 commented at 9:55 PM on August 24, 2022:

The function has ended up being removed.

in src/wallet/wallet.cpp:1922 in 9a556564e9 outdated

1918 | @@ -1914,15 +1919,14 @@ void CWallet::ResendWalletTransactions()
1919 |      { // cs_wallet scope
1920 |          LOCK(cs_wallet);
1921 |  
1922 | -        // Relay transactions
1923 | -        for (std::pair<const uint256, CWalletTx>& item : mapWallet) {
1924 | -            CWalletTx& wtx = item.second;
1925 | +        // Try to add wallet transactions to memory pool

naumenkogs commented at 8:39 AM on August 24, 2022:

"and relay"?

pablomartin4btc commented at 4:05 PM on August 24, 2022:

you meant the variable? the change is on void CWallet::ResendWalletTransactions().

achow101 commented at 9:55 PM on August 24, 2022:

Done

ghost commented at 9:03 AM on August 24, 2022: none

This PR resolves this issue by refactoring ReacceptWalletTransactions sorting code into a separate function, then using that function in both ReacceptWalletTransactions and ResendWalletTransactions so that both functions will sort the transactions in the same order before processing for rebroadcast.

Is it possible to shuffle transactions before SubmitTxMemoryPoolAndRelay rebroadcasting using std::shuffle? Since only wallet txs are rebroadcasted which is a separate privacy issue, we could at least hide the order in which they were inserted in wallet db.

in test/functional/wallet_resendwallettransactions.py:88 in e40ddf36be outdated

  83 | +        # Ideally we would add a transaction that would spend the existing transaction and also be
  84 | +        # in mapWallet before it. However mapWallet is a std::unsorted_map which means that it is
  85 | +        # hard for us to externally construct such a transaction. So we will ignore that fact and
  86 | +        # rely on the fact that because there are only two items in mapWallet, 50% of the time the
  87 | +        # child will be placed before the parent in mapWallet. The incorrect behavior that this
  88 | +        # test tries to catch should thus fail intermittently at a rate of 50%.

rajarshimaitra commented at 10:05 AM on August 24, 2022:

I am a bit confused on the intermittent test failure occurring here, maybe I am missing something.

Isn't sorting the order by wallet insertion index is what this PR fixes? So why would it still fail to reboadcast if child.txid() < parent.txid()?

Also, does it make sense to have some unit testing to ensure this behavior of the GetSorted() function itself?

achow101 commented at 4:16 PM on August 24, 2022:

Isn't sorting the order by wallet insertion index is what this PR fixes? So why would it still fail to reboadcast if child.txid() < parent.txid()?

The comment is talking about the code before this PR and how the test tries to catch it, not that this test should fail intermittently after the PR.

Also, does it make sense to have some unit testing to ensure this behavior of the GetSorted() function itself?

There are already tests for the underlying wtxOrdered itself (or tests that end up testing that behavior, although perhaps not specifically).

rajarshimaitra commented at 4:49 PM on August 24, 2022:

So that means the code should not fail intermittently?? I will double check again that I am testing the correct binary, but I thoughts that's expected behavior from the comment..

achow101 commented at 4:54 PM on August 24, 2022:

It should only fail intermittently on master.

rajarshimaitra commented at 10:07 AM on August 24, 2022: contributor

Concept + tACK e40ddf36bed81bdf28d386eb961c9ed22b69e207

Verified that the tests are intermittently failing.
Failure occurring at debug log assertion ResendWalletTransactions: resubmit 2 unconfirmed transactions

Below are just one nit comment and a conceptual question.

in test/functional/wallet_resendwallettransactions.py:109 in e40ddf36be outdated

  97 | +        block.solve()
  98 | +        node.submitblock(block.serialize().hex())
  99 | +        node.syncwithvalidationinterfacequeue()
 100 | +
 101 | +        # Evict these txs from the mempool
 102 | +        evict_time = block_time + 60 * 60 * 336 + 5

stickies-v commented at 1:52 PM on August 24, 2022:

nit: DEFAULT_MEMPOOL_EXPIRY_HOURS is defined in test/functional/mempool_expiry.py. Similarly to #25810, could move that to messages.py and have the benefit of using a named var instead of a less obvious literal?

achow101 commented at 8:41 PM on August 25, 2022:

Done

in test/functional/wallet_resendwallettransactions.py:119 in e40ddf36be outdated

 107 | +        assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, txid)
 108 | +        assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, child_txid)
 109 | +
 110 | +        # Rebroadcast and check that parent and child are both in the mempool
 111 | +        with node.assert_debug_log(['ResendWalletTransactions: resubmit 2 unconfirmed transactions']):
 112 | +            node.setmocktime(evict_time + 36 * 60 * 60)

stickies-v commented at 2:42 PM on August 24, 2022:

nit: perhaps useful to explain the 36 literal?

            node.setmocktime(evict_time + 36 * 60 * 60)  # 36h is the upper limit in CWallet::ResendWalletTransactions()`

achow101 commented at 8:42 PM on August 25, 2022:

Done

achow101 commented at 3:54 PM on August 24, 2022: member

Is it possible to shuffle transactions before SubmitTxMemoryPoolAndRelay rebroadcasting using std::shuffle? Since only wallet txs are rebroadcasted which is a separate privacy issue, we could at least hide the order in which they were inserted in wallet db.

No. Shuffling is the reason we have this issue in the first place (there is no order in an unsorted map so iterating it is basically the same as iterating a shuffled list). I don't think the insertion order is a privacy leak since it's the same order as when the transactions were originally broadcast which is public information to anyone that's connected to the network. Additionally, this is not a unilateral broadcast - it is up to the mempool relay code to determine when, how, and which transactions are actually rebroadcast. Only transactions that are not already in the mempool will be rebroadcast.

ghost commented at 4:22 PM on August 24, 2022: none

No. Shuffling is the reason we have this issue in the first place (there is no order in an unsorted map so iterating it is basically the same as iterating a shuffled list).

I was suggesting to shuffle the sorted list before rebroadcast. So we have all the transactions we need but do not relay in that order.

I don't think the insertion order is a privacy leak since it's the same order as when the transactions were originally broadcast which is public information to anyone that's connected to the network.

In that case, feel free to ignore my suggestion.

ghost commented at 4:23 PM on August 24, 2022: none

Concept ACK

achow101 commented at 4:34 PM on August 24, 2022: member

I was suggesting to shuffle the sorted list before rebroadcast. So we have all the transactions we need but do not relay in that order.

Shuffling after sorting would just undo the sorting. Sorting is not used for filtering; it is used because we need to have the transactions in order. Specifically parent transactions must be rebroadcast before child transactions, and that specific order can be obtained through the insertion order.

While we could do something more complicated like actually building packages so that the actually order dependent things are grouped together, and then shuffling the packages, but I don't see the benefit in doing that. That might actually harm privacy because we would be broadcasting transactions that are related to each other at the same time, but only those transactions that are related to our wallet. Since a package that the wallet builds may exclude other transactions in the package that the mempool would know about, we could essentially be telling other nodes some of the scripts that our wallet is watching for as they could then look at what the rebroadcast package has that the rest of the package in the mempool does not.

achow101 force-pushed on Aug 24, 2022

achow101 commented at 9:52 PM on August 24, 2022: member

Based on some discussions during the PR review club today, I've made several changes to this PR. The first is that ResendWalletTransaction and ReacceptWalletTransactions are now combined into a single function ResubmitWalletTranasctions. There were some subtle differences between the two functions that are being preserved by using two optional parameters. The first difference is that ReacceptWalletTransactions would not actually ask the mempool to relay the transactions (there is a parameter for relay in SubmitTxMemoryPoolAndRelay that was different in both). There is a parameter relay with a default of true to handle that. The second is that ResendWalletTransactions would look chain status (is ibd, reindexing, etc.) and transaction time in order to determine whether to try submitting a transaction, whereas ReacceptWalletTransactions would not. To match the ReaccpetWalletTransactions behavior, a force option with a default of false is added that will bypass those checks.

Additionally, nNextResend was initialized the first time ResendWalletTransactions was run. As the new ResubmitWalletTransactions is always run on start, I've removed from the test the need to force it to be run as well as the handling around not actually trying to resend during that first run (we want to do the reaccept behavior in that first run). I've also removed GetSortedTxs and have instead taken the suggestion of filtering then sorting the transactions. This is done by inserting the CWalletTxs that we want to submit into a std::set with a custom comparator that uses CWalletTx.nOrderPos to determine the sort order.

Lastly I've updated the test to reliably fail on master. I found that listreceivedbyaddress and related RPCs will output txids in the order that they are found in mapWallet. The new test will use that to check if the child is positioned before the parent, and if it is not, it will remake the child until it does.

achow101 force-pushed on Aug 24, 2022

unknown approved

unknown commented at 9:59 PM on August 24, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/25768/commits/fee0dea1b1e32832dc5efca3621cfc8ba7c6c8f9

hernanmarino approved

hernanmarino commented at 1:18 AM on August 25, 2022: contributor

ACK fee0dea

Now the test consistently fails on master, as it should.

in src/wallet/wallet.cpp:1931 in bec0579357 outdated

1878 |  {
1879 | +    // Don't attempt to resubmit if the wallet is configured to not broadcast,
1880 | +    // even if forcing.
1881 | +    if (!fBroadcastTransactions) return;
1882 | +
1883 |      // During reindex, importing and IBD, old wallet transactions become

naumenkogs commented at 7:27 AM on August 25, 2022:

I'd say this comment is now slightly outdated. My first thought was "so all those calls with force=true will spam now?". Then I realized they all have relay=false.

I think extending this comment would improve readability. Might as well document what force does exactly (in broader terms is fine).

achow101 commented at 3:23 PM on August 25, 2022:

I've expanded the comment and also added relay to the check. This way we guarantee that we won't spam if we want to relay, regardless of force.

in src/wallet/wallet.cpp:1949 in bec0579357 outdated

1890 | -    if (GetTime() < nNextResend || !fBroadcastTransactions) return;
1891 | -    bool fFirst = (nNextResend == 0);
1892 | +    if (!force && GetTime() < nNextResend) return;
1893 |      // resend 12-36 hours from now, ~1 day on average.
1894 |      nNextResend = GetTime() + (12 * 60 * 60) + GetRand(24 * 60 * 60);
1895 | -    if (fFirst) return;

naumenkogs commented at 7:31 AM on August 25, 2022:

It took me some time to realize why it's safe. I think it's worth expanding On startup, we will do this for all unconfirmed // transactions but will not ask the mempool to relay them. saying it protects privacy as a side effect?

achow101 commented at 3:23 PM on August 25, 2022:

Done.

in src/wallet/wallet.cpp:1956 in bec0579357 outdated

1914 | +            if (!wtx.isUnconfirmed()) continue;
1915 | +
1916 | +            // For periodic rebroadcast (force == false),
1917 | +            // attempt to rebroadcast all txes more than 5 minutes older than
1918 | +            // the last block.
1919 | +            if (!force && wtx.nTimeReceived > m_best_block_time - 5 * 60) continue;

naumenkogs commented at 7:38 AM on August 25, 2022:

Slightly off-topic, is this code prone to privacy leaks? Imagine a fork appearing: one (empty) block with a timestamp in the past. Would many own transactions get rebroadcast? If yes, i assume other nodes won't forward them because they had them already, and it becomes apparent who is the source?

achow101 commented at 2:48 PM on August 25, 2022:

I don't think so. Your own transactions would be submitted to your node's mempool first, and only if it does not know about those transactions will it try to broadcast them.

naumenkogs commented at 9:22 AM on August 26, 2022:

Right, thank you.

in src/wallet/wallet.cpp:1962 in fee0dea1b1 outdated

1921 | +        }
1922 | +        // Now try submitting the transactions to the memory pool and (optionally) relay them.
1923 | +        for (auto wtx : to_submit) {
1924 |              std::string unused_err_string;
1925 | -            if (SubmitTxMemoryPoolAndRelay(wtx, unused_err_string, true)) ++submitted_tx_count;
1926 | +            if (SubmitTxMemoryPoolAndRelay(*wtx, unused_err_string, relay)) ++submitted_tx_count;

rajarshimaitra commented at 2:44 PM on August 25, 2022:

I might be wrong, but I thought we write bool variables with an f prefix, so should this be frelay?

achow101 commented at 3:24 PM on August 25, 2022:

No. We have had a new style in effect for the past several years. It only applies to new code, so a bunch of the old code written under the old style is still around.

in test/functional/wallet_resendwallettransactions.py:73 in fee0dea1b1 outdated

  70 | +        with node.assert_debug_log(['resubmit 1 unconfirmed transactions']):
  71 |              node.setmocktime(now + 36 * 60 * 60)
  72 |              # Tell scheduler to call MaybeResendWalletTxn now.
  73 |              node.mockscheduler(1)
  74 |          # Give some time for trickle to occur
  75 |          node.setmocktime(now + 36 * 60 * 60 + 600)

rajarshimaitra commented at 2:55 PM on August 25, 2022:

Maybe this is simpler when written as mockscheduler(600)?

achow101 commented at 3:25 PM on August 25, 2022:

mockscheduler does not change the mocked time.

rajarshimaitra approved

rajarshimaitra commented at 3:09 PM on August 25, 2022: contributor

tACK fee0dea1b1e32832dc5efca3621cfc8ba7c6c8f9

Verified the test is failing reliably in master..

Just two more nit comments (questions)..

achow101 force-pushed on Aug 25, 2022

in src/wallet/wallet.cpp:1925 in 71b7709fa0 outdated

1884 | +// The `force` option results in all unconfirmed transactions being submitted to
1885 | +// the mempool. This does not necessarily result in those transactions being relayed,
1886 | +// that depends on the `relay` option. Periodic rebroadcast uses the pattern
1887 | +// relay=true force=false (also the default values), while loading into the mempool
1888 | +// (on start, or after import) uses relay=false force=true.
1889 | +void CWallet::ResubmitWalletTransactions(bool relay, bool force)

stickies-v commented at 4:07 PM on August 25, 2022:

nit: intuitively I'd expect a force parameter to not respect flags like fBroadcastTransactions. I think something like force_ready would make it more obvious that this is not an absolute force. Probably personal preference?

void CWallet::ResubmitWalletTransactions(bool relay, bool force_ready)

achow101 commented at 8:37 PM on August 25, 2022:

I agree that force implies that more will happen than actually does, but I couldn't think of anything better to call it. I don't think force_ready is any clearer.

in src/wallet/wallet.cpp:1917 in 71b7709fa0 outdated

1926 | +            // Only rebroadcast unconfirmed txs
1927 | +            if (!wtx.isUnconfirmed()) continue;
1928 | +
1929 | +            // For periodic rebroadcast (force == false),
1930 | +            // attempt to rebroadcast all txes more than 5 minutes older than
1931 | +            // the last block.

stickies-v commented at 4:20 PM on August 25, 2022:

nit: I think this comment is confusing, from just this I'd think the below code only executes for periodic rebroadcast.

            // Attempt to rebroadcast all txes more than 5 minutes older than
            // the last block, or all txes if force is true.

achow101 commented at 8:42 PM on August 25, 2022:

Done

in src/wallet/wallet.cpp:1874 in 71b7709fa0 outdated

1871 | +//
1872 | +// Otherwise this function is called periodically in order to relay our unconfirmed txs.
1873 | +// We do this on a random timer to slightly obfuscate which transactions
1874 | +// come from our wallet.
1875 |  //
1876 |  // Ideally, we'd only resend transactions that we think should have been

stickies-v commented at 4:27 PM on August 25, 2022:

nit: while you're touching this docstring and fn, worth slapping in a TODO here for increased visibility?

achow101 commented at 8:42 PM on August 25, 2022:

Done

in src/wallet/transaction.h:309 in 71b7709fa0 outdated

 304 | @@ -305,6 +305,13 @@ class CWalletTx
 305 |      CWalletTx(CWalletTx const &) = delete;
 306 |      void operator=(CWalletTx const &x) = delete;
 307 |  };
 308 | +
 309 | +struct WalletTxOrderComparator {

stickies-v commented at 5:13 PM on August 25, 2022:

What do you think about a lambda function instead? Quite a bit less code, and imo about equally readable? As long as we're not using this comparator anywhere else the lambda would have my preference, but I'm okay with the current implementation too.

diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h
index 27983e356..c36cbeb23 100644
--- a/src/wallet/transaction.h
+++ b/src/wallet/transaction.h
@@ -306,12 +306,6 @@ public:
     void operator=(CWalletTx const &x) = delete;
 };
 
-struct WalletTxOrderComparator {
-    bool operator()(const CWalletTx* a, const CWalletTx* b) const
-    {
-        return a->nOrderPos < b->nOrderPos;
-    }
-};
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_TRANSACTION_H
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 69957ed9e..ad20f332a 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1906,8 +1906,9 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force)
         LOCK(cs_wallet);
 
         // First filter for the transactions we want to rebroadcast.
-        // We use a set with WalletTxOrderComparator so that rebroadcasting occurs in insertion order
-        std::set<CWalletTx*, WalletTxOrderComparator> to_submit;
+        // We use a set ordered by nOrderPos so that rebroadcasting occurs in insertion order
+        auto cmp = [](const CWalletTx* a, const CWalletTx* b) { return a->nOrderPos < b->nOrderPos; };
+        std::set<CWalletTx*, decltype(cmp)> to_submit(cmp);
         for (auto& [txid, wtx] : mapWallet) {
             // Only rebroadcast unconfirmed txs
             if (!wtx.isUnconfirmed()) continue;

</details>

achow101 commented at 5:38 PM on August 25, 2022:

I think the comparator might actually be useful in other places too.

stickies-v approved

stickies-v commented at 5:16 PM on August 25, 2022: contributor

utACK 71b7709fa075dc4c9b55bb07304687724f67f0cb

The refactoring into ResubmitWalletTransactions() makes a lot of sense and improves understanding the codebase imo.

in src/wallet/wallet.cpp:1976 in 71b7709fa0 outdated

1934 | @@ -1936,7 +1935,7 @@ void CWallet::ResendWalletTransactions()
1935 |  void MaybeResendWalletTxs(WalletContext& context)
1936 |  {
1937 |      for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
1938 | -        pwallet->ResendWalletTransactions();
1939 | +        pwallet->ResubmitWalletTransactions();

LarryRuane commented at 7:32 PM on August 25, 2022:

nit, this is the only call site that takes advantage of default arguments, so making this change would allow this method to not need default arguments. (Personally, I dislike default arguments unless there's a good reason for them.)

        pwallet->ResubmitWalletTransactions(/*relay=*/true, /*force=*/false);

achow101 commented at 4:42 PM on August 29, 2022:

Removed the default arguments.

achow101 force-pushed on Aug 25, 2022

in test/functional/wallet_resendwallettransactions.py:77 in 789b78430b outdated

  72 | @@ -68,6 +73,53 @@ def run_test(self):
  73 |          node.setmocktime(now + 36 * 60 * 60 + 600)
  74 |          peer_second.wait_for_broadcast([txid])
  75 |  
  76 | +        self.log.info("Chain of unconfirmed not-in-mempool txs are rebroadcast")
  77 | +        # This tests that the node broadcasts the parent transaction before the parent transaction.

naumenkogs commented at 9:55 AM on August 26, 2022:

the parent transaction before the parent transaction?

achow101 commented at 4:07 PM on August 26, 2022:

Oops, fixed.

in src/wallet/wallet.cpp:1901 in 789b78430b outdated

1895 | @@ -1924,43 +1896,69 @@ std::set<uint256> CWallet::GetTxConflicts(const CWalletTx& wtx) const
1896 |      return result;
1897 |  }
1898 |  
1899 | -// Rebroadcast transactions from the wallet. We do this on a random timer
1900 | -// to slightly obfuscate which transactions come from our wallet.
1901 | +// Resubmit transactions from the wallet to the mempool, optionally asking the
1902 | +// mempool to relay them. On startup, we will do this for all unconfirmed

stickies-v commented at 3:13 PM on August 26, 2022:

nit: perhaps this first sentence should live in wallet.h?

achow101 force-pushed on Aug 26, 2022

DrahtBot added the label Needs rebase on Aug 26, 2022

achow101 force-pushed on Aug 27, 2022

DrahtBot removed the label Needs rebase on Aug 27, 2022

in test/functional/wallet_resendwallettransactions.py:78 in 748d5c75d6 outdated

  72 | @@ -68,6 +73,53 @@ def run_test(self):
  73 |          node.setmocktime(now + 36 * 60 * 60 + 600)
  74 |          peer_second.wait_for_broadcast([txid])
  75 |  
  76 | +        self.log.info("Chain of unconfirmed not-in-mempool txs are rebroadcast")
  77 | +        # This tests that the node broadcasts the parent transaction before the child transaction.
  78 | +        # The previous behavior is that the rebroadcasting would simply iterate mapWallet. As

naumenkogs commented at 6:29 AM on August 29, 2022:

nit: I think the previous behavior stuff belongs to the commit message, not to the file content.

achow101 commented at 4:41 PM on August 29, 2022:

Done

LarryRuane approved

LarryRuane commented at 3:44 PM on August 29, 2022: contributor

ACK 748d5c75d60505a3d84d36782f5cf4949224e103

wallet: Deduplicate Resend and ReacceptWalletTransactions

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.

10d91c5abe

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.

3405f3eed5

achow101 force-pushed on Aug 29, 2022

unknown approved

unknown commented at 6:04 PM on August 29, 2022: none

reACK https://github.com/bitcoin/bitcoin/pull/25768/commits/3405f3eed5cf841b23a569b64a376c2e5b5026cd

naumenkogs commented at 6:19 AM on August 30, 2022: member

utACK 3405f3eed5cf841b23a569b64a376c2e5b5026cd

in src/wallet/wallet.cpp:1938 in 10d91c5abe outdated

1940 |  
1941 |      // Do this infrequently and randomly to avoid giving away
1942 |      // that these are our transactions.
1943 | -    if (GetTime() < nNextResend || !fBroadcastTransactions) return;
1944 | -    bool fFirst = (nNextResend == 0);
1945 | +    if (!force && GetTime() < nNextResend) return;

furszy commented at 12:38 PM on August 30, 2022:

At this point of the review, this is just a minor nit but code would be a bit clearer in this way:

if (!force) {
    // During reindex, importing and IBD, old wallet transactions become
    // unconfirmed. Don't resend them as that would spam other nodes.
    // We only allow forcing mempool submission when not relaying to avoid this spam.
    if (relay && !chain().isReadyToBroadcast()) return;

    // Do this infrequently and randomly to avoid giving away
    // that these are our transactions.
    if (GetTime() < nNextResend) return;
}

stickies-v commented at 11:59 AM on September 5, 2022:

I think both these checks would be more appropriate in MaybeResendWalletTxs() since they're more scheduling-like in nature. It would also address furszy's comment them since we don't need to check for force anymore (it's always True there). If you do this, you probably also want to duplicate if (!fBroadcastTransactions) there since it's a very cheap check and we could avoid iterating over the wallets.

Unfortunately, we still need the force parameter when iterating over the transactions, otherwise ResubmitWalletTransactions()'s signature could have been simplified.

furszy approved

furszy commented at 12:56 PM on August 30, 2022: member

Late code review ACK 3405f3ee

The only orthogonal question that arises from my side is why are we trying to resubmit txes after every import RPC command. Can anything change at the, already created, wallet txes level after an import?

in test/functional/wallet_resendwallettransactions.py:107 in 3405f3eed5

 103 | +
 104 | +        # Evict these txs from the mempool
 105 | +        evict_time = block_time + 60 * 60 * DEFAULT_MEMPOOL_EXPIRY_HOURS + 5
 106 | +        node.setmocktime(evict_time)
 107 | +        indep_send = node.send(outputs=[{node.getnewaddress(): 1}], options={"inputs": [indep_utxo]})
 108 | +        node.syncwithvalidationinterfacequeue()

maflcko commented at 2:31 PM on August 30, 2022:

Can you explain why this is needed? Generally this is only needed when forcing the wallet to receive all in-flight mempool notifications. However, there is no wallet call that follows. Also, there is no m_best_block_time update.

achow101 commented at 5:22 PM on August 30, 2022:

Also, there is no m_best_block_time update.

Why not? IIRC this was done in order to update m_best_block_time so that the transaction would be properly evicted later.

maflcko commented at 5:24 PM on August 30, 2022:

The transaction is evicted from the mempool, not the wallet. (Only the wallet has an m_best_block_time)

Also, there is no new best block, so m_best_block_time won't be updated either way.

achow101 commented at 5:43 PM on August 30, 2022:

Oh, wrong line. I think this syncwithvalidationinterfacequeue is probably unneeded.

in test/functional/wallet_resendwallettransactions.py:101 in 3405f3eed5

  97 | +        block_time = entry_time + 6 * 60
  98 | +        node.setmocktime(block_time)
  99 | +        block = create_block(int(node.getbestblockhash(), 16), create_coinbase(node.getblockcount() + 1), block_time)
 100 | +        block.solve()
 101 | +        node.submitblock(block.serialize().hex())
 102 | +        node.syncwithvalidationinterfacequeue()

maflcko commented at 2:35 PM on August 30, 2022:

Can you explain why this is needed? Maybe duplicate the comment "Set correct m_best_block_time" from above?

in src/wallet/wallet.cpp:1923 in 3405f3eed5

1922 | -void CWallet::ResendWalletTransactions()
1923 | +//
1924 | +// The `force` option results in all unconfirmed transactions being submitted to
1925 | +// the mempool. This does not necessarily result in those transactions being relayed,
1926 | +// that depends on the `relay` option. Periodic rebroadcast uses the pattern
1927 | +// relay=true force=false (also the default values), while loading into the mempool

maflcko commented at 2:39 PM on August 30, 2022:

// relay=true force=false, while loading into the mempool

in src/wallet/wallet.cpp:1954 in 3405f3eed5

1964 | +        std::set<CWalletTx*, WalletTxOrderComparator> to_submit;
1965 | +        for (auto& [txid, wtx] : mapWallet) {
1966 | +            // Only rebroadcast unconfirmed txs
1967 | +            if (!wtx.isUnconfirmed()) continue;
1968 | +
1969 | +            // attempt to rebroadcast all txes more than 5 minutes older than

maflcko commented at 2:40 PM on August 30, 2022:

            // Attempt to rebroadcast all txes more than 5 minutes older than

Any reason that this A changed into a?

maflcko commented at 2:40 PM on August 30, 2022: member

left some minor nits, which can be addressed the next time this code is touched

glozow requested review from stickies-v on Sep 1, 2022

stickies-v commented at 12:00 PM on September 5, 2022: contributor

ACK 3405f3eed5cf841b23a569b64a376c2e5b5026cd

maflcko added this to the milestone 24.0 on Sep 5, 2022

glozow merged this on Sep 5, 2022

glozow closed this on Sep 5, 2022

sidhujag referenced this in commit 66165db695 on Sep 5, 2022

bitcoin deleted a comment on Sep 5, 2022

maflcko referenced this in commit fa44670f9a on Sep 14, 2022

maflcko referenced this in commit faa4916529 on Sep 14, 2022

in src/wallet/rpc/backup.cpp:297 in 3405f3eed5

 294 | @@ -295,7 +295,7 @@ RPCHelpMan importaddress()
 295 |          RescanWallet(*pwallet, reserver);
 296 |          {
 297 |              LOCK(pwallet->cs_wallet);

maflcko commented at 10:09 AM on September 20, 2022:

The locks are no longer needed and can be removed

maflcko commented at 4:40 PM on September 20, 2022: member

If a user calls importaddress every 6 hours, this will block any p2p relay from ever happening?

stickies-v commented at 5:05 PM on September 29, 2022: contributor

If a user calls importaddress every 6 hours, this will block any p2p relay from ever happening?

I addressed that in #26205

fanquake referenced this in commit aa6fb37acc on Oct 13, 2022

sidhujag referenced this in commit 15e781feaf on Oct 23, 2022

janus referenced this in commit d23ce23a94 on Jan 20, 2023

bitcoin locked this on Nov 4, 2023