[wallet] restore coinbase and confirmed/conflicted checks in SubmitMemoryPoolAndRelay()

jnewbery commented at 6:39 pm on August 6, 2019: member

These checks don’t change mempool acceptance/relay behaviour, but reduce log spam.

[wallet] restore coinbase check in SubmitMemoryPoolAndRelay()

This check doesn't change mempool acceptance/relay behaviour, but reduces log spam.

214c4ecb9a

jnewbery commented at 6:40 pm on August 6, 2019: member

Reported by @kallewoof

ping @MarcoFalke and @ariard for review.

in src/wallet/wallet.cpp:2165 in 214c4ecb9a outdated

2159@@ -2160,6 +2160,9 @@ bool CWalletTx::SubmitMemoryPoolAndRelay(std::string& err_string, bool relay)
2160     if (!pwallet->GetBroadcastTransactions()) return false;
2161     // Don't relay abandoned transactions
2162     if (isAbandoned()) return false;
2163+    // Don't try to submit coinbase transactions. These would fail anyway but would
2164+    // cause log spam.
2165+    if (IsCoinBase()) return false;

ariard commented at 7:39 pm on August 6, 2019:

Sorry for that. You may need also to restore the GetDepthInMainChain, if you follow what @kallewoof was saying non-coinbase confirmed txn were also in log spams.

kallewoof commented at 0:06 am on August 7, 2019:

~~No, I don’t believe non-coinbase confirmed txn were in log spams but there were thousands so I can’t easily verify.~~

Edit: With patch it was easily verifiable that non-coinbase txs also appear.

ariard commented at 7:44 pm on August 6, 2019: member

Once wallet rebroadcast logic get reworked, I think log spams due to massive rebroadcast of confirmed txn should disappear, but yes that’s not a thing for now.

DrahtBot added the label Wallet on Aug 6, 2019

promag commented at 0:40 am on August 7, 2019: member

ACK 214c4ecb9ab306627a08abb35cf82c73f10ec627.

ariard commented at 2:45 am on August 7, 2019: member

ACK 214c4ec

kallewoof commented at 8:56 am on August 7, 2019: member

I applied this patch to my node and restarted it.

This does not only happen during reorgs, it seems to be happening regularly. I think the node has gotten in its head that it needs to rebroadcast all transactions.
This is not only coinbase transactions. After applying the patch I see tons of txs. Picking two at random, both turned out to be regular txs. See e.g. https://explorer.bc-2.jp/tx/a2ba0e7b09474565c92ef495a35fd932691f3ee3468992cd6a7e5bce100a6b3a (2019-08-07T08:53:22Z [default wallet] Submitting wtx a2ba0e7b09474565c92ef495a35fd932691f3ee3468992cd6a7e5bce100a6b3a to mempool for relay)
This did not happen in conjunction with a new block, either. It seems the wallet is doing this as part of its regular wallet tx resend. But these have thousands of confirmations.

The debug.log file is more or less dominated by these messages, although it may be less severe after the patch.

0$ wc debug.log
1   77751   776919 10791973 debug.log
2$ grep -v "to mempool for relay" debug.log|wc
3    300    2414   26399

ariard commented at 3:21 pm on August 7, 2019: member

Did you try to run without #15713 but with IsCoinBase and GetDepthInMainChain checks turn off ? I guess you should see the same logs than right now. Wallet is always trying to rebroadcast, even with thousands of confirmations, removing checks just make it apparent ?

kallewoof commented at 3:33 pm on August 7, 2019: member

@ariard No. If I am the only one seeing these messages, however, this may be a problem with signet. No clue why signet would cause this, though, tbh.

MarcoFalke commented at 4:52 pm on August 7, 2019: member

#15713 also changed the logging, IIRC. So the bug can be fixed by either (or both):

Fixed logging
Restored checks

ariard commented at 2:56 am on August 8, 2019: member

@kallewoof is not a problem with signet, it’s a problem with my PR. I’ve played a bit with a regtest node on a commit before it. If I generate thousands of coinbase txn, every time we hit nNextResend (once in a 30min period), and 5min older than last block we are going to call RelayWalletTransaction for every one of them. You can see them in this method if you add a printer before IsCoinBase check. Previously printer was only triggered if they have been accepted by mempool. Now, with the new refactored SubmitMemoryPoolAndRelay we have removed these checks, because they are already performed in BroadcastTransaction or lower, and log print whatever their mempool acceptance/relay are. The resend logic has always been there for unconfirmed txn, it’s just now we see it.

While waiting for this logic to be rationalized, to avoid spam we can move log print to BroadcastTransaction after inserting successfully in mempool or re-add both IsCoinBase and GetDepthInMainChain checks in SubmitMemoryPoolAndRelay.

Note: IMO, comment “only rebroadcast unconfirmed txes older than 5 minutes before the last block was found” in ResendWalletTransactions is ambiguous and suggests confirmation check has already been done at this step, but in fact it relies on the callee function to do so.

[wallet] Restore confirmed/conflicted tx check in SubmitMemoryPoolAndRelay()

Restores the confirmed/conflicted tx check removed in
8753f5652b4710e66b50ce87788bf6f33619b75a. There should be no external
behaviour change (these txs would not get accepted to the mempool
anyway), but not having the check in the wallet causes log spam.

Also adds a comment to ResentWalletTransactions() that
confirmed/conflicted tx check is done in SubmitMemoryPoolAndRelay().

c8b53c3bea

jnewbery commented at 3:11 pm on August 9, 2019: member

I’ve restored the GetDepthInMainChain() check in SubmitMemoryPoolAndRelay(). That meant passing the locked_chain through to SubmitMemoryPoolAndRelay() (at least until #15931 is merged).

Also clarified comment in ResendWalletTransactions().

jnewbery renamed this:
~~[wallet] restore coinbase check in SubmitMemoryPoolAndRelay()~~
[wallet] restore coinbase and confirmed/conflicted checks in SubmitMemoryPoolAndRelay()
on Aug 9, 2019

DrahtBot commented at 3:23 pm on August 9, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

MarcoFalke commented at 4:51 pm on August 9, 2019: member

ACK c8b53c3beafa289dcdbd8c2ee9f957bdeca79cbc (non-doc changes are mostly a git revert 8753f5652b4710e66b50ce87788bf6f33619b75a)

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK c8b53c3beafa289dcdbd8c2ee9f957bdeca79cbc (non-doc changes are mostly a git revert 8753f5652b4710e66b50ce87788bf6f33619b75a)
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgQAQv/Z6Y5v5REeQFDBT4A4iyBncehSPa36ikjjvjE1SRHw8gMLoYffjC96s0n
 8aZ1vpEUrUPhnQhiOWmr68rEFoAkoW13FeMqdc7iN4UBcW71vX2vGVrDmQilqrkid
 9TcTdogmucTkRCXyrkU6fjSDgO9TAUoF4ANZNuwusWW9oxcQurt8/86PAGI3jAxmy
10yEYTmjoWt55EcnzC57rlMQ7mCR+okiEtgV+pC5tXerJvjdCZRM4LqXhYbto1UtOm
11i6Qxn6gp4Z1eRjM86KQ6D9YfsP1sxtdS1xVDLXH2JbCD1EB7kE1FzB/IveFyYg3x
1256sBd9tenZ8F6UKbhbbnvfDB/AiZwEnkYyBxclB7fVJsH+2N80pGJMcqUwlKBP3y
13bHBr+IJhekG70belcOTK02eoWC9OJR0Ga5cNGJwIKmWPScHItnhjC0lzjE4PO0yA
14+aJAnuQpeySSfD9TvunAfPxqQ/4EvYk52WOtBxdqHPbNnr0I7Rhtbk0GKaZYWpn3
15O4uPQ/hf
16=TlR4
17-----END PGP SIGNATURE-----

Timestamp of file with hash d299ae4192e52358020d1c810082e7d19625bf7a9adef077079a3e02624e9dd6 -

ariard commented at 4:57 pm on August 9, 2019: member

utACK c8b53c3

kallewoof commented at 11:59 pm on August 9, 2019: member

ACK

Applied this to my node.

Ran for awhile. No spam. Tried sending a tx. Message appeared as appropriately in log. I don’t think I am able to test the CWallet::ResendWalletTransactions part easily as it depends on a new block without tx being mined. Did try restarting the node though, and the log message appeared as expected.

in src/wallet/wallet.cpp:2384 in c8b53c3bea

2378@@ -2377,11 +2379,12 @@ void CWallet::ResendWalletTransactions()
2379         // Relay transactions
2380         for (std::pair<const uint256, CWalletTx>& item : mapWallet) {
2381             CWalletTx& wtx = item.second;
2382-            // only rebroadcast unconfirmed txes older than 5 minutes before the
2383-            // last block was found
2384+            // Attempt to rebroadcast all txes more than 5 minutes older than
2385+            // the last block. SubmitMemoryPoolAndRelay() will not rebroadcast
2386+            // any confirmed or conflicting txs.

Sjors commented at 4:53 pm on August 10, 2019:

Why not move the coinbase check here then?

jnewbery commented at 2:42 pm on August 12, 2019:

I agree that would make sense - the other callers of SubmitMemoryPoolAndRelay() do checks before calling into SubmitMemoryPoolAndRelay(). However, this PR is just to restore previous behaviour. There are a few other PRs in progress on the node/wallet interface, so let’s keep the existing behaviour and look for ways to tidy it up later.

MarcoFalke merged this on Aug 12, 2019

MarcoFalke closed this on Aug 12, 2019

MarcoFalke referenced this in commit b499d8576f on Aug 12, 2019

jnewbery deleted the branch on Aug 12, 2019

sidhujag referenced this in commit 968322a24f on Aug 13, 2019

deadalnix referenced this in commit 0da6d99ee2 on Jul 28, 2020

DrahtBot locked this on Dec 16, 2021

[wallet] restore coinbase and confirmed/conflicted checks in SubmitMemoryPoolAndRelay() #16557

Conflicts