Auto merge of #4675 - str4d:zip-207-ncc-comments, r=nuttycom

ZIP 207 and ZIP 214 NCC comments

Changes to the ZIP 207 and ZIP 214 implementations as suggested in the NCC audit.

src/consensus/funding.cpp

@@ -10,7 +10,7 @@ namespace Consensus
  * General information about each funding stream.
  * Ordered by Consensus::FundingStreamIndex.
  */
-const struct FSInfo FundingStreamInfo[Consensus::MAX_FUNDING_STREAMS] = {
+constexpr struct FSInfo FundingStreamInfo[Consensus::MAX_FUNDING_STREAMS] = {
     {
         .recipient = "Electric Coin Company",
         .specification = "https://zips.z.cash/zip-0214",
@@ -31,6 +31,16 @@ const struct FSInfo FundingStreamInfo[Consensus::MAX_FUNDING_STREAMS] = {
     }
 };
 
+static constexpr bool validateFundingStreamInfo(uint32_t idx) {
+    return (idx >= Consensus::MAX_FUNDING_STREAMS || (
+        FundingStreamInfo[idx].valueNumerator < FundingStreamInfo[idx].valueDenominator &&
+        FundingStreamInfo[idx].valueNumerator < (INT64_MAX / MAX_MONEY) &&
+        validateFundingStreamInfo(idx + 1)));
+}
+static_assert(
+    validateFundingStreamInfo(Consensus::FIRST_FUNDING_STREAM),
+    "Invalid FundingStreamInfo");
+
 CAmount FSInfo::Value(CAmount blockSubsidy) const
 {
     // Integer division is floor division for nonnegative integers in C++
@@ -43,17 +53,22 @@ std::set<FundingStreamElement> GetActiveFundingStreamElements(
     const Consensus::Params& params)
 {
     std::set<std::pair<FundingStreamAddress, CAmount>> requiredElements;
-    for (uint32_t idx = Consensus::FIRST_FUNDING_STREAM; idx < Consensus::MAX_FUNDING_STREAMS; idx++) {
+
-        // The following indexed access is safe as Consensus::MAX_FUNDING_STREAMS is used
+    // Funding streams are disabled if Canopy is not active.
-        // in the definition of vFundingStreams.
+    if (params.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_CANOPY)) {
-        auto fs = params.vFundingStreams[idx];
+        for (uint32_t idx = Consensus::FIRST_FUNDING_STREAM; idx < Consensus::MAX_FUNDING_STREAMS; idx++) {
-        // Funding period is [startHeight, endHeight)
+            // The following indexed access is safe as Consensus::MAX_FUNDING_STREAMS is used
-        if (fs && nHeight >= fs.get().GetStartHeight() && nHeight < fs.get().GetEndHeight()) {
+            // in the definition of vFundingStreams.
-            requiredElements.insert(std::make_pair(
+            auto fs = params.vFundingStreams[idx];
-                fs.get().RecipientAddress(params, nHeight),
+            // Funding period is [startHeight, endHeight)
-                FundingStreamInfo[idx].Value(blockSubsidy)));
+            if (fs && nHeight >= fs.get().GetStartHeight() && nHeight < fs.get().GetEndHeight()) {
+                requiredElements.insert(std::make_pair(
+                    fs.get().RecipientAddress(params, nHeight),
+                    FundingStreamInfo[idx].Value(blockSubsidy)));
+            }
         }
     }
+
     return requiredElements;
 };
 
@@ -62,12 +77,17 @@ std::vector<FSInfo> GetActiveFundingStreams(
     const Consensus::Params& params)
 {
     std::vector<FSInfo> activeStreams;
-    for (uint32_t idx = Consensus::FIRST_FUNDING_STREAM; idx < Consensus::MAX_FUNDING_STREAMS; idx++) {
+
-        auto fs = params.vFundingStreams[idx];
+    // Funding streams are disabled if Canopy is not active.
-        if (fs && nHeight >= fs.get().GetStartHeight() && nHeight < fs.get().GetEndHeight()) {
+    if (params.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_CANOPY)) {
-            activeStreams.push_back(FundingStreamInfo[idx]);
+        for (uint32_t idx = Consensus::FIRST_FUNDING_STREAM; idx < Consensus::MAX_FUNDING_STREAMS; idx++) {
+            auto fs = params.vFundingStreams[idx];
+            if (fs && nHeight >= fs.get().GetStartHeight() && nHeight < fs.get().GetEndHeight()) {
+                activeStreams.push_back(FundingStreamInfo[idx]);
+            }
         }
     }
+
     return activeStreams;
 };
 

src/consensus/funding.h

@@ -14,11 +14,17 @@ namespace Consensus
 {
 
 struct FSInfo {
-    std::string recipient;
+    const char* recipient;
-    std::string specification;
+    const char* specification;
     uint64_t valueNumerator;
     uint64_t valueDenominator;
 
+    /**
+     * Returns the inherent value of this funding stream.
+     *
+     * For the active funding streams at a given height, use
+     * GetActiveFundingStreams() or GetActiveFundingStreamElements().
+     */
     CAmount Value(CAmount blockSubsidy) const;
 };
 

src/consensus/params.cpp

@@ -47,6 +47,9 @@ namespace Consensus {
      * first halving.
      */
     int Params::HalvingHeight(int nHeight, int halvingIndex) const {
+        assert(nHeight >= 0);
+        assert(halvingIndex > 0);
+
         // zip208
         // HalvingHeight(i) := max({ height ⦂ N | Halving(height) < i }) + 1
         //

src/main.cpp

@@ -4182,6 +4182,10 @@ bool ContextualCheckBlock(
 
     if (consensusParams.NetworkUpgradeActive(nHeight, Consensus::UPGRADE_CANOPY)) {
         // Funding streams are checked inside ContextualCheckTransaction.
+        // This empty conditional branch exists to enforce this ZIP 207 consensus rule:
+        //
+        //     Once the Canopy network upgrade activates, the existing consensus rule for
+        //     payment of the Founders' Reward is no longer active.
     } else if ((nHeight > 0) && (nHeight <= consensusParams.GetLastFoundersRewardBlockHeight(nHeight))) {
         // Coinbase transaction must include an output sending 20% of
         // the block subsidy to a Founders' Reward script, until the last Founders'