Handle receiving a Pong with changed NodeID - #5452
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5452 +/- ##
==========================================
+ Coverage 61.83% 61.88% +0.04%
==========================================
Files 345 345
Lines 28745 28784 +39
Branches 3261 3263 +2
==========================================
+ Hits 17775 17813 +38
- Misses 9812 9816 +4
+ Partials 1158 1155 -3 |
|
Windows build timed out |
|
Rebased, please review @chfast @halfalicious |
| protected: | ||
| /** | ||
| * NodeValidation is used to record Pinged node's endpoint, the timepoint of sent PING, | ||
| * NodeValidation is used to record Pinged node's ID, the timepoint of sent PING, |
There was a problem hiding this comment.
"the timepoint of sent PING" and "time of sending" is redundant
| @@ -322,7 +322,7 @@ void NodeTable::ping(Node const& _node, shared_ptr<NodeEntry> _replacementNodeEn | |||
| return; | |||
|
|
|||
| // Don't sent Ping if one is already sent | |||
There was a problem hiding this comment.
Not your changes, but minor grammar issue (1st sent should be send)
|
|
||
| BOOST_REQUIRE(nodeTable->nodeExists(newNodeKeyPair.pub())); | ||
| auto addedNode = nodeTable->nodeEntry(newNodePubKey); | ||
| BOOST_CHECK(addedNode->lastPongReceivedTime > 0); |
There was a problem hiding this comment.
Shouldn't this (and the rest of the BOOST_CHECKs in this test case) be BOOST_REQUIRE? Or is the reasoning that if the checks failed other tests would fail (in which case is there value in having the checks at all)?
There was a problem hiding this comment.
The reasoning is that updating the node in the node table, updating m_sentPings and removing the old node are kind of independent pieces of code, and if one fails, we can still get some useful info about whether other ones work
There was a problem hiding this comment.
The reasoning is that updating the node in the node table, updating
m_sentPingsand removing the old node are kind of independent pieces of code, and if one fails, we can still get some useful info about whether other ones work
Yes, but I think the checks will only provide value when you do a local run (let me know if I'm wrong here)? The problem is that failed boost_checks don't trigger test failures and boost_require triggers a test failure and ceases test execution and the boost_checks are all after a boost_require, meaning that if the boost_require fails the subsequent boost_checks won't be executed, and if the boost_require passes the boost_checks might fail but won't trigger a test failure (so we won't know to look at the logs)? Also, do boost_check failures show up in CircleCI logs even if the tests themselves passed?
Can we place the boost_checks before the boost_require?
There was a problem hiding this comment.
I believe BOOST_CHECK failures always show up in logs.
|
@halfalicious Thank you for the review, all comments addressed |
| // (answers with Pong), then it will be added to the bucket of the node table | ||
| uint16_t tcpPort = 0; | ||
| // Time we sent Ping - used to handle timeouts | ||
| TimePoint pingSendTime; |
There was a problem hiding this comment.
(Nit) I think this should be called pingSentTime (since it's referring to a past event)
|
Approved, remaining issues aren't blockers |
4 participants
Fixes #5431