From e33cc642a966ffff03e5ae5033487b9d1e1f9baa Mon Sep 17 00:00:00 2001
From: "isabel@lotus" <isabel@izabyl.com>
Date: Thu, 8 Nov 2018 11:51:54 -0800
Subject: [PATCH] review comments

---
 pbft/pbft.cpp           |  6 ++++++
 pbft/pbft_operation.cpp | 26 +++++++++++++-------------
 pbft/pbft_operation.hpp | 22 +++++++++++-----------
 3 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/pbft/pbft.cpp b/pbft/pbft.cpp
index fffcf685..10482cdf 100644
--- a/pbft/pbft.cpp
+++ b/pbft/pbft.cpp
@@ -78,6 +78,12 @@ pbft::start()
                         [weak_this = this->weak_from_this(), fd = this->failure_detector]
                                 (std::shared_ptr<pbft_operation> op)
                                         {
+                                            if (!op)
+                                            {
+                                                // TODO: Get real pbft_operation pointers from pbft_service
+                                                LOG(error) << "Ignoring null operation pointer recieved from pbft_service";
+                                            }
+
                                             fd->request_executed(op->request_hash);
 
                                             if (op->sequence % CHECKPOINT_INTERVAL == 0)
diff --git a/pbft/pbft_operation.cpp b/pbft/pbft_operation.cpp
index c1619df0..4b896688 100644
--- a/pbft/pbft_operation.cpp
+++ b/pbft/pbft_operation.cpp
@@ -18,22 +18,22 @@
 
 using namespace bzn;
 
-pbft_operation::pbft_operation(uint64_t view, uint64_t sequence, bzn::hash_t request_hash, std::shared_ptr<const std::vector<peer_address_t>> peers)
+pbft_operation::pbft_operation(uint64_t view, uint64_t sequence, const bzn::hash_t& request_hash, std::shared_ptr<const std::vector<peer_address_t>> peers)
         : view(view)
           , sequence(sequence)
-          , request_hash(std::move(request_hash))
+          , request_hash(request_hash)
           , peers(std::move(peers))
 {
 }
 
 bool
-pbft_operation::has_request()
+pbft_operation::has_request() const
 {
     return this->request_saved;
 }
 
 const pbft_request&
-pbft_operation::get_request()
+pbft_operation::get_request() const
 {
     if (!this->request_saved)
     {
@@ -44,7 +44,7 @@ pbft_operation::get_request()
 }
 
 const bzn::encoded_message&
-pbft_operation::get_encoded_request()
+pbft_operation::get_encoded_request() const
 {
     if (!this->request_saved)
     {
@@ -76,7 +76,7 @@ pbft_operation::record_preprepare(const bzn_envelope& /*encoded_preprepare*/)
 }
 
 bool
-pbft_operation::has_preprepare()
+pbft_operation::has_preprepare() const
 {
     return this->preprepare_seen;
 }
@@ -95,7 +95,7 @@ pbft_operation::faulty_nodes_bound() const
 }
 
 bool
-pbft_operation::is_prepared()
+pbft_operation::is_prepared() const
 {
     return this->has_request() && this->has_preprepare() && this->prepares_seen.size() > 2 * this->faulty_nodes_bound();
 }
@@ -107,7 +107,7 @@ pbft_operation::record_commit(const bzn_envelope& encoded_commit)
 }
 
 bool
-pbft_operation::is_committed()
+pbft_operation::is_committed() const
 {
     return this->is_prepared() && this->commits_seen.size() > 2 * this->faulty_nodes_bound();
 }
@@ -135,19 +135,19 @@ pbft_operation::end_commit_phase()
 }
 
 operation_key_t
-pbft_operation::get_operation_key()
+pbft_operation::get_operation_key() const
 {
-    return std::tuple<uint64_t, uint64_t, hash_t>(this->view, this->sequence, this->request_hash);
+    return {this->view, this->sequence, this->request_hash};
 }
 
 pbft_operation_state
-pbft_operation::get_state()
+pbft_operation::get_state() const
 {
     return this->state;
 }
 
 std::string
-pbft_operation::debug_string()
+pbft_operation::debug_string() const
 {
     return boost::str(boost::format("(v%1%, s%2%) - %3%") % this->view % this->sequence % this->parsed_request.ShortDebugString());
 }
@@ -159,7 +159,7 @@ pbft_operation::set_session(std::weak_ptr<bzn::session_base> session)
 }
 
 std::weak_ptr<bzn::session_base>
-pbft_operation::session()
+pbft_operation::session() const
 {
     return this->listener_session;
 }
diff --git a/pbft/pbft_operation.hpp b/pbft/pbft_operation.hpp
index a824e69e..ed4bad90 100644
--- a/pbft/pbft_operation.hpp
+++ b/pbft/pbft_operation.hpp
@@ -39,38 +39,38 @@ namespace bzn
     {
     public:
 
-        pbft_operation(uint64_t view, uint64_t sequence, bzn::hash_t request_hash, std::shared_ptr<const std::vector<peer_address_t>> peers);
+        pbft_operation(uint64_t view, uint64_t sequence, const bzn::hash_t& request_hash, std::shared_ptr<const std::vector<peer_address_t>> peers);
 
         void set_session(std::weak_ptr<bzn::session_base>);
 
-        operation_key_t get_operation_key();
-        pbft_operation_state get_state();
+        operation_key_t get_operation_key() const;
+        pbft_operation_state get_state() const;
 
         void record_preprepare(const bzn_envelope& encoded_preprepare);
-        bool has_preprepare();
+        bool has_preprepare() const;
 
         void record_prepare(const bzn_envelope& encoded_prepare);
-        bool is_prepared();
+        bool is_prepared() const;
 
         void record_commit(const bzn_envelope& encoded_commit);
-        bool is_committed();
+        bool is_committed() const;
 
         void begin_commit_phase();
         void end_commit_phase();
 
-        std::weak_ptr<bzn::session_base> session();
+        std::weak_ptr<bzn::session_base> session() const;
 
-        const pbft_request& get_request();
-        const bzn::encoded_message& get_encoded_request();
+        const pbft_request& get_request() const;
+        const bzn::encoded_message& get_encoded_request() const;
 
         void record_request(const bzn::encoded_message& encoded_request);
-        bool has_request();
+        bool has_request() const;
 
         const uint64_t view;
         const uint64_t sequence;
         const bzn::hash_t request_hash;
 
-        std::string debug_string();
+        std::string debug_string() const;
 
         size_t faulty_nodes_bound() const;