Skip to content
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

Jump to bottom

net: correctly initialize nMinPingUsecTime #6636

Merged
merged 1 commit into from
Sep 4, 2015
Merged

net: correctly initialize nMinPingUsecTime #6636

merged 1 commit into from
Sep 4, 2015

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Sep 4, 2015

nMinPingUsecTime was left uninitialized in CNode.
The correct initialization for a minimum-until-now is int64_t's max value, so initialize it to that.
Thanks @MarcoFalke for noticing.

@laanwj laanwj added the P2P label Sep 4, 2015
@paveljanik
Copy link
Contributor

tested ACK
It could be shocking for some people to see:

    "minping": 0.39083,
    "minping": 0.139336,
    "minping": 9223372036854.775,

;-)

@sipa
Copy link
Member

sipa commented Sep 4, 2015

Untested ACK.

However, if you care for bikeshedding: how about returning null, or not even including the entry if there is no result?

@laanwj
Copy link
Member Author

laanwj commented Sep 4, 2015

I think there's some confusion here: I'm fine with changing what is returned on RPC, but that's not the point of this pull. RPC code isn't even touched.
The idea is to fix the connection slot DoS exhaustion #6374, which uses this newly-introduced CNode field.

@laanwj laanwj force-pushed the 2015_09_initialize_minpingtime branch 2 times, most recently from 6a3d736 to 93ff1b9 Compare September 4, 2015 14:27
@laanwj
net: correctly initialize nMinPingUsecTime
93ff1b9 
`nMinPingUsecTime` was left uninitialized in CNode.
The correct initialization for a minimum-until-now is int64_t's max value, so initialize it to that.
Thanks @MarcoFalke for noticing.
@sipa
Copy link
Member

sipa commented Sep 4, 2015 via email

@sipa sipa mentioned this pull request Sep 4, 2015
Closed
@laanwj laanwj merged commit 93ff1b9 into bitcoin:master Sep 4, 2015
laanwj added a commit that referenced this pull request Sep 4, 2015
@laanwj
Merge pull request #6636
536207f 
93ff1b9 net: correctly initialize nMinPingUsecTime (Wladimir J. van der Laan)
@maflcko
Copy link
Member

maflcko commented Sep 4, 2015

tested ACK (3f99d70 which is about the same as the merged one)

@laanwj laanwj mentioned this pull request Apr 8, 2016
Closed
@defuse defuse mentioned this pull request Aug 16, 2016
Open
@maflcko maflcko mentioned this pull request Jan 5, 2019
Merged
laanwj added a commit that referenced this pull request Jan 9, 2019
@laanwj
Merge #15109: refactor: Use C++11 default member initializers
3f12515 
fa2510d Use C++11 default member initializers (MarcoFalke)

Pull request description:

  Changes:
  * Remove unused constructors that leave some members uninitialized
  * Remove manual initialization in each constructor and prefer C++11 default member initializers

  This is not a stylistic change, but a change that avoids bugs such as:

  *  fix uninitialized read when stringifying an addrLocal #14728
  *  qt: Initialize members in WalletModel #12426
  *  net: correctly initialize nMinPingUsecTime #6636
  * ...

Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b
@maflcko maflcko mentioned this pull request Jan 10, 2019
Merged
laanwj added a commit that referenced this pull request Jan 14, 2019
@laanwj
Merge #15144: [refactor] CNode: Use C++11 default member initializers
070eaf7 
fac2f5e Use C++11 default member initializers (MarcoFalke)

Pull request description:

  The second and last change on this topic (c.f. #15109). Split up because the diff would otherwise interleave, making review harder than necessary.

  This is not a stylistic change, but a change that avoids bugs such as:

  *  fix uninitialized read when stringifying an addrLocal #14728
  *  qt: Initialize members in WalletModel #12426
  *  net: correctly initialize nMinPingUsecTime #6636
  * ...

Tree-SHA512: 547ae72b87aeaed5890eb5fdcff612bfc93354632b238d89e1e1c0487187f39609bcdc537ef21345e0aea8cfcf1ea48da432d672c5386dd87cf58742446a86b1
@maflcko maflcko mentioned this pull request Oct 9, 2019
Closed
@Fuzzbawls Fuzzbawls mentioned this pull request Jun 23, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Jul 1, 2020
@random-zebra
Merge #1704: [Net] Pre-requirements for network encapsulation
8bbc065 
6f41b3e [QA] Missing mempool sync in pos_coldStaking and zc_publicspends tests (random-zebra)
efaf727 net: correctly initialize nMinPingUsecTime (Wladimir J. van der Laan)
61c8ffe Do not add random inbound peers to addrman. (Gregory Maxwell)
e6a1726 Added feeler connections increasing good addrs in the tried table. (Ethan Heilman)
070b6fb Actually only use filterInventoryKnown with MSG_TX inventory messages. (Gregory Maxwell)
9ac6b28 Only use filterInventoryKnown with MSG_TX inventory messages. (Patick Strateman)
01273db Rename setInventoryKnown filterInventoryKnown (Patick Strateman)
4f11eb2 Remove mruset as it is no longer used. (Gregory Maxwell)
2e3b05c Replace setInventoryKnown with a rolling bloom filter. (Gregory Maxwell)
409aa83 Replace trickle nodes with per-node/message Poisson delays (Pieter Wuille)
93e8c46 Move recentRejects initialization to top of InitBlockIndex (Wladimir J. van der Laan)
9a59420 Keep track of recently rejected transactions (Peter Todd)
34ee777 Only use randomly created nonces in CRollingBloomFilter. (Pieter Wuille)
338d346 Make CRollingBloomFilter set nTweak for you (Peter Todd)
dcd15bc Reuse vector hashing code for uint256 (Pieter Wuille)
3230143 Add uint256 support to CRollingBloomFilter (Peter Todd)
128d644 Better mruset unit test (Pieter Wuille)
89740ed Use ring buffer of set iterators instead of deque of copies in mruset (Pieter Wuille)
14c88ee Replace mruset setAddrKnown with CRollingBloomFilter addrKnown (Gavin Andresen)
e0bebbd Rolling bloom filter class (Gavin Andresen)
7c03bd5 Add correct bool combiner for net signals (Pieter Wuille)
5cb5fd6 Stop exporting ConnectNode (Fuzzbawls)
819295d Stop using ConnectNode in layer 2 code (Fuzzbawls)
851b6b4 net: No need to export DumpBanlist (Cory Fields)
4486d4e net: make Ban/Unban/ClearBan functionality consistent (Cory Fields)
a5e278d net: Drop CNodeRef for AttemptToEvictConnection (Cory Fields)
9fd357d net: use the exposed GetNodeSignals() rather than g_signals directly (Cory Fields)
7962bcc net: remove unused set (Cory Fields)
fabf358 Use network group instead of CNetAddr in final pass to select node to disconnect (Patrick Strateman)
7f030fe Fix comment (Patrick Strateman)
18af800 Acquire cs_vNodes before changing refrence counts (Patrick Strateman)
7aa827f CNodeRef copy constructor and assignment operator (Patrick Strateman)
b3f95e7 Return false early if vEvictionCandidates is empty (Patrick Strateman)
85886c9 Better support for nodes with non-standard nMaxConnections (Patrick Strateman)
9c9e55b RAII wrapper for CNode* (Patrick Strateman)
e92780d Add comments to AttemptToEvictConnection (Patrick Strateman)
0ca7ce3 Prefer to disconnect peers in favor of whitelisted peers (Patrick Strateman)
a1c4aaf AttemptToEvictConnection (Patrick Strateman)
aa7ce9b Record nMinPingUsecTime (Patrick Strateman)
fd7bab0 Refactor: Move failure conditions to the top of AcceptConnection (Patrick Strateman)
fcb732b Refactor: Bail early in AcceptConnection (Patrick Strateman)
411766d Refactor: AcceptConnection (Patrick Strateman)

Pull request description:

  This is a culmination of several upstream PRs touching the P2P/Networking code, and resulting in a state just prior to the P2P/Network encapsulation, which will be it's own PR.

  Backported upstream PRs included here:
  - bitcoin#5859
  - bitcoin#6064
  - bitcoin#6374
  - bitcoin#6498
  - bitcoin#6636
  - bitcoin#7133
  - bitcoin#7125
  - bitcoin#7906
  - bitcoin#8282
  - bitcoin#8594

ACKs for top commit:
  random-zebra:
    Great job. ACK 6f41b3e and merging...
  furszy:
    Have been running it the past days and all is looking good, ACK 6f41b3e .

Tree-SHA512: 1cc4b1271516f000a06141b5b069f3ee00f3eb77056e40d2c021c484f749d9d8db2b76ce490f63572372705b646fad342666f6f90ca5fc69abcacf7b207d058f
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 9, 2021
@laanwj @pravblockc
Merge bitcoin#15109: refactor: Use C++11 default member initializers
a7a5a1c 
fa2510d Use C++11 default member initializers (MarcoFalke)

Pull request description:

  Changes:
  * Remove unused constructors that leave some members uninitialized
  * Remove manual initialization in each constructor and prefer C++11 default member initializers

  This is not a stylistic change, but a change that avoids bugs such as:

  *  fix uninitialized read when stringifying an addrLocal bitcoin#14728
  *  qt: Initialize members in WalletModel bitcoin#12426
  *  net: correctly initialize nMinPingUsecTime bitcoin#6636
  * ...

Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 11, 2021
@laanwj @pravblockc
Merge bitcoin#15109: refactor: Use C++11 default member initializers
8e21d90 
fa2510d Use C++11 default member initializers (MarcoFalke)

Pull request description:

  Changes:
  * Remove unused constructors that leave some members uninitialized
  * Remove manual initialization in each constructor and prefer C++11 default member initializers

  This is not a stylistic change, but a change that avoids bugs such as:

  *  fix uninitialized read when stringifying an addrLocal bitcoin#14728
  *  qt: Initialize members in WalletModel bitcoin#12426
  *  net: correctly initialize nMinPingUsecTime bitcoin#6636
  * ...

Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 11, 2021
@laanwj @pravblockc
Merge bitcoin#15109: refactor: Use C++11 default member initializers
95d588e 
fa2510d Use C++11 default member initializers (MarcoFalke)

Pull request description:

  Changes:
  * Remove unused constructors that leave some members uninitialized
  * Remove manual initialization in each constructor and prefer C++11 default member initializers

  This is not a stylistic change, but a change that avoids bugs such as:

  *  fix uninitialized read when stringifying an addrLocal bitcoin#14728
  *  qt: Initialize members in WalletModel bitcoin#12426
  *  net: correctly initialize nMinPingUsecTime bitcoin#6636
  * ...

Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b
5tefan pushed a commit to 5tefan/dash that referenced this pull request Aug 12, 2021
@laanwj @5tefan
Merge bitcoin#15109: refactor: Use C++11 default member initializers
bfba8f2 
fa2510d Use C++11 default member initializers (MarcoFalke)

Pull request description:

  Changes:
  * Remove unused constructors that leave some members uninitialized
  * Remove manual initialization in each constructor and prefer C++11 default member initializers

  This is not a stylistic change, but a change that avoids bugs such as:

  *  fix uninitialized read when stringifying an addrLocal bitcoin#14728
  *  qt: Initialize members in WalletModel bitcoin#12426
  *  net: correctly initialize nMinPingUsecTime bitcoin#6636
  * ...

Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@laanwj @paveljanik @sipa @maflcko