hyperledger-iroha / iroha-dco

Iroha - A simple, decentralized ledger
http://iroha.tech
Apache License 2.0
988 stars 296 forks source link

Improve check of signatures subset in stateful validator #628

Closed neewy closed 6 years ago

neewy commented 7 years ago

Currently set inclusion is verified in non-effective way. New solution should not increase time complexity of existing solution. This is a good start for a newcomer to contribute to the code:

https://github.com/hyperledger/iroha/blob/5fd84ec8191ceaf69bd49991ffdcbb4f1fb0440d/irohad/validation/impl/stateful_validator_impl.cpp#L74

darsh7807 commented 6 years ago
std::map<pubkey_t,bool>map_keys;
for (auto sign : signatures) {
        map_keys[sign.pubkey] = True;
  }
for (auto pubkey : public_keys) {
       if(map_keys[pubkey] == False) return False;
      }
return True;

Is this approach acceptable?

Warchant commented 6 years ago

@darsh7807 please look at the latest code, approach may be different:

https://github.com/hyperledger/iroha/blob/develop/irohad/validation/impl/stateful_validator_impl.cpp#L97-L112

Please, always look at the latest code (develop branch) when you're studying iroha :-)

Few things I want to add: 1) SignatureSetType is actually already std::unordered_set, this copy is redundant https://github.com/hyperledger/iroha/blob/develop/irohad/validation/impl/stateful_validator_impl.cpp#L102-L105 2) PublicKeys can be compared with operator==, no need in casting to string.

darsh7807 commented 6 years ago

@Warchant Ok, I looked at it, I think it is pretty much same thing with the previous one. I have discussed on the telegram channel and found this approach suitable for this condition:

for( auto pubkey : pulic_keys){
   for(auto sign : signatures){
         if( sign.pubkey == pubkey)  {
             count++; break;
          }     
    }
}
if ( count == public_key.size()) return True;
return False;
darsh7807 commented 6 years ago

I made a PR please review it PR

steephengeorge commented 6 years ago

I am new here. Can I make this change as follows. So I can learn the process and take part in this collaborative effort:

 bool StatefulValidatorImpl::signaturesSubset(
        const shared_model::interface::SignatureSetType &signatures,
        const std::vector<shared_model::crypto::PublicKey> &public_keys) {
      // TODO 09/10/17 Lebedev: simplify the subset verification IR-510
      // #goodfirstissue
      return std::all_of(public_keys.begin(),
                         public_keys.end(),
                         [&signatures](const auto &public_key) {
                           return signatures.find(public_key)
                               != signatures.end();
                         });
    }
l4l commented 6 years ago

@steephengeorge looks good, couple minors that need to be fixed are:

l4l commented 6 years ago

Fixed in #1339