litecoin-project/litecoin — pull request #166 by DeathRay1977 (2014-06-28 20:49:19+00:00)
Title: Unit test for CDarkSendPool::SetCollateralAddress and change function to return bool.

Added a couple of Unit tests to darksend_tests.cpp to test the correct setting of collateral address. In doing so, I made a small change in the definition of the function in main.h and main.cpp to make it return a bool (true if address was set OK and false otherwise). I also return false before collateralPubKey.SetDestination(address.Get()); is called if the address was invalid. We don't want to be setting the destination with a value that is not valid. Although this may seem trivial now, CDarkSendPool::SetCollateralAddress could potentially be called from anywhere in the code and, if called with an invalid key as an argument, it would result in 'undefined behaviour' whereby a masternode would be rejected and marked as 'misbehaving'. At present this is not a problem as it is only called in on place. In its own constructor.

CDarkSendPool()
{

std::string strAddress = "";
if(!fTestNet) {
strAddress = "Xq19GqFvajRrEdDHYRKGYjTsQfpV5jyipF";
} else {
strAddress = "mxE2Rp3oYpSEFdsN5TdHWhZvEHm3PJQQVm";
}

SetCollateralAddress(strAddress);
}

Note: Constructor has not been changed as part of this fix and still looks the same as above. Since however the function is not protected, anybody could call it from anywhere. I would recommend that it be made a protected function to prevent this but this is entirely up to you. Furthermore, the class could be made a singleton class which would negate the need for a globally declared instance of the class in main.cpp. Food for thought in the future?

In summary, all this pull request does is change the function to return a bool so I could write some unit tests. The functionality in the darkcoind and darkcoin-qt remains unchanged in as much as the bool return is ignored and the error whereby the address is invalid is left unhandled.