Merge #950: V0.12.1.x governance pr 2

13316a4 Return true from IsBlockValueValid when masternode data is not synced
  - This restores behavior very close to that in 12.0
  - Needed to prevent the forking problem currently being seen on
    testnet between online and offline nodes
  - This is expected to be a temporary fix while we develop a
    long-term solution for this problem

427086e Restore miner payments for superblocks

794b90d Added IsSynced field to JSON output of mnsync status RPC command
  - This is needed to allow fixing RPC tests so that they wait until
    the nodes are fully synced before performing tests

a9ddf6f Wait for nodes to sync masternode data during p2p-fullblocktest

qa/rpc-tests/invalidblockrequest.py

@@ -34,6 +34,7 @@ def run_test(self):
         self.tip = None
         self.block_time = None
         NetworkThread().start() # Start up network handling in another thread
+        sync_masternodes(self.nodes)
         test.run()
 
     def get_tests(self):

qa/rpc-tests/p2p-fullblocktest.py

@@ -44,6 +44,7 @@ def run_test(self):
         test = TestManager(self, self.options.tmpdir)
         test.add_all_connections(self.nodes)
         NetworkThread().start() # Start up network handling in another thread
+        sync_masternodes(self.nodes)
         test.run()
 
     def add_transactions_to_block(self, block, tx_list):

qa/rpc-tests/test_framework/util.py

@@ -55,6 +55,15 @@ def get_rpc_proxy(url, node_number, timeout=None):
 
     return coverage.AuthServiceProxyWrapper(proxy, coverage_logfile)
 
+def get_mnsync_status(node):
+    result = node.mnsync("status")
+    return result['IsSynced']
+
+def wait_to_sync(node):
+    synced = False
+    while not synced:
+        synced = get_mnsync_status(node)
+        time.sleep(0.5)
 
 def p2p_port(n):
     return 11000 + n + os.getpid()%999
@@ -96,6 +105,10 @@ def sync_mempools(rpc_connections, wait=1):
             break
         time.sleep(wait)
 
+def sync_masternodes(rpc_connections):
+    for node in rpc_connections:
+        wait_to_sync(node)
+
 bitcoind_processes = {}
 
 def initialize_datadir(dirname, n):

src/governance-classes.cpp

@@ -415,18 +415,18 @@ void CSuperblockManager::CreateSuperblock(CMutableTransaction& txNew, CAmount nF
 
     // CONFIGURE SUPERBLOCK OUTPUTS 
 
+    // Superblock payments are appended to the end of the coinbase vout vector
+
     DBG( cout << "CSuperblockManager::CreateSuperblock Number payments: " << pBlock->CountPayments() << endl; );
 
-    txNew.vout.resize(pBlock->CountPayments());
     for(int i = 0; i < pBlock->CountPayments(); i++)  {
         CGovernancePayment payment;
         DBG( cout << "CSuperblockManager::CreateSuperblock i = " << i << endl; );
         if(pBlock->GetPayment(i, payment))  {
             DBG( cout << "CSuperblockManager::CreateSuperblock Payment found " << endl; );
             // SET COINBASE OUTPUT TO SUPERBLOCK SETTING
 
-            txNew.vout[i].scriptPubKey = payment.script;
-            txNew.vout[i].nValue = payment.nAmount;
+            txNew.vout.push_back(CTxOut(payment.nAmount, payment.script));
 
             // PRINT NICE LOG OUTPUT FOR SUPERBLOCK PAYMENT
 
@@ -590,27 +590,43 @@ bool CSuperblock::IsValid(const CTransaction& txNew)
 
     // CONFIGURE SUPERBLOCK OUTPUTS 
 
-    int nPayments = CountPayments();    
+    int nOutputs = txNew.vout.size();
+    int nPayments = CountPayments();
+    int nMinerPayments = nOutputs - nPayments;
+
+    // We require an exact match (including order) between the expected
+    // superblock payments and the payments actually in the block, after
+    // skipping any initial miner payments.
+
+    if(nMinerPayments<0)  {
+        // This means the block cannot have all the superblock payments
+        // so it is not valid.
+        LogPrintf("CSuperblock::IsValid WARNING: Block invalid: Too few superblock payments");
+        return false;
+    }
+
     for(int i = 0; i < nPayments; i++) {
         CGovernancePayment payment;
-        if(GetPayment(i, payment)) {
-            // SET COINBASE OUTPUT TO SUPERBLOCK SETTING
+        if(!GetPayment(i, payment))  {
+            // This shouldn't happen so log a warning
+            LogPrintf("CSuperblock::IsValid WARNING: Failed to find payment: %d of %d total payments", i, nPayments);
+            continue;
+        }
 
-            if(payment.script == txNew.vout[i].scriptPubKey && payment.nAmount == txNew.vout[i].nValue) {
-                // WE FOUND THE CORRECT SUPERBLOCK OUTPUT!
-            } else {
-                // MISMATCHED SUPERBLOCK OUTPUT!
+        int nVoutIndex = nMinerPayments + i;
 
-                CTxDestination address1;
-                ExtractDestination(payment.script, address1);
-                CBitcoinAddress address2(address1);
+        bool fPaymentMatch = ((payment.script == txNew.vout[nVoutIndex].scriptPubKey) &&
+                              (payment.nAmount == txNew.vout[nVoutIndex].nValue));
 
-                // TODO: PRINT NICE N.N DASH OUTPUT
+        if(!fPaymentMatch)  {
+            // MISMATCHED SUPERBLOCK OUTPUT!
 
-                LogPrintf("SUPERBLOCK: output n %d payment %d to %s\n", i, payment.nAmount, address2.ToString());
+            CTxDestination address1;
+            ExtractDestination(payment.script, address1);
+            CBitcoinAddress address2(address1);
+            LogPrintf("CSuperblock::IsValid WARNING: Block invalid: output n %d payment %d to %s\n", nVoutIndex, payment.nAmount, address2.ToString());
 
-                return false;
-            }
+            return false;
         }
     }
 

src/governance-classes.h

@@ -244,7 +244,7 @@ class CSuperblock : public CGovernanceObject
 
     bool GetPayment(int nPaymentIndex, CGovernancePayment& paymentOut) 
     {
-        if(nPaymentIndex >= (int)vecPayments.size()) {
+        if((nPaymentIndex<0) || (nPaymentIndex >= (int)vecPayments.size())) {
             return false;
         }
 

src/masternode-payments.cpp

@@ -72,11 +72,11 @@ bool IsBlockValueValid(const CBlock& block, CAmount nExpectedValue){
 
     bool valueok = (block.vtx[0].GetValueOut() <= nExpectedValue);
 
-    // IF WE'RE NOT SYNCED, WE MAY NOT HAVE SUPERBLOCK DATA, SO RETURN THE USUAL CHECK 
+    // IF WE'RE NOT SYNCED, WE MAY NOT HAVE SUPERBLOCK DATA, SO RETURN TRUE FOR NOW 
 
     if(!masternodeSync.IsSynced()) {
         // IF NOT SYNCED, WE WILL SIMPLY FIND THE LONGEST CHAIN
-        return valueok;
+        return true;
     }
 
     // IF THIS IS A VALID SUPERBLOCK RETURN TRUE SINCE SUPERBLOCKS ARE CHECKED

src/rpcmisc.cpp

@@ -153,6 +153,7 @@ UniValue mnsync(const UniValue& params, bool fHelp)
         UniValue obj(UniValue::VOBJ);
 
         obj.push_back(Pair("IsBlockchainSynced", masternodeSync.IsBlockchainSynced()));
+        obj.push_back(Pair("IsSynced", masternodeSync.IsSynced()));
         obj.push_back(Pair("CurrentSyncingAssetName", masternodeSync.GetAssetName()));
         obj.push_back(Pair("lastMasternodeList", masternodeSync.lastMasternodeList));
         obj.push_back(Pair("lastMasternodeWinner", masternodeSync.lastMasternodeWinner));