Add true/false return value to prioritisetransaction #9947

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2017/03/ptx changing 4 files +13 −5

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.
Add true/false return value to prioritisetransaction 3e5d538b30
jonasschnelli added the label Mining on Mar 8, 2017
in src/txmempool.cpp:None 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).
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.

in src/rpc/mining.cpp:None 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

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.
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 .
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.
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...
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...
jonasschnelli closed this on Mar 29, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels