Allow multiple updates of a name per block.

This removes the mempool-restriction of allowing only one update
per name and block.  That restriction was introduced originally to
simplify the mempool implementation, but is actually easy to lift.

With this change, we allow a chain of updates of one name to be in the
mempool at once; this can be useful e.g. to quickly fix an erraneous
previous update without the need to wait for confirmations in between.
For name_new and name_firstupdate, no changes are made.  Only updates
are allowed to be chained on a pending operation, although updates can
be chained to a name_firstupdate.

Note that the consensus layer of Namecoin already allows such chains,
and the restriction lifted is purely a policy one (in other words, this
is not a fork of any sort).  For now, we only remove the restriction
from the mempool itself; the name_update RPC method still won't allow
creating such chains.  We will remove the RPC restriction in a second
step later on, once miners and nodes have updated to the new policy
so that such chains of updates can actually be expected to be relayed
and mined.

This fixes #304.

src/names/main.cpp

@@ -221,10 +221,15 @@ CheckNameTransaction (const CTransaction& tx, unsigned nHeight,
                               REJECT_INVALID, "tx-nameupdate-name-mismatch",
                               "NAME_UPDATE name mismatch to name input");
 
-      /* This is actually redundant, since expired names are removed
-         from the UTXO set and thus not available to be spent anyway.
-         But it does not hurt to enforce this here, too.  It is also
-         exercised by the unit tests.  */
+      /* If the name input is pending, then no further checks with respect
+         to the name input in the name database are done.  Otherwise, we verify
+         that the name input matches the name database; this is redundant
+         as UTXO handling takes care of it anyway, but we do it for
+         an extra safety layer.  */
+      const unsigned inHeight = coinIn.nHeight;
+      if (inHeight == MEMPOOL_HEIGHT)
+        return true;
+
       CNameData oldName;
       if (!view.GetName (name, oldName))
         return state.Invalid (ValidationInvalidReason::CONSENSUS, false,
@@ -234,11 +239,7 @@ CheckNameTransaction (const CTransaction& tx, unsigned nHeight,
         return state.Invalid (ValidationInvalidReason::CONSENSUS, false,
                               REJECT_INVALID, "tx-nameupdate-expired",
                               "NAME_UPDATE on an expired name");
-
-      /* This is an internal consistency check.  If everything is fine,
-         the input coins from the UTXO database should match the
-         name database.  */
-      assert (static_cast<unsigned> (coinIn.nHeight) == oldName.getHeight ());
+      assert (inHeight == oldName.getHeight ());
       assert (tx.vin[nameIn].prevout == oldName.getUpdateOutpoint ());
 
       return true;

src/names/mempool.cpp

@@ -14,6 +14,79 @@
 
 /* ************************************************************************** */
 
+namespace
+{
+
+/**
+ * Returns the outpoint matching the name operation in a given mempool tx, if
+ * there is any.  The txid must be for an entry in the mempool.
+ */
+COutPoint
+getNameOutput (const CTxMemPool& pool, const uint256& txid)
+{
+  AssertLockHeld (pool.cs);
+
+  const auto mit = pool.mapTx.find (txid);
+  assert (mit != pool.mapTx.end ());
+  const auto& vout = mit->GetTx ().vout;
+
+  for (unsigned i = 0; i != vout.size (); ++i)
+    {
+      const CNameScript nameOp(vout[i].scriptPubKey);
+      if (nameOp.isNameOp ())
+        return COutPoint (txid, i);
+    }
+
+  return COutPoint ();
+}
+
+} // anonymous namespace
+
+COutPoint
+CNameMemPool::lastNameOutput (const valtype& name) const
+{
+  const auto itUpd = updates.find (name);
+  if (itUpd != updates.end ())
+    {
+      /* From all the pending updates, we have to find the last one.  This is
+         the unique outpoint that is not also spent by some other transaction.
+         Thus, we keep track of all the transactions spent as well, and then
+         remove those from the sets of candidates.  Doing so by txid (rather
+         than outpoint) is enough, as those transactions must be in a "chain"
+         anyway.  */
+
+      const std::set<uint256>& candidateTxids = itUpd->second;
+      std::set<uint256> spentTxids;
+
+      for (const auto& txid : candidateTxids)
+        {
+          const auto mit = pool.mapTx.find (txid);
+          assert (mit != pool.mapTx.end ());
+          for (const auto& in : mit->GetTx ().vin)
+            spentTxids.insert (in.prevout.hash);
+        }
+
+      COutPoint res;
+      for (const auto& txid : candidateTxids)
+        {
+          if (spentTxids.count (txid) > 0)
+            continue;
+
+          assert (res.IsNull ());
+          res = getNameOutput (pool, txid);
+        }
+
+      assert (!res.IsNull ());
+      return res;
+    }
+
+  const auto itReg = mapNameRegs.find (name);
+  if (itReg != mapNameRegs.end ())
+    return getNameOutput (pool, itReg->second);
+
+  return COutPoint ();
+}
+
 void
 CNameMemPool::addUnchecked (const CTxMemPoolEntry& entry)
 {
@@ -23,7 +96,7 @@ CNameMemPool::addUnchecked (const CTxMemPoolEntry& entry)
   if (entry.isNameNew ())
     {
       const valtype& newHash = entry.getNameNewHash ();
-      const NameTxMap::const_iterator mit = mapNameNews.find (newHash);
+      const auto mit = mapNameNews.find (newHash);
       if (mit != mapNameNews.end ())
         assert (mit->second == txHash);
       else
@@ -40,8 +113,12 @@ CNameMemPool::addUnchecked (const CTxMemPoolEntry& entry)
   if (entry.isNameUpdate ())
     {
       const valtype& name = entry.getName ();
-      assert (mapNameUpdates.count (name) == 0);
-      mapNameUpdates.insert (std::make_pair (name, txHash));
+      const auto mit = updates.find (name);
+
+      if (mit == updates.end ())
+        updates.emplace (name, std::set<uint256> ({txHash}));
+      else
+        mit->second.insert (txHash);
     }
 }
 
@@ -52,15 +129,21 @@ CNameMemPool::remove (const CTxMemPoolEntry& entry)
 
   if (entry.isNameRegistration ())
     {
-      const NameTxMap::iterator mit = mapNameRegs.find (entry.getName ());
+      const auto mit = mapNameRegs.find (entry.getName ());
       assert (mit != mapNameRegs.end ());
       mapNameRegs.erase (mit);
     }
+
   if (entry.isNameUpdate ())
     {
-      const NameTxMap::iterator mit = mapNameUpdates.find (entry.getName ());
-      assert (mit != mapNameUpdates.end ());
-      mapNameUpdates.erase (mit);
+      const auto itName = updates.find (entry.getName ());
+      assert (itName != updates.end ());
+      auto& txids = itName->second;
+      const auto itTxid = txids.find (entry.GetTx ().GetHash ());
+      assert (itTxid != txids.end ());
+      txids.erase (itTxid);
+      if (txids.empty ())
+        updates.erase (itName);
     }
 }
 
@@ -78,10 +161,10 @@ CNameMemPool::removeConflicts (const CTransaction& tx)
       if (nameOp.isNameOp () && nameOp.getNameOp () == OP_NAME_FIRSTUPDATE)
         {
           const valtype& name = nameOp.getOpName ();
-          const NameTxMap::const_iterator mit = mapNameRegs.find (name);
+          const auto mit = mapNameRegs.find (name);
           if (mit != mapNameRegs.end ())
             {
-              const CTxMemPool::txiter mit2 = pool.mapTx.find (mit->second);
+              const auto mit2 = pool.mapTx.find (mit->second);
               assert (mit2 != pool.mapTx.end ());
               pool.removeRecursive (mit2->GetTx (),
                                     MemPoolRemovalReason::NAME_CONFLICT);
@@ -100,7 +183,7 @@ CNameMemPool::removeUnexpireConflicts (const std::set<valtype>& unexpired)
       LogPrint (BCLog::NAMES, "unexpired: %s, mempool: %u\n",
                 EncodeNameForMessage (name), mapNameRegs.count (name));
 
-      const NameTxMap::const_iterator mit = mapNameRegs.find (name);
+      const auto mit = mapNameRegs.find (name);
       if (mit != mapNameRegs.end ())
         {
           const CTxMemPool::txiter mit2 = pool.mapTx.find (mit->second);
@@ -118,17 +201,25 @@ CNameMemPool::removeExpireConflicts (const std::set<valtype>& expired)
 
   for (const auto& name : expired)
     {
-      LogPrint (BCLog::NAMES, "expired: %s, mempool: %u\n",
-                EncodeNameForMessage (name), mapNameUpdates.count (name));
+      LogPrint (BCLog::NAMES, "expired: %s\n", EncodeNameForMessage (name));
+
+      const auto mit = updates.find (name);
+      if (mit == updates.end ())
+        continue;
 
-      const NameTxMap::const_iterator mit = mapNameUpdates.find (name);
-      if (mit != mapNameUpdates.end ())
+      /* We need to make sure that we keep our copy of txids even when the
+         transactions are removed one by one.  */
+      const std::set<uint256> txidsCopy = mit->second;
+
+      for (const auto& txid : txidsCopy)
         {
-          const CTxMemPool::txiter mit2 = pool.mapTx.find (mit->second);
+          const CTxMemPool::txiter mit2 = pool.mapTx.find (txid);
           assert (mit2 != pool.mapTx.end ());
           pool.removeRecursive (mit2->GetTx (),
                                 MemPoolRemovalReason::NAME_CONFLICT);
         }
+
+      assert (updates.count (name) == 0);
     }
 }
 
@@ -145,14 +236,14 @@ CNameMemPool::check (const CCoinsView& coins) const
     nHeight = mapBlockIndex.find (blockHash)->second->nHeight;
 
   std::set<valtype> nameRegs;
-  std::set<valtype> nameUpdates;
+  std::map<valtype, unsigned> nameUpdates;
   for (const auto& entry : pool.mapTx)
     {
       const uint256 txHash = entry.GetTx ().GetHash ();
       if (entry.isNameNew ())
         {
           const valtype& newHash = entry.getNameNewHash ();
-          const NameTxMap::const_iterator mit = mapNameNews.find (newHash);
+          const auto mit = mapNameNews.find (newHash);
 
           assert (mit != mapNameNews.end ());
           assert (mit->second == txHash);
@@ -162,7 +253,7 @@ CNameMemPool::check (const CCoinsView& coins) const
         {
           const valtype& name = entry.getName ();
 
-          const NameTxMap::const_iterator mit = mapNameRegs.find (name);
+          const auto mit = mapNameRegs.find (name);
           assert (mit != mapNameRegs.end ());
           assert (mit->second == txHash);
 
@@ -181,31 +272,25 @@ CNameMemPool::check (const CCoinsView& coins) const
         {
           const valtype& name = entry.getName ();
 
-          const NameTxMap::const_iterator mit = mapNameUpdates.find (name);
-          assert (mit != mapNameUpdates.end ());
-          assert (mit->second == txHash);
+          const auto mit = updates.find (name);
+          assert (mit != updates.end ());
+          assert (mit->second.count (txHash) > 0);
 
-          assert (nameUpdates.count (name) == 0);
-          nameUpdates.insert (name);
+          ++nameUpdates[name];
 
           /* As above, use nHeight+1 for the expiration check.  */
           CNameData data;
-          if (!coins.GetName (name, data))
-            assert (false);
-          assert (!data.isExpired (nHeight + 1));
+          if (coins.GetName (name, data))
+            assert (!data.isExpired (nHeight + 1));
+          else
+            assert (registersName (name));
         }
     }
 
   assert (nameRegs.size () == mapNameRegs.size ());
-  assert (nameUpdates.size () == mapNameUpdates.size ());
-
-  /* Check that nameRegs and nameUpdates are disjoint.  They must be since
-     a name can only be in either category, depending on whether it exists
-     at the moment or not.  */
-  for (const auto& name : nameRegs)
-    assert (nameUpdates.count (name) == 0);
-  for (const auto& name : nameUpdates)
-    assert (nameRegs.count (name) == 0);
+  assert (nameUpdates.size () == updates.size ());
+  for (const auto& upd : nameUpdates)
+    assert (updates.at (upd.first).size () == upd.second);
 }
 
 bool
@@ -216,11 +301,6 @@ CNameMemPool::checkTx (const CTransaction& tx) const
   if (!tx.IsNamecoin ())
     return true;
 
-  /* In principle, multiple name_updates could be performed within the
-     mempool at once (building upon each other).  This is disallowed, though,
-     since the current mempool implementation does not like it.  (We keep
-     track of only a single update tx for each name.)  */
-
   for (const auto& txout : tx.vout)
     {
       const CNameScript nameOp(txout.scriptPubKey);
@@ -248,12 +328,11 @@ CNameMemPool::checkTx (const CTransaction& tx) const
           }
 
         case OP_NAME_UPDATE:
-          {
-            const valtype& name = nameOp.getOpName ();
-            if (updatesName (name))
-              return false;
-            break;
-          }
+          /* Multiple updates of the same name in a chain are perfectly fine.
+             The main mempool logic takes care that updates are ordered
+             properly and really a chain, as this is automatic due to the
+             coloured-coin nature of names.  */
+          break;
 
         default:
           assert (false);