backport: Merge bitcoin#24237, 23631, 24609 #6309
Open
+10
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vijaydasmp
force-pushed
the
bp23_2024_10_06
branch
from
ace2451
to
2cc9ba4
Compare
vijaydasmp
force-pushed
the
bp23_2024_10_06
branch
from
2cc9ba4
to
545dffe
Compare
vijaydasmp
changed the title
knst
reviewed
Nov 25, 2024
| @@ -3651,7 +3651,11 @@ void PeerManagerImpl::ProcessMessage( | |||
|
|
|||
| int64_t nTimeOffset = nTime - GetTime(); | |||
| pfrom.nTimeOffset = nTimeOffset; | |||
| AddTimeData(pfrom.addr, nTimeOffset); | |||
| if (!pfrom.IsInboundConn()) { | |||
There was a problem hiding this comment.
pick also bitcoin#24609 which update documentation also; this 2 backports should be merged together or one-after-one
vijaydasmp
changed the title
knst
previously approved these changes
Nov 26, 2024
UdjinM6
requested changes
Nov 26, 2024
src/init.cpp
Outdated
| @@ -570,7 +570,7 @@ void SetupServerArgs(ArgsManager& argsman) | |||
| argsman.AddArg("-maxconnections=<n>", strprintf("Maintain at most <n> 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=<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 memory usage for the 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 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); | |||
There was a problem hiding this comment.
24609:
Suggested change
| 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("-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); |
fad8154 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 fad8154 Tree-SHA512: 0f9e91617dfb6ceda99831e6cf4b4bf0d951054957c159b1a05a178ab6090798fae7368edefe12800da24585bcdf7299ec3534f4d3bbf5ce6a6eca74dd3bb766
… Adjusted Time 0c85dc3 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 dashpay#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 0c85dc3 naumenkogs: ACK 0c85dc3 vasild: ACK 0c85dc3 Tree-SHA512: 2d6375305bcae034d68b58b7a07777b40ac430dfed554c88e681a048c527536691e1b7d08c0ef995247d356f8e81aa0a4b983bf2674faf6a416264e5f1af0a96
vijaydasmp
force-pushed
the
bp23_2024_10_06
branch
from
7fb9ac2
to
60a32a3
Compare
…ers influence timedata 1bba72d Clarify in -maxtimeadjustment that only outbound peers influence time data (Jon Atack) Pull request description: bitcoin#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 1bba72d mzumsande: code Review ACK 1bba72d brunoerg: crACK 1bba72d Tree-SHA512: ad610ab3038fb83134e21d31cca952ef9ac926e88992ff93023b7010f2499f9a4d952e8e98a0ec56f8949872d966e5ffdd01a81e6b6115768f1992bd81be7a56
vijaydasmp
force-pushed
the
bp23_2024_10_06
branch
from
60a32a3
to
e237301
Compare
UdjinM6
approved these changes
Nov 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
bitcoin backports