Remove default ctors from SecretKey and PublicKey: (#4607)

* It is now an invariant that all constructed Public Keys are valid,
  non-empty and contain 33 bytes of data.
* Additionally, the memory footprint of the PublicKey class is reduced.
  The size_ data member is declared as static.
* Distinguish and identify the PublisherList retrieved from the local
  config file, versus the ones obtained from other validators.
* Fixes #2942

src/ripple/app/consensus/RCLConsensus.cpp

@@ -98,20 +98,22 @@ RCLConsensus::Adaptor::Adaptor(
     JLOG(j_.info()) << "Consensus engine started (cookie: " +
             std::to_string(valCookie_) + ")";
 
-    if (validatorKeys_.nodeID != beast::zero)
+    if (validatorKeys_.nodeID != beast::zero && validatorKeys_.keys)
     {
         std::stringstream ss;
 
         JLOG(j_.info()) << "Validator identity: "
                         << toBase58(
                                TokenType::NodePublic,
-                               validatorKeys_.masterPublicKey);
+                               validatorKeys_.keys->masterPublicKey);
 
-        if (validatorKeys_.masterPublicKey != validatorKeys_.publicKey)
+        if (validatorKeys_.keys->masterPublicKey !=
+            validatorKeys_.keys->publicKey)
         {
             JLOG(j_.debug())
                 << "Validator ephemeral signing key: "
-                << toBase58(TokenType::NodePublic, validatorKeys_.publicKey)
+                << toBase58(
+                       TokenType::NodePublic, validatorKeys_.keys->publicKey)
                 << " (seq: " << std::to_string(validatorKeys_.sequence) << ")";
         }
     }
@@ -210,13 +212,20 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal)
         proposal.prevLedger().begin(), proposal.prevLedger().size());
     prop.set_proposeseq(proposal.proposeSeq());
     prop.set_closetime(proposal.closeTime().time_since_epoch().count());
-    prop.set_nodepubkey(
-        validatorKeys_.publicKey.data(), validatorKeys_.publicKey.size());
 
-    auto sig = signDigest(
-        validatorKeys_.publicKey,
-        validatorKeys_.secretKey,
-        proposal.signingHash());
+    if (!validatorKeys_.keys)
+    {
+        JLOG(j_.warn()) << "RCLConsensus::Adaptor::propose: ValidatorKeys "
+                           "not set: \n";
+        return;
+    }
+
+    auto const& keys = *validatorKeys_.keys;
+
+    prop.set_nodepubkey(keys.publicKey.data(), keys.publicKey.size());
+
+    auto sig =
+        signDigest(keys.publicKey, keys.secretKey, proposal.signingHash());
 
     prop.set_signature(sig.data(), sig.size());
 
@@ -225,7 +234,7 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal)
         proposal.prevLedger(),
         proposal.proposeSeq(),
         proposal.closeTime(),
-        validatorKeys_.publicKey,
+        keys.publicKey,
         sig);
 
     app_.getHashRouter().addSuppression(suppression);
@@ -799,10 +808,19 @@ RCLConsensus::Adaptor::validate(
         validationTime = lastValidationTime_ + 1s;
     lastValidationTime_ = validationTime;
 
+    if (!validatorKeys_.keys)
+    {
+        JLOG(j_.warn()) << "RCLConsensus::Adaptor::validate: ValidatorKeys "
+                           "not set\n";
+        return;
+    }
+
+    auto const& keys = *validatorKeys_.keys;
+
     auto v = std::make_shared<STValidation>(
         lastValidationTime_,
-        validatorKeys_.publicKey,
-        validatorKeys_.secretKey,
+        keys.publicKey,
+        keys.secretKey,
         validatorKeys_.nodeID,
         [&](STValidation& v) {
             v.setFieldH256(sfLedgerHash, ledger.id());
@@ -960,7 +978,7 @@ 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.size() != 0 &&
+    validating_ = validatorKeys_.keys &&
         prevLgr.seq() >= app_.getMaxDisallowedLedger() &&
         !app_.getOPs().isBlocked();
 
@@ -1033,7 +1051,7 @@ RCLConsensus::Adaptor::laggards(
 bool
 RCLConsensus::Adaptor::validator() const
 {
-    return !validatorKeys_.publicKey.empty();
+    return validatorKeys_.keys.has_value();
 }
 
 void

src/ripple/app/main/Application.cpp

@@ -185,7 +185,7 @@ class ApplicationImp : public Application, public BasicApp
 
     NodeCache m_tempNodeCache;
     CachedSLEs cachedSLEs_;
-    std::pair<PublicKey, SecretKey> nodeIdentity_;
+    std::optional<std::pair<PublicKey, SecretKey>> nodeIdentity_;
     ValidatorKeys const validatorKeys_;
 
     std::unique_ptr<Resource::Manager> m_resourceManager;
@@ -587,13 +587,20 @@ class ApplicationImp : public Application, public BasicApp
     std::pair<PublicKey, SecretKey> const&
     nodeIdentity() override
     {
-        return nodeIdentity_;
+        if (nodeIdentity_)
+            return *nodeIdentity_;
+
+        LogicError(
+            "Accessing Application::nodeIdentity() before it is initialized.");
     }
 
-    PublicKey const&
+    std::optional<PublicKey const>
     getValidationPublicKey() const override
     {
-        return validatorKeys_.publicKey;
+        if (!validatorKeys_.keys)
+            return {};
+
+        return validatorKeys_.keys->publicKey;
     }
 
     NetworkOPs&
@@ -1345,7 +1352,7 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline)
         return false;
     }
 
-    if (validatorKeys_.publicKey.size())
+    if (validatorKeys_.keys)
         setMaxDisallowedLedger();
 
     // Configure the amendments the server supports
@@ -1466,9 +1473,19 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline)
 
             publisherManifests_->load(getWalletDB(), "PublisherManifests");
 
+            // It is possible to have a valid ValidatorKeys object without
+            // setting the signingKey or masterKey. This occurs if the
+            // configuration file does not have either
+            // SECTION_VALIDATOR_TOKEN or SECTION_VALIDATION_SEED section.
+
+            // masterKey for the configuration-file specified validator keys
+            std::optional<PublicKey> localSigningKey;
+            if (validatorKeys_.keys)
+                localSigningKey = validatorKeys_.keys->publicKey;
+
             // Setup trusted validators
             if (!validators_->load(
-                    validatorKeys_.publicKey,
+                    localSigningKey,
                     config().section(SECTION_VALIDATORS).values(),
                     config().section(SECTION_VALIDATOR_LIST_KEYS).values()))
             {

src/ripple/app/main/Application.h

@@ -242,7 +242,7 @@ class Application : public beast::PropertyStream::Source
     virtual std::pair<PublicKey, SecretKey> const&
     nodeIdentity() = 0;
 
-    virtual PublicKey const&
+    virtual std::optional<PublicKey const>
     getValidationPublicKey() const = 0;
 
     virtual Resource::Manager&

src/ripple/app/misc/Manifest.h

@@ -86,15 +86,33 @@ struct Manifest
     PublicKey masterKey;
 
     /// The ephemeral key associated with this manifest.
-    PublicKey signingKey;
+    // A revoked manifest does not have a signingKey
+    // This field is specified as "optional" in manifestFormat's
+    // SOTemplate
+    std::optional<PublicKey> signingKey;
 
     /// The sequence number of this manifest.
     std::uint32_t sequence = 0;
 
     /// The domain, if one was specified in the manifest; empty otherwise.
     std::string domain;
 
-    Manifest() = default;
+    Manifest() = delete;
+
+    Manifest(
+        std::string const& serialized_,
+        PublicKey const& masterKey_,
+        std::optional<PublicKey> const& signingKey_,
+        std::uint32_t seq,
+        std::string const& domain_)
+        : serialized(serialized_)
+        , masterKey(masterKey_)
+        , signingKey(signingKey_)
+        , sequence(seq)
+        , domain(domain_)
+    {
+    }
+
     Manifest(Manifest const& other) = delete;
     Manifest&
     operator=(Manifest const& other) = delete;
@@ -110,6 +128,12 @@ struct Manifest
     uint256
     hash() const;
 
+    /// Returns `true` if manifest revokes master key
+    // The maximum possible sequence number means that the master key has
+    // been revoked
+    static bool
+    revoked(std::uint32_t sequence);
+
     /// Returns `true` if manifest revokes master key
     bool
     revoked() const;
@@ -266,7 +290,7 @@ class ManifestCache
 
         May be called concurrently
     */
-    PublicKey
+    std::optional<PublicKey>
     getSigningKey(PublicKey const& pk) const;
 
     /** Returns ephemeral signing key's master public key.

src/ripple/app/misc/NegativeUNLVote.cpp

@@ -90,15 +90,15 @@ NegativeUNLVote::doVoting(
             auto n =
                 choose(prevLedger->info().hash, candidates.toDisableCandidates);
             assert(nidToKeyMap.count(n));
-            addTx(seq, nidToKeyMap[n], ToDisable, initialSet);
+            addTx(seq, nidToKeyMap.at(n), ToDisable, initialSet);
         }
 
         if (!candidates.toReEnableCandidates.empty())
         {
             auto n = choose(
                 prevLedger->info().hash, candidates.toReEnableCandidates);
             assert(nidToKeyMap.count(n));
-            addTx(seq, nidToKeyMap[n], ToReEnable, initialSet);
+            addTx(seq, nidToKeyMap.at(n), ToReEnable, initialSet);
         }
     }
 }

src/ripple/app/misc/NetworkOPs.cpp

@@ -1970,9 +1970,9 @@ NetworkOPsImp::pubManifest(Manifest const& mo)
 
         jvObj[jss::type] = "manifestReceived";
         jvObj[jss::master_key] = toBase58(TokenType::NodePublic, mo.masterKey);
-        if (!mo.signingKey.empty())
+        if (mo.signingKey)
             jvObj[jss::signing_key] =
-                toBase58(TokenType::NodePublic, mo.signingKey);
+                toBase58(TokenType::NodePublic, *mo.signingKey);
         jvObj[jss::seq] = Json::UInt(mo.sequence);
         if (auto sig = mo.getSignature())
             jvObj[jss::signature] = strHex(*sig);
@@ -2456,10 +2456,11 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters)
 
     if (admin)
     {
-        if (!app_.getValidationPublicKey().empty())
+        if (auto const localPubKey = app_.validators().localPublicKey();
+            localPubKey && app_.getValidationPublicKey())
         {
-            info[jss::pubkey_validator] = toBase58(
-                TokenType::NodePublic, app_.validators().localPublicKey());
+            info[jss::pubkey_validator] =
+                toBase58(TokenType::NodePublic, localPubKey.value());
         }
         else
         {

src/ripple/app/misc/ValidatorKeys.h

@@ -36,13 +36,35 @@ class Config;
 class ValidatorKeys
 {
 public:
-    PublicKey masterPublicKey;
-    PublicKey publicKey;
-    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(
+            PublicKey const& masterPublic_,
+            PublicKey const& public_,
+            SecretKey const& secret_)
+            : 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;
 
+    ValidatorKeys() = delete;
     ValidatorKeys(Config const& config, beast::Journal j);
 
     bool

src/ripple/app/misc/ValidatorList.h

@@ -247,7 +247,17 @@ class ValidatorList
     // a seed, the signing key is the same as the master key.
     hash_set<PublicKey> trustedSigningKeys_;
 
-    PublicKey localPubKey_;
+    std::optional<PublicKey> localPubKey_;
+
+    // The below variable contains the Publisher list specified in the local
+    // config file under the title of SECTION_VALIDATORS or [validators].
+    // This list is not associated with the masterKey of any publisher.
+
+    // Appropos PublisherListCollection fields, localPublisherList does not
+    // have any "remaining" manifests. It is assumed to be perennially
+    // "available". The "validUntil" field is set to the highest possible
+    // value of the field, hence this list is always valid.
+    PublisherList localPublisherList;
 
     // The master public keys of the current negative UNL
     hash_set<PublicKey> negativeUNL_;
@@ -331,7 +341,7 @@ class ValidatorList
     */
     bool
     load(
-        PublicKey const& localSigningKey,
+        std::optional<PublicKey> const& localSigningKey,
         std::vector<std::string> const& configKeys,
         std::vector<std::string> const& publisherKeys);
 
@@ -553,13 +563,14 @@ class ValidatorList
     bool
     trustedPublisher(PublicKey const& identity) const;
 
-    /** Returns local validator public key
+    /** This function returns the local validator public key
+     * or a std::nullopt
 
         @par Thread Safety
 
         May be called concurrently
     */
-    PublicKey
+    std::optional<PublicKey>
     localPublicKey() const;
 
     /** Invokes the callback once for every listed validation public key.
@@ -766,6 +777,8 @@ class ValidatorList
         std::optional<uint256> const& hash,
         lock_guard const&);
 
+    // This function updates the keyListings_ counts for all the trusted
+    // master keys
     void
     updatePublisherList(
         PublicKey const& pubKey,
@@ -849,11 +862,10 @@ class ValidatorList
 
         Calling public member function is expected to lock mutex
     */
-    ListDisposition
+    std::pair<ListDisposition, std::optional<PublicKey>>
     verify(
         lock_guard const&,
         Json::Value& list,
-        PublicKey& pubKey,
         std::string const& manifest,
         std::string const& blob,
         std::string const& signature);