RTA Flow: Implement Auth Sample Validation on Graftnode (GNRTA-283) #290

cybay · 2019-05-21T14:41:50Z

No description provided.

…NRTA-283

mbg033

Please fix PR title - Gntra 283 means nothing for someone who doesn't have an access to internal issue tracker, this is open source project
Any tests?

mbg033 · 2019-05-27T14:30:30Z

why target branch is feature/disqual-lookup ?

mbg033

General notes:
it make sense to introduce a library (say librta) and move implementation of belongs_to_auth_sample and check_rta_signatures there. Not sure there should be any other "check_" functions as public interface

mbg033 · 2019-05-27T14:33:06Z

src/cryptonote_core/tx_pool.cpp

+
+  //---------------------------------------------------------------------------------
+
+  uint32_t get_rta_hdr_supernode_public_key_offset(const rta_header& rta_hdr)


name is wrong - all signatures belongs to supernodes - some of these supernodes are auth sample

at least 6 signatures required as auth sample

You can see that it's not about signatures.
Signatures live in another container - "std::vector<rta_signature> rta_signs".

Here we get offset to 8 supernode pub-keys inside of rta_header.keys.
Does it make sense now?

sorry, I meant keys of course. all the keys (except POS_KEY_INDEX) belongs to supernodes (pos proxy and wallet proxy are supernodes as well)

Then ok. You're rather right. Will fix it. But all this about p1. Not p.2. P.2 - is not related to the matter.

mbg033 · 2019-05-27T14:37:32Z

src/cryptonote_core/tx_pool.cpp

+    if(!check_rta_keys_count(rta_hdr, txid))
+      return false;
+
+    const auto sn_pkeys_off = get_rta_hdr_supernode_public_key_offset(rta_hdr);


s/sn_pkeys_off/auth_sample_keys_offset/ ?

mbg033 · 2019-05-27T14:41:14Z

src/cryptonote_core/tx_pool.cpp

@@ -1137,14 +1241,15 @@ namespace cryptonote
    return stake ? stake->amount >= config::graft::TIER1_STAKE_AMOUNT : false;
  };

-  bool tx_memory_pool::belongs_to_auth_sample(const rta_header& rta_hdr) const
+  bool tx_memory_pool::belongs_to_auth_sample(const rta_header& rta_hdr, const uint32_t sn_pkeys_off) const


I would change it so it accepts two sets of public keys - one set from tx and another set is auth sample. This way it doesn't need to be a tx_memory_pool member and can be covered with unit test

Will do, thanks.

mbg033 · 2019-05-27T14:44:09Z

tests/supernode_tests/rta_tx_validation.cpp

@@ -0,0 +1,161 @@
+// Copyright (c) 2017, The Graft Project


copyright year should be 2019

Thank you! Will fix it.

mbg033 · 2019-05-27T14:44:40Z

tests/supernode_tests/rta_tx_validation.cpp

+#include <string>
+
+#include <cstdint>
+using u32 = std::uint32_t;


any reasons for introducing such type aliases?

The only reason for most of aliases - convenience.
What's wrong in this case?

can't see any "convenience" by introducing alias for standard type here

len(std::uint32_t) = 13
len(u32) = 3
10 char - is WIN
EVERY time you are writing this. But what is more important - every time when you READ this.
Without any loss.
Weak point?

cybay · 2019-05-28T10:06:47Z

why target branch is feature/disqual-lookup ?

It was started from 'feature/disqual-lookup'.
We can easily retarget it to 'develop'. At any moment.
But in this case we will have 22 changed files instead of 8 that was really affected in this PR.

cybay · 2019-05-28T10:08:54Z

General notes:
it make sense to introduce a library (say librta) and move implementation of belongs_to_auth_sample and check_rta_signatures there. Not sure there should be any other "check_" functions as public interface

No any special objections.
We never discussed such approach before. If we want to do it - we need clearly understand why. And when/how we will use it.

mbg033 · 2019-05-28T10:27:07Z

why target branch is feature/disqual-lookup ?

It was started from 'feature/disqual-lookup'.
We can easily retarget it to 'develop'. At any moment.
But in this case we will have 22 changed files instead of 8 that was really affected in this PR.

it needs to point to develop at least. if it depends on feature/disqual-lookup it can be merged only after feature/disqual-lookup merged

Agree with it.
But let's do that switch/retargeting just before to merge and let it be in this way (as it now is) during this discussion.

mbg033 · 2019-05-28T10:35:31Z

General notes:
it make sense to introduce a library (say librta) and move implementation of belongs_to_auth_sample and check_rta_signatures there. Not sure there should be any other "check_" functions as public interface

No any special objections.
We never discussed such approach before. If we want to do it - we need clearly understand why. And when/how we will use it.

Reasons:

we might want to use need this functionality in some another places;
bool belongs_to_auth_sample(const rta_header& rta_hdr, uint32_t sn_pkeys_off) const is not covered with test
these functions are not related to tx_pool:

 bool check_rta_sign_count(const std::vector<rta_signature>& rta_signs, const crypto::hash& txid);
  bool check_rta_keys_count(const rta_header& rta_hdr, const crypto::hash& txid);
  bool check_rta_sign_key_indexes(const std::vector<rta_signature>& rta_signs, const crypto::hash& txid, uint32_t sn_pkeys_off);
  bool check_rta_signatures(const std::vector<rta_signature>& rta_signs, const rta_header& rta_hdr, const crypto::hash& txid, uint32_t sn_pkeys_off);
  uint32_t get_rta_hdr_supernode_public_key_offset(const rta_header& rta_hdr);

Added check against auth sample at tx_memory_pool::validate_rta_tx, G…

f900390

…NRTA-283

cybay requested review from mbg033, EDDragonWolf and AlexanderSuprunenko May 21, 2019 14:41

cybay changed the base branch from master to feature/disqual-lookup May 21, 2019 14:42

mbg033 requested changes May 21, 2019

View reviewed changes

Applied bug-fix in selectSample func plus minor changes

c52eab5

cybay changed the title ~~Gnrta 283~~ RTA Flow: Implement Auth Sample Validation on Graftnode (GNRTA-283) May 23, 2019

Added signature rta tx validation

3fdebd9

mbg033 requested changes May 27, 2019

View reviewed changes

cybay changed the base branch from feature/disqual-lookup to develop May 28, 2019 10:03

cybay changed the base branch from develop to feature/disqual-lookup May 28, 2019 10:03

Fixed PR-review issues

3f74c2b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTA Flow: Implement Auth Sample Validation on Graftnode (GNRTA-283) #290

RTA Flow: Implement Auth Sample Validation on Graftnode (GNRTA-283) #290

cybay commented May 21, 2019

mbg033 left a comment

mbg033 commented May 27, 2019

mbg033 left a comment

mbg033 May 27, 2019

cybay May 28, 2019 •

edited

Loading

mbg033 May 28, 2019

cybay May 28, 2019

cybay May 29, 2019

mbg033 May 27, 2019

cybay May 28, 2019

cybay May 29, 2019

mbg033 May 27, 2019

cybay May 28, 2019

cybay May 29, 2019

mbg033 May 27, 2019

cybay May 28, 2019

cybay May 29, 2019

mbg033 May 27, 2019

cybay May 28, 2019 •

edited

Loading

mbg033 May 28, 2019

cybay May 28, 2019

cybay commented May 28, 2019

cybay commented May 28, 2019

mbg033 commented May 28, 2019 •

edited by cybay

Loading

mbg033 commented May 28, 2019


		//---------------------------------------------------------------------------------

		uint32_t get_rta_hdr_supernode_public_key_offset(const rta_header& rta_hdr)

RTA Flow: Implement Auth Sample Validation on Graftnode (GNRTA-283) #290

Are you sure you want to change the base?

RTA Flow: Implement Auth Sample Validation on Graftnode (GNRTA-283) #290

Conversation

cybay commented May 21, 2019

mbg033 left a comment

Choose a reason for hiding this comment

mbg033 commented May 27, 2019

mbg033 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybay May 28, 2019 • edited Loading

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

cybay May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybay commented May 28, 2019

cybay commented May 28, 2019

mbg033 commented May 28, 2019 • edited by cybay Loading

mbg033 commented May 28, 2019

cybay May 28, 2019 •

edited

Loading

cybay May 28, 2019 •

edited

Loading

mbg033 commented May 28, 2019 •

edited by cybay

Loading