JSON-RPC method: prioritisetransaction <txid> <priority delta> #1583
Conversation
|
Code seems ACK-worthy. Use cases? |
|
Various people (@gmaxwell and @gavinandresen included) expressed interest in this - one example was to allow captcha-solving as an alternative to fees. |
src/main.h
Outdated
There was a problem hiding this comment.
Have 2 empty lines a special meaning in the code?
|
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 . |
|
Updated with suggestions. Also, is it intentional that GetMinFee with GMF_BLOCK is never used anymore? |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/2c63ef9156288b3cc72668fdb6ce44ec3a440076 for binaries and test log. |
|
@luke-jr Which commit removed the call to GetMinFee with GMF_BLOCK? |
src/main.cpp
Outdated
There was a problem hiding this comment.
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.
|
Needs a rebase |
|
rebased |
src/main.h
Outdated
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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.
|
Ok, finally redid this using a map. @gavinandresen , look good? |
|
No objection... though I still prioritize with the 'z' :) Who the heck uses an 's'? :) |
src/main.cpp
Outdated
There was a problem hiding this comment.
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.
|
It occurs to me that the map should be cleaned at some point. Any opinions on when to remove a txid from it? |
|
Check it when removing transactions from the mempool? |
|
Probably don't want to lose priority adjustments if your block gets knocked off the main chain... |
|
@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... |
|
Rebase needed. |
|
Rebased. For purging from the map.. how about when we see a block confirm a transaction using it as an input? |
|
Rebased and added mapDeltas pruning when transactions are removed from the memory pool (ie, included in a block). |
|
This pull has been open for almost two years now. |
|
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. |
|
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 |
src/miner.cpp
Outdated
There was a problem hiding this comment.
These implementations should be in txmempool.cpp.
…ity tx fee> Accepts the transaction into mined blocks at a higher (or lower) priority
|
Rebased with @laanwj 's change. |
2a72d45 JSON-RPC method: prioritisetransaction <txid> <priority delta> <priority tx fee> (Luke Dashjr)
8108470 Update copyright headers for files changed in 2020 (Fuzzbawls) Pull request description: pre-4.1 release update of the copyright headers ACKs for top commit: furszy: utACK 8108470 random-zebra: utACK 8108470 and merging... Tree-SHA512: cd9874856e49f83c454f042d6ac0133e036e56d92c8daf250700299b8661472a1caffcf3c5d9f335f4369d3e5db605febd67e5fa755532e25eb02f882c96af99
9 participants
Accepts the transaction into mined blocks at a higher (or lower) priority