JSON-RPC method: prioritisetransaction <txid> <priority delta> #1583

luke-jr commented at 6:54 PM on July 11, 2012: member

Accepts the transaction into mined blocks at a higher (or lower) priority

jgarzik commented at 7:11 PM on July 11, 2012: contributor

Code seems ACK-worthy.

Use cases?

luke-jr commented at 7:17 PM on July 11, 2012: member

Various people (@gmaxwell and @gavinandresen included) expressed interest in this - one example was to allow captcha-solving as an alternative to fees.

in src/main.h:None in 428c7ca4dd outdated

 398 | @@ -396,6 +399,9 @@ class CTransaction
 399 |      mutable int nDoS;
 400 |      bool DoS(int nDoSIn, bool fIn) const { nDoS += nDoSIn; return fIn; }
 401 |  
 402 | +    double dPriorityDelta;
 403 | +

Diapolo commented at 7:33 PM on July 11, 2012:

Have 2 empty lines a special meaning in the code?

in src/main.h:None in 428c7ca4dd outdated

 546 | @@ -540,6 +547,9 @@ class CTransaction
 547 |  
 548 |      int64 GetMinFee(unsigned int nBlockSize=1, bool fAllowFree=true, enum GetMinFee_mode mode=GMF_BLOCK) const
 549 |      {
 550 | +        if (dPriorityDelta > 0)

gavinandresen commented at 3:04 PM on July 12, 2012:

What's the logic here? Why would setting a >0 delta automatically imply a minimum fee of zero?

luke-jr commented at 4:08 PM on July 12, 2012:

It seems to me, that if someone wants to improve the priority of a transaction, they do actually want to mine that transaction. What's the point of setting a priority of a transaction that you're not going to mine regardless?

gavinandresen commented at 12:32 PM on July 13, 2012:

The point is "increase priority" and "requires no fee" should, in my opinion, be independent notions. Somebody might want to bump up the priority of transactions from the Bitcoin Faucet a little because they like the idea of giving newbies a good bitcoin experience, but don't want to crowd out other high-priority/low-fee transactions.

luke-jr commented at 3:37 PM on July 13, 2012:

I can take this out if you want me to, but I don't see the point. A priority bump of just 1 has no practical effect on the overall priority comparison, but could be used to whitelist against the fee requirements, and the priority is entirely ignored if a transaction doesn't meet the fee requirements, so it makes no sense to increase the priority while not overriding the fee requirements, IMO.

in src/main.h:None in 428c7ca4dd outdated

  91 | @@ -92,6 +92,9 @@
  92 |  bool SendMessages(CNode* pto, bool fSendTrickle);
  93 |  bool LoadExternalBlockFile(FILE* fileIn);
  94 |  void GenerateBitcoins(bool fGenerate, CWallet* pwallet);
  95 | +bool PrioritiseTransaction(const uint256 hash, const std::string strHash, double dPriorityDelta);
  96 | +bool PrioritiseTransaction(const uint256 hash, double dPriorityDelta);
  97 | +bool PrioritiseTransaction(const std::string strHash, double dPriorityDelta);

gavinandresen commented at 3:05 PM on July 12, 2012:

Three versions of this is excessive. Less code, please.

gmaxwell commented at 7:14 PM on August 12, 2012: contributor

Re: Ignoring minfee: This call is by txid. If you don't like the fee, don't call it on the transaction. Ignoring minfee is the right thing to do here.

Though this should also have a fee delta, because we now prioritize above minfee txn by fee per KB. If someone is paying you behind the scenes to mine a transaction you should be able to add the amount you're being paid (or whatever) to the fee used in that calculation. e.g. prioritizetransaction <txid> <priodelta> <feedelta>.

luke-jr commented at 5:54 AM on August 13, 2012: member

Updated with suggestions. Also, is it intentional that GetMinFee with GMF_BLOCK is never used anymore?

BitcoinPullTester commented at 9:14 AM on August 13, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/2c63ef9156288b3cc72668fdb6ce44ec3a440076 for binaries and test log.

sipa commented at 3:54 PM on August 13, 2012: member

@luke-jr Which commit removed the call to GetMinFee with GMF_BLOCK?

luke-jr commented at 3:56 PM on August 13, 2012: member

@sipa c555400ca134991e39d5e3a565fcd2215abe56f6

in src/main.cpp:None in 2c63ef9156 outdated

3355 | +    {
3356 | +        printf("PrioritiseTransaction: cannot find %s\n", strHash.c_str());
3357 | +        return false;
3358 | +    }
3359 | +
3360 | +    CTransaction &txn = mempool.mapTx[hash];

sipa commented at 4:01 PM on August 13, 2012:

This needs a LOCK on mempool.cs, imho. Even if it's somehow already safe because of the lock on cs_main, I'd prefer having it there, for when RPC locks get pushed down.

luke-jr commented at 4:08 PM on August 13, 2012:

Fixed

gmaxwell commented at 12:30 PM on August 23, 2012: contributor

Needs a rebase

luke-jr commented at 7:03 PM on November 14, 2012: member

rebased

in src/main.h:None in 2fa776a092 outdated

 478 | @@ -477,6 +479,10 @@ class CTransaction
 479 |      std::vector<CTxOut> vout;
 480 |      unsigned int nLockTime;
 481 |  
 482 | +    double dPriorityDelta;
 483 | +    int64 nFeeDelta;

gavinandresen commented at 11:00 PM on February 13, 2013:

Adding 16 bytes to every in-memory transaction to support a feature that will not be used by 99.9% of users seems like the wrong thing to do.

How about instead keeping a map<transaction_id, pair<priorityDelta,feeDelta> > in the memory pool class, and have prioritisetransaction add to that map?

Finally: need to check to see if the free transaction rate limiter code needs to take this into account (I assume you should be able to prioritisetransaction <txid> and then if you receive the transaction over the network it should sail through the limiter and make it into the memory pool -- or is it assumed that the transaction will get into the memory pool some other way, like a sendrawtransaction call?).

luke-jr commented at 11:08 PM on February 13, 2013:

With the deltas stored on CTransaction, applying them to not-received ones was impractical. Obviously using a map as you suggest solves this problem too - will do that.

luke-jr commented at 4:43 AM on April 12, 2013: member

Ok, finally redid this using a map. @gavinandresen , look good?

jgarzik commented at 5:04 PM on May 30, 2013: contributor

No objection... though I still prioritize with the 'z' :) Who the heck uses an 's'? :)

in src/main.cpp:None in 91c22a6885 outdated

 573 | @@ -574,6 +574,16 @@ bool CTransaction::CheckTransaction(CValidationState &state) const
 574 |  int64 CTransaction::GetMinFee(unsigned int nBlockSize, bool fAllowFree,
 575 |                                enum GetMinFee_mode mode) const
 576 |  {
 577 | +    {
 578 | +        LOCK(mempool.cs);
 579 | +        uint256 hash = GetHash();
 580 | +        double dPriorityDelta = 0;
 581 | +        int64 nFeeDelta = 0;
 582 | +        mempool.ApplyDeltas(hash, dPriorityDelta, nFeeDelta);

laanwj commented at 5:33 PM on May 30, 2013:

I don't like all these side-effects in a const getter function

Edit: hmm, never mind, ApplyDeltas does not actually change anything in the mempool object.

luke-jr commented at 2:46 PM on August 21, 2013: member

It occurs to me that the map should be cleaned at some point. Any opinions on when to remove a txid from it?

gmaxwell commented at 3:56 PM on August 21, 2013: contributor

Check it when removing transactions from the mempool?

luke-jr commented at 5:30 PM on August 21, 2013: member

Probably don't want to lose priority adjustments if your block gets knocked off the main chain...

petertodd commented at 7:35 PM on August 22, 2013: contributor

@luke-jr Set a expiry height after they get knocked off the main chain and remove them from the map after n blocks? If n=100 is reached we have bigger problems...

gavinandresen commented at 1:02 AM on October 21, 2013: contributor

Rebase needed.

luke-jr commented at 1:02 AM on October 25, 2013: member

Rebased. For purging from the map.. how about when we see a block confirm a transaction using it as an input?

luke-jr commented at 4:40 PM on December 3, 2013: member

Rebased and added mapDeltas pruning when transactions are removed from the memory pool (ie, included in a block).

laanwj added the label RPC on May 9, 2014

laanwj added the label Improvement on May 9, 2014

laanwj commented at 12:59 PM on June 3, 2014: member

This pull has been open for almost two years now. What still has to be done for this to me merged? (apart from a rebase)

jgarzik commented at 4:31 PM on June 3, 2014: contributor

ACK -- appears to be done, to me, modulo a rebase.

This is IMO important to merge. We want to give miners controls like this, so that they may innovate and compete.

Related: Miners also need an "accept this TX even if it's non-standard" RPC or RPC flag.

BitcoinPullTester commented at 5:02 PM on June 23, 2014: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/p1583_4427dc5e6a6fd0f7349e6ea3e0d6a084491cf265/ for test log.

This pull does not merge cleanly onto current master This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

JSON-RPC method: prioritisetransaction <txid> <priority delta> <priority tx fee>

Accepts the transaction into mined blocks at a higher (or lower) priority

2a72d4591f

in src/miner.cpp:None in 4427dc5e6a outdated

  78 | @@ -77,6 +79,35 @@ class COrphan
  79 |  };
  80 |  
  81 |  
  82 | +void CTxMemPool::PrioritiseTransaction(const uint256 hash, const string strHash, double dPriorityDelta, int64_t nFeeDelta)

laanwj commented at 9:00 AM on June 25, 2014:

These implementations should be in txmempool.cpp.

luke-jr commented at 11:51 AM on June 26, 2014: member

Rebased with @laanwj 's change.

laanwj merged this on Jun 26, 2014

laanwj closed this on Jun 26, 2014

laanwj referenced this in commit ffb32acfab on Jun 26, 2014

suprnurd referenced this in commit ae909d0a09 on Dec 5, 2017

lateminer referenced this in commit 7fdf29f816 on May 6, 2020

DrahtBot locked this on Sep 8, 2021