cleanup transaction replacement return codes

src/consensus/validation.h

@@ -17,6 +17,7 @@ static const unsigned char REJECT_NONSTANDARD = 0x40;
 static const unsigned char REJECT_DUST = 0x41;
 static const unsigned char REJECT_INSUFFICIENTFEE = 0x42;
 static const unsigned char REJECT_CHECKPOINT = 0x43;
+static const unsigned char REJECT_HAVEBETTER = 0x44;
 
 /** Capture information about block/transaction validation */
 class CValidationState {

src/main.cpp

@@ -1181,7 +1181,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
     }
     }
 
-    CTxReplacementPoolResult rpr = RP_NotReplace;
+    CTxReplacementPoolResult rpr = RP_NotReplaceable;
 
     {
         CCoinsView dummy;
@@ -1339,22 +1339,31 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
         if ( komodo_is_notarytx(tx) == 0 )
             KOMODO_ON_DEMAND++;
 
-        if (fAcceptReplacement) {
+        if (fAcceptReplacement)
+        {
             CTxReplacementPoolItem item(tx, GetHeight());
             if (SetReplacementParams(item)) {
                 rpr = replacementPool.replace(item);
             }
         }
 
-        if (rpr == RP_Invalid) {
+        if (rpr == RP_HaveBetter)
+        {
             // already have a better one
-            // TODO: Shouldn't neccesary log this as invalid, not sure if punishing peers is the best idea
             fprintf(stderr,"accept failure.20\n");
-            return error("AcceptToMemoryPool: Replacement is worse %s", hash.ToString());
+            return state.DoS(0, error("AcceptToMemoryPool: Replacement is worse"), REJECT_HAVEBETTER, "replacement-is-worse");
+        }
+
+        if (rpr == RP_Invalid)
+        {
+            // Not valid according to replaceability rules
+            fprintf(stderr,"accept failure.21\n");
+            return state.DoS(0, error("AcceptToMemoryPool: Replacement is invalid"), REJECT_INVALID, "replacement-is-worse");
         }
 
         // If there is no replacement action happening...
-        if (rpr == RP_NotReplace) {
+        if (rpr == RP_NotReplaceable)
+        {
             // Store transaction in memory
             pool.addUnchecked(hash, entry, !IsInitialBlockDownload());
         }
@@ -1362,7 +1371,7 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
 
     // in order for replaceable transactions to sync with wallet, replacementpool should advise
     // wallet of transaction eviction
-    if (rpr == RP_NotReplace) {
+    if (rpr == RP_NotReplaceable) {
         SyncWithWallets(tx, NULL);
     }
 

src/replacementpool.cpp

@@ -1,39 +1,11 @@
 
-#include <map>
-#include <string>
-#include <iterator>
-
 #include "main.h"
-#include "coins.h"
 #include "replacementpool.h"
 
 
 CTxReplacementPool replacementPool;
 
 
-/*
- * Add a transaction to the pool, with a priority.
- * Return true if valid, false if not valid. */
-bool ValidateReplacementPoolItem(CTxReplacementPoolItem item)
-{
-    // Perform some validations.
-    if (item.tx.vin.size() > 1) {
-        // Replaceable transactions with multiple inputs are disabled for now. It's not yet clear
-        // what edge cases may arise. It is speculated that it will "just work", since if
-        // replaceable transactions spend multiple outputs using the replacement protocol,
-        // they will never conflict in the replaceMap data structure. But for now, to be prudent, disable.
-        return false;
-    }
-
-    // A transaction with 0 priority is not valid.
-    if (item.priority == 0) {
-        return false;
-    }
-
-    return true;
-}
-
-
 /*
  * Validate the item
  * 
@@ -45,7 +17,17 @@ bool ValidateReplacementPoolItem(CTxReplacementPoolItem item)
  */
 CTxReplacementPoolResult CTxReplacementPool::replace(CTxReplacementPoolItem &item)
 {
-    if (!ValidateReplacementPoolItem(item)) {
+    AssertLockHeld(cs_main);
+
+    // Perform some validations.
+    if (item.tx.vin.size() > 1) {
+        // Replaceable transactions with multiple inputs are disabled.
+        // It seems like quite alot of additional complexity.
+        return RP_Invalid;
+    }
+
+    // A transaction with 0 priority is not valid.
+    if (item.priority == 0) {
         return RP_Invalid;
     }
 
@@ -53,22 +35,25 @@ CTxReplacementPoolResult CTxReplacementPool::replace(CTxReplacementPoolItem &ite
 
     if (it != replaceMap.end()) {
         if (it->second.priority >= item.priority) {
-            // Already have a transaction with equal or greater priority; this is not valid
+            return RP_HaveBetter; // (ThanksThough)
-            return RP_Invalid;
         }
-        // copy the previous starting block over
-        item.startBlock = it->second.startBlock;
     }
 
     // This transaction has higher priority
     replaceMap[item.tx.vin[0].prevout] = item;
+    replaceMap[item.tx.vin[0].prevout].startBlock = it->second.startBlock;
 
-    return RP_Valid;
+    return RP_Accepted;
 }
 
 
+/*
+ * Remove and return any spends that have matured
+ */
 void CTxReplacementPool::removePending(int height, std::vector<CTransaction> &txs)
 {
+    AssertLockHeld(cs_main);
+
     for (auto it = replaceMap.begin(); it != replaceMap.end(); /**/) {
         CTxReplacementPoolItem &rep = it->second;
         if (rep.GetTargetBlock() <= height) {

src/replacementpool.h

@@ -5,11 +5,11 @@
 #ifndef KOMODO_REPLACEMENTCACHE_H
 #define KOMODO_REPLACEMENTCACHE_H
 
-#include "coins.h"
 #include "primitives/transaction.h"
 
 
-enum CTxReplacementPoolResult { RP_NotReplace, RP_Valid, RP_Invalid };
+enum CTxReplacementPoolResult { RP_Accepted, RP_HaveBetter, RP_Invalid, RP_NotReplaceable };
+
 
 class CTxReplacementPoolItem
 {
@@ -28,41 +28,43 @@ public:
         replacementWindow = 0;
     }
 
-    int GetTargetBlock()
+    int GetTargetBlock() { return startBlock + replacementWindow; }
-    {
-        return startBlock + replacementWindow;
-    }
 };
 
 /**
- * CTxReplacementPool stores valid-according-to-the-current-best-chain (??? do we need to do this?)
+ * CTxReplacementPool stores transactions that are valid but held for
- * transactions that are valid but held for period of time during which
+ * period of time during which they may be replaced.
- * they may be replaced.
  *
  * Transactions are added when they are found to be valid but not added
  * to the mempool until a timeout.
  *
  * Replacement pool is like another mempool before the main mempool.
+ *
+ * Transactions in the replacement pool are indexed by the output
+ * that they are spending. Once a replaceable transaction tries to
+ * spend an output, a countdown of blocks begins at the current block
+ * plus a window that is set by "userland" code. If another, better
+ * transaction replaces the spend that's already pending, the countdown
+ * start block remains the same.
  */
 class CTxReplacementPool
 {
 private:
-    // A potential replacement is first stored here, not in the replaceMap.
-    // This is in case some other checks fail, during AcceptToMemoryPool.
-    // Later on, if all checks pass, processReplacement() is called.
-    
     /* Index of spends that may be replaced */
     std::map<COutPoint, CTxReplacementPoolItem> replaceMap;
 public:
+    /* Try to replace a transaction in the index */
     CTxReplacementPoolResult replace(CTxReplacementPoolItem &item);
 
-    // Remove and return all transactions up to a given block height.
+    /* Remove and return all transactions up to a given block height */
     void removePending(int height, std::vector<CTransaction> &txs);
 
+    /* Find a transaction in the index by it's hash. */
     bool lookup(uint256 txHash, CTransaction &tx);
 };
 
 
+/* Global instance */
 extern CTxReplacementPool replacementPool;
 
 #endif // KOMODO_REPLACEMENTCACHE_H