I'm wondering whether we need these locks while trying to stake

[dooglus]

In wallet.cpp, inside the loop that runs through all staking outputs looking for opportunities to stake a new block, we see:

    CTxIndex txindex;
    {
-->     LOCK2(cs_main, cs_wallet);
        if (!txdb.ReadTxIndex(pcoin.first->hash, txindex)) {
            LogPrint("stake", "[STAKE] skip %s:%-3d (%s CLAM) - can't read txindex\n",
                      pcoin.first->hash.ToString(), pcoin.second, FormatMoney(pcoin.first->vout[pcoin.second].nValue));
            continue;
        }
    }

    // Read block header
    CBlock block;
    {
-->     LOCK2(cs_main, cs_wallet);
        if (!block.ReadFromDisk(txindex.pos.nFile, txindex.pos.nBlockPos, false)) {
            LogPrint("stake", "[STAKE] skip %s:%-3d (%s CLAM) - can't read block\n",
                      pcoin.first->hash.ToString(), pcoin.second, FormatMoney(pcoin.first->vout[pcoin.second].nValue));
            continue;
        }
    }

The two LOCK2() calls are causing staking to hang up quite regularly, as the wallet is doing other things at the same time.

I am wondering whether we need to obtain locks before doing read-only operations on the wallet. I've tried commenting them both out, and things seem to be working fine (and staking no longer hangs for minutes at a time). But am I missing something? What's the worst that can happen if the txdb changes just as I'm trying to read it?

[tryphe]

In Qt, this sort of situation works fine without lockers/mutexes. I'm sure boost is the same. Not sure why that's there at all unless it also locks the db from being written? Alternatively, you could lock the db during write operations elsewhere, and then remove those locks and use a semaphore. In Qt this is expressed as semaphores and read/write mutexes but I'm sure boost has something similar yet completely differently worded.

[l0rdicon]

Removing those locks should be fine. It is only reading.

There's also plenty of usage in the code that implements ReadTxIndex and ReadFromDisk without applying any locks.

I'm compiling now will test if for the evening but the change looks good and should be good to push to master

[creativecuriosity]

dooglus,

Have you been running this live since this conversation? If so, can we get it merged and closed?

[dooglus]

No, I took the locks out for a while, but I see they're back in place now. I don't know how long I tested without them since I didn't intentionally put them back in place.

[creativecuriosity]

@dooglus, but I see they're back in place now.

Update? Do we have solid evidence that this is a wise update?