From d356985279f7bf705d3d28f96873fe8370d0ee21 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 22 May 2014 23:33:02 +0200 Subject: [PATCH] Reference counting fixes for Trie. --- alethzero/MainWin.cpp | 2 +- libethcore/TrieDB.cpp | 28 ++++++++++++++++------ libethcore/TrieDB.h | 33 +++++++++++++++++++------ libethereum/State.cpp | 56 +++++++++++++++++++++++++++++++++++-------- 4 files changed, 94 insertions(+), 25 deletions(-) diff --git a/alethzero/MainWin.cpp b/alethzero/MainWin.cpp index 75de6f76f..11049b44b 100644 --- a/alethzero/MainWin.cpp +++ b/alethzero/MainWin.cpp @@ -1053,7 +1053,7 @@ void Main::on_send_clicked() u256 totalReq = value() + fee(); eth::ClientGuard l(&*m_client); for (auto i: m_myKeys) - if (m_client->state().balance(i.address()) >= totalReq) + if (m_client->postState().balance(i.address()) >= totalReq) { m_client->unlock(); Secret s = i.secret(); diff --git a/libethcore/TrieDB.cpp b/libethcore/TrieDB.cpp index a4297baf2..5418ff04f 100644 --- a/libethcore/TrieDB.cpp +++ b/libethcore/TrieDB.cpp @@ -24,16 +24,23 @@ using namespace std; using namespace eth; +#define tdebug ndebug + namespace eth { const h256 c_shaNull = sha3(rlp("")); -std::string BasicMap::lookup(h256 _h, bool _enforceRefs) const +std::string BasicMap::lookup(h256 _h) const { auto it = m_over.find(_h); - if (it != m_over.end() && (!_enforceRefs || (m_refCount.count(it->first) && m_refCount.at(it->first)))) - return it->second; + if (it != m_over.end()) + { + if (!m_enforceRefs || (m_refCount.count(it->first) && m_refCount.at(it->first))) + return it->second; + else if (m_enforceRefs && m_refCount.count(it->first) && !m_refCount.at(it->first)) + cwarn << "XXX Lookup required for value with no refs:" << _h.abridged(); + } return std::string(); } @@ -41,12 +48,19 @@ void BasicMap::insert(h256 _h, bytesConstRef _v) { m_over[_h] = _v.toString(); m_refCount[_h]++; + tdebug << "INST" << _h.abridged() << "=>" << m_refCount[_h]; } void BasicMap::kill(h256 _h) { - if (m_refCount[_h] > 0) - --m_refCount[_h]; + if (m_refCount.count(_h)) + { + if (m_refCount[_h] > 0) + --m_refCount[_h]; + else + cwarn << "Decreasing DB node ref count below zero. Probably have a corrupt Trie."; + } + tdebug << "KILL" << _h.abridged() << "=>" << m_refCount[_h]; } void BasicMap::purge() @@ -91,9 +105,9 @@ void Overlay::rollback() m_refCount.clear(); } -std::string Overlay::lookup(h256 _h, bool _enforceRefs) const +std::string Overlay::lookup(h256 _h) const { - std::string ret = BasicMap::lookup(_h, _enforceRefs); + std::string ret = BasicMap::lookup(_h); if (ret.empty() && m_db) m_db->Get(m_readOptions, ldb::Slice((char const*)_h.data(), 32), &ret); return ret; diff --git a/libethcore/TrieDB.h b/libethcore/TrieDB.h index dd8a36fd5..b5179b698 100644 --- a/libethcore/TrieDB.h +++ b/libethcore/TrieDB.h @@ -28,18 +28,22 @@ #include "TrieCommon.h" namespace ldb = leveldb; +#define tdebug ndebug + namespace eth { class BasicMap { + friend class EnforceRefs; + public: BasicMap() {} void clear() { m_over.clear(); } std::map const& get() const { return m_over; } - std::string lookup(h256 _h, bool _enforceRefs = false) const; + std::string lookup(h256 _h) const; void insert(h256 _h, bytesConstRef _v); void kill(h256 _h); void purge(); @@ -47,6 +51,8 @@ public: protected: std::map m_over; std::map m_refCount; + + bool m_enforceRefs = false; }; inline std::ostream& operator<<(std::ostream& _out, BasicMap const& _m) @@ -62,6 +68,7 @@ inline std::ostream& operator<<(std::ostream& _out, BasicMap const& _m) } class InvalidTrie: public std::exception {}; +class EnforceRefs; class Overlay: public BasicMap { @@ -75,7 +82,7 @@ public: void commit(); void rollback(); - std::string lookup(h256 _h, bool _enforceRefs = false) const; + std::string lookup(h256 _h) const; private: using BasicMap::clear; @@ -86,6 +93,17 @@ private: ldb::WriteOptions m_writeOptions; }; +class EnforceRefs +{ +public: + EnforceRefs(BasicMap& _o, bool _r): m_o(_o), m_r(_o.m_enforceRefs) { _o.m_enforceRefs = _r; } + ~EnforceRefs() { m_o.m_enforceRefs = m_r; } + +private: + BasicMap& m_o; + bool m_r; +}; + extern const h256 c_shaNull; /** @@ -419,6 +437,8 @@ template void GenericTrieDB::init() template void GenericTrieDB::insert(bytesConstRef _key, bytesConstRef _value) { + cdebug << "Insert" << toHex(_key.cropped(0, 4)); + std::string rv = node(m_root); assert(rv.size()); bytes b = mergeAt(RLP(rv), NibbleSlice(_key), _value); @@ -469,7 +489,7 @@ template std::string GenericTrieDB::atAux(RLP const& _here, Nibbl template bytes GenericTrieDB::mergeAt(RLP const& _orig, NibbleSlice _k, bytesConstRef _v) { -// ::operator<<(std::cout << "mergeAt ", _orig) << _k << _v.toString() << std::endl; + tdebug << "mergeAt " << _orig << _k << sha3(_orig.data()).abridged(); // The caller will make sure that the bytes are inserted properly. // - This might mean inserting an entry into m_over @@ -534,6 +554,7 @@ template bytes GenericTrieDB::mergeAt(RLP const& _orig, NibbleSli template void GenericTrieDB::mergeAtAux(RLPStream& _out, RLP const& _orig, NibbleSlice _k, bytesConstRef _v) { + tdebug << "mergeAtAux " << _orig << _k << sha3(_orig.data()).abridged() << _orig.toHash().abridged(); RLP r = _orig; std::string s; if (!r.isList() && !r.isEmpty()) @@ -541,17 +562,15 @@ template void GenericTrieDB::mergeAtAux(RLPStream& _out, RLP cons s = node(_orig.toHash()); r = RLP(s); assert(!r.isNull()); - killNode(_orig.toHash()); } - else - killNode(_orig); bytes b = mergeAt(r, _k, _v); -// ::operator<<(std::cout, RLP(b)) << std::endl; streamNode(_out, b); } template void GenericTrieDB::remove(bytesConstRef _key) { + tdebug << "Remove" << toHex(_key.cropped(0, 4).toBytes()); + std::string rv = node(m_root); bytes b = deleteAt(RLP(rv), NibbleSlice(_key)); if (b.size()) diff --git a/libethereum/State.cpp b/libethereum/State.cpp index 13e9da421..a6becb274 100644 --- a/libethereum/State.cpp +++ b/libethereum/State.cpp @@ -551,9 +551,21 @@ u256 State::playbackRaw(bytesConstRef _block, BlockInfo const& _grandParent, boo if (_fullCommit) { + if (!isTrieGood()) + { + cwarn << "INVALID TRIE prior to database commit!"; + throw InvalidTrie(); + } + // Commit the new trie to disk. m_db.commit(); + if (!isTrieGood()) + { + cwarn << "INVALID TRIE immediately after database commit!"; + throw InvalidTrie(); + } + m_previousBlock = m_currentBlock; } else @@ -850,20 +862,41 @@ bytes const& State::code(Address _contract) const bool State::isTrieGood() { - for (auto const& i: m_state) { - RLP r(i.second); - TrieDB storageDB(&m_db, r[2].toHash()); - try + EnforceRefs r(m_db, false); + for (auto const& i: m_state) { - for (auto const& j: storageDB) {} + RLP r(i.second); + TrieDB storageDB(&m_db, r[2].toHash()); + try + { + for (auto const& j: storageDB) { (void)j; } + } + catch (InvalidTrie) + { + cwarn << "BAD TRIE [unenforced refs]"; + return false; + } + if (r[3].toHash() != EmptySHA3 && m_db.lookup(r[3].toHash()).empty()) + return false; } - catch (InvalidTrie) + } + { + EnforceRefs r(m_db, true); + for (auto const& i: m_state) { - return false; + RLP r(i.second); + TrieDB storageDB(&m_db, r[2].toHash()); + try + { + for (auto const& j: storageDB) { (void)j; } + } + catch (InvalidTrie) + { + cwarn << "BAD TRIE [enforced refs]"; + return false; + } } - if (r[3].toHash() != EmptySHA3 && m_db.lookup(r[3].toHash()).empty()) - return false; } return true; } @@ -899,7 +932,10 @@ u256 State::execute(bytesConstRef _rlp) commit(); if (e.t().receiveAddress) - assert(!storageRoot(e.t().receiveAddress) || m_db.lookup(storageRoot(e.t().receiveAddress), true).size()); + { + EnforceRefs r(m_db, true); + assert(!storageRoot(e.t().receiveAddress) || m_db.lookup(storageRoot(e.t().receiveAddress)).size()); + } cnote << "Executed; now" << rootHash(); cnote << old.diff(*this);