Apply suggestions from code review

Co-authored-by: Daira Hopwood <daira@jacaranda.org>
This commit is contained in:
Kris Nuttycombe 2021-10-13 13:59:06 -06:00 committed by Kris Nuttycombe
parent daf443c9e6
commit 666d135e2e
6 changed files with 46 additions and 27 deletions

View File

@ -14,7 +14,7 @@
#define MAKE_STRING(x) std::string((x), (x)+sizeof(x))
const uint32_t BIP44_TESTNET_TYPE = 1;
const uint32_t SLIP44_TESTNET_TYPE = 1;
TEST(KeystoreTests, StoreAndRetrieveHDSeed) {
CBasicKeyStore keyStore;
@ -25,7 +25,7 @@ TEST(KeystoreTests, StoreAndRetrieveHDSeed) {
EXPECT_FALSE(seedOut.has_value());
// Generate a random seed
auto seed = MnemonicSeed::Random(BIP44_TESTNET_TYPE);
auto seed = MnemonicSeed::Random(SLIP44_TESTNET_TYPE);
// We should be able to set and retrieve the seed
ASSERT_TRUE(keyStore.SetMnemonicSeed(seed));
@ -35,7 +35,7 @@ TEST(KeystoreTests, StoreAndRetrieveHDSeed) {
EXPECT_EQ(seed, seedOut.value());
// Generate another random seed
auto seed2 = MnemonicSeed::Random(BIP44_TESTNET_TYPE);
auto seed2 = MnemonicSeed::Random(SLIP44_TESTNET_TYPE);
EXPECT_NE(seed, seed2);
// We should not be able to set and retrieve a different seed
@ -291,7 +291,7 @@ TEST(KeystoreTests, StoreAndRetrieveHDSeedInEncryptedStore) {
GetRandBytes(vMasterKey.data(), 32);
// 1) Test adding a seed to an unencrypted key store, then encrypting it
auto seed = MnemonicSeed::Random(BIP44_TESTNET_TYPE);
auto seed = MnemonicSeed::Random(SLIP44_TESTNET_TYPE);
EXPECT_FALSE(keyStore.HaveMnemonicSeed());
auto seedOut = keyStore.GetMnemonicSeed();
EXPECT_FALSE(seedOut.has_value());
@ -323,7 +323,7 @@ TEST(KeystoreTests, StoreAndRetrieveHDSeedInEncryptedStore) {
EXPECT_EQ(seed, seedOut.value());
// 2) Test replacing the seed in an already-encrypted key store fails
auto seed2 = MnemonicSeed::Random(BIP44_TESTNET_TYPE);
auto seed2 = MnemonicSeed::Random(SLIP44_TESTNET_TYPE);
EXPECT_FALSE(keyStore.SetMnemonicSeed(seed2));
EXPECT_TRUE(keyStore.HaveMnemonicSeed());
seedOut = keyStore.GetMnemonicSeed();
@ -343,7 +343,7 @@ TEST(KeystoreTests, StoreAndRetrieveHDSeedInEncryptedStore) {
seedOut = keyStore2.GetMnemonicSeed();
EXPECT_FALSE(seedOut.has_value());
auto seed3 = MnemonicSeed::Random(BIP44_TESTNET_TYPE);
auto seed3 = MnemonicSeed::Random(SLIP44_TESTNET_TYPE);
ASSERT_TRUE(keyStore2.SetMnemonicSeed(seed3));
EXPECT_TRUE(keyStore2.HaveMnemonicSeed());
seedOut = keyStore2.GetMnemonicSeed();

View File

@ -306,14 +306,38 @@ extern "C" {
unsigned char *xfvk_i
);
/// Derive a PaymentAddress from an ExtendedFullViewingKey.
/**
* Derive a PaymentAddress from an ExtendedFullViewingKey. Returns 'false'
* if no valid address can be derived for the specified diversifier index.
*
* Arguments:
* - xfvk: [c_uchar; 169] the serialized form of a Sapling extended full viewing key
* - j: [c_uchar; 11] the 88-bit diversifier address at which to start searching,
* encoded in little-endian order
* - addr_ret: [c_uchar; 43] array to which the returned address will be written,
* if the specified diversifier index `j` produces a valid address.
*/
bool librustzcash_zip32_xfvk_address(
const unsigned char *xfvk,
const unsigned char *j,
unsigned char *addr_ret
);
/// Derive a PaymentAddress from an ExtendedFullViewingKey.
/**
* Derive a PaymentAddress from an ExtendedFullViewingKey by searching the
* space of valid diversifiers starting at diversifier index `j`. This will
* always return a valid address along with the diversifier index that produced
* the address unless no addresses can be derived at any diversifier index >= `j`,
* in which case this function will return `false`.
*
* Arguments:
* - xfvk: [c_uchar; 169] the serialized form of a Sapling extended full viewing key
* - j: [c_uchar; 11] the 88-bit diversifier address at which to start searching,
* encoded in little-endian order
* - j_ret: [c_uchar; 11] array that will store the diversifier index at which the
* returned address was found
* - addr_ret: [c_uchar; 43] array to which the returned address will be written
*/
bool librustzcash_zip32_find_xfvk_address(
const unsigned char *xfvk,
const unsigned char *j,

View File

@ -566,10 +566,10 @@ BOOST_AUTO_TEST_CASE( getmaxcoverage ) // some more tests just to get 100% cover
BOOST_AUTO_TEST_CASE( blob88_increment )
{
blob88 bzero(0);
blob88 bone(1);
BOOST_CHECK(bzero.increment());
BOOST_CHECK(bzero == bone);
blob88 b_zero(0);
blob88 b_one(1);
BOOST_CHECK(b_zero.increment());
BOOST_CHECK(b_zero == b_one);
}
BOOST_AUTO_TEST_SUITE_END()

View File

@ -192,7 +192,7 @@ CAmount AsyncRPCOperation_saplingmigration::chooseAmount(const CAmount& availabl
}
// Unless otherwise specified, the migration destination address is the address for Sapling account 0
// at the smallest diversifier index which produces a valid diversified address.
// at the smallest diversifier index that produces a valid diversified address.
libzcash::SaplingPaymentAddress AsyncRPCOperation_saplingmigration::getMigrationDestAddress(const HDSeed& seed) {
KeyIO keyIO(Params());
if (mapArgs.count("-migrationdestaddress")) {

View File

@ -148,17 +148,14 @@ static std::optional<MnemonicSeed> DecryptMnemonicSeed(
// Use seed's fingerprint as IV
// TODO: Handle IV properly when we make encryption a supported feature
if(DecryptSecret(vMasterKey, vchCryptedSecret, seedFp, vchSecret)) {
if (DecryptSecret(vMasterKey, vchCryptedSecret, seedFp, vchSecret)) {
CSecureDataStream ss(vchSecret, SER_NETWORK, PROTOCOL_VERSION);
auto seed = MnemonicSeed::Read(ss);
if (seed.Fingerprint() == seedFp) {
return seed;
} else {
return std::nullopt;
}
} else {
return std::nullopt;
}
return std::nullopt;
}
static std::optional<HDSeed> DecryptLegacyHDSeed(
@ -170,16 +167,13 @@ static std::optional<HDSeed> DecryptLegacyHDSeed(
// Use seed's fingerprint as IV
// TODO: Handle IV properly when we make encryption a supported feature
if(DecryptSecret(vMasterKey, vchCryptedSecret, seedFp, vchSecret)) {
if (DecryptSecret(vMasterKey, vchCryptedSecret, seedFp, vchSecret)) {
auto seed = HDSeed(vchSecret);
if (seed.Fingerprint() == seedFp) {
return seed;
} else {
return std::nullopt;
}
} else {
return std::nullopt;
}
return std::nullopt;
}
static bool DecryptKey(const CKeyingMaterial& vMasterKey, const std::vector<unsigned char>& vchCryptedSecret, const CPubKey& vchPubKey, CKey& key)
@ -265,8 +259,7 @@ bool CCryptoKeyStore::Unlock(const CKeyingMaterial& vMasterKeyIn)
if (!cryptedMnemonicSeed.first.IsNull()) {
// Check that we can successfully decrypt the mnemonic seed, if present
auto seed = DecryptMnemonicSeed(vMasterKeyIn, cryptedMnemonicSeed.second, cryptedMnemonicSeed.first);
if (!seed.has_value())
{
if (!seed.has_value()) {
keyFail = true;
} else {
keyPass = true;
@ -277,8 +270,7 @@ bool CCryptoKeyStore::Unlock(const CKeyingMaterial& vMasterKeyIn)
auto seed = DecryptLegacyHDSeed(vMasterKeyIn,
cryptedLegacySeed.value().second,
cryptedLegacySeed.value().first);
if (!seed.has_value())
{
if (!seed.has_value()) {
keyFail = true;
} else {
keyPass = true;

View File

@ -35,6 +35,9 @@ TEST(WalletZkeysTest, StoreAndLoadSaplingZkeys) {
// Load the all-zeroes seed
std::string mnemonic("abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon art");
MnemonicSeed seed(English, mnemonic);
// The legacy seed used to be automatically derived from randomness; since
// non-mnemonic random generation has been removed we just use the
// all-zeros mnemonic for these tests.
wallet.LoadLegacyHDSeed(seed);
// Now this call succeeds