Extract p2p::FinalizerCommits component #753

frolosofsky · 2019-03-07T15:03:12Z

Signed-off-by: Stanislav Frolov [email protected]

This PR extracts basement changes from #744. No logical changes in the code.

Move p2p commits handlers from src/finalizarion/p2p* to src/p2p/finalizer_commits*.
Rename commits to FinalizerCommits in type and file names.
Pack commits handlers in the component FinalizerCommits.
Adjust "relaxing" of the code by newlines to ease merge with Implement commits full sync #744.
Adds PushMessage helper function to net_processing.

Signed-off-by: Stanislav Frolov <[email protected]>

src/p2p/finalizer_commits_impl.h

src/p2p/finalizer_commits.h

src/injector.h

src/p2p/finalizer_commits_types.cpp

src/p2p/finalizer_commits_impl.h

src/p2p/finalizer_commits_impl.cpp

src/p2p/finalizer_commits_types.h

src/p2p/finalizer_commits_impl.cpp

frolosofsky · 2019-03-07T17:40:56Z

@scravy While answering your questions I had to refer the parent PR #744 plenty of times. Maybe it makes sense to close this PR and keep the changeset as part of #744?

Signed-off-by: Stanislav Frolov <[email protected]>

scravy · 2019-03-11T08:44:57Z

While answering your questions I had to refer the parent PR #744 plenty of times. Maybe it makes sense to close this PR and keep the changeset as part of #744?

I for one do not mind either way. Of course it's always nicer to have smaller pieces to review, but if the individual pieces do not make sense. I am mostly just wondering about tests and TODOs. Since it is a component it should be testable as a unit. If it's simpler to have it as part of #744, well then.

frolosofsky · 2019-03-11T11:09:55Z

I ported all the fix to the main PR. Feel free to review them #744.

Gnappuraz

utACK 67c5e83

kostyantyn · 2019-03-12T16:20:52Z

src/p2p/finalizer_commits_handler.h

+class FinalizerCommitsHandler {
+ public:
+  //! \brief Process getcommits message
+  virtual void OnGetCommits(


WDYS about Process<message name> pattern name? Basically, call it ProcessGetCommits as this is the function which processes getcommits p2p message. It matches quite well with the existing functions we have e,g. ProcessNewBlock, with the comment above and is invoked in the net_processing.cpp file.

For me, OnGetCommits looks like the event that was produced by ProcessGetCommits function and other components can subscribe to.

Although this function processes getcommits message, the component is designed like a handler that responses on network events so that on_<message> sounds reasonable to me. Also, in python, we use on_<message> pattern. After all, every function processes something...

But, If others want to rename it to Processes<message>, I will rename.

kostyantyn · 2019-03-12T16:36:22Z

src/p2p/finalizer_commits_handler_impl.cpp

+      }
+      best_index = index;
+
+    } else if (index->nHeight > best_index->nHeight && m_active_chain->Contains(*index)) {


&& m_active_chain->Contains(*index) can be ommited as we get index from m_active_chain so it must be there.

src/p2p/finalizer_commits_handler_impl.cpp

kostyantyn · 2019-03-12T17:10:03Z

src/p2p/finalizer_commits_handler_impl.cpp

+
+  FinalizerCommitsResponse response;
+  do {
+    walk = m_active_chain->AtHeight(walk->nHeight + 1);


nit: CChain has Next function, could be used instead of manually computing the height.

It's staking::ActiveChain component which doesn't have such function. Will extend and update this code.

kostyantyn · 2019-03-12T17:38:47Z

src/p2p/finalizer_commits_handler.h

+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef UNITE_P2P_FINALIZER_COMMITS_HANDLER


Include guards usually end with _H. Would be good to keep the same pattern as in other files.

Hmm anybody cares about such a "pattern"? Will do!

kostyantyn · 2019-03-12T17:43:17Z

src/p2p/finalizer_commits_handler_impl.h

+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef UNITE_P2P_FINALIZER_COMMITS_HANDLER_IMPL


would be good to add _H to the include guard to keep the same naming.

kostyantyn · 2019-03-12T17:49:46Z

src/p2p/finalizer_commits_handler_impl.h

+
+namespace p2p {
+
+class FinalizerCommitsHandlerImpl : public ::p2p::FinalizerCommitsHandler {


I see we put implementation classes directly to the cpp. Then we can hide it even inside the anonymous namespace. What is the reason for exposing FinalizerCommitsHandlerImpl?

nit: ::p2p:: can be ommited.

The reason is the unit tests.

::p2p:: can be ommited.

kostyantyn · 2019-03-12T17:51:58Z

src/p2p/finalizer_commits_types.h

 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.

-#ifndef UNITE_FINALIZATION_P2P
-#define UNITE_FINALIZATION_P2P
+#ifndef UNITE_P2P_FINALIZER_COMMITS_TYPES


would be good to add _H to include guard

src/p2p/finalizer_commits_handler_impl.cpp

frolosofsky · 2019-03-13T13:03:27Z

I guess we decided to have solid #744 instead of splitting.

Extract p2p::FinalizerCommits component

6999b3a

Signed-off-by: Stanislav Frolov <[email protected]>

frolosofsky added the refactoring Changes which clean up code but don't change the user-visible behavior label Mar 7, 2019

frolosofsky added this to the 0.1 milestone Mar 7, 2019

frolosofsky self-assigned this Mar 7, 2019

frolosofsky requested review from AM5800 and a team March 7, 2019 15:03

frolosofsky mentioned this pull request Mar 7, 2019

Implement commits full sync #744

Merged

scravy reviewed Mar 7, 2019

View reviewed changes

frolosofsky added 7 commits March 8, 2019 08:17

renames

b7f5ca9

Signed-off-by: Stanislav Frolov <[email protected]>

unit tests

d8884f9

Signed-off-by: Stanislav Frolov <[email protected]>

todos

d247c09

Signed-off-by: Stanislav Frolov <[email protected]>

Change description

727ce5e

Signed-off-by: Stanislav Frolov <[email protected]>

from disk

d43e85f

Signed-off-by: Stanislav Frolov <[email protected]>

add class description

d761764

Signed-off-by: Stanislav Frolov <[email protected]>

fix compilation error

67c5e83

Signed-off-by: Stanislav Frolov <[email protected]>

Gnappuraz approved these changes Mar 12, 2019

View reviewed changes

kostyantyn reviewed Mar 12, 2019

View reviewed changes

src/p2p/finalizer_commits_handler_impl.cpp Show resolved Hide resolved

kostyantyn reviewed Mar 12, 2019

View reviewed changes

src/p2p/finalizer_commits_handler_impl.cpp Show resolved Hide resolved

frolosofsky closed this Mar 13, 2019

frolosofsky deleted the p2p-finalizer-commits branch April 1, 2019 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract p2p::FinalizerCommits component #753

Extract p2p::FinalizerCommits component #753

frolosofsky commented Mar 7, 2019

frolosofsky commented Mar 7, 2019

scravy commented Mar 11, 2019

frolosofsky commented Mar 11, 2019

Gnappuraz left a comment

kostyantyn Mar 12, 2019

frolosofsky Mar 12, 2019

kostyantyn Mar 12, 2019

kostyantyn Mar 12, 2019

frolosofsky Mar 12, 2019

kostyantyn Mar 12, 2019

frolosofsky Mar 12, 2019

kostyantyn Mar 12, 2019

kostyantyn Mar 12, 2019

frolosofsky Mar 12, 2019

scravy Mar 13, 2019

kostyantyn Mar 12, 2019

frolosofsky commented Mar 13, 2019


		namespace p2p {

		class FinalizerCommitsHandlerImpl : public ::p2p::FinalizerCommitsHandler {

Extract p2p::FinalizerCommits component #753

Extract p2p::FinalizerCommits component #753

Conversation

frolosofsky commented Mar 7, 2019

frolosofsky commented Mar 7, 2019

scravy commented Mar 11, 2019

frolosofsky commented Mar 11, 2019

Gnappuraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frolosofsky commented Mar 13, 2019