Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: feature/disqual-lookup
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ log/
# vim swap files
*.swp
*.swo
.ycm*
TAGS
!TAGS/
tags
Expand Down Expand Up @@ -103,4 +104,4 @@ local.properties
.texlipse
.idea/

/testnet
/testnet
15 changes: 15 additions & 0 deletions src/cryptonote_core/stake_transaction_processor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -614,3 +614,18 @@ void StakeTransactionProcessor::invoke_update_blockchain_based_list_handler(bool

invoke_update_blockchain_based_list_handler_impl(depth);
}

bool StakeTransactionProcessor::get_auth_sample_keys(const uint64_t auth_sample_height,
const std::string& payment_id, std::vector<crypto::public_key>& auth_sample_keys) const
{
const size_t depth = m_blockchain_based_list->block_height() - auth_sample_height;
auto& tiers = m_blockchain_based_list->tiers(depth);
auto bbl_idxs = makeBBLindexes(tiers);

std::vector<TI> bbqs_idxs;
graft::generator::select_AuthSample(payment_id, bbl_idxs, bbqs_idxs);
auth_sample_keys = fromIndexes(tiers, bbqs_idxs);

return (auth_sample_keys.size() == graft::generator::AUTH_SAMPLE_SIZE);
}

2 changes: 2 additions & 0 deletions src/cryptonote_core/stake_transaction_processor.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class StakeTransactionProcessor
/// Force invoke update handler for blockchain based list
void invoke_update_blockchain_based_list_handler(bool force = true, size_t depth = 1);

bool get_auth_sample_keys(uint64_t auth_sample_height, const std::string& payment_id, std::vector<crypto::public_key>& auth_sample_keys) const;

private:
void init_storages_impl();
void process_block(uint64_t block_index, const block& block, const crypto::hash& block_hash, bool update_storage = true);
Expand Down
147 changes: 146 additions & 1 deletion src/cryptonote_core/tx_pool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@
#include "stake_transaction_processor.h"
#include "graft_rta_config.h"

#include "utils/sample_generator.h" // bad idea to include the whole heavy header just
// because of one const - graft::generator::AUTH_SAMPLE_SIZE
// It's better to hold all const's in separate lightweight header

#undef MONERO_DEFAULT_LOG_CATEGORY
#define MONERO_DEFAULT_LOG_CATEGORY "txpool"

Expand Down Expand Up @@ -1085,16 +1089,116 @@ namespace cryptonote
return true;
}


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

uint32_t get_rta_hdr_supernode_public_key_offset(const rta_header& rta_hdr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. name is wrong - all signatures belongs to supernodes - some of these supernodes are auth sample
  2. at least 6 signatures required as auth sample

Copy link
Author

@cybay cybay May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

{
return (rta_hdr.keys.size() == 8) ? 0 : 3; // supernode public keys offset
}

bool check_rta_sign_count(const std::vector<rta_signature>& rta_signs, const crypto::hash& txid)
{
const uint32_t cnt = rta_signs.size();
// according to spec/design there can be 6-8 signatures and we do check it in here
const bool ok = !((cnt < 6) || (cnt > graft::generator::AUTH_SAMPLE_SIZE));
if(!ok)
MERROR("Wrong amount of signatures:" << cnt << " for tx:" << txid << " It should be 6-"
<< graft::generator::AUTH_SAMPLE_SIZE << ".");
return ok;
}

bool check_rta_keys_count(const rta_header& rta_hdr, const crypto::hash& txid)
{
const uint32_t cnt = rta_hdr.keys.size();

// so far there can be cases when we have only graft::generator::AUTH_SAMPLE_SIZE
// records (3 starting are missing)
const bool ok = (cnt == graft::generator::AUTH_SAMPLE_SIZE)
|| (cnt == (3 + graft::generator::AUTH_SAMPLE_SIZE));

if(!ok)
MERROR("Failed to validate rta tx, wrong amount ("
<< cnt << ") of auth sample keys for tx:" << txid << ". Expected "
<< (3 + graft::generator::AUTH_SAMPLE_SIZE));

return ok;
}

bool check_rta_sign_key_indexes(const std::vector<rta_signature>& rta_signs,
const crypto::hash& txid, const uint32_t sn_pkeys_off)
{
bool ok = true;
for(const auto& rs : rta_signs)
{
if((rs.key_index < sn_pkeys_off)
|| (rs.key_index > (sn_pkeys_off + graft::generator::AUTH_SAMPLE_SIZE - 1)))
{
MERROR("Signature: " << rs.signature << " has wrong key index: "
<< rs.key_index << ", tx: " << txid);
ok = false;
break;
}
}
return ok;
}

bool check_rta_signatures(const std::vector<rta_signature>& rta_signs,
const rta_header& rta_hdr, const crypto::hash& txid, const uint32_t sn_pkeys_off)
{
bool ok = true;
for(const auto& rs : rta_signs)
{
const int32_t idx = rs.key_index + sn_pkeys_off;

if(idx > (int32_t)(rta_hdr.keys.size() - 1))
{
MERROR("Fail at check_rta_signatures - out of index!");
return false;
}

const auto& key = rta_hdr.keys[idx];
if(!crypto::check_signature(txid, key, rs.signature))
{
MERROR("Failed to validate rta tx " << std::endl
<< "signature: " << epee::string_tools::pod_to_hex(rs.signature) << std::endl
<< "for key: " << key << std::endl
<< "tx-id: " << epee::string_tools::pod_to_hex(txid));
ok = false;
break;
}
}
return ok;
}

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

bool tx_memory_pool::validate_rta_tx(const crypto::hash &txid, const std::vector<rta_signature> &rta_signs, const rta_header &rta_hdr) const
{
bool result = true;

if(!check_rta_sign_count(rta_signs, txid))
return false;

if(!check_rta_keys_count(rta_hdr, txid))
return false;

const auto sn_pkeys_off = get_rta_hdr_supernode_public_key_offset(rta_hdr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/sn_pkeys_off/auth_sample_keys_offset/ ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


if(!check_rta_sign_key_indexes(rta_signs, txid, sn_pkeys_off))
return false;

if(!check_rta_signatures(rta_signs, rta_hdr, txid, sn_pkeys_off))
return false;

#if 0

if (rta_hdr.keys.size() == 0) {
MERROR("Failed to validate rta tx, missing auth sample keys for tx: " << txid );
return false;
}
#if 0 // don't validate signatures for rta mining

// don't validate signatures for rta mining
if (rta_hdr.keys.size() != rta_signs.size()) {
MERROR("Failed to validate rta tx: " << txid << ", keys.size() != signatures.size()");
return false;
Expand All @@ -1116,6 +1220,10 @@ namespace cryptonote
}
}
#endif

if(!belongs_to_auth_sample(rta_hdr, sn_pkeys_off))
return false;

for (const crypto::public_key &key : rta_hdr.keys) {
result &= validate_supernode(rta_hdr.auth_sample_height, key);
if (!result) {
Expand All @@ -1132,4 +1240,41 @@ namespace cryptonote
supernode_stake * stake = const_cast<supernode_stake*>(m_stp->find_supernode_stake(height, epee::string_tools::pod_to_hex(id)));
return stake ? stake->amount >= config::graft::TIER1_STAKE_AMOUNT : false;
};

bool tx_memory_pool::belongs_to_auth_sample(const rta_header& rta_hdr, const uint32_t sn_pkeys_off) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do, thanks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

{
bool ok = false; // ok is true when every supernode from rta_hdr is member of auth_sample
std::vector<crypto::public_key> ask; // auth sample keys
if(ok = (m_stp->get_auth_sample_keys(rta_hdr.auth_sample_height, rta_hdr.payment_id, ask)))
{
for(uint32_t i = sn_pkeys_off, cnt = rta_hdr.keys.size(); i < cnt; ++i)
{
const auto& key = rta_hdr.keys[i];
if(!(ok = std::any_of(ask.cbegin(), ask.cend(), [&key](const crypto::public_key& k) { return key == k; })))
{
MERROR("Key " << key << " does not belong to auth-sample");
break;
}
}
}
else
MERROR("Obtaining of auth sample keys is failed");

#if 0
{
std::ostringstream m;
m << std::endl << "### DBG: keys to be checked against belonging to auth sample (cnt:"
<< rta_hdr.keys.size() << "):";

for(const auto& k : rta_hdr.keys) m << std::endl << k;
m << std::endl << "### DBG: keys of auth sample (cnt:" << ask.size() << "):";
for(const auto& k : ask) m << std::endl << k;
MDEBUG(m.str());
}
#endif

return ok;
}

}

7 changes: 7 additions & 0 deletions src/cryptonote_core/tx_pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ namespace cryptonote
bool validate_rta_tx(const crypto::hash &txid, const std::vector<cryptonote::rta_signature> &rta_signs, const cryptonote::rta_header &rta_hdr) const;

bool validate_supernode(uint64_t height, const crypto::public_key &id) const;
bool belongs_to_auth_sample(const rta_header& rta_hdr, uint32_t sn_pkeys_off) const;

//TODO: confirm the below comments and investigate whether or not this
// is the desired behavior
Expand Down Expand Up @@ -515,6 +516,12 @@ namespace cryptonote
Blockchain& m_blockchain; //!< reference to the Blockchain object
StakeTransactionProcessor * m_stp = nullptr;
};

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);
}

namespace boost
Expand Down
5 changes: 0 additions & 5 deletions src/utils/sample_generator.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,6 @@ bool selectSample(size_t sample_size, const Tiers& bbl_tiers, std::vector<T>& ou
auto& dst = tier_supernodes[i];
dst.reserve(sample_size);
uniform_select(do_not_seed{}, sample_size, src, dst);
if (dst.size() != sample_size)
{
LOG_ERROR("unable to select supernodes for " << prefix << " sample");
return false;
}
MDEBUG("..." << dst.size() << " supernodes has been selected for tier " << (i + 1) << " from blockchain based list with " << src.size() << " supernodes");
}

Expand Down
2 changes: 2 additions & 0 deletions tests/supernode_tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ set(supernode_tests_sources
walletproxy_test.cpp
graft_wallet_tests.cpp
graft_splitted_tx_test.cpp
rta_tx_validation.cpp
)

set(supernode_tests_headers
Expand Down Expand Up @@ -74,3 +75,4 @@ endif ()
add_test(
NAME supernode_tests
COMMAND supernode_tests)

Loading