From e102037e61a2274cc807983f3aa150611e471740 Mon Sep 17 00:00:00 2001 From: Scott Sadler Date: Thu, 8 Mar 2018 16:35:39 -0300 Subject: [PATCH] cleanup transaction replacement return codes --- src/consensus/validation.h | 1 + src/main.cpp | 23 ++++++++++++----- src/replacementpool.cpp | 53 ++++++++++++++------------------------ src/replacementpool.h | 30 +++++++++++---------- 4 files changed, 52 insertions(+), 55 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index c62adcd8f..6c4db4c59 100644 --- a/src/consensus/validation.h +++ b/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 { diff --git a/src/main.cpp b/src/main.cpp index 2ff543b8c..09ff2871c 100644 --- a/src/main.cpp +++ b/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); } diff --git a/src/replacementpool.cpp b/src/replacementpool.cpp index 0a509ef37..42da4fa7c 100644 --- a/src/replacementpool.cpp +++ b/src/replacementpool.cpp @@ -1,39 +1,11 @@ -#include -#include -#include - #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_Invalid; + return RP_HaveBetter; // (ThanksThough) } - // 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 &txs) { + AssertLockHeld(cs_main); + for (auto it = replaceMap.begin(); it != replaceMap.end(); /**/) { CTxReplacementPoolItem &rep = it->second; if (rep.GetTargetBlock() <= height) { diff --git a/src/replacementpool.h b/src/replacementpool.h index 71cb775fd..6bf577999 100644 --- a/src/replacementpool.h +++ b/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() - { - return startBlock + replacementWindow; - } + int GetTargetBlock() { return startBlock + replacementWindow; } }; /** - * CTxReplacementPool stores valid-according-to-the-current-best-chain (??? do we need to do this?) - * transactions that are valid but held for period of time during which - * they may be replaced. + * CTxReplacementPool stores transactions that are valid but held for + * period of time during which 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 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 &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