Add true/false return value to prioritisetransaction #9947

1. 
   jonasschnelli commented at 10:02 am on March 8, 2017: contributor

   prioritisetransaction currently always return true, even if the transactions is not in the mempool. This PR fixes that by returning false in case of a failed prioritization.
2. Add true/false return value to prioritisetransaction 3e5d538b30
3. jonasschnelli added the label Mining on Mar 8, 2017
4. in src/txmempool.cpp: in 3e5d538b30

   894@@ -895,7 +895,7 @@ CTxMemPool::ReadFeeEstimates(CAutoFile& filein)
   895     return true;
   896 }
   897 
   898-void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeDelta)
   899+bool CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeDelta)
   900 {
   901     {
   902         LOCK(cs);
   

   ---

   ---

   

   bitkevin commented at 10:11 am on March 8, 2017:

   we should check the tx hash to find out if it’s exist in mapTx before add nFeeDelta to mapDeltas.

   ---

   

   jonasschnelli commented at 10:14 am on March 8, 2017:

   Good point.. I’ll fix that.

   ---

   

   jonasschnelli commented at 11:02 am on March 8, 2017:

   Oh wait. This seems to be intentional. This allows to increase fees (add a fee delta) before the transaction hits the mempool.

   ---

   

   gmaxwell commented at 8:14 pm on March 8, 2017:

   And an important feature: without the ability to add deltas before a transaction exists, you may not be able to prioritize a transaction at all (because of rejection).
5. 
   dcousens commented at 11:27 am on March 8, 2017: contributor

   As discussed on IRC, the return value would probably better approximate “whether the transaction was in the memory pool”, as prioritization should always occur.
6. in src/rpc/mining.cpp: in 3e5d538b30

   278@@ -279,7 +279,7 @@ UniValue prioritisetransaction(const JSONRPCRequest& request)
   279     uint256 hash = ParseHashStr(request.params[0].get_str(), "txid");
   280     CAmount nAmount = request.params[1].get_int64();
   281 
   282-    mempool.PrioritiseTransaction(hash, nAmount);
   283+    return mempool.PrioritiseTransaction(hash, nAmount);
   284     return true;
   

   ---

   ---

   

   laanwj commented at 2:39 pm on March 8, 2017:

   the return true can go here
7. 
   jnewbery commented at 3:06 pm on March 8, 2017: member

   I don’t understand why this is required. As mentioned above, prioritisetransaction can be used for transactions which are not yet in the mempool, and then the fee delta is:

   • used to determine whether the transaction should be allowed into the mempool (the ApplyDeltas() call in AcceptToMemoryPoolWorker())
   • applied to the transaction as it enters the mempool (mapTx.modify(newit, update_fee_delta(deltas.second)); in CTxMemPool::addUnchecked())

   If you really need to check whether the transaction is already in the mempool you can use getrawmempool.

8. 
   MarcoFalke commented at 3:11 pm on March 8, 2017: member

   Agree with jnewbery. Imo it is confusing for prioritisetransaction to return a bool. (One could think that false means the call failed, which is not true.)

   There are other methods to check if a transaction is in the mempool.

   On Wed, Mar 8, 2017 at 4:07 PM, John Newbery notifications@github.com wrote:

   I don’t understand why this is required. As mentioned above, prioritisetransaction can be used for transactions which are not yet in the mempool, and then the fee delta is:

   • used to determine whether the transaction should be allowed into the mempool (the ApplyDeltas() call in AcceptToMemoryPoolWorker())
   • applied to the transaction as it enters the mempool (mapTx.modify(newit, update_fee_delta(deltas.second)); in CTxMemPool::addUnchecked())

   If you really need to check whether the transaction is already in the mempool you can use getrawmempool.

   — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9947#issuecomment-285065832, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv9r7yqgfqZsLlkoin69fcVEkgfDWks5rjsQZgaJpZM4MWloU .

9. 
   ghost commented at 3:12 am on March 9, 2017: none

   You could return a struct that contains a bool, that way the said bool value is given a name, making the intention clear.
10. 
    jonasschnelli commented at 8:00 am on March 9, 2017: contributor

    I think @jnewbery and @MarcoFalke are right. I misunderstood it’s behaviour. It can prioritise future transaction and I wasn’t aware of that fact.

    An option would be to return true/false wether the transaction was in the mempool or not…

11. 
    jonasschnelli commented at 2:27 pm on March 29, 2017: contributor

    There are other methods to check if a transaction is in the mempool. @MarcoFalke is right here. Closing…

12. jonasschnelli closed this on Mar 29, 2017

    ---

13. MarcoFalke locked this on Sep 8, 2021

Contributors
jonasschnelli bitkevin gmaxwell dcousens laanwj jnewbery MarcoFalke ghost

Labels
Mining