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

Merged
merged 1 commit into from Jun 26, 2014

Conversation

Projects
None yet
9 participants
@luke-jr
Member

luke-jr commented Jul 11, 2012

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

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 11, 2012

Contributor

Code seems ACK-worthy.

Use cases?

Contributor

jgarzik commented Jul 11, 2012

Code seems ACK-worthy.

Use cases?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 11, 2012

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

@Diapolo

View changes

src/main.h
@@ -396,6 +399,9 @@ class CTransaction
mutable int nDoS;
bool DoS(int nDoSIn, bool fIn) const { nDoS += nDoSIn; return fIn; }
+ double dPriorityDelta;
+

This comment has been minimized.

@Diapolo

Diapolo Jul 11, 2012

Have 2 empty lines a special meaning in the code?

@Diapolo

Diapolo Jul 11, 2012

Have 2 empty lines a special meaning in the code?

@gavinandresen

View changes

src/main.h
@@ -540,6 +547,9 @@ class CTransaction
int64 GetMinFee(unsigned int nBlockSize=1, bool fAllowFree=true, enum GetMinFee_mode mode=GMF_BLOCK) const
{
+ if (dPriorityDelta > 0)

This comment has been minimized.

@gavinandresen

gavinandresen Jul 12, 2012

Contributor

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

@gavinandresen

gavinandresen Jul 12, 2012

Contributor

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

This comment has been minimized.

@luke-jr

luke-jr Jul 12, 2012

Member

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?

@luke-jr

luke-jr Jul 12, 2012

Member

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?

This comment has been minimized.

@gavinandresen

gavinandresen Jul 13, 2012

Contributor

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.

@gavinandresen

gavinandresen Jul 13, 2012

Contributor

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.

This comment has been minimized.

@luke-jr

luke-jr Jul 13, 2012

Member

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.

@luke-jr

luke-jr Jul 13, 2012

Member

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.

@gavinandresen

View changes

src/main.h
@@ -92,6 +92,9 @@
bool SendMessages(CNode* pto, bool fSendTrickle);
bool LoadExternalBlockFile(FILE* fileIn);
void GenerateBitcoins(bool fGenerate, CWallet* pwallet);
+bool PrioritiseTransaction(const uint256 hash, const std::string strHash, double dPriorityDelta);
+bool PrioritiseTransaction(const uint256 hash, double dPriorityDelta);
+bool PrioritiseTransaction(const std::string strHash, double dPriorityDelta);

This comment has been minimized.

@gavinandresen

gavinandresen Jul 12, 2012

Contributor

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

@gavinandresen

gavinandresen Jul 12, 2012

Contributor

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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Aug 12, 2012

Member

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 .

Member

gmaxwell commented Aug 12, 2012

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 .

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 13, 2012

Member

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

Member

luke-jr commented Aug 13, 2012

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

@luke-jr luke-jr referenced this pull request Aug 13, 2012

Closed

next 2012-08-13 #1671

@BitcoinPullTester

This comment has been minimized.

Show comment
Hide comment

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 13, 2012

Member

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

Member

sipa commented Aug 13, 2012

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

@luke-jr

This comment has been minimized.

Show comment
Hide comment
Member

luke-jr commented Aug 13, 2012

src/main.cpp
+ return false;
+ }
+
+ CTransaction &txn = mempool.mapTx[hash];

This comment has been minimized.

@sipa

sipa Aug 13, 2012

Member

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.

@sipa

sipa Aug 13, 2012

Member

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.

This comment has been minimized.

@luke-jr

luke-jr Aug 13, 2012

Member

Fixed

@luke-jr

luke-jr Aug 13, 2012

Member

Fixed

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Aug 23, 2012

Member

Needs a rebase

Member

gmaxwell commented Aug 23, 2012

Needs a rebase

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 14, 2012

Member

rebased

Member

luke-jr commented Nov 14, 2012

rebased

src/main.h
@@ -477,6 +479,10 @@ class CTransaction
std::vector<CTxOut> vout;
unsigned int nLockTime;
+ double dPriorityDelta;
+ int64 nFeeDelta;

This comment has been minimized.

@gavinandresen

gavinandresen Feb 13, 2013

Contributor

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 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?).

@gavinandresen

gavinandresen Feb 13, 2013

Contributor

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 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?).

This comment has been minimized.

@luke-jr

luke-jr Feb 13, 2013

Member

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

luke-jr Feb 13, 2013

Member

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Apr 12, 2013

Member

Ok, finally redid this using a map.

@gavinandresen , look good?

Member

luke-jr commented Apr 12, 2013

Ok, finally redid this using a map.

@gavinandresen , look good?

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik May 30, 2013

Contributor

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

Contributor

jgarzik commented May 30, 2013

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

@laanwj

View changes

src/main.cpp
+ uint256 hash = GetHash();
+ double dPriorityDelta = 0;
+ int64 nFeeDelta = 0;
+ mempool.ApplyDeltas(hash, dPriorityDelta, nFeeDelta);

This comment has been minimized.

@laanwj

laanwj May 30, 2013

Member

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.

@laanwj

laanwj May 30, 2013

Member

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 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?

Member

luke-jr commented Aug 21, 2013

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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Aug 21, 2013

Member

Check it when removing transactions from the mempool?

Member

gmaxwell commented Aug 21, 2013

Check it when removing transactions from the mempool?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 21, 2013

Member

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

Member

luke-jr commented Aug 21, 2013

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

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Aug 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...

Contributor

petertodd commented Aug 22, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Oct 21, 2013

Contributor

Rebase needed.

Contributor

gavinandresen commented Oct 21, 2013

Rebase needed.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 25, 2013

Member

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

Member

luke-jr commented Oct 25, 2013

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

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 3, 2013

Member

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

Member

luke-jr commented Dec 3, 2013

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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)

Member

laanwj commented Jun 3, 2014

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

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 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.

Contributor

jgarzik commented Jun 3, 2014

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

This comment has been minimized.

Show comment
Hide comment
@BitcoinPullTester

BitcoinPullTester Jun 23, 2014

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.

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.

src/miner.cpp
@@ -77,6 +79,35 @@ class COrphan
};
+void CTxMemPool::PrioritiseTransaction(const uint256 hash, const string strHash, double dPriorityDelta, int64_t nFeeDelta)

This comment has been minimized.

@laanwj

laanwj Jun 25, 2014

Member

These implementations should be in txmempool.cpp.

@laanwj

laanwj Jun 25, 2014

Member

These implementations should be in txmempool.cpp.

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

Accepts the transaction into mined blocks at a higher (or lower) priority
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 26, 2014

Member

Rebased with @laanwj 's change.

Member

luke-jr commented Jun 26, 2014

Rebased with @laanwj 's change.

@laanwj laanwj merged commit 2a72d45 into bitcoin:master Jun 26, 2014

laanwj added a commit that referenced this pull request Jun 26, 2014

Merge pull request #1583
2a72d45 JSON-RPC method: prioritisetransaction <txid> <priority delta> <priority tx fee> (Luke Dashjr)

suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment