service_node_list: add multiple stakers, reward shares, etc #101

jackmurray90 · 2018-07-18T11:29:46Z

This is as yet untested, and still needs the CLI to construct registration TX with multiple parties.

@Doy-lee Please review

jackmurray90 · 2018-07-18T12:05:13Z

@Haafingar looks like this is going to make it into the release ;)

@Doy-lee I also need to add the service node pubkey signature check, but I'll do that in a seperate commit (and not squash). If you want to start reviewing we can work in parallel, I don't expect to change this commit much

Doy-lee · 2018-07-18T13:27:15Z

src/cryptonote_config.h

@@ -52,6 +52,7 @@
 #define STAKING_REQUIREMENT_LOCK_BLOCKS_EXCESS          20
 #define STAKING_REQUIREMENT_LOCK_BLOCKS                 (30*24*31)
 #define STAKING_RELOCK_WINDOW_BLOCKS                    (30*6)
+#define STAKING_SHARES                                  (uint32_t)((1ULL << 32)-1ULL)


We should just make this, ((uint32_t)-1) or UINT32_MAX and MAX_STAKING_SHARES

It's not a maximum, it's fixed. There are always this many shares in total.

I changed to UINT32_MAx

Doy-lee · 2018-07-18T13:37:20Z

src/cryptonote_basic/cryptonote_format_utils.cpp

+    tx_extra.resize(tx_extra.size() + tx_extra_str.size());
+    memcpy(&tx_extra[pos], tx_extra_str.data(), tx_extra_str.size());
+    return true;
+  }


Using do_serialize is necessary if there's a non POD we're trying to serialize. This should work using just

add_data_to_tx_extra(tx_extra, reinterpret_cast<const char*>(&winner), sizeof(winner), TX_EXTRA_TAG_SERVICE_NODE_WINNER);

Then we can say this function can't trivially fail and remove the return value and the need to check failure when using this function.

Doy-lee · 2018-07-18T13:41:07Z

src/cryptonote_basic/cryptonote_format_utils.cpp

+  bool add_service_node_register_to_tx_extra(std::vector<uint8_t>& tx_extra, const std::vector<cryptonote::account_public_address>& addresses, const std::vector<uint32_t>& shares, const crypto::public_key& service_node_key)
+  {
+    std::vector<crypto::public_key> public_view_keys;
+    std::vector<crypto::public_key> public_spend_keys;


You can resize the array in its initializer
std::vector<crypto::public_key> public_view_keys(addresses.size());

And then change the assignment to
public_view_keys[i] = addresses[i].m_view_public_key;

Doy-lee · 2018-07-18T13:45:18Z

src/cryptonote_core/cryptonote_tx_utils.cpp

@@ -168,6 +181,8 @@ namespace cryptonote
      add_tx_pub_key_to_extra(tx, gov_key.pub);
    }

+    add_service_node_winner_to_tx_extra(tx.extra, service_node_key);
+


If you make the above change to return void this is ok. Otherwise, you should check if this fails.

Doy-lee · 2018-07-18T13:46:43Z

src/cryptonote_core/cryptonote_tx_utils.cpp

-      tx.vout.push_back(out);
+      for (size_t i = 0; i < service_node_info.size(); i++)
+      {
+        crypto::key_derivation derivation = AUTO_VAL_INIT(derivation);;


Try not to comment on whitespace or ;; or similar things in reviews. By all means fix it if you see it in code while you're working, but the time consumed by context switching to fix something so small is really not worth it.

(also fixed)

Doy-lee · 2018-07-19T03:56:42Z

src/cryptonote_core/service_node_list.cpp


-    if (miner_tx.vout[1].target.type() != typeid(cryptonote::txout_to_key))
+    for (size_t i = miner_tx.vout.size()-1-addresses.size(), j = 0; i < miner_tx.vout.size()-1; i++, j++)


This is confusing. Some annotations can make this easier to understand.

To be simpler it could just be j < addresses.size(); Or alternatively, I like

for (size_t i = 0; i < addresses.size(); i++) { // NOTE: Can't always assume the miner_tx will be there. size_t const vout_index = (miner_tx.vout.size() - 1 /*Governance*/ - addresses.size()) + i; ... }

Doy-lee · 2018-07-19T04:06:29Z

src/cryptonote_core/service_node_list.cpp

-    cryptonote::keypair gov_key = cryptonote::get_deterministic_keypair_from_height(height);
+      crypto::key_derivation derivation = AUTO_VAL_INIT(derivation);;
+      crypto::public_key out_eph_public_key = AUTO_VAL_INIT(out_eph_public_key);
+      cryptonote::account_public_address address = addresses[j];


Make a reference.

Doy-lee · 2018-07-19T04:16:36Z

src/cryptonote_basic/cryptonote_format_utils.cpp

+    }
+    // convert to variant
+    tx_extra_field field = tx_extra_service_node_register{ public_spend_keys, public_view_keys, shares, service_node_key };
+    // serialize


if shares.size() != addresses.size() we should fail when using our interface to make a register TX.

Doy-lee · 2018-07-19T04:25:18Z

src/cryptonote_core/service_node_list.h

+      // This value is given by block height, and differentiated by transaction index for
+      // registrations that occured in the same block. index = 0 for block reward, 1 for first transaction, etc.
+      // hence a std::pair<uint64_t, size_t> is used here for this value.
+      std::pair<uint64_t, size_t> height;


Not exactly necessary anymore.

It would be more readable now to have their own variable. Personally it's a bit of a mental burden for me to disambiguate what this pair is when I see it in code and I'm somewhat comfortable with the code.

I find this easier to read because I'm using them as an index, and STL pairs have operator< defined as indexed on the first, then the second element.

I'll go ahead and change it here though and construct the pairs on the fly when I do the comparison.

Much nicer now! std::pairs are pretty bad, nearly always it's clearer not to use them... they're so convenient though

Fair enough, changed

Doy-lee · 2018-07-19T05:33:09Z

src/cryptonote_core/service_node_list.cpp

      return false;
    }
-    service_nodes_last_reward[m_address] = m_height_index;
-    service_nodes_keys[m_address] = m_key;
+    service_nodes_infos[m_key] = m_info;


Am I right in thinking, if a node expires a rollback_change is made for that node and its key is erased. A block is popped, we apply this rollback_change() that was just stored.

The key is no longer in the service node, apply fails and then you re init() the service node list. Same problem with the erasing if it's a deregistration.

Good catch. Rollback change should apply even if the node does not exist in the list.

Doy-lee

99% good.

Doy-lee · 2018-07-20T02:55:58Z

src/cryptonote_core/service_node_list.cpp

-    block_added_generic(block, txs);
-  }
+      crypto::key_derivation derivation;
+      crypto::generate_key_derivation(service_node_addresses[i].m_view_public_key, gov_key.sec, derivation);


As discussed. An attacker could produce a transaction that has all the necessary staking information to pass validation but include an extra output, this will index out of bounds for service_node_addresses and potentially crash.

Doy-lee · 2018-07-20T03:14:36Z

src/cryptonote_core/service_node_list.cpp

@@ -465,75 +424,108 @@ namespace service_nodes
      return expired_nodes;
    }

+    size_t index = 0;


int index = 0;

jackmurray90 requested a review from Doy-lee July 18, 2018 11:29

Doy-lee reviewed Jul 19, 2018

View reviewed changes

jackmurray90 requested a review from Doy-lee July 20, 2018 01:00

Doy-lee suggested changes Jul 20, 2018

View reviewed changes

service_node_list: add multiple stakers, reward shares, etc

8eeef45

jackmurray90 requested a review from Doy-lee July 20, 2018 04:09

jackmurray90 merged commit 003f070 into oxen-io:dev Jul 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service_node_list: add multiple stakers, reward shares, etc #101

service_node_list: add multiple stakers, reward shares, etc #101

jackmurray90 commented Jul 18, 2018

jackmurray90 commented Jul 18, 2018

Doy-lee Jul 18, 2018 •

edited

jackmurray90 Jul 20, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 18, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 18, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 18, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 18, 2018

jackmurray90 Jul 20, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 19, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 19, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 19, 2018

jackmurray90 Jul 20, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 19, 2018

jackmurray90 Jul 20, 2018

jackmurray90 Jul 20, 2018 •

edited

jackmurray90 Jul 20, 2018

Doy-lee Jul 19, 2018

jackmurray90 Jul 20, 2018

jackmurray90 Jul 20, 2018

Doy-lee left a comment

Doy-lee Jul 20, 2018

jackmurray90 Jul 20, 2018

Doy-lee Jul 20, 2018

jackmurray90 Jul 20, 2018


		if (miner_tx.vout[1].target.type() != typeid(cryptonote::txout_to_key))
		for (size_t i = miner_tx.vout.size()-1-addresses.size(), j = 0; i < miner_tx.vout.size()-1; i++, j++)

service_node_list: add multiple stakers, reward shares, etc #101

service_node_list: add multiple stakers, reward shares, etc #101

Conversation

jackmurray90 commented Jul 18, 2018

jackmurray90 commented Jul 18, 2018

Doy-lee Jul 18, 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

jackmurray90 Jul 20, 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

Doy-lee 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

Doy-lee Jul 18, 2018 •

edited

jackmurray90 Jul 20, 2018 •

edited