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

kallewoof commented at 8:25 AM on August 16, 2017: member

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).

fanquake added the label Mempool on Aug 16, 2017

in src/validation.cpp:4317 in a0b6e94df1 outdated

4303 | @@ -4303,7 +4304,15 @@ bool LoadMempool(void)
4304 |                  if (state.IsValid()) {
4305 |                      ++count;
4306 |                  } else {
4307 | -                    ++failed;
4308 | +                    // mempool may contain the transaction already, e.g. from
4309 | +                    // wallet(s) having loaded it while we were processing
4310 | +                    // mempool transactions; consider these as valid, instead of
4311 | +                    // failed, but mark them as 'already existed'
4312 | +                    if (mempool.exists(tx->GetHash())) {

promag commented at 2:49 PM on August 17, 2017:

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

Also, IMO this is prettier:

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

promag commented at 2:56 PM on August 17, 2017:

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

sipa commented at 11:28 PM on August 17, 2017:

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

promag commented at 11:35 PM on August 17, 2017:

What about #10503?

promag commented at 11:50 PM on August 17, 2017:

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") {

sipa commented at 11:56 PM on August 17, 2017:

That's pretty ugly. There's no good way to distinuish replay from double spend... for almost everything the distinction doesn't matter.

promag commented at 12:24 AM on August 18, 2017:

Got it. So in this case if (mempool.exists(tx->GetHash()) can come before AcceptToMemoryPoolWithTime?

kallewoof commented at 12:37 AM on August 18, 2017:

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.

promag commented at 12:45 AM on August 18, 2017:

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.

kallewoof commented at 12:50 AM on August 18, 2017:

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.

promag commented at 12:57 AM on August 18, 2017:

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

kallewoof commented at 1:59 AM on August 18, 2017:

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

in src/validation.cpp:4277 in a0b6e94df1 outdated

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

promag commented at 2:57 PM on August 17, 2017:

Maybe just increment skipped?

kallewoof commented at 12:36 AM on August 18, 2017:

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

promag commented at 1:03 AM on August 18, 2017:

Rigth, maybe rename skipped to expired.

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

kallewoof commented at 2:05 AM on August 18, 2017:

Good point. Done.

promag commented at 3:00 PM on August 17, 2017: member

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

sipa commented at 11:39 PM on August 17, 2017: member

Yes, what about it?

kallewoof force-pushed on Aug 18, 2017

sipa commented at 5:39 AM on August 18, 2017: member

utACK

TheBlueMatt commented at 9:05 PM on August 20, 2017: member

utACK 8c8ef83edc8724f2dc7e363e9c412c5df564e08f

MarcoFalke commented at 12:30 PM on September 1, 2017: member

utACK 8c8ef83

in src/validation.cpp:4277 in 8c8ef83edc outdated

4271 | @@ -4272,8 +4272,9 @@ bool LoadMempool(void)
4272 |      }
4273 |  
4274 |      int64_t count = 0;
4275 | -    int64_t skipped = 0;
4276 | +    int64_t expired = 0;
4277 |      int64_t failed = 0;
4278 | +    int64_t alreadythere = 0;

jnewbery commented at 7:15 PM on September 1, 2017:

nit: snake case already_there

promag commented at 9:46 PM on September 1, 2017:

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

kallewoof commented at 5:49 AM on September 4, 2017:

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

in src/validation.cpp:4310 in 8c8ef83edc outdated

4303 | @@ -4303,10 +4304,18 @@ bool LoadMempool(void)
4304 |                  if (state.IsValid()) {
4305 |                      ++count;
4306 |                  } else {
4307 | -                    ++failed;
4308 | +                    // mempool may contain the transaction already, e.g. from
4309 | +                    // wallet(s) having loaded it while we were processing
4310 | +                    // mempool transactions; consider these as valid, instead of
4311 | +                    // failed, but mark them as 'already existed'

jnewbery commented at 7:16 PM on September 1, 2017:

nit: mark them as 'already there'

jnewbery commented at 7:18 PM on September 1, 2017: member

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.

[mempool] Mark unaccepted txs present in mempool 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.

258d33b41a

kallewoof force-pushed on Sep 4, 2017

kallewoof commented at 5:48 AM on September 4, 2017: member

@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 commented at 12:01 PM on September 4, 2017: member

already_there seems fine to me. Will retest later.

kallewoof commented at 1:16 AM on September 5, 2017: member

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

jnewbery commented at 12:15 AM on September 6, 2017: member

Tested ACK 258d33b41a27917f59e3aee856d032a23cbb5b05

MarcoFalke commented at 12:27 AM on September 6, 2017: member

re-utACK 258d33b

promag commented at 12:31 AM on September 6, 2017: member

utACK 258d33b.

sipa merged this on Oct 18, 2017

sipa closed this on Oct 18, 2017

sipa referenced this in commit 26fee4f6bd on Oct 18, 2017

kallewoof deleted the branch on Oct 18, 2017

ptschip referenced this in commit d2a40c0a60 on Oct 22, 2019

PastaPastaPasta referenced this in commit 7d3e10e0c8 on Dec 22, 2019

jonspock referenced this in commit 0d57edef41 on Dec 27, 2019

jonspock referenced this in commit fa93f1badf on Dec 29, 2019

PastaPastaPasta referenced this in commit 3b223ebe71 on Jan 2, 2020

PastaPastaPasta referenced this in commit e56d5f1931 on Jan 4, 2020

PastaPastaPasta referenced this in commit bd1e474c0e on Jan 12, 2020

PastaPastaPasta referenced this in commit 0bd9f73d22 on Jan 12, 2020

PastaPastaPasta referenced this in commit 7b6d7e6d76 on Jan 12, 2020

PastaPastaPasta referenced this in commit e1575f93dc on Jan 12, 2020

PastaPastaPasta referenced this in commit 6c1932595e on Jan 12, 2020

PastaPastaPasta referenced this in commit 5bb960db9e on Jan 12, 2020

PastaPastaPasta referenced this in commit 4cdb8f143c on Jan 16, 2020

jonspock referenced this in commit ea8f9b2614 on Oct 1, 2020

jonspock referenced this in commit ebfc328004 on Oct 1, 2020

jonspock referenced this in commit 69670877e6 on Oct 5, 2020

jonspock referenced this in commit 2d2beb680d on Oct 10, 2020

ckti referenced this in commit 8c0ce29bce on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021