Remove default constructor from SecretKey class

src/ripple/app/consensus/RCLConsensus.cpp

@@ -105,13 +105,13 @@ RCLConsensus::Adaptor::Adaptor(
         JLOG(j_.info()) << "Validator identity: "
                         << toBase58(
                                TokenType::NodePublic,
-                               *validatorKeys_.masterPublicKey);
+                               *validatorKeys_.getMasterPublicKey());
 
-        if (validatorKeys_.masterPublicKey != validatorKeys_.publicKey)
+        if (validatorKeys_.getMasterPublicKey() != validatorKeys_.getPublicKey())
         {
             JLOG(j_.debug())
                 << "Validator ephemeral signing key: "
-                << toBase58(TokenType::NodePublic, *validatorKeys_.publicKey)
+                << toBase58(TokenType::NodePublic, *validatorKeys_.getPublicKey())
                 << " (seq: " << std::to_string(validatorKeys_.sequence) << ")";
         }
     }
@@ -211,11 +211,11 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal)
     prop.set_proposeseq(proposal.proposeSeq());
     prop.set_closetime(proposal.closeTime().time_since_epoch().count());
     prop.set_nodepubkey(
-        validatorKeys_.publicKey->data(), validatorKeys_.publicKey->size());
+        validatorKeys_.getPublicKey()->data(), validatorKeys_.getPublicKey()->size());
 
     auto sig = signDigest(
-        *validatorKeys_.publicKey,
-        *validatorKeys_.secretKey,
+        *validatorKeys_.getPublicKey(),
+        *validatorKeys_.getSecretKey(),
         proposal.signingHash());
 
     prop.set_signature(sig.data(), sig.size());
@@ -225,7 +225,7 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal)
         proposal.prevLedger(),
         proposal.proposeSeq(),
         proposal.closeTime(),
-        *validatorKeys_.publicKey,
+        *validatorKeys_.getPublicKey(),
         sig);
 
     app_.getHashRouter().addSuppression(suppression);
@@ -801,8 +801,8 @@ RCLConsensus::Adaptor::validate(
 
     auto v = std::make_shared<STValidation>(
         lastValidationTime_,
-        *validatorKeys_.publicKey,
-        *validatorKeys_.secretKey,
+        *validatorKeys_.getPublicKey(),
+        *validatorKeys_.getSecretKey(),
         validatorKeys_.nodeID,
         [&](STValidation& v) {
             v.setFieldH256(sfLedgerHash, ledger.id());
@@ -960,8 +960,8 @@ RCLConsensus::Adaptor::preStartRound(
 {
     // We have a key, we do not want out of sync validations after a restart
     // and are not amendment blocked.
-    validating_ = validatorKeys_.publicKey &&
-        validatorKeys_.publicKey->size() != 0 &&
+    validating_ = validatorKeys_.getPublicKey() &&
+        validatorKeys_.getPublicKey() != PublicKey::emptyPubKey &&
         prevLgr.seq() >= app_.getMaxDisallowedLedger() &&
         !app_.getOPs().isBlocked();
 
@@ -1034,7 +1034,7 @@ RCLConsensus::Adaptor::laggards(
 bool
 RCLConsensus::Adaptor::validator() const
 {
-    return validatorKeys_.publicKey.has_value();
+    return validatorKeys_.getPublicKey().has_value();
 }
 
 void

src/ripple/app/main/Application.cpp

@@ -593,9 +593,12 @@ class ApplicationImp : public Application, public BasicApp
     PublicKey const&
     getValidationPublicKey() const override
     {
-        if (!validatorKeys_.publicKey)
+        if (!validatorKeys_.getPublicKey())
             return PublicKey::emptyPubKey;
-        return *validatorKeys_.publicKey;
+
+        // do not use the getter function from ValidatorKeys class because
+        // const& is returned from this function
+        return validatorKeys_.keys->publicKey;
     }
 
     NetworkOPs&
@@ -1198,7 +1201,7 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline)
         return false;
     }
 
-    if (validatorKeys_.publicKey)
+    if (validatorKeys_.getPublicKey().has_value())
         setMaxDisallowedLedger();
 
     // Configure the amendments the server supports
@@ -1326,8 +1329,8 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline)
             // ValidatorKeys object.
 
             PublicKey localSigningKey = PublicKey::emptyPubKey;
-            if (validatorKeys_.publicKey)
-                localSigningKey = *validatorKeys_.publicKey;
+            if (validatorKeys_.getPublicKey())
+                localSigningKey = *validatorKeys_.getPublicKey();
 
             // Setup trusted validators
             if (!validators_->load(

src/ripple/app/misc/ValidatorKeys.h

@@ -36,9 +36,30 @@ class Config;
 class ValidatorKeys
 {
 public:
-    std::optional<PublicKey> masterPublicKey;
-    std::optional<PublicKey> publicKey;
-    std::optional<SecretKey> secretKey;
+    // Group all keys in a struct. Either all keys are valid or none are.
+    struct Keys
+    {
+        PublicKey masterPublicKey;
+        PublicKey publicKey;
+        SecretKey secretKey;
+
+        Keys() = delete;
+        Keys(
+            SecretKey const& secret_,
+            PublicKey const& public_,
+            PublicKey const& masterPublic_
+            )
+            : masterPublicKey(masterPublic_)
+            , publicKey(public_)
+            , secretKey(secret_)
+        { }
+    };
+
+    // Note: The existence of keys cannot be used as a proxy for checking the
+    // validity of a configuration. It is possible to have a valid
+    // configuration while not setting the keys, as per the constructor of
+    // the ValidatorKeys class.
+    std::optional<Keys> keys;
     NodeID nodeID;
     std::string manifest;
     std::uint32_t sequence = 0;
@@ -52,6 +73,33 @@ class ValidatorKeys
         return configInvalid_;
     }
 
+    // Note: prefer the use of getter functions. If a const& is required for
+    // the publicKey, then the data member is accessed directly. An example
+    // is used in ApplicationImp::getValidationPublicKey()
+    std::optional<PublicKey const> getPublicKey() const
+    {
+        if (keys)
+            return keys->publicKey;
+
+        return {};
+    }
+
+    std::optional<PublicKey const> getMasterPublicKey() const
+    {
+        if (keys)
+            return keys->masterPublicKey;
+
+        return {};
+    }
+
+    std::optional<SecretKey const> getSecretKey() const
+    {
+        if (keys)
+            return keys->secretKey;
+
+        return {};
+    }
+
 private:
     bool configInvalid_ = false;  //< Set to true if config was invalid
 };

src/ripple/app/misc/impl/ValidatorKeys.cpp

@@ -56,9 +56,7 @@ ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j)
             }
             else
             {
-                secretKey = token->validationSecret;
-                publicKey = pk;
-                masterPublicKey = m->masterKey;
+                keys.emplace(token->validationSecret, pk, m->masterKey);
                 nodeID = calcNodeID(m->masterKey);
                 sequence = m->sequence;
                 manifest = std::move(token->manifest);
@@ -83,10 +81,11 @@ ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j)
         }
         else
         {
-            secretKey = generateSecretKey(KeyType::secp256k1, *seed);
-            publicKey = derivePublicKey(KeyType::secp256k1, *secretKey);
-            masterPublicKey = *publicKey;
-            nodeID = calcNodeID(*publicKey);
+            SecretKey sk = generateSecretKey(KeyType::secp256k1, *seed);
+            PublicKey pk = derivePublicKey(KeyType::secp256k1, sk);
+            keys.emplace(sk,
+                         derivePublicKey(KeyType::secp256k1, sk), pk);
+            nodeID = calcNodeID(pk);
             sequence = 0;
         }
     }

src/test/app/ValidatorKeys_test.cpp

@@ -107,7 +107,7 @@ class ValidatorKeys_test : public beast::unit_test::suite
             // No config -> no key but valid
             Config c;
             ValidatorKeys k{c, journal};
-            BEAST_EXPECT(!k.publicKey);
+            BEAST_EXPECT(!k.getPublicKey());
             BEAST_EXPECT(k.manifest.empty());
             BEAST_EXPECT(!k.configInvalid());
         }
@@ -117,8 +117,8 @@ class ValidatorKeys_test : public beast::unit_test::suite
             c.section(SECTION_VALIDATION_SEED).append(seed);
 
             ValidatorKeys k{c, journal};
-            BEAST_EXPECT(k.publicKey == seedPublicKey);
-            BEAST_EXPECT(k.secretKey == seedSecretKey);
+            BEAST_EXPECT(k.getPublicKey() == seedPublicKey);
+            BEAST_EXPECT(k.getSecretKey() == seedSecretKey);
             BEAST_EXPECT(k.nodeID == seedNodeID);
             BEAST_EXPECT(k.manifest.empty());
             BEAST_EXPECT(!k.configInvalid());
@@ -131,7 +131,7 @@ class ValidatorKeys_test : public beast::unit_test::suite
 
             ValidatorKeys k{c, journal};
             BEAST_EXPECT(k.configInvalid());
-            BEAST_EXPECT(!k.publicKey);
+            BEAST_EXPECT(!k.getPublicKey());
             BEAST_EXPECT(k.manifest.empty());
         }
 
@@ -141,8 +141,8 @@ class ValidatorKeys_test : public beast::unit_test::suite
             c.section(SECTION_VALIDATOR_TOKEN).append(tokenBlob);
             ValidatorKeys k{c, journal};
 
-            BEAST_EXPECT(k.publicKey == tokenPublicKey);
-            BEAST_EXPECT(k.secretKey == tokenSecretKey);
+            BEAST_EXPECT(k.getPublicKey() == tokenPublicKey);
+            BEAST_EXPECT(k.getSecretKey() == tokenSecretKey);
             BEAST_EXPECT(k.nodeID == tokenNodeID);
             BEAST_EXPECT(k.manifest == tokenManifest);
             BEAST_EXPECT(!k.configInvalid());
@@ -153,7 +153,7 @@ class ValidatorKeys_test : public beast::unit_test::suite
             c.section(SECTION_VALIDATOR_TOKEN).append("badtoken");
             ValidatorKeys k{c, journal};
             BEAST_EXPECT(k.configInvalid());
-            BEAST_EXPECT(!k.publicKey);
+            BEAST_EXPECT(!k.getPublicKey());
             BEAST_EXPECT(k.manifest.empty());
         }
 
@@ -165,7 +165,7 @@ class ValidatorKeys_test : public beast::unit_test::suite
             ValidatorKeys k{c, journal};
 
             BEAST_EXPECT(k.configInvalid());
-            BEAST_EXPECT(!k.publicKey);
+            BEAST_EXPECT(!k.getPublicKey());
             BEAST_EXPECT(k.manifest.empty());
         }
 
@@ -176,7 +176,7 @@ class ValidatorKeys_test : public beast::unit_test::suite
             ValidatorKeys k{c, journal};
 
             BEAST_EXPECT(k.configInvalid());
-            BEAST_EXPECT(!k.publicKey);
+            BEAST_EXPECT(!k.getPublicKey());
             BEAST_EXPECT(k.manifest.empty());
         }
     }