From abfa8c0bd48db91d803668af19594b74d6efe9bb Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 7 Feb 2022 14:03:49 +0100 Subject: [PATCH 1/3] Merge bitcoin/bitcoin#24237: test: Avoid testing negative block heights fad81548fa03861c244397201d6b6e6cbf883c38 test: Avoid testing negative block heights (MarcoFalke) Pull request description: A negative chain height is only used to denote an empty chain, not the height of any block. So stop testing that and remove a suppression. ACKs for top commit: brunoerg: crACK fad81548fa03861c244397201d6b6e6cbf883c38 Tree-SHA512: 0f9e91617dfb6ceda99831e6cf4b4bf0d951054957c159b1a05a178ab6090798fae7368edefe12800da24585bcdf7299ec3534f4d3bbf5ce6a6eca74dd3bb766 --- src/test/coins_tests.cpp | 6 +++--- src/test/fuzz/coins_view.cpp | 2 +- test/sanitizer_suppressions/ubsan | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 6c8ab69bb5..678679da40 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -334,7 +334,7 @@ BOOST_AUTO_TEST_CASE(updatecoins_simulation_test) tx.vout.resize(1); tx.vout[0].nValue = i; //Keep txs unique unless intended to duplicate tx.vout[0].scriptPubKey.assign(InsecureRand32() & 0x3F, 0); // Random sizes so we can test memory usage accounting - unsigned int height = InsecureRand32(); + const int height{int(InsecureRand32() >> 1)}; Coin old_coin; // 2/20 times create a new coinbase @@ -403,11 +403,11 @@ BOOST_AUTO_TEST_CASE(updatecoins_simulation_test) // Update the expected result to know about the new output coins assert(tx.vout.size() == 1); const COutPoint outpoint(tx.GetHash(), 0); - result[outpoint] = Coin(tx.vout[0], height, CTransaction(tx).IsCoinBase()); + result[outpoint] = Coin(tx.vout[0], height, CTransaction{tx}.IsCoinBase()); // Call UpdateCoins on the top cache CTxUndo undo; - UpdateCoins(CTransaction(tx), *(stack.back()), undo, height); + UpdateCoins(CTransaction{tx}, *(stack.back()), undo, height); // Update the utxo set for future spends utxoset.insert(outpoint); diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 3c7d7d2682..86be7adf94 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -214,7 +214,7 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view) return; } bool expected_code_path = false; - const int height = fuzzed_data_provider.ConsumeIntegral(); + const int height{int(fuzzed_data_provider.ConsumeIntegral() >> 1)}; const bool possible_overwrite = fuzzed_data_provider.ConsumeBool(); try { AddCoins(coins_view_cache, transaction, height, possible_overwrite); diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index f0828c01cd..42df86bbdd 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -51,7 +51,6 @@ implicit-integer-sign-change:bech32.cpp implicit-integer-sign-change:common/bloom.cpp implicit-integer-sign-change:chain.cpp implicit-integer-sign-change:chain.h -implicit-integer-sign-change:coins.h implicit-integer-sign-change:compat/stdin.cpp implicit-integer-sign-change:compressor.h implicit-integer-sign-change:crc32c/ From e101fd8e3b5b5ab175566e64be5866579669908f Mon Sep 17 00:00:00 2001 From: Vijay <2348066+vijaydasmp@users.noreply.github.com> Date: Sun, 24 Nov 2024 21:02:27 +0530 Subject: [PATCH 2/3] Merge bitcoin/bitcoin#23631: p2p: Don't use timestamps from inbound peers for Adjusted Time 0c85dc30e6b628f7538a67776c7eefcb84ef4f82 p2p: Don't use timestamps from inbound peers (Martin Zumsande) Pull request description: `GetAdjustedTime()` (used e.g. in validation and addrman) returns a time with an offset that is influenced by timestamps that our peers have sent us in their version message. Currently, timestamps from all peers are used for this. However, I think that it would make sense to ignore the timedata samples from inbound peers, making it much harder for others to influence the Adjusted Time in a targeted way. With the extra feeler connections (every 2 minutes on average) and extra block-relay-only connections (every 5 minutes on average) there are also now plenty of opportunities to gather a meaningful number of timedata samples from outbound peers. There are some measures in place to prevent abuse: the `-maxtimeadjustment` parameter with a default of 70 minutes, warnings in cases of large deviations, only using the first 200 samples ([explanation](https://github.com/bitcoin/bitcoin/blob/383d350bd5107bfe00e3b90a00cab9a3c1397c72/src/timedata.cpp#L57-L72)), but I think that only using samples from outbound connections in the first place would be an additional safety measure that would make sense. See also issue #4521 for further context and links: There have been several discussions in the past about replacing or abolishing the existing timedata system. ACKs for top commit: jnewbery: Concept and code review ACK 0c85dc30e6b628f7538a67776c7eefcb84ef4f82 naumenkogs: ACK 0c85dc30e6b628f7538a67776c7eefcb84ef4f82 vasild: ACK 0c85dc30e6b628f7538a67776c7eefcb84ef4f82 Tree-SHA512: 2d6375305bcae034d68b58b7a07777b40ac430dfed554c88e681a048c527536691e1b7d08c0ef995247d356f8e81aa0a4b983bf2674faf6a416264e5f1af0a96 --- src/net_processing.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7a8677e0e0..1ca66c68d7 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3651,7 +3651,11 @@ void PeerManagerImpl::ProcessMessage( int64_t nTimeOffset = nTime - GetTime(); pfrom.nTimeOffset = nTimeOffset; - AddTimeData(pfrom.addr, 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); + } // Feeler connections exist only to verify if address is online. if (pfrom.IsFeelerConn()) { From e237301b76b923b2c44c81ae40b7e31f5a1db429 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 18 Mar 2022 14:10:09 +0100 Subject: [PATCH 3/3] Merge bitcoin/bitcoin#24609: Clarify -maxtimeadjustment that only outbound peers influence timedata 1bba72d824224f8a2625f529963d8982a00dfe14 Clarify in -maxtimeadjustment that only outbound peers influence time data (Jon Atack) Pull request description: #23631 changed our adjusted time to only take into account time from outbound peers. Update `-maxtimeadjustment` to clarify this for users. ACKs for top commit: MarcoFalke: cr ACK 1bba72d824224f8a2625f529963d8982a00dfe14 mzumsande: code Review ACK 1bba72d824224f8a2625f529963d8982a00dfe14 brunoerg: crACK 1bba72d824224f8a2625f529963d8982a00dfe14 Tree-SHA512: ad610ab3038fb83134e21d31cca952ef9ac926e88992ff93023b7010f2499f9a4d952e8e98a0ec56f8949872d966e5ffdd01a81e6b6115768f1992bd81be7a56 --- src/init.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/init.cpp b/src/init.cpp index 618cf3a478..8c444a42ae 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -570,7 +570,7 @@ void SetupServerArgs(ArgsManager& argsman) argsman.AddArg("-maxconnections=", strprintf("Maintain at most connections to peers (temporary service connections excluded) (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=", strprintf("Maximum per-connection receive buffer, *1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); argsman.AddArg("-maxsendbuffer=", strprintf("Maximum per-connection memory usage for the send buffer, *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 peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), 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=", 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=", "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=", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);