Create CPID classes and clean up CPID code

[cyrossignol]

Part 1 of some changes that clean up CPID handling. To make this easier to review, I'm splitting it up into two PRs and pulling it out of the contract/beacon updates.

These changes add a dedicated Cpid structure and a MiningId class that describes the identity of a miner (researcher or investor, for future serialization or potential support for surrogate ID to multiple CPIDs). It removes some legacy CPID handling code and the "boinckey" import mechanism. We can discuss a better way to implement that later.

The classes help with upcoming beacon format changes and move a bit more Gridcoin-specific code out of main.cpp.

[cyrossignol]

By the way, we can ignore most of the changes to HarvestCPIDs() and GetNextProject()...those have been rewritten in part 2.

[denravonska]

Consider returning a copy here unless we want to use a reference for performance reason. It looks like the class is immutable so this should be safe, but the reference will be dangling if this object is used in a container which is then reorganized.

[jamescowens]

Should we also change from an MD5 to sha256 hash for the CPID::Hash? Is it necessary for security? How much would it break things? Also, would have to be coded as a mandatory I think.

[cyrossignol]

@denravonska Ah, good point--let's use a copy. Only the tests use this method right now, so the overhead should be fine. Can I change that in the part 2 PR?

@jamescowens The Hash() method is used to generate an external CPID from an internal CPID and email address so that the wallet can validate the CPIDs loaded from client_state.xml locally to avoid running with a corrupt CPID. It replaces the obfuscated MD5 algorithm from the cpid.cpp class that I removed. BOINC's external CPIDs are MD5 hashes, so we can't change the format, but we won't use it in the protocol.

[denravonska]

@cyrossignol Sure :)

[cyrossignol]

@denravonska I remember why this returns a reference now--in the upcoming PR, I specialized std::hash<T> so that we can use Cpid objects as keys in unordered_maps (for lookup by CPID--beacons for example). A copy would happen there every time we access the maps by key if we go that route because it's calling this method to get the data to hash.

I'll do some reading about dangling references. In this case, Raw() is provided for low-level access which should happen only sparingly outside of the interface provided by the rest of the class. We can review in the next PR to determine how we want to keep it 🙂

[denravonska]

Looks great! utACK.

[jamescowens]

Really nice work IMHO.

[jamescowens]

Should we also change from an MD5 to sha256 hash for the CPID::Hash? Is it necessary for security? How much would it break things? Also, would have to be coded as a mandatory I think.

[jamescowens]

Yep. That makes sense.

…

Sent from my iPhone

On Jun 10, 2019, at 2:33 AM, Cy Rossignol ***@***.***> wrote: @cyrossignol commented on this pull request. In src/neuralnet/cpid.cpp: > + return m_bytes != other.m_bytes; +} + +bool Cpid::IsZero() const +{ + const auto zero = [](const unsigned char& i) { return i == 0; }; + + return std::all_of(m_bytes.begin(), m_bytes.end(), zero); +} + +bool Cpid::Matches(const std::string& internal, const std::string& email) const +{ + return m_bytes == Cpid::Hash(internal, email).m_bytes; +} + +const std::array<unsigned char, 16>& Cpid::Raw() const @denravonska Ah, good point--let's use a copy. Only the tests use this method right now, so the overhead should be fine. Can I change that in the part 2 PR? @jamescowens The Hash() method is used to generate an external CPID from an internal CPID and email address so that the wallet can validate the CPIDs loaded from client_state.xml locally to avoid running with a corrupt CPID. It replaces the obfuscated MD5 algorithm from the cpid.cpp class that I removed. BOINC's external CPIDs are MD5 hashes, so we can't change the format, but we won't use it in the protocol. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jamescowens]

@cyrossignol have you tested this on testnet for compatibility?

[jamescowens]

I am running this PR on my Windows test node on testnet. Looks good so far.

[cyrossignol]

@jamescowens I synced testnet from 0 and staked a couple blocks...no issues so far that I can tell. I'll do mainnet in the next PR.

[jamescowens]

I am merging this one. It is running fine on my Windows testnet node.