QCop autonomously sends deregisters #105

Doy-lee · 2018-07-24T02:31:28Z

Quorum cop takes the core and service node list to query the quorum and begins voting off if it's participating in the quorum for nodes up to the (latest height - some safety buffer) if no ping has been seen since some threshold.

Nodes ping every 1 hour and also send out a ping on demand if they detect that they are included in the quorum for the latest block height.

There's an implicit dependency in Quorum Cop that it relies on Service Node Lists to have finished calculating the quorum state before Quorum Cop starts voting otherwise it receives an empty quorum.
I've chosen to enforce this by making it that the hooks for service node lists have to be registered explicitly and requires you to pass in the quorum cop.

Doy-lee · 2018-07-24T02:34:30Z

src/cryptonote_core/quorum_cop.cpp

+      return false;
+
+    if (!m_service_node_list.is_service_node(pubkey))
+      return false;


The only dependency on service_node_lists. Once we get db storage of service node lists we can remove this and check if the pubkey belongs to some range of heights.

I can imagine we'll want to store uptime proof over some range of heights so we have the tools to provide statistics on the network.

jackmurray90 · 2018-07-24T03:46:05Z

src/cryptonote_core/quorum_cop.cpp

+                ", which is greater than the recommended REORG_SAFETY_BUFFER_IN_BLOCKS: " << REORG_SAFETY_BUFFER_IN_BLOCKS);
+    }
+
+    m_last_height = (height < REORG_SAFETY_BUFFER_IN_BLOCKS) ? height : height - REORG_SAFETY_BUFFER_IN_BLOCKS;


m_last_height should not be updated in the event of a blockchain detach

What's the rationale? You need to start voting from the height onwards again. This probably should be m_last_height = height; because you may not have the vote data anymore depending on the height you popped back to.

m_last_height may be from well before the blockchain detach height.

Those blocks still need to be checked.

if m_last_height >= blockchain detach height then we have problems with our assumption that large reorgs will not happen

jackmurray90 · 2018-07-24T03:48:13Z

src/cryptonote_core/quorum_cop.cpp

+  void quorum_cop::blockchain_detached(uint64_t height)
+  {
+    uint64_t delta_height = std::abs(static_cast<int64_t>(m_core.get_current_blockchain_height() - height));
+    if (delta_height > REORG_SAFETY_BUFFER_IN_BLOCKS)


m_core.get_current_blockchain_height() should always be >= height, I think...

Changed to use m_last_height. Height will be the height that the chain split at and blockchain has already been popped back to the split height.

Should be

if (m_last_height >= height) {

Warn user; adjust m_last_height; }

jackmurray90 · 2018-07-24T03:50:49Z

src/cryptonote_core/quorum_cop.cpp

+    m_last_height = (height < REORG_SAFETY_BUFFER_IN_BLOCKS) ? height : height - REORG_SAFETY_BUFFER_IN_BLOCKS;
+  }
+
+  static bool participates_in_quorum(const std::vector<crypto::public_key>& quorum, crypto::public_key const &my_key, size_t *index_in_quorum = nullptr)


The written function returns true if the pubkey does not exist in the quorum. I don't think this is intended behaviour.

Implement or replace entirely with a call to std::find(). You can get the index by subtracting vec.begin() from the returned iterator

Fixed the bool return.

jackmurray90 · 2018-07-24T04:02:41Z

src/cryptonote_core/quorum_cop.cpp

+    if (height < execute_justice_from_height)
+      return;
+
+    if (height == latest_height)


We don't need this branch. The uptime proofs are submitted every hour by the daemon when it is idle, not just when it is in the list of nodes to test.

jackmurray90 · 2018-07-24T04:37:26Z

src/cryptonote_core/quorum_cop.cpp

+        bool vote_off_node = false;
+
+        CRITICAL_REGION_LOCAL(m_lock);
+        auto const it = m_uptime_proof_seen.find(node_key);


vote_off_node = (this daemon has been running for at least two hours && m_uptime_proof_seen.find(node_key) == m_uptime_proof_seen.end());

Somewhere else in the code we must remove old entries in the m_uptime_proof_seen table.

jackmurray90 · 2018-07-24T04:43:11Z

src/cryptonote_core/quorum_cop.cpp

+      }
+    }
+
+    prune_uptime_proof_below(execute_justice_from_height);


I would triggered this during idle in m_core, say once every 30 seconds. Works fine here too, just doesn't feel right because the uptime proof thing is not happening related to the blockchain. This also means the block height stuff in this function is unnecessary - you just have to remove them as long as their timestamp < now - 2 hours and 5 minutes

jackmurray90 · 2018-07-24T04:46:26Z

src/cryptonote_core/quorum_cop.cpp

+      return false;
+
+    if (!m_service_node_list.is_service_node(pubkey))
+      return false;


jackmurray90 · 2018-07-24T04:51:08Z

src/cryptonote_core/quorum_cop.cpp

+      return;
+    }
+
+    const uint64_t prune_from_timestamp = blocks.front().timestamp;


prune_from_timestamp = now() - 2 hours 5 minutes is fine

jackmurray90 · 2018-07-24T04:59:00Z

src/cryptonote_core/quorum_cop.cpp

+    CRITICAL_REGION_LOCAL(m_lock);
+
+    for (auto it = m_uptime_proof_seen.begin(); it != m_uptime_proof_seen.end();)
+      it = (it->second < prune_from_timestamp) ? m_uptime_proof_seen.erase(it) : it;


This is an infinite loop.

change to:

if (it->second < prune_from_timestamp) it = m_uptime_proof_seen.erase(it); else it++;

Alternatively just add ++it to the second branch in your ternary operator. Not sure if this is defined behaviour.

Doy-lee · 2018-07-24T07:02:55Z

src/cryptonote_core/quorum_cop.cpp

+    m_last_height = (height < REORG_SAFETY_BUFFER_IN_BLOCKS) ? height : height - REORG_SAFETY_BUFFER_IN_BLOCKS;
+  }
+
+  static bool participates_in_quorum(const std::vector<crypto::public_key>& quorum, crypto::public_key const &my_key, size_t *index_in_quorum = nullptr)


Fixed the bool return.

Doy-lee · 2018-07-24T07:04:12Z

src/cryptonote_core/quorum_cop.cpp

+  void quorum_cop::blockchain_detached(uint64_t height)
+  {
+    uint64_t delta_height = std::abs(static_cast<int64_t>(m_core.get_current_blockchain_height() - height));
+    if (delta_height > REORG_SAFETY_BUFFER_IN_BLOCKS)


Changed to use m_last_height. Height will be the height that the chain split at and blockchain has already been popped back to the split height.

Doy-lee · 2018-07-24T07:07:23Z

src/cryptonote_core/quorum_cop.cpp

+                ", which is greater than the recommended REORG_SAFETY_BUFFER_IN_BLOCKS: " << REORG_SAFETY_BUFFER_IN_BLOCKS);
+    }
+
+    m_last_height = (height < REORG_SAFETY_BUFFER_IN_BLOCKS) ? height : height - REORG_SAFETY_BUFFER_IN_BLOCKS;


What's the rationale? You need to start voting from the height onwards again. This probably should be m_last_height = height; because you may not have the vote data anymore depending on the height you popped back to.

Doy-lee · 2018-07-24T07:11:28Z

src/cryptonote_core/quorum_cop.cpp

+        const crypto::public_key &node_key = state->nodes_to_test[node_index];
+
+        CRITICAL_REGION_LOCAL(m_lock);
+        bool vote_off_node = (m_uptime_proof_seen.find(node_key) != m_uptime_proof_seen.end());


Changed to just find the key, since the entries in the list should be under UPTIME_PROOF_MAX_IN_SECONDS +- 30seconds (depending on the pruner being called every 30s on idle).

I would add a buffer. Right now there is a chance that we will drop the ping from 1 hour ago, and then this evaluation happens before the next ping arrives.

If worked this out in my head correctly, this change would just require L155 to change from

if (m_uptime_proof_seen[pubkey] > now - UPTIME_PROOF_MAX_TIME_IN_SECONDS) return false; // already received one uptime proof for this node recently.

to

if (m_uptime_proof_seen[pubkey] >= now - (UPTIME_PROOF_FREQUENCY_IN_SECONDS / 2)) return false; // already received one uptime proof for this node recently.

So we accept proofs every 30mins, everything else is the same and prune proofs older than 1hr 10mins.

Yeah sounds good

Doy-lee · 2018-07-24T07:12:11Z

src/cryptonote_core/quorum_cop.cpp

+      if (it->second < prune_from_timestamp)
+        m_uptime_proof_seen.erase(it);
+      else
+        it++;


Fixed iterator inf loop.

This should be:

it = map.erase(it);

Doy-lee · 2018-07-24T07:14:49Z

src/cryptonote_core/quorum_cop.cpp

+    }
+
+    m_last_height = (height < REORG_SAFETY_BUFFER_IN_BLOCKS) ? height : height - REORG_SAFETY_BUFFER_IN_BLOCKS;
+  }


Comment became outdated. So just rewriting it here.

What's the rationale? You need to start voting from the height onwards again. This probably should be m_last_height = height; because you may not have the vote data anymore depending on the height you popped back to.

Doy-lee · 2018-07-24T07:19:18Z

src/cryptonote_core/quorum_cop.cpp

+                " blocks which is greater than the recommended REORG_SAFETY_BUFFER_IN_BLOCKS: " << REORG_SAFETY_BUFFER_IN_BLOCKS);
+    }
+
+    m_last_height = height;


Comment became outdated, so just rewriting it here.
What's the rationale? You need to start voting from the height you popped back to and onwards again. We could recover from this by making a mandatory ping here?

m_last_height may be from well before the blockchain detach height.

Those blocks still need to be checked.

if m_last_height >= blockchain detach height then we have problems with our assumption that large reorgs will not happen

Doy-lee · 2018-07-24T07:21:35Z

src/cryptonote_core/cryptonote_core.h

@@ -1004,6 +1033,8 @@ namespace cryptonote
     epee::math_helper::once_a_time_seconds<60*2, false> m_deregisters_auto_relayer; //!< interval for checking re-relaying deregister votes
     epee::math_helper::once_a_time_seconds<60*60*12, true> m_check_updates_interval; //!< interval for checking for new versions
     epee::math_helper::once_a_time_seconds<60*10, true> m_check_disk_space_interval; //!< interval for checking for disk space
+     epee::math_helper::once_a_time_seconds<UPTIME_PROOF_FREQUENCY_IN_SECONDS, true> m_submit_uptime_proof_interval; //!< interval for submitting uptime proof
+     epee::math_helper::once_a_time_seconds<30, true> m_uptime_proof_pruner;


Prune triggered every 30s if you're a service node.

jackmurray90 · 2018-07-31T06:21:40Z

src/cryptonote_core/quorum_cop.cpp

+
+  void quorum_cop::blockchain_detached(uint64_t height)
+  {
+    uint64_t delta_height = std::abs(static_cast<int64_t>(m_last_height - height));


Why is this abs() ?

This check should simply be if (m_last_height > height)

jackmurray90 · 2018-07-31T06:38:12Z

src/cryptonote_core/quorum_cop.cpp

+      return;
+
+    time_t const now         = time(nullptr);
+    bool been_alive_for_2hrs = (now - m_core.get_start_time()) >= (60 * 60 * 2);


#define please, or const time_t, whatever

jackmurray90 · 2018-08-02T00:30:46Z

src/cryptonote_core/quorum_cop.cpp

+
+  void quorum_cop::blockchain_detached(uint64_t height)
+  {
+    if (m_last_height >= height)


This is always an error, when a very large reorg occurs that it is before m_last_height.

This is sufficient:

if (m_last_height >= height) { LOG_ERROR("The blockchain was detached to height: " << etc << " This should never happen, please report"); m_last_height = height; }

jackmurray90 · 2018-08-02T00:35:58Z

src/cryptonote_core/quorum_cop.cpp

+        continue;
+      }
+
+      auto it = std::find_if(state->quorum_nodes.begin(), state->quorum_nodes.end(), [&my_pubkey](crypto::public_key const &key) {


Does this not work? std::find(nodes->quorum_nodes.begin(), state->quorum_nodes.end(), my_pubkey);

Doy-lee requested a review from jackmurray90 July 24, 2018 02:31

Doy-lee commented Jul 24, 2018

View reviewed changes

jackmurray90 suggested changes Jul 24, 2018

View reviewed changes

Doy-lee force-pushed the QCop branch from dcf5fce to a03a56a Compare July 24, 2018 07:15

Doy-lee commented Jul 24, 2018

View reviewed changes

jcktm and others added 3 commits July 31, 2018 10:43

quorum_cop: added quorum cop and service node keys to lokid

9d34045

Submit deregistration autonomously, prune uptime_proofs

c1589b6

Move pruner to relayer, fix inf. loop, quorum participation bool return

60bcd58

Doy-lee force-pushed the QCop branch from a03a56a to 60bcd58 Compare July 31, 2018 00:53

jackmurray90 reviewed Jul 31, 2018

View reviewed changes

Doy-lee added 2 commits July 31, 2018 17:08

find_if, fix reorg logic, make 2hrs check a variable

f2ad954

Add iterator assign on erase, adjust detached check

4339cb3

jackmurray90 reviewed Aug 2, 2018

View reviewed changes

jcktm and others added 2 commits August 2, 2018 11:05

qacop: review iteration, changed warnings and std::find call

434963e

Accept proofs excluding the added time buffer

a9adc3c

Doy-lee force-pushed the QCop branch from f35b3f2 to a9adc3c Compare August 2, 2018 05:40

jackmurray90 approved these changes Aug 2, 2018

View reviewed changes

jackmurray90 merged commit 049ce2d into oxen-io:dev Aug 2, 2018

QCop autonomously sends deregisters #105

QCop autonomously sends deregisters #105

Conversation

Doy-lee commented Jul 24, 2018

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

jackmurray90 Jul 31, 2018 • edited

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

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

Doy-lee Aug 2, 2018 • edited

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

jackmurray90 Aug 2, 2018 • edited

Choose a reason for hiding this comment

jackmurray90 Aug 2, 2018 • edited

Choose a reason for hiding this comment

jackmurray90 Jul 31, 2018 •

edited

Doy-lee Aug 2, 2018 •

edited

jackmurray90 Aug 2, 2018 •

edited

jackmurray90 Aug 2, 2018 •

edited