Skip to content

p2p: remove adjusted time #25908

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.

Sign up for GitHub

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

Closed
fanquake wants to merge 6 commits into from
Closed

p2p: remove adjusted time #25908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f30a2fb
p2p: stop sampling timedata from peers
fanquake Aug 22, 2022
b5a55b7
timedata: remove TimeOffset from AdjustedTime
fanquake Aug 22, 2022
970996b
time: replace GetAdjustedTime() with NodeClock::now()
fanquake Aug 22, 2022
e300fb3
rpc: remove timeoffset from getnetworkinfo() RPC
fanquake Aug 22, 2022
7fdea2f
init: remove -maxtimeadjustment option
fanquake Aug 22, 2022
644f93f
time: remove timedata
fanquake Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,6 @@ BITCOIN_CORE_H = \
support/lockedpool.h \
sync.h \
threadsafety.h \
timedata.h \
torcontrol.h \
txdb.h \
txmempool.h \
Expand Down Expand Up @@ -439,7 +438,6 @@ libbitcoin_node_a_SOURCES = \
script/sigcache.cpp \
shutdown.cpp \
signet.cpp \
timedata.cpp \
torcontrol.cpp \
txdb.cpp \
txmempool.cpp \
Expand Down
2 changes: 0 additions & 2 deletions src/Makefile.test.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ BITCOIN_TESTS =\
test/streams_tests.cpp \
test/sync_tests.cpp \
test/system_tests.cpp \
test/timedata_tests.cpp \
test/torcontrol_tests.cpp \
test/transaction_tests.cpp \
test/translation_tests.cpp \
Expand Down Expand Up @@ -335,7 +334,6 @@ test_fuzz_fuzz_SOURCES = \
test/fuzz/string.cpp \
test/fuzz/strprintf.cpp \
test/fuzz/system.cpp \
test/fuzz/timedata.cpp \
test/fuzz/torcontrol.cpp \
test/fuzz/transaction.cpp \
test/fuzz/tx_in.cpp \
Expand Down
1 change: 0 additions & 1 deletion src/addrman_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <protocol.h>
#include <serialize.h>
#include <sync.h>
#include <timedata.h>
#include <uint256.h>
#include <util/time.h>

Expand Down
3 changes: 1 addition & 2 deletions src/headerssync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#include <headerssync.h>
#include <logging.h>
#include <pow.h>
#include <timedata.h>
#include <util/check.h>

// The two constants below are computed using the simulation script on
Expand Down Expand Up @@ -40,7 +39,7 @@ HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus
// exceeds this bound, because it's not possible for a consensus-valid
// chain to be longer than this (at the current time -- in the future we
// could try again, if necessary, to sync a longer chain).
m_max_commitments = 6*(Ticks<std::chrono::seconds>(GetAdjustedTime() - NodeSeconds{std::chrono::seconds{chain_start->GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME) / HEADER_COMMITMENT_PERIOD;
m_max_commitments = 6*(Ticks<std::chrono::seconds>(NodeClock::now() - NodeSeconds{std::chrono::seconds{chain_start->GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME) / HEADER_COMMITMENT_PERIOD;

LogPrint(BCLog::NET, "Initial headers sync started with peer=%d: height=%i, max_commitments=%i, min_work=%s\n", m_id, m_current_height, m_max_commitments, m_minimum_required_work.ToString());
}
Expand Down
4 changes: 1 addition & 3 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
#include <script/standard.h>
#include <shutdown.h>
#include <sync.h>
#include <timedata.h>
#include <torcontrol.h>
#include <txdb.h>
#include <txmempool.h>
Expand Down Expand Up @@ -485,7 +484,6 @@ void SetupServerArgs(ArgsManager& argsman)
argsman.AddArg("-maxconnections=<n>", strprintf("Maintain at most <n> connections to peers (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by outbound peers forward or backward by this amount (default: %u seconds).", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target per 24h. Limit does not apply to peers with 'download' permission or blocks created within past week. 0 = no limit (default: %s). Optional suffix units [k|K|m|M|g|G|t|T] (default: M). Lowercase is 1000 base while uppercase is 1024 base", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
Expand Down Expand Up @@ -1444,7 +1442,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
ChainstateManager::Options chainman_opts{
.chainparams = chainparams,
.datadir = args.GetDataDirNet(),
.adjusted_time_callback = GetAdjustedTime,
.adjusted_time_callback = NodeClock::now,
};
Assert(!ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction

Expand Down
17 changes: 5 additions & 12 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
#include <scheduler.h>
#include <streams.h>
#include <sync.h>
#include <timedata.h>
#include <tinyformat.h>
#include <txmempool.h>
#include <txorphanage.h>
Expand Down Expand Up @@ -1257,7 +1256,7 @@ bool PeerManagerImpl::TipMayBeStale()

bool PeerManagerImpl::CanDirectFetch()
{
return m_chainman.ActiveChain().Tip()->Time() > GetAdjustedTime() - m_chainparams.GetConsensus().PowTargetSpacing() * 20;
return m_chainman.ActiveChain().Tip()->Time() > NodeClock::now() - m_chainparams.GetConsensus().PowTargetSpacing() * 20;
}

static bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
Expand Down Expand Up @@ -3380,13 +3379,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
peer->m_starting_height, addrMe.ToStringAddrPort(), fRelay, pfrom.GetId(),
remoteAddr, (mapped_as ? strprintf(", mapped_as=%d", mapped_as) : ""));

int64_t nTimeOffset = nTime - GetTime();
pfrom.nTimeOffset = nTimeOffset;
if (!pfrom.IsInboundConn()) {
// Don't use timedata samples from inbound peers to make it
// harder for others to tamper with our adjusted time.
AddTimeData(pfrom.addr, nTimeOffset);
}
pfrom.nTimeOffset = nTime - GetTime();

@ajtowns ajtowns Aug 26, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps:

if (!pfrom.IsInboundConn() && std::abs(pfrom.nTimeOffset) >= 1800) {
    ++m_bad_non_inbound_time_offset;
    if (m_bad_non_inbound_time_offset == 3) {
        SetMiscWarning("Please check your computer's date and time are correct");
    }
}

(ie, add a counter to PeerManagerImpl, and on the third outbound connection that has a time offset of 30mins or more, set a warning that will show up in getblockchaininfo, getnetworkinfo, getmininginfo until restart)

@ajtowns ajtowns Oct 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we regularly do new feeler and blocks-only connections, perhaps we could keep a rolling average of abs(peer.nTimeOffset) and report that in getnetworkinfo?


// If the peer is old enough to have the old alert system, send it the final alert.
if (greatest_common_version <= 70012) {
Expand Down Expand Up @@ -5432,7 +5425,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)

if (!state.fSyncStarted && CanServeBlocks(*peer) && !m_chainman.m_blockman.LoadingBlocks()) {
// Only actively request headers from a single peer, unless we're close to today.
if ((nSyncStarted == 0 && sync_blocks_and_headers_from_peer) || m_chainman.m_best_header->Time() > GetAdjustedTime() - 24h) {
if ((nSyncStarted == 0 && sync_blocks_and_headers_from_peer) || m_chainman.m_best_header->Time() > NodeClock::now() - 24h) {
const CBlockIndex* pindexStart = m_chainman.m_best_header;
/* If possible, start at the block preceding the currently
best known header. This ensures that we always get a
Expand All @@ -5452,7 +5445,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
// Convert HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER to microseconds before scaling
// to maintain precision
std::chrono::microseconds{HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER} *
Ticks<std::chrono::seconds>(GetAdjustedTime() - m_chainman.m_best_header->Time()) / consensusParams.nPowTargetSpacing
Ticks<std::chrono::seconds>(NodeClock::now() - m_chainman.m_best_header->Time()) / consensusParams.nPowTargetSpacing
);
nSyncStarted++;
}
Expand Down Expand Up @@ -5778,7 +5771,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
// Check for headers sync timeouts
if (state.fSyncStarted && peer->m_headers_sync_timeout < std::chrono::microseconds::max()) {
// Detect whether this is a stalling initial-headers-sync peer
if (m_chainman.m_best_header->Time() <= GetAdjustedTime() - 24h) {
if (m_chainman.m_best_header->Time() <= NodeClock::now() - 24h) {
if (current_time > peer->m_headers_sync_timeout && nSyncStarted == 1 && (m_num_preferred_download_peers - state.fPreferredDownload >= 1)) {
// Disconnect a peer (without NetPermissionFlags::NoBan permission) if it is our only sync peer,
// and we have others we could be using instead.
Expand Down
7 changes: 3 additions & 4 deletions src/node/miner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include <policy/policy.h>
#include <pow.h>
#include <primitives/transaction.h>
#include <timedata.h>
#include <util/moneystr.h>
#include <validation.h>

Expand All @@ -30,7 +29,7 @@ namespace node {
int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev)
{
int64_t nOldTime = pblock->nTime;
int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(GetAdjustedTime()))};
int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};

if (nOldTime < nNewTime) {
pblock->nTime = nNewTime;
Expand Down Expand Up @@ -132,7 +131,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
pblock->nVersion = gArgs.GetIntArg("-blockversion", pblock->nVersion);
}

pblock->nTime = TicksSinceEpoch<std::chrono::seconds>(GetAdjustedTime());
pblock->nTime = TicksSinceEpoch<std::chrono::seconds>(NodeClock::now());
m_lock_time_cutoff = pindexPrev->GetMedianTimePast();

int nPackagesSelected = 0;
Expand Down Expand Up @@ -170,7 +169,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc

BlockValidationState state;
if (m_options.test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev,
GetAdjustedTime, /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
NodeClock::now, /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {
throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString()));
}
const auto time_2{SteadyClock::now()};
Expand Down
5 changes: 2 additions & 3 deletions src/rpc/mining.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include <script/script.h>
#include <script/signingprovider.h>
#include <shutdown.h>
#include <timedata.h>
#include <txmempool.h>
#include <univalue.h>
#include <util/strencodings.h>
Expand Down Expand Up @@ -375,7 +374,7 @@ static RPCHelpMan generateblock()
LOCK(cs_main);

BlockValidationState state;
if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), GetAdjustedTime, false, false)) {
if (!TestBlockValidity(state, chainman.GetParams(), chainman.ActiveChainstate(), block, chainman.m_blockman.LookupBlockIndex(block.hashPrevBlock), NodeClock::now, false, false)) {
throw JSONRPCError(RPC_VERIFY_ERROR, strprintf("TestBlockValidity failed: %s", state.ToString()));
}
}
Expand Down Expand Up @@ -648,7 +647,7 @@ static RPCHelpMan getblocktemplate()
if (block.hashPrevBlock != pindexPrev->GetBlockHash())
return "inconclusive-not-best-prevblk";
BlockValidationState state;
TestBlockValidity(state, chainman.GetParams(), active_chainstate, block, pindexPrev, GetAdjustedTime, false, true);
TestBlockValidity(state, chainman.GetParams(), active_chainstate, block, pindexPrev, NodeClock::now, false, true);
return BIP22ValidationResult(state);
}

Expand Down
3 changes: 0 additions & 3 deletions src/rpc/net.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include <rpc/server_util.h>
#include <rpc/util.h>
#include <sync.h>
#include <timedata.h>
#include <util/chaintype.h>
#include <util/strencodings.h>
#include <util/string.h>
Expand Down Expand Up @@ -596,7 +595,6 @@ static RPCHelpMan getnetworkinfo()
{RPCResult::Type::STR, "SERVICE_NAME", "the service name"},
}},
{RPCResult::Type::BOOL, "localrelay", "true if transaction relay is requested from peers"},
{RPCResult::Type::NUM, "timeoffset", "the time offset"},
{RPCResult::Type::NUM, "connections", "the total number of connections"},
{RPCResult::Type::NUM, "connections_in", "the number of inbound connections"},
{RPCResult::Type::NUM, "connections_out", "the number of outbound connections"},
Expand Down Expand Up @@ -646,7 +644,6 @@ static RPCHelpMan getnetworkinfo()
if (node.peerman) {
obj.pushKV("localrelay", !node.peerman->IgnoresIncomingTxs());
}
obj.pushKV("timeoffset", GetTimeOffset());

@maflcko maflcko Aug 24, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe keep this field and instead return the median offset of the currently connected outbounds or so? See also #24606 (comment)

if (node.connman) {
obj.pushKV("networkactive", node.connman->GetNetworkActive());
obj.pushKV("connections", node.connman->GetNodeCount(ConnectionDirection::Both));
Expand Down
2 changes: 0 additions & 2 deletions src/test/denialofservice_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include <serialize.h>
#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <timedata.h>
#include <util/string.h>
#include <util/time.h>
#include <validation.h>
Expand Down Expand Up @@ -72,7 +71,6 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
/*local_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS),
/*version=*/PROTOCOL_VERSION,
/*relay_txs=*/true);
TestOnlyResetTimeData();

// This test requires that we have a chain with non-zero work.
{
Expand Down
31 changes: 0 additions & 31 deletions src/test/fuzz/timedata.cpp
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion src/test/miner_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <script/standard.h>
#include <test/util/random.h>
#include <test/util/txmempool.h>
#include <timedata.h>
#include <txmempool.h>
#include <uint256.h>
#include <util/strencodings.h>
Expand Down
5 changes: 0 additions & 5 deletions src/test/net_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include <streams.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
#include <timedata.h>
#include <util/strencodings.h>
#include <util/string.h>
#include <validation.h>
Expand Down Expand Up @@ -899,10 +898,6 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
chainstate.ResetIbd();
m_node.args->ForceSetArg("-capturemessages", "0");
m_node.args->ForceSetArg("-bind", "");
// PeerManager::ProcessMessage() calls AddTimeData() which changes the internal state
// in timedata.cpp and later confuses the test "timedata_tests/addtimedata". Thus reset
// that state as it was before our test was run.
TestOnlyResetTimeData();
}

BOOST_AUTO_TEST_SUITE_END()
Loading