[mempool] Mark mempool import fails that were found in mempool as 'already there' #11062

kallewoof · 2017-08-16T08:25:10Z

I was investigating the reasons for failed imports in mempool and noticed that LoadMempool() and pwallet->postInitProcess() (for all wallets) are executed concurrently. The wallet will end up importing transactions that LoadMempool() later tries to import; the latter will fail due to the tx already being in the mempool.

This PR changes the log message, adding an additional "already there" entry. For transactions not accepted into mempool, a check if they are in the mempool is done first, and if found, they are counted as 'already there', otherwise counted as 'failed'.

Also slight rewording for consistency (successes, failed, expired, ... -> succeeded, failed, expired).

promag

Switch commit order? Commit messages are a bit long, maybe reduce?

promag · 2017-08-17T14:49:05Z

src/validation.cpp

+                    // wallet(s) having loaded it while we were processing
+                    // mempool transactions; consider these as valid, instead of
+                    // failed, but mark them as 'already existed'
+                    if (mempool.exists(tx->GetHash())) {


Check state.GetRejectCode() == REJECT_DUPLICATE? See this.

Also, IMO this is prettier:

if (state.IsValid()) { ... } else if (state.GetRejectCode() == REJECT_DUPLICATE) { ... } else { ... }

There is more than one reason forREJECT_DUPLICATE, don't know if that matters here. @sipa?

REJECT_DUPLICATE is used to indicate double spends, so that wouldn't be correct.

What about #10503?

But REJECT_DUPLICATE is set if it's in the mempool which happens to be what's tested here. So it should be?

} else if (state.GetRejectCode() == REJECT_DUPLICATE && state.GetRejectReason() == "txn-already-in-mempool") {

As @sipa noted, REJECT_DUPLICATE is ambiguous. I don't want to say it was already there, when a conflicted different transaction was there instead. The safest course would seem to simply check if it is there after a failure, as I am doing now.

Understood, can't use REJECT_DUPLICATE.

Still, the way it's now is like catching the error, whereas testing before it's like expecting that behaviour.

Yes. There's a race condition going on here, which makes this better than the alternative (i.e. checking if exists and then trying to accept).

Thread A is doing wallet mempool insertions.
Thread B is calling LoadMempool().

Case 1:

Thread A imports tx X

Thread B tests for presence of tx X. Found. Marks already there and moves on.

Case 2:

Thread B tests for presence of tx X. Not found.

Thread A imports tx X.

Thread B attempts to import tx X and fails.

Both cases should result in 'already there', but by checking presence before attempt to accept, the second case will result in a failure.

Doesn't the above LOCK(cs_main) prevents that case (and the current implementation for all that matters)?

Hm, you're right, it does. I think.

promag · 2017-08-17T14:57:14Z

src/validation.cpp

@@ -4274,6 +4274,7 @@ bool LoadMempool(void)
    int64_t count = 0;
    int64_t skipped = 0;
    int64_t failed = 0;
+    int64_t alreadythere = 0;


Maybe just increment skipped?

But it wasn't skipped. It was already there.

Rigth, maybe rename skipped to expired.

Edit: the message on the bottom already prints expired.. 🙄

Good point. Done.

sipa · 2017-08-17T23:39:51Z

Yes, what about it?

sipa · 2017-08-18T05:39:30Z

utACK

TheBlueMatt · 2017-08-20T21:05:03Z

utACK 8c8ef83edc8724f2dc7e363e9c412c5df564e08f

maflcko · 2017-09-01T12:30:14Z

utACK 8c8ef83

jnewbery

Seems reasonable. utACK 8c8ef83edc8724f2dc7e363e9c412c5df564e08f , although I think the commits can be squashed down into one commit for merge. I have a couple of supernits on variable names and comments which you can take or leave.

Shame there aren't any LoadMempool unit tests that you could add to. I think adding them from scratch is beyond the scope of this PR.

jnewbery · 2017-09-01T19:15:56Z

src/validation.cpp

    int64_t failed = 0;
+    int64_t alreadythere = 0;


nit: snake case already_there

IMO consider renaming to one of already_imported, already_loaded, already_in_mempool.

Personally feel the current name is fine as is, but will change if people want it.

jnewbery · 2017-09-01T19:16:15Z

src/validation.cpp

+                    // mempool may contain the transaction already, e.g. from
+                    // wallet(s) having loaded it while we were processing
+                    // mempool transactions; consider these as valid, instead of
+                    // failed, but mark them as 'already existed'


nit: mark them as 'already there'

On startup, the wallets will start pumping wallet transactions into the mempool in a different thread while LoadMempool() is running. This will sometimes result in transactions "failing" to be accepted into mempool, but only for the reason that they were already put there by a wallet. The log message for mempool load would note this as a 'failure' to import, which was misleading; it should instead mark it as the transaction already being in the mempool.

kallewoof · 2017-09-04T05:48:51Z

@jnewbery Thanks for the review! You're right, I think I was overthinking things when I tried to split commits. I've squashed into one commit and addressed your nits.

jnewbery · 2017-09-04T12:01:16Z

already_there seems fine to me. Will retest later.

kallewoof · 2017-09-05T01:16:27Z

Didn't realize test failed. Restarting as it's obviously unrelated.

jnewbery · 2017-09-06T00:15:40Z

Tested ACK 258d33b

maflcko · 2017-09-06T00:27:56Z

re-utACK 258d33b

promag · 2017-09-06T00:31:22Z

utACK 258d33b.

…mempool as 'already there' 258d33b [mempool] Mark unaccepted txs present in mempool as 'already there'. (Karl-Johan Alm) Pull request description: I was investigating the reasons for failed imports in mempool and noticed that `LoadMempool()` and `pwallet->postInitProcess()` (for all wallets) are executed concurrently. The wallet will end up importing transactions that `LoadMempool()` later tries to import; the latter will fail due to the tx already being in the mempool. This PR changes the log message, adding an additional "already there" entry. For transactions not accepted into mempool, a check if they are in the mempool is done first, and if found, they are counted as 'already there', otherwise counted as 'failed'. Also slight rewording for consistency (successes, failed, expired, ... -> succeeded, failed, expired). Tree-SHA512: 1a6134a25260917f2768365e0dfd8b278fe3f8287cab38bb028b7de3d517718a2d37696186dc7a23ceab338cc755fbbe7d45358ee94e573610fddd2a0620d6e5

…ready there' Summary: I was investigating the reasons for failed imports in mempool and noticed that `LoadMempool()` and `pwallet->postInitProcess()` (for all wallets) are executed concurrently. The wallet will end up importing transactions that `LoadMempool()` later tries to import; the latter will fail due to the tx already being in the mempool. Backport of Bitcoin Core PR11062 bitcoin/bitcoin#11062 Test Plan: ``` make check-all ``` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: Fabien, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4266

…und in mempool as 'already there' 258d33b [mempool] Mark unaccepted txs present in mempool as 'already there'. (Karl-Johan Alm) Pull request description: I was investigating the reasons for failed imports in mempool and noticed that `LoadMempool()` and `pwallet->postInitProcess()` (for all wallets) are executed concurrently. The wallet will end up importing transactions that `LoadMempool()` later tries to import; the latter will fail due to the tx already being in the mempool. This PR changes the log message, adding an additional "already there" entry. For transactions not accepted into mempool, a check if they are in the mempool is done first, and if found, they are counted as 'already there', otherwise counted as 'failed'. Also slight rewording for consistency (successes, failed, expired, ... -> succeeded, failed, expired). Tree-SHA512: 1a6134a25260917f2768365e0dfd8b278fe3f8287cab38bb028b7de3d517718a2d37696186dc7a23ceab338cc755fbbe7d45358ee94e573610fddd2a0620d6e5

…ready there' Summary: I was investigating the reasons for failed imports in mempool and noticed that `LoadMempool()` and `pwallet->postInitProcess()` (for all wallets) are executed concurrently. The wallet will end up importing transactions that `LoadMempool()` later tries to import; the latter will fail due to the tx already being in the mempool. Backport of Bitcoin Core PR11062 bitcoin/bitcoin#11062 Test Plan: ``` make check-all ``` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: Fabien, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4266

…und in mempool as 'already there' 258d33b [mempool] Mark unaccepted txs present in mempool as 'already there'. (Karl-Johan Alm) Pull request description: I was investigating the reasons for failed imports in mempool and noticed that `LoadMempool()` and `pwallet->postInitProcess()` (for all wallets) are executed concurrently. The wallet will end up importing transactions that `LoadMempool()` later tries to import; the latter will fail due to the tx already being in the mempool. This PR changes the log message, adding an additional "already there" entry. For transactions not accepted into mempool, a check if they are in the mempool is done first, and if found, they are counted as 'already there', otherwise counted as 'failed'. Also slight rewording for consistency (successes, failed, expired, ... -> succeeded, failed, expired). Tree-SHA512: 1a6134a25260917f2768365e0dfd8b278fe3f8287cab38bb028b7de3d517718a2d37696186dc7a23ceab338cc755fbbe7d45358ee94e573610fddd2a0620d6e5

…ready there' Summary: I was investigating the reasons for failed imports in mempool and noticed that `LoadMempool()` and `pwallet->postInitProcess()` (for all wallets) are executed concurrently. The wallet will end up importing transactions that `LoadMempool()` later tries to import; the latter will fail due to the tx already being in the mempool. Backport of Bitcoin Core PR11062 bitcoin/bitcoin#11062 Test Plan: ``` make check-all ``` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: Fabien, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4266

fanquake added the Mempool label Aug 16, 2017

promag reviewed Aug 17, 2017

View reviewed changes

kallewoof force-pushed the mempool-alreadythere branch 2 times, most recently from d2eba62 to 08b7e22 Compare August 18, 2017 00:34

jnewbery reviewed Sep 1, 2017

View reviewed changes

kallewoof force-pushed the mempool-alreadythere branch from 8c8ef83 to 258d33b Compare September 4, 2017 05:48

sipa merged commit 258d33b into bitcoin:master Oct 18, 2017

kallewoof deleted the mempool-alreadythere branch October 18, 2017 09:40

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mempool] Mark mempool import fails that were found in mempool as 'already there' #11062

[mempool] Mark mempool import fails that were found in mempool as 'already there' #11062

kallewoof commented Aug 16, 2017

promag left a comment

promag Aug 17, 2017

promag Aug 17, 2017

sipa Aug 17, 2017

promag Aug 17, 2017

promag Aug 17, 2017

kallewoof Aug 18, 2017

promag Aug 18, 2017

kallewoof Aug 18, 2017

promag Aug 18, 2017

kallewoof Aug 18, 2017

promag Aug 17, 2017

kallewoof Aug 18, 2017

promag Aug 18, 2017 •

edited

kallewoof Aug 18, 2017

sipa commented Aug 17, 2017

sipa commented Aug 18, 2017

TheBlueMatt commented Aug 20, 2017

maflcko commented Sep 1, 2017

jnewbery left a comment

jnewbery Sep 1, 2017

promag Sep 1, 2017

kallewoof Sep 4, 2017

jnewbery Sep 1, 2017

kallewoof commented Sep 4, 2017

jnewbery commented Sep 4, 2017

kallewoof commented Sep 5, 2017

jnewbery commented Sep 6, 2017

maflcko commented Sep 6, 2017

promag commented Sep 6, 2017

[mempool] Mark mempool import fails that were found in mempool as 'already there' #11062

[mempool] Mark mempool import fails that were found in mempool as 'already there' #11062

Conversation

kallewoof commented Aug 16, 2017

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

promag Aug 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa commented Aug 17, 2017

sipa commented Aug 18, 2017

TheBlueMatt commented Aug 20, 2017

maflcko commented Sep 1, 2017

jnewbery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kallewoof commented Sep 4, 2017

jnewbery commented Sep 4, 2017

kallewoof commented Sep 5, 2017

jnewbery commented Sep 6, 2017

maflcko commented Sep 6, 2017

promag commented Sep 6, 2017

promag Aug 18, 2017 •

edited