-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: make adjusted time type-safe (bitcoin#25786) #7459
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -193,15 +193,17 @@ class CCoinJoinQueue | |
|
|
||
| CCoinJoinQueue() = default; | ||
|
|
||
| CCoinJoinQueue(int nDenom, const COutPoint& outpoint, const uint256& proTxHash, int64_t nTime, bool fReady) : | ||
| CCoinJoinQueue(int nDenom, const COutPoint& outpoint, const uint256& proTxHash, NodeClock::time_point time, bool fReady) : | ||
| nDenom(nDenom), | ||
| masternodeOutpoint(outpoint), | ||
| m_protxHash(proTxHash), | ||
| nTime(nTime), | ||
| nTime(TicksSinceEpoch<std::chrono::seconds>(time)), | ||
| fReady(fReady) | ||
| { | ||
| } | ||
|
|
||
| NodeSeconds Time() const { return NodeSeconds{std::chrono::seconds{nTime}}; } | ||
|
|
||
| SERIALIZE_METHODS(CCoinJoinQueue, obj) | ||
| { | ||
| READWRITE(obj.nDenom, obj.m_protxHash, obj.nTime, obj.fReady); | ||
|
|
@@ -217,7 +219,8 @@ class CCoinJoinQueue | |
| [[nodiscard]] bool CheckSignature(const CBLSPublicKey& blsPubKey) const; | ||
|
|
||
| /// Check if a queue is too old or too far into the future | ||
| [[nodiscard]] bool IsTimeOutOfBounds(int64_t current_time = GetAdjustedTime()) const; | ||
| [[nodiscard]] bool IsTimeOutOfBounds(NodeSeconds current_time) const; | ||
| [[nodiscard]] bool IsTimeOutOfBounds() const; | ||
|
Comment on lines
+222
to
+223
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2 methods with the same name is a bit strange too. consider renaming one from
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's for the overload? |
||
|
|
||
| [[nodiscard]] std::string ToString() const; | ||
|
|
||
|
|
@@ -247,11 +250,11 @@ class CCoinJoinBroadcastTx | |
| { | ||
| } | ||
|
|
||
| CCoinJoinBroadcastTx(CTransactionRef _tx, const COutPoint& _outpoint, const uint256& proTxHash, int64_t _sigTime) : | ||
| CCoinJoinBroadcastTx(CTransactionRef _tx, const COutPoint& _outpoint, const uint256& proTxHash, NodeClock::time_point time) : | ||
| tx(std::move(_tx)), | ||
| masternodeOutpoint(_outpoint), | ||
| m_protxHash(proTxHash), | ||
| sigTime(_sigTime) | ||
| sigTime(TicksSinceEpoch<std::chrono::seconds>(time)) | ||
| { | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -278,25 +278,25 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj) | |
| AssertLockNotHeld(cs_relay); | ||
|
|
||
| uint256 nHash = govobj.GetHash(); | ||
| std::vector<vote_time_pair_t> vecVotePairs; | ||
| cmmapOrphanVotes.GetAll(nHash, vecVotePairs); | ||
| std::vector<governance::OrphanVote> orphan_votes; | ||
| cmmapOrphanVotes.GetAll(nHash, orphan_votes); | ||
|
|
||
| ScopedLockBool guard(cs_store, fRateChecksEnabled, false); | ||
|
|
||
| int64_t nNow = GetAdjustedTime(); | ||
| const auto now{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime())}; | ||
| const auto tip_mn_list = m_dmnman.GetListAtChainTip(); | ||
| for (const auto& pairVote : vecVotePairs) { | ||
| const auto& [vote, time] = pairVote; | ||
| for (const auto& orphan_vote : orphan_votes) { | ||
| const auto& vote = orphan_vote.vote; | ||
| bool fRemove = false; | ||
| CGovernanceException e; | ||
| if (time < nNow) { | ||
| if (orphan_vote.expiration < now) { | ||
| fRemove = true; | ||
| } else if (govobj.ProcessVote(m_mn_metaman, fRateChecksEnabled, tip_mn_list, vote, e)) { | ||
| RelayVote(vote); | ||
| fRemove = true; | ||
| } | ||
| if (fRemove) { | ||
| cmmapOrphanVotes.Erase(nHash, pairVote); | ||
| cmmapOrphanVotes.Erase(nHash, orphan_vote); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -733,21 +733,21 @@ bool CGovernanceManager::MasternodeRateCheck(const CGovernanceObject& govobj, bo | |
| } | ||
|
|
||
| const COutPoint& masternodeOutpoint = govobj.GetMasternodeOutpoint(); | ||
| int64_t nTimestamp = govobj.GetCreationTime(); | ||
| int64_t nNow = GetAdjustedTime(); | ||
| int64_t nSuperblockCycleSeconds = Params().GetConsensus().nSuperblockCycle * Params().GetConsensus().nPowTargetSpacing; | ||
| const auto timestamp{govobj.CreationTime()}; | ||
| const auto now{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime())}; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why not just Type is already specified,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. time point cast is a function not a type |
||
| const auto superblock_cycle{Params().GetConsensus().nSuperblockCycle * Params().GetConsensus().PowTargetSpacing()}; | ||
|
|
||
| std::string strHash = govobj.GetHash().ToString(); | ||
|
|
||
| if (nTimestamp < nNow - 2 * nSuperblockCycleSeconds) { | ||
| if (timestamp < now - 2 * superblock_cycle) { | ||
| LogPrint(BCLog::GOBJECT, "CGovernanceManager::MasternodeRateCheck -- object %s rejected due to too old timestamp, masternode = %s, timestamp = %d, current time = %d\n", | ||
| strHash, masternodeOutpoint.ToStringShort(), nTimestamp, nNow); | ||
| strHash, masternodeOutpoint.ToStringShort(), govobj.GetCreationTime(), TicksSinceEpoch<std::chrono::seconds>(now)); | ||
| return false; | ||
| } | ||
|
|
||
| if (nTimestamp > nNow + count_seconds(MAX_TIME_FUTURE_DEVIATION)) { | ||
| if (timestamp > now + MAX_TIME_FUTURE_DEVIATION) { | ||
| LogPrint(BCLog::GOBJECT, "CGovernanceManager::MasternodeRateCheck -- object %s rejected due to too new (future) timestamp, masternode = %s, timestamp = %d, current time = %d\n", | ||
| strHash, masternodeOutpoint.ToStringShort(), nTimestamp, nNow); | ||
| strHash, masternodeOutpoint.ToStringShort(), govobj.GetCreationTime(), TicksSinceEpoch<std::chrono::seconds>(now)); | ||
| return false; | ||
| } | ||
|
|
||
|
|
@@ -760,20 +760,20 @@ bool CGovernanceManager::MasternodeRateCheck(const CGovernanceObject& govobj, bo | |
| } | ||
|
|
||
| // Allow 1 trigger per mn per cycle, with a small fudge factor | ||
| double dMaxRate = 2 * 1.1 / double(nSuperblockCycleSeconds); | ||
| double dMaxRate = 2 * 1.1 / static_cast<double>(count_seconds(superblock_cycle)); | ||
|
|
||
| // Temporary copy to check rate after new timestamp is added | ||
| CRateCheckBuffer buffer = it->second.triggerBuffer; | ||
|
|
||
| buffer.AddTimestamp(nTimestamp); | ||
| buffer.AddTimestamp(govobj.GetCreationTime()); | ||
| double dRate = buffer.GetRate(); | ||
|
|
||
| if (dRate < dMaxRate) { | ||
| return true; | ||
| } | ||
|
|
||
| LogPrint(BCLog::GOBJECT, "CGovernanceManager::MasternodeRateCheck -- Rate too high: object hash = %s, masternode = %s, object timestamp = %d, rate = %f, max rate = %f\n", | ||
| strHash, masternodeOutpoint.ToStringShort(), nTimestamp, dRate, dMaxRate); | ||
| strHash, masternodeOutpoint.ToStringShort(), govobj.GetCreationTime(), dRate, dMaxRate); | ||
|
|
||
| if (fUpdateFailStatus) { | ||
| it->second.fStatusOK = false; | ||
|
|
@@ -823,8 +823,7 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc | |
| std::string msg{strprintf("CGovernanceManager::%s -- Unknown parent object %s, MN outpoint = %s", __func__, | ||
| nHashGovobj.ToString(), vote.GetMasternodeOutpoint().ToStringShort())}; | ||
| exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_WARNING); | ||
| if (cmmapOrphanVotes.Insert(nHashGovobj, vote_time_pair_t(vote, count_seconds(GetTime<std::chrono::seconds>() + | ||
| GOVERNANCE_ORPHAN_EXPIRATION_TIME)))) { | ||
| if (cmmapOrphanVotes.Insert(nHashGovobj, governance::OrphanVote{vote, Now<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) { | ||
| hashToRequest = nHashGovobj; // Caller should request this object | ||
| } | ||
| LogPrint(BCLog::GOBJECT, "%s\n", msg); | ||
|
|
@@ -883,20 +882,19 @@ void CGovernanceManager::CheckPostponedObjects() | |
|
|
||
|
|
||
| // Perform additional relays for triggers | ||
| int64_t nNow = GetAdjustedTime(); | ||
| int64_t nSuperblockCycleSeconds = Params().GetConsensus().nSuperblockCycle * Params().GetConsensus().nPowTargetSpacing; | ||
| const auto now{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime())}; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: same here for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same; time_point_cast is a function not a type |
||
| const auto superblock_cycle{Params().GetConsensus().nSuperblockCycle * Params().GetConsensus().PowTargetSpacing()}; | ||
|
|
||
| for (auto it = setAdditionalRelayObjects.begin(); it != setAdditionalRelayObjects.end();) { | ||
| auto itObject = mapObjects.find(*it); | ||
| if (itObject != mapObjects.end()) { | ||
| const auto& govobj = *Assert(itObject->second); | ||
|
|
||
| int64_t nTimestamp = govobj.GetCreationTime(); | ||
| const auto timestamp{govobj.CreationTime()}; | ||
|
|
||
| bool fValid = (nTimestamp <= nNow + count_seconds(MAX_TIME_FUTURE_DEVIATION)) && | ||
| (nTimestamp >= nNow - 2 * nSuperblockCycleSeconds); | ||
| bool fReady = (nTimestamp <= | ||
| nNow + count_seconds(MAX_TIME_FUTURE_DEVIATION) - count_seconds(RELIABLE_PROPAGATION_TIME)); | ||
| const bool fValid{timestamp <= now + MAX_TIME_FUTURE_DEVIATION && | ||
| timestamp >= now - 2 * superblock_cycle}; | ||
| const bool fReady{timestamp <= now + MAX_TIME_FUTURE_DEVIATION - RELIABLE_PROPAGATION_TIME}; | ||
|
|
||
| if (fValid) { | ||
| if (fReady) { | ||
|
|
@@ -1092,15 +1090,14 @@ std::vector<uint256> CGovernanceManager::GetOrphanVoteObjectHashes() | |
| { | ||
| LOCK(cs_store); | ||
|
|
||
| int64_t nNow = GetTime<std::chrono::seconds>().count(); | ||
| const auto now{Now<NodeSeconds>()}; | ||
|
|
||
| // Clean up expired orphan votes | ||
| const vote_cmm_t::list_t& items = cmmapOrphanVotes.GetItemList(); | ||
| for (auto it = items.begin(); it != items.end();) { | ||
| auto prevIt = it; | ||
| ++it; | ||
| const auto& [_, time] = prevIt->value; | ||
| if (time < nNow) { | ||
| if (prevIt->value.expiration < now) { | ||
| cmmapOrphanVotes.Erase(prevIt->key, prevIt->value); | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: technically, it's valid code, but looks a bit strange without 0s