diff -uNr a/bitcoin/manifest b/bitcoin/manifest --- a/bitcoin/manifest 5565b5a7778455f1064f17374339e51aa09e1cbea749b59d6dbd6a624ea0e22e4f149de2d6311e56a8a322b383fd6e6e4cefdef1504226979916d9a63603028d +++ b/bitcoin/manifest 4771feddc73ba3a542b7a93dbd95942d742205b3ee99e0a651a16698b3dc6e3bea6e746fd806667e4e77603473f8a938b6caa9c532100a6360b26bd64f49393f @@ -54,3 +54,4 @@ 833339 bitcoin_db_shutdown_checkpoint_calming jfw Remove various frantic, overdone database-related activities, mostly in the shutdown path where they were doing more harm than good. The goal is to make shutdown fast and somewhat easier to analyze. Checkpointing comes up because it's no longer necessarily done at shutdown. The conditions for periodic checkpoints are simplified; in particular, it's no longer done ~every time a db handle object goes out of scope. Given the use of DB_TXN_WRITE_NOSYNC this means update durability with respect to system crash is somewhat relaxed, but this seems fine given the nature of the application. The size limit on db transaction log files is raised 10x, restoring its earlier value. 835694 bitcoin_drop_bdb_locking jfw Turn off BDB's page-level locking subsystem, with some tweaks to firm up the already largely single-threaded pattern of database use. This allows removing the magic-number limits on locks, lockers and lock objects; the numbers were reported by past authority to be sufficient for any 1MB block, but have proven insufficient for large reorganizations, because BDB accumulates locks for the duration of a transaction. It also saves nearly 2GB of preallocations in the database environment (__db.00[1-5] files) to support the large limits. 835698 bitcoin_easy_warning_fixes jfw Quiet the vast majority of compiler warning spew so any actual trouble stands a chance of being spotted. Continue incremental conversion from CRITICAL_BLOCK to SCOPED_LOCK, as that and BOOST_FOREACH are the main sources of spurious -Wparentheses due to the macros' internal if-statements. Besides the more cosmetic changes, this cures a reckless stack buffer allocation (200k, when truncating the debug log) and a potential integer overflow (in string formatting code). Related string handling routines still need attention though. +835709 bitcoin_reorg_bounded_space jfw Remove linear heap allocation by Reorganize, at the "cost" of rereading the affected blocks from disk after database commit to update the mempool. As usual, even the smallest of fixes uncovers other landmines: resurrected transactions were not checked for minimum relay fee; TxnAbort was doubly invoked in the case of ConnectBlock failure; invalid blocks were not distinguished from db failure - and still aren't, but the warning text is finessed (again) so as not to lie outright. The mempool itself remains unbounded, though at least now the minimum fee upholds a cost for entering it. diff -uNr a/bitcoin/src/main.cpp b/bitcoin/src/main.cpp --- a/bitcoin/src/main.cpp 45474609f59cfd70e2f3191c8bdabb57f23f5ef3f63926190016b1cd8777f2609f91df5caa70c7f9dc715def1373f6690d4024cad93a08d6b2b531d3fef48346 +++ b/bitcoin/src/main.cpp 5d2369099187febaa86bd7e760543be6d6b8846123285646407f03a22a068d0b205b988bfdd146b80b7752edef00f7416fd804bd0d7c0bbb4c39bcd4a1a09941 @@ -720,7 +720,7 @@ printf("InvalidChainFound: current best=%s height=%d work=%s\n", hashBestChain.ToString().c_str(), nBestHeight, bnBestChainWork.ToString().c_str()); if (pindexBest && bnBestInvalidWork > bnBestChainWork + pindexBest->GetBlockWork() * 6) // message partially duplicated in GetWarnings - printf("InvalidChainFound: WARNING: Received invalid chain with greater proof-of-work. Displayed transactions may not be correct! Your database or other nodes may be corrupted.\n"); + printf("InvalidChainFound: WARNING: Failed to validate chain with greater proof-of-work. Displayed transactions may not be correct! Your database or other nodes may be corrupted.\n"); } @@ -1039,6 +1039,23 @@ return true; } +bool static ReorgFailAbort(CTxDB& txdb, char const *msg) +{ + error("Reorganize() : %s", msg); + txdb.TxnAbort(); + return false; +} + +bool static ReorgFailLate() +{ + error("Reorganize() : system failure while updating memory pool after db commit ; clearing pool so as not to leave it inconsistent"); + SCOPED_LOCK(cs_mapTransactions); + mapTransactions.clear(); + mapNextTx.clear(); + nTransactionsUpdated++; + return false; +} + bool static Reorganize(CTxDB& txdb, CBlockIndex* pindexNew) { printf("REORGANIZE\n"); @@ -1050,11 +1067,11 @@ { while (plonger->nHeight > pfork->nHeight) if (!(plonger = plonger->pprev)) - return error("Reorganize() : plonger->pprev is null"); + return ReorgFailAbort(txdb, "plonger->pprev is null"); if (pfork == plonger) break; if (!(pfork = pfork->pprev)) - return error("Reorganize() : pfork->pprev is null"); + return ReorgFailAbort(txdb, "pfork->pprev is null"); } printf("Reorganize() : fork height=%d with %d to disconnect, %d to connect\n", @@ -1074,47 +1091,35 @@ reverse(vConnect.begin(), vConnect.end()); // Disconnect shorter branch - vector vResurrect; BOOST_FOREACH(CBlockIndex* pindex, vDisconnect) { CBlock block; if (!block.ReadFromDisk(pindex)) - return error("Reorganize() : ReadFromDisk for disconnect failed"); + return ReorgFailAbort(txdb, "ReadFromDisk for disconnect failed"); if (!block.DisconnectBlock(txdb, pindex)) - return error("Reorganize() : DisconnectBlock failed"); - - // Queue memory transactions to resurrect - BOOST_FOREACH(const CTransaction& tx, block.vtx) - if (!tx.IsCoinBase()) - vResurrect.push_back(tx); + return ReorgFailAbort(txdb, "DisconnectBlock failed"); } // Connect longer branch - vector vDelete; for (int i = 0; i < vConnect.size(); i++) { CBlockIndex* pindex = vConnect[i]; CBlock block; if (!block.ReadFromDisk(pindex)) - return error("Reorganize() : ReadFromDisk for connect failed"); + return ReorgFailAbort(txdb, "ReadFromDisk for connect failed"); if (!block.ConnectBlock(txdb, pindex)) { - // Invalid block - txdb.TxnAbort(); - return error("Reorganize() : ConnectBlock failed"); + // Invalid block (OR database failure...) + return ReorgFailAbort(txdb, "ConnectBlock failed"); } - - // Queue memory transactions to delete - BOOST_FOREACH(const CTransaction& tx, block.vtx) - vDelete.push_back(tx); } if (!txdb.WriteHashBestChain(pindexNew->GetBlockHash())) - return error("Reorganize() : WriteHashBestChain failed"); + return ReorgFailAbort(txdb, "WriteHashBestChain failed"); // Make sure it's successfully written to disk before changing memory structure printf("Reorganize() : successfully connected longer branch; committing now\n"); if (!txdb.TxnCommit()) - return error("Reorganize() : TxnCommit failed"); + return ReorgFailAbort(txdb, "TxnCommit failed"); printf("Reorganize() : updating memory block index\n"); @@ -1131,12 +1136,28 @@ printf("Reorganize() : updating memory pool transactions\n"); // Resurrect memory transactions that were in the disconnected branch - BOOST_FOREACH(CTransaction& tx, vResurrect) - tx.AcceptToMemoryPool(txdb, false); + BOOST_FOREACH(CBlockIndex* pindex, vDisconnect) + { + CBlock block; + if (!block.ReadFromDisk(pindex)) + return ReorgFailLate(); + BOOST_FOREACH(CTransaction& tx, block.vtx) + { + if (!tx.IsCoinBase()) + // XXX using fCheckInputs=true repeats more expensive checks that were already done when accepting the block, but is currently required to derive the fee and thus enforce mempool entry requirements + tx.AcceptToMemoryPool(txdb, true); + } + } // Delete redundant memory transactions that are in the connected branch - BOOST_FOREACH(CTransaction& tx, vDelete) - tx.RemoveFromMemoryPool(); + BOOST_FOREACH(CBlockIndex* pindex, vConnect) + { + CBlock block; + if (!block.ReadFromDisk(pindex)) + return ReorgFailLate(); + BOOST_FOREACH(CTransaction& tx, block.vtx) + tx.RemoveFromMemoryPool(); + } return true; } @@ -1178,7 +1199,8 @@ // New best branch if (!Reorganize(txdb, pindexNew)) { - txdb.TxnAbort(); + // Reorganize() is responsible for either committing or aborting; a false return doesn't necessarily mean it didn't commit. + // XXX it also doesn't mean an invalid chain was positively found; e.g. it could be a database I/O error. But this confusion applies fractally and recursively, so at this point InvalidChainFound is the only way to signal the gravity of the situation. InvalidChainFound(pindexNew); return error("SetBestChain() : Reorganize failed"); } @@ -1637,10 +1659,10 @@ if (strMiscWarning != "") strStatusBar = strMiscWarning; - // Longer invalid proof-of-work chain + // Longer proof-of-work chain that failed validation if (pindexBest && bnBestInvalidWork > bnBestChainWork + pindexBest->GetBlockWork() * 6) // message partially duplicated in InvalidChainFound - strStatusBar = strRPC = "WARNING: Received invalid chain with greater proof-of-work. Displayed transactions may not be correct! Your database or other nodes may be corrupted."; + strStatusBar = strRPC = "WARNING: Failed to validate chain with greater proof-of-work. Displayed transactions may not be correct! Your database or other nodes may be corrupted."; if (strFor == "statusbar") return strStatusBar;