Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Staking wallets crash; debugging output shows Error:'Assertion failed: lock cs_main not held in wallet.cpp:4261; locks held:' #34

Closed
Kokary opened this issue Mar 12, 2018 · 3 comments

Comments

@Kokary
Copy link
Contributor

Kokary commented Mar 12, 2018

Issue: assert failed

Error:'Assertion failed: lock cs_main not held in wallet.cpp:4261; locks held:'

wallet.cpp:4261:

int CMerkleTx::GetDepthInMainChainINTERNAL(const CBlockIndex*& pindexRet) const
{
    if (hashBlock == 0 || nIndex == -1)
        return 0;
    AssertLockHeld(cs_main);

which is called from two places:

1: wallet.cpp: 4285

int CMerkleTx::GetDepthInMainChain(const CBlockIndex*& pindexRet, bool enableIX) const
{
    AssertLockHeld(cs_main);
    int nResult = GetDepthInMainChainINTERNAL(pindexRet);

Here, the call is preceded with a check for the lock.

2: wallet.h:761

The definition of CMerkleTx:

    bool IsInMainChain() const
    {
        const CBlockIndex* pindexRet;
        return GetDepthInMainChainINTERNAL(pindexRet) > 0;
    }

Here, an assertion check is missing.
It is called from 8 places (related to qt transactions and wallet transactions), all without assertions.

Analysis

TransactionRecord::updateStatus() checks for a lock on cs_main.

The lock is set on:
TransactionRecord* index(int idx)

Trace

(gdb) backtrace
#0 AssertLockHeldInternal (pszName=0x5555560d36a1 "cs_main", pszFile=0x5555560d338d "wallet.cpp", nLine=4261, cs=0x55555663a360 <cs_main>) at sync.cpp:148
#1 0x0000555555bcbadb in CMerkleTx::GetDepthInMainChainINTERNAL (this=0x55556cd6ca30, pindexRet=@0x7fffc37fcd10: 0x5aa42c20) at wallet.cpp:4261
#2 0x0000555555bd4ee8 in CMerkleTx::IsInMainChain (this=0x55556cd6ca30) at wallet.h:761
#3 0x0000555555bba220 in CWallet::SelectStakeCoins (this=0x55556cd4ebc0, setCoins=std::set with 1 elements = {...}, nTargetAmount=1697997840) at wallet.cpp:2064
#4 0x0000555555bc0dff in CWallet::CreateCoinStake (this=0x55556cd4ebc0, keystore=..., nBits=503382015, nSearchInterval=0, txNew=..., nTxNewTime=@0x7fffc37fd5c0: 0) at wallet.cpp:2867
#5 0x00005555557f2bfa in CreateNewBlock (scriptPubKeyIn=..., pwallet=0x55556cd4ebc0, fProofOfStake=true) at miner.cpp:143
#6 0x00005555557f51bb in CreateNewBlockWithKey (reservekey=..., pwallet=0x55556cd4ebc0, fProofOfStake=true) at miner.cpp:500
#7 0x00005555557f597c in BitcoinMiner (pwallet=0x55556cd4ebc0, fProofOfStake=true) at miner.cpp:596

3: No changes to PIVX; bitcois is different.
4: No changes to PIVX
5: No changes to PIVX; line 177: LOCK2(cs_main, mempool.cs);
6: No changes to PIVX
7: No changes to PIVX

Bitcoin and IsInMainChain()

PivX introduced functions that call IsInMainChain() in addition to the functions inherited from Bitcoin. The three new functions below do not lock cs_main and aren't called from functions that lock cs_main (while the others do).

New as compared to bitcoin code:
wallet.cpp:938

int64_t CWalletTx::GetComputedTxTime() const
{
    if (IsZerocoinSpend() || IsZerocoinMint()) {
        if (IsInMainChain())
            return mapBlockIndex.at(hashBlock)->GetBlockTime();
        else
            return nTimeReceived;
    }
    return GetTxTime();
}

wallet.cpp:2050

bool CWallet::SelectStakeCoins(std::set<std::pair<const CWalletTx*, unsigned int> >& setCoins, CAmount nTargetAmount) const
{
    vector<COutput> vCoins;
    AvailableCoins(vCoins, true, NULL, false, STAKABLE_COINS);
    CAmount nAmountSelected = 0;

    for (const COutput& out : vCoins) {
        //make sure not to outrun target amount
        if (nAmountSelected + out.tx->vout[out.i].nValue > nTargetAmount)
            continue;

        //if zerocoinspend, then use the block time
        int64_t nTxTime = out.tx->GetTxTime();
        if (out.tx->IsZerocoinSpend()) {
            if (!out.tx->IsInMainChain())
                continue;
            nTxTime = mapBlockIndex.at(out.tx->hashBlock)->GetBlockTime();
        }

wallet.cpp:2084

bool CWallet::MintableCoins()
{
    CAmount nBalance = GetBalance();
    if (mapArgs.count("-reservebalance") && !ParseMoney(mapArgs["-reservebalance"], nReserveBalance))
        return error("MintableCoins() : invalid reserve balance amount");
    if (nBalance <= nReserveBalance)
        return false;

    vector<COutput> vCoins;
    AvailableCoins(vCoins, true);

    for (const COutput& out : vCoins) {
        int64_t nTxTime = out.tx->GetTxTime();
        if (out.tx->IsZerocoinSpend()) {
            if (!out.tx->IsInMainChain())
                continue;
            nTxTime = mapBlockIndex.at(out.tx->hashBlock)->GetBlockTime();
        }

        if (GetAdjustedTime() - nTxTime > nStakeMinAge)
            return true;
    }

    return false;
}

Locking of other functions that call IsInMainChain() e.g. GetImmatureCredit():
wallet.cpp:1830

CAmount CWallet::GetImmatureBalance() const
{
    CAmount nTotal = 0;
    {
        LOCK2(cs_main, cs_wallet);
        for (map<uint256, CWalletTx>::const_iterator it = mapWallet.begin(); it != mapWallet.end(); ++it) {
            const CWalletTx* pcoin = &(*it).second;
            nTotal += pcoin->GetImmatureCredit();
        }
    }
    return nTotal;
}

Adding locks

  • GetComputedTxTime() is only called by decomposeTransaction(), which is in a locked section. No need for additional locking.
  • SelectStakeCoins() is called from CreateCoinStake(), which doesn't set locks.
  • SelectStakeCoins() has a loop that needs a lock. This critical section follows on a call to AvailableCoins, which locks cs_main and cs_wallet.
  • MintableCoins() is called from the stake miner, where it isn't locked. Lock can be set within MintableCoins().
  • MintableCoins() is also called from getstakingstatus(), where both cs_main and potentially pwalletMain->cs_wallet are locked
  • MintableCoins() has a loop that needs a lock. This critical section follows on a call to AvailableCoins, which locks cs_main and cs_wallet.

Bitcoin's analysis

laanwj writes about a similar crash:
bitcoin/bitcoin#3997 (comment)
But more likely still is that there is a small time at which chainActive is invalid. I wonder if we acquire the right mutexes there.

This explains why the logfile at crash-related places also shows entries of new blocks being rejected because nHeight is wrong.

Kokary pushed a commit that referenced this issue Mar 12, 2018
…tDepthInMainChainINTERNAL(); SelectStakeCoins() and MintableCoins() aren't locked elsewhere, GetComputedTxTime() is called from a function that already locks cs_main.
@WagerrTor WagerrTor added Bug Patch Fix/Patch labels Mar 12, 2018
Kokary added a commit that referenced this issue Mar 12, 2018
…tDepthInMainChainINTERNAL(); SelectStakeCoins() and MintableCoins() aren't locked elsewhere, GetComputedTxTime() is called from a function that already locks cs_main.
@Kokary
Copy link
Contributor Author

Kokary commented Mar 12, 2018

Issue is resolved by this commit, without measurable time spent locking/unlocking mutexes.

Remaining questions:

  • Are there other relevant locks?
  • Is the lock place or should it move higher/lower in the call stack?

@WagerrTor
Copy link
Contributor

Does this issue still persist or can we close @Kokary ?

Review & apply PIVX fixes/patches from fork to 28. Feb. 2018 automation moved this from In progress to Done Aug 15, 2018
@DingGeng-GitHub
Copy link

错误:

2019-02-26T08:18:21Z POTENTIAL DEADLOCK DETECTED
2019-02-26T08:18:21Z Previous lock order was:
2019-02-26T08:18:21Z cs_main net_processing.cpp:2462
2019-02-26T08:18:21Z (1) pool.cs validation.cpp:398
2019-02-26T08:18:21Z cs_main wallet/wallet.cpp:1279
2019-02-26T08:18:21Z (2) cs_wallet wallet/wallet.cpp:1279
2019-02-26T08:18:21Z Current lock order is:
2019-02-26T08:18:21Z cs_main wallet/wallet.cpp:1800
2019-02-26T08:18:21Z (2) cs_wallet wallet/wallet.cpp:1800
2019-02-26T08:18:21Z (1) g_mempool.cs wallet/wallet.cpp:1821

环境:Ubuntu16.04
Bitcoin ABC RPC client version v0.19.0.0-e619210

只要启动环境就报上面的错,请问该怎么解决?QQ:707170913
解决有红包

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Patch Fix/Patch
Projects
No open projects
Development

No branches or pull requests

6 participants