From d54fb5e7297f9a3c7efb301e3baddd888fee936b Mon Sep 17 00:00:00 2001 From: jamescowens Date: Fri, 12 Mar 2021 17:40:47 -0500 Subject: [PATCH 1/4] Fix a subtle condition in ApplyContracts causing sync from zero errors This fixes a problem in ApplyContracts where the 2nd and succeeding beacon contracts in the same block could fail to be inserted into the beacon database causing the wallet to block on stakes involving the affected CPID(s). --- src/gridcoin/beacon.cpp | 33 ++++++++++++++++++++++++------ src/gridcoin/beacon.h | 2 +- src/gridcoin/contract/contract.cpp | 15 +++++++++++--- src/init.cpp | 4 ---- 4 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/gridcoin/beacon.cpp b/src/gridcoin/beacon.cpp index f359a6856e..35451ce8ed 100644 --- a/src/gridcoin/beacon.cpp +++ b/src/gridcoin/beacon.cpp @@ -369,8 +369,9 @@ bool BeaconRegistry::TryRenewal(Beacon_ptr& current_beacon_ptr, int& height, con // Put the renewal beacon into the db. if (!m_beacon_db.insert(renewal.m_hash, height, renewal)) { - LogPrint(LogFlags::BEACON, "WARNING: %s: In renewal of beacon for cpid %s, address %s, hash %s, beacon db record " - "already exists.", + LogPrint(LogFlags::BEACON, "INFO: %s: In renewal of beacon for cpid %s, address %s, hash %s, beacon db record " + "already exists. This can be expected on a restart of the wallet to ensure multiple " + "contracts in the same block get stored/replayed.", __func__, renewal.m_cpid.ToString(), renewal.GetAddress().ToString(), @@ -435,7 +436,16 @@ void BeaconRegistry::Add(const ContractContext& ctx) historical.m_cpid = payload.m_cpid; historical.m_status = BeaconStatusForStorage::ACTIVE; - m_beacon_db.insert(ctx.m_tx.GetHash(), height, historical); + if (!m_beacon_db.insert(ctx.m_tx.GetHash(), height, historical)) + { + LogPrint(LogFlags::BEACON, "INFO: %s: In activation of v1 beacon for cpid %s, address %s, hash %s, beacon db record " + "already exists. This can be expected on a restart of the wallet to ensure multiple " + "contracts in the same block get stored/replayed.", + __func__, + historical.m_cpid.ToString(), + historical.GetAddress().ToString(), + historical.m_hash.GetHex()); + } m_beacons[payload.m_cpid] = std::make_shared(m_beacon_db[ctx.m_tx.GetHash()]); return; } @@ -458,7 +468,16 @@ void BeaconRegistry::Add(const ContractContext& ctx) pending.m_status = BeaconStatusForStorage::PENDING; // Insert the entry into the db. - m_beacon_db.insert(ctx.m_tx.GetHash(), height, static_cast(pending)); + if (!m_beacon_db.insert(ctx.m_tx.GetHash(), height, static_cast(pending))) + { + LogPrint(LogFlags::BEACON, "INFO: %s: In advertisement of beacon for cpid %s, address %s, hash %s, beacon db record " + "already exists. This can be expected on a restart of the wallet to ensure multiple " + "contracts in the same block get stored/replayed.", + __func__, + pending.m_cpid.ToString(), + pending.GetAddress().ToString(), + pending.m_hash.GetHex()); + } // Insert a pointer to the entry in the m_pending map. m_pending[pending.GetId()] = std::make_shared(m_beacon_db[ctx.m_tx.GetHash()]); @@ -1313,11 +1332,12 @@ bool BeaconRegistry::BeaconDB::insert(const uint256 &hash, const int& height, co } else { - LogPrint(LogFlags::BEACON, "INFO %s - store beacon: cpid %s, address %s, timestamp %" PRId64 + LogPrint(LogFlags::BEACON, "INFO %s - store beacon: cpid %s, address %s, height %i, timestamp %" PRId64 ", hash %s, prev_beacon_hash %s, status = %u.", __func__, beacon.m_cpid.ToString(), // cpid beacon.GetAddress().ToString(), // address + height, // height beacon.m_timestamp, // timestamp beacon.m_hash.GetHex(), // transaction hash beacon.m_prev_beacon_hash.GetHex(), // prev beacon transaction hash @@ -1338,12 +1358,13 @@ bool BeaconRegistry::BeaconDB::update(const uint256 &hash, const int& height, co { bool status = false; - LogPrint(LogFlags::BEACON, "INFO %s - store beacon: cpid %s, address %s, timestamp %" PRId64 + LogPrint(LogFlags::BEACON, "INFO %s - store beacon: cpid %s, address %s, height %i, timestamp %" PRId64 ", hash %s, prev_beacon_hash %s, status = %u.", __func__, beacon.m_cpid.ToString(), // cpid beacon.GetAddress().ToString(), // address beacon.m_timestamp, // timestamp + height, // height beacon.m_hash.GetHex(), // transaction hash beacon.m_prev_beacon_hash.GetHex(), // prev beacon transaction hash beacon.m_status.Raw() // status diff --git a/src/gridcoin/beacon.h b/src/gridcoin/beacon.h index 62a43b0b10..53058fae40 100644 --- a/src/gridcoin/beacon.h +++ b/src/gridcoin/beacon.h @@ -714,7 +714,7 @@ class BeaconRegistry : public IContractHandler //! will ensure that when the wallet is restarted, the level db beacon storage will be cleared and //! reloaded from the contract replay with the correct lookback scope. //! - static constexpr uint32_t CURRENT_VERSION = 1; + static constexpr uint32_t CURRENT_VERSION = 2; //! //! \brief Initializes the Beacon Registry map structures from the replay of the beacon states stored diff --git a/src/gridcoin/contract/contract.cpp b/src/gridcoin/contract/contract.cpp index e7a592fcbc..0e022ecb44 100644 --- a/src/gridcoin/contract/contract.cpp +++ b/src/gridcoin/contract/contract.cpp @@ -463,7 +463,9 @@ void GRC::ReplayContracts(const CBlockIndex* pindex_end, const CBlockIndex* pind // Only apply activations that have not already been stored/loaded into // the beacon DB. This is at the block level, so we have to be careful here. // If the pindex->nHeight is equal to the beacon_db_height, then the ActivatePending - // has already been replayed. + // has already been replayed for this block and we do not need to call it again for that block. + // BECAUSE ActivatePending is called at the block level. We do not need to worry about multiple + // calls within the same block like below in ApplyContracts. if (pindex->nHeight > beacon_db_height) { GetBeaconRegistry().ActivatePending( @@ -509,8 +511,15 @@ void GRC::ApplyContracts( { for (const auto& contract : tx.GetContracts()) { // Do not (re)apply contracts that have already been stored/loeaded into - // the beacon DB. - if ((pindex->nHeight <= beacon_db_height) && contract.m_type == ContractType::BEACON) + // the beacon DB up to the block BEFORE the beacon db height. Because the beacon + // db height is at the block level, and is updated on each beacon insert, when + // in a sync from zero situation where the contracts are played as each block is validated, + // any beacon contract in the block EQUAL to the beacon db height must pass this test + // and be inserted again, because otherwise the second and succeeding contracts on the + // same block will not be inserted and those CPID's will not be recorded properly. + // This was the cause of the failure to sync through 2069264 that started on 20210312. See + // github issue #2045. + if ((pindex->nHeight < beacon_db_height) && contract.m_type == ContractType::BEACON) { LogPrint(BCLog::LogFlags::CONTRACT, "INFO: %s: ApplyContract tx skipped: " "pindex->height = %i <= beacon_db_height = %i and " diff --git a/src/init.cpp b/src/init.cpp index 8cbc9d8346..5da7b31c2e 100755 --- a/src/init.cpp +++ b/src/init.cpp @@ -397,10 +397,6 @@ void InitLogging() } } - // TODO: enable tally and accrual debug log categories during Fern testing: - LogInstance().EnableCategory("tally"); - LogInstance().EnableCategory("accrual"); - std::vector excluded_categories; if (mapArgs.count("-debugexclude") && mapMultiArgs["-debugexclude"].size() > 0) From b14d3d933a6802386f0dca99e41b2a961ab505f5 Mon Sep 17 00:00:00 2001 From: "James C. Owens" Date: Sat, 13 Mar 2021 16:48:48 -0500 Subject: [PATCH 2/4] Correct block index for missed contract application --- src/gridcoin/beacon.cpp | 86 ++++++++++++++++++++---------- src/gridcoin/beacon.h | 51 ++++++++++++------ src/gridcoin/contract/contract.cpp | 58 +++++++++++++++++--- src/gridcoin/contract/contract.h | 2 +- src/gridcoin/gridcoin.cpp | 2 +- 5 files changed, 146 insertions(+), 53 deletions(-) diff --git a/src/gridcoin/beacon.cpp b/src/gridcoin/beacon.cpp index 35451ce8ed..4d8957ca72 100644 --- a/src/gridcoin/beacon.cpp +++ b/src/gridcoin/beacon.cpp @@ -742,6 +742,16 @@ int BeaconRegistry::GetDBHeight() return height; } +bool BeaconRegistry::NeedsIsContractCorrection() +{ + return m_beacon_db.NeedsIsContractCorrection(); +} + +bool BeaconRegistry::SetNeedsIsContractCorrection(bool flag) +{ + return m_beacon_db.SetNeedsIsContractCorrection(flag); +} + bool BeaconRegistry::Validate(const Contract& contract, const CTransaction& tx) const { if (contract.m_version <= 1) { @@ -1017,6 +1027,7 @@ int BeaconRegistry::BeaconDB::Initialize(PendingBeaconMap& m_pending, BeaconMap& bool status = true; int height = 0; uint32_t version = 0; + bool needs_IsContract_correction = false; // First load the beacon db version from leveldb and check it against the constant in the class. { @@ -1038,8 +1049,22 @@ int BeaconRegistry::BeaconDB::Initialize(PendingBeaconMap& m_pending, BeaconMap& version, CURRENT_VERSION); + // Version 1, which corresponds to the 5.2.1.0 release contained a bug in ApplyContracts which prevented the + // application of multiple beacon contracts contained in a block under certain circumstances. In particular, + // the case with superblock 2053368, which contained beacon activations AND two beacon contracts was affected + // and once recorded in CBlockIndex, IsContract() returns the incorrect value. This flag will be used by + // ApplyContracts to check every block during the ContractReplay to correct the situation. + if (version == 1) + { + needs_IsContract_correction = true; + } + clear_leveldb(); + // After clearing the leveldb state, set the needs IsContract correction to the proper state. If the version that + // was on disk was 1, then this will be set to true. + SetNeedsIsContractCorrection(needs_IsContract_correction); + LogPrint(LogFlags::BEACON, "INFO: %s: Leveldb beacon area cleared. Version level set to %u.", __func__, CURRENT_VERSION); @@ -1057,6 +1082,21 @@ int BeaconRegistry::BeaconDB::Initialize(PendingBeaconMap& m_pending, BeaconMap& // Set m_database_init to true. This will cause LoadDBHeight hereinafter to simply report // the value of m_height_stored rather than loading the stored height from leveldb. m_database_init = true; + + // We are in a restart where at least some of a rescan was completed during a prior run. It is possible + // that the rescan may have not been completed before a shutdown was issued. In that case the + // needs_IsContract_correction flag will be set to true in leveldb, so restore that state for this run + // to ensure the correction finishes. When ReplayContracts finishes the corrections, it will mark the flag + // false. + CTxDB txdb("r"); + + std::pair key = std::make_pair("beacon_db", "needs_IsContract_correction"); + + bool status = txdb.ReadGenericSerializable(key, needs_IsContract_correction); + + if (!status) needs_IsContract_correction = false; + + m_needs_IsContract_correction = needs_IsContract_correction; } LogPrint(LogFlags::BEACON, "INFO: %s: db stored height at block %i.", @@ -1265,6 +1305,7 @@ bool BeaconRegistry::BeaconDB::clear_leveldb() m_height_stored = 0; m_recnum_stored = 0; m_database_init = false; + m_needs_IsContract_correction = false; return status; } @@ -1281,6 +1322,24 @@ size_t BeaconRegistry::BeaconDB::size() return m_historical.size(); } +bool BeaconRegistry::BeaconDB::NeedsIsContractCorrection() +{ + return m_needs_IsContract_correction; +} + +bool BeaconRegistry::BeaconDB::SetNeedsIsContractCorrection(bool flag) +{ + // Update the in-memory flag. + m_needs_IsContract_correction = flag; + + // Update leveldb + CTxDB txdb("rw"); + + std::pair key = std::make_pair("beacon_db", "needs_IsContract_correction"); + + return txdb.WriteGenericSerializable(key, m_needs_IsContract_correction); +} + bool BeaconRegistry::BeaconDB::StoreDBHeight(const int& height_stored) { // Update the in-memory bookmark variable. @@ -1354,33 +1413,6 @@ bool BeaconRegistry::BeaconDB::insert(const uint256 &hash, const int& height, co } } -bool BeaconRegistry::BeaconDB::update(const uint256 &hash, const int& height, const Beacon &beacon) -{ - bool status = false; - - LogPrint(LogFlags::BEACON, "INFO %s - store beacon: cpid %s, address %s, height %i, timestamp %" PRId64 - ", hash %s, prev_beacon_hash %s, status = %u.", - __func__, - beacon.m_cpid.ToString(), // cpid - beacon.GetAddress().ToString(), // address - beacon.m_timestamp, // timestamp - height, // height - beacon.m_hash.GetHex(), // transaction hash - beacon.m_prev_beacon_hash.GetHex(), // prev beacon transaction hash - beacon.m_status.Raw() // status - ); - - // update m_historical entry - m_historical[hash] = beacon; - - // update leveldb - status = Store(hash, static_cast(beacon)); - - if (height) status &= StoreDBHeight(height); - - return status; -} - bool BeaconRegistry::BeaconDB::erase(uint256 hash) { auto iter = m_historical.find(hash); diff --git a/src/gridcoin/beacon.h b/src/gridcoin/beacon.h index 53058fae40..fd34127c74 100644 --- a/src/gridcoin/beacon.h +++ b/src/gridcoin/beacon.h @@ -697,6 +697,19 @@ class BeaconRegistry : public IContractHandler //! void ResetInMemoryOnly(); + //! + //! \brief Returns whether IsContract correction is needed in ReplayContracts during initialization + //! \return + //! + bool NeedsIsContractCorrection(); + + //! + //! \brief Sets the state of the IsContract needs correction flag in memory and leveldb + //! \param flag The state to set + //! \return + //! + bool SetNeedsIsContractCorrection(bool flag); + private: BeaconMap m_beacons; //!< Contains the active registered beacons. PendingBeaconMap m_pending; //!< Contains beacons awaiting verification. @@ -714,6 +727,10 @@ class BeaconRegistry : public IContractHandler //! will ensure that when the wallet is restarted, the level db beacon storage will be cleared and //! reloaded from the contract replay with the correct lookback scope. //! + //! Version 0: <= 5.2.0.0 + //! Version 1: = 5.2.1.0 + //! Version 2: 5.2.1.0 with hotfix and > 5.2.1.0 + //! static constexpr uint32_t CURRENT_VERSION = 2; //! @@ -753,6 +770,19 @@ class BeaconRegistry : public IContractHandler //! size_t size(); + //! + //! \brief Returns whether IsContract correction is needed in ReplayContracts during initialization + //! \return + //! + bool NeedsIsContractCorrection(); + + //! + //! \brief Sets the state of the IsContract needs correction flag in memory and leveldb + //! \param flag The state to set + //! \return + //! + bool SetNeedsIsContractCorrection(bool flag); + //! //! \brief This stores the height to which the database entries are valid (the db scope). Note that it //! is not desired to expose this function as a public function, but currently the Revert function @@ -791,22 +821,6 @@ class BeaconRegistry : public IContractHandler //! bool insert(const uint256& hash, const int& height, const Beacon& beacon); - //! - //! \brief Update a beacon state record into the historical database. - //! - //! \param hash The hash for the key to the historical record. (This must be unique. It is usually - //! the transaction hash that of the transaction that contains the beacon contract, but also can be - //! a synthetic hash created from the hash of the superblock hash and the cpid hash if recording - //! beacon activations or expired pendings, which are handled in ActivatePending. - //! \param height The height of the block from which the beacon state record originates. - //! \param beacon The beacon state record to insert (which includes the appropriate status). - //! - //! \return Success or Failure. This is just like insert but it always succeeds (unless there is a - //! problem with the storage layer). If a record already exists it will be replaced. If it is new, - //! it will be inserted. - //! - bool update(const uint256& hash, const int& height, const Beacon& beacon); - //! //! \brief Erase a record from the database. //! @@ -896,6 +910,11 @@ class BeaconRegistry : public IContractHandler //! uint64_t m_recnum_stored = 0; + //! + //! \brief The flag that indicates whether IsContract correction is needed in ReplayContracts during initialization. + //! + bool m_needs_IsContract_correction = false; + //! //! \brief Store a beacon object in leveldb with the provided key value. //! diff --git a/src/gridcoin/contract/contract.cpp b/src/gridcoin/contract/contract.cpp index 0e022ecb44..9258c610d7 100644 --- a/src/gridcoin/contract/contract.cpp +++ b/src/gridcoin/contract/contract.cpp @@ -411,11 +411,11 @@ Contract GRC::MakeLegacyContract( return contract; } -void GRC::ReplayContracts(const CBlockIndex* pindex_end, const CBlockIndex* pindex_start) +void GRC::ReplayContracts(CBlockIndex* pindex_end, CBlockIndex* pindex_start) { static BlockFinder blockFinder; - const CBlockIndex*& pindex = pindex_start; + CBlockIndex*& pindex = pindex_start; // If there is no pindex_start (i.e. default value of nullptr), then set standard lookback. A Non-standard lookback // where there is a specific pindex_start argument supplied, is only used in the GRC InitializeContracts call for @@ -440,17 +440,52 @@ void GRC::ReplayContracts(const CBlockIndex* pindex_end, const CBlockIndex* pind LogPrint(BCLog::LogFlags::BEACON, "Beacon database at height %i", beacon_db_height); + if (beacons.NeedsIsContractCorrection()) + { + LogPrintf("INFO %s: The NeedsIsContractCorrection flag is set. All blocks within the scan range " + "will be checked to ensure the contains contract flag is set correctly and corrections made. " + "This may take a little longer than the standard replay.", + __func__); + } + CBlock block; // These are memorized consecutively in order from oldest to newest. for (; pindex; pindex = pindex->pnext) { - if (pindex->IsContract()) { + + // If the NeedsIsContractCorrection flag is set which means all blocks within the scan range + // have to be checked, OR the block index entry is already marked to contain contract(s), + // then apply the contracts found in the block. + if (beacons.NeedsIsContractCorrection() || pindex->IsContract()) { if (!block.ReadFromDisk(pindex)) { continue; } - bool unused; - ApplyContracts(block, pindex, beacon_db_height, unused); + bool found_contract; + ApplyContracts(block, pindex, beacon_db_height, found_contract); + + // If a contract was found and the NeedsIsContractCorrection flag is set, then + // record that a contract was found in the block index. This corrects the block index + // record. + if (found_contract && beacons.NeedsIsContractCorrection() && !pindex->IsContract()) + { + LogPrintf("WARNING %s: There were found contract(s) in block %i but IsContract() is false. " + "Correcting IsContract flag to true in the block index.", + __func__, + pindex->nHeight); + + pindex->MarkAsContract(); + + CTxDB txdb("rw"); + + CDiskBlockIndex disk_block_index(pindex); + if (!txdb.WriteBlockIndex(disk_block_index)) + { + error("%s: Block index correction of IsContract flag for block %i failed.", + __func__, + pindex->nHeight); + } + } } if (pindex->IsSuperblock() && pindex->nVersion >= 11) { @@ -482,7 +517,14 @@ void GRC::ReplayContracts(const CBlockIndex* pindex_end, const CBlockIndex* pind } } - if (pindex == pindex_end) break; + if (pindex == pindex_end) + { + // Finished the rescan. If the NeedsIsContractCorrection was set to true, then reset + // to false. + if (beacons.NeedsIsContractCorrection()) beacons.SetNeedsIsContractCorrection(false); + + break; + } } Researcher::Refresh(); @@ -510,11 +552,11 @@ void GRC::ApplyContracts( bool& out_found_contract) { for (const auto& contract : tx.GetContracts()) { - // Do not (re)apply contracts that have already been stored/loeaded into + // Do not (re)apply contracts that have already been stored/loaded into // the beacon DB up to the block BEFORE the beacon db height. Because the beacon // db height is at the block level, and is updated on each beacon insert, when // in a sync from zero situation where the contracts are played as each block is validated, - // any beacon contract in the block EQUAL to the beacon db height must pass this test + // any beacon contract in the block EQUAL to the beacon db height must fail this test // and be inserted again, because otherwise the second and succeeding contracts on the // same block will not be inserted and those CPID's will not be recorded properly. // This was the cause of the failure to sync through 2069264 that started on 20210312. See diff --git a/src/gridcoin/contract/contract.h b/src/gridcoin/contract/contract.h index 13a716d632..e1d10d9578 100644 --- a/src/gridcoin/contract/contract.h +++ b/src/gridcoin/contract/contract.h @@ -470,7 +470,7 @@ Contract MakeLegacyContract( //! //! \param pindex Block index to start with. //! -void ReplayContracts(const CBlockIndex* pindex_end, const CBlockIndex* pindex_start = nullptr); +void ReplayContracts(CBlockIndex *pindex_end, CBlockIndex *pindex_start = nullptr); //! //! \brief Apply contracts from transactions in a block by passing them to the diff --git a/src/gridcoin/gridcoin.cpp b/src/gridcoin/gridcoin.cpp index e928bdb60e..339f2d023f 100644 --- a/src/gridcoin/gridcoin.cpp +++ b/src/gridcoin/gridcoin.cpp @@ -76,7 +76,7 @@ bool InitializeResearchRewardAccounting(CBlockIndex* pindexBest) //! //! \param pindexBest Block index of the tip of the chain. //! -void InitializeContracts(const CBlockIndex* pindexBest) +void InitializeContracts(CBlockIndex* pindexBest) { LogPrintf("Gridcoin: Loading beacon history..."); uiInterface.InitMessage(_("Loading beacon history...")); From 759bd6dc1727c764d3aae2445be6b84ba705c28a Mon Sep 17 00:00:00 2001 From: jamescowens Date: Sun, 14 Mar 2021 13:58:37 -0400 Subject: [PATCH 3/4] Update changelog for 5.2.2.0 release --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7806832290..523adace06 100755 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +## [5.2.2.0] 2021-03-14, leisure +### Fixed +- beacon, contracts: Fix sync from zero issue due to ApplyContracts problem in 5.2.1.0 #2047 (@jamescowens) + ## [5.2.1.0] 2021-03-07, leisure ### Added - voting: Add wait warning to voting tab loading message #2039 (@cyrossignol) From 174f5ddf82b59f35cb2ed390d872b80b7862f62c Mon Sep 17 00:00:00 2001 From: jamescowens Date: Sun, 14 Mar 2021 13:59:30 -0400 Subject: [PATCH 4/4] Increment version to 5.2.2.0 for release. --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 70bfb932d8..5afdbc7e74 100755 --- a/configure.ac +++ b/configure.ac @@ -2,7 +2,7 @@ dnl require autoconf 2.60 (AS_ECHO/AS_ECHO_N) AC_PREREQ([2.60]) define(_CLIENT_VERSION_MAJOR, 5) define(_CLIENT_VERSION_MINOR, 2) -define(_CLIENT_VERSION_REVISION, 1) +define(_CLIENT_VERSION_REVISION, 2) define(_CLIENT_VERSION_BUILD, 0) define(_CLIENT_VERSION_IS_RELEASE, true) define(_COPYRIGHT_YEAR, 2021)