mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0 #27501

glozow · 2023-04-20T15:01:04Z

Add an RPC to get prioritised transactions (also tells you whether the tx is in mempool or not), helping users clean up mapDeltas manually. When CTxMemPool::PrioritiseTransaction sets a delta to 0, remove the entry from mapDeltas.

Motivation / Background

mapDeltas entries are never removed from mapDeltas except when the tx is mined in a block or conflicted.
- Mostly it is a feature to allow prioritisetransaction for a tx that isn't in the mempool {yet, anymore}. A user can may resbumit a tx and it retains its priority, or mark a tx as "definitely accept" before it is seen.
- Since Store mempool and prioritization data to disk #8448, mapDeltas is persisted to mempool.dat and loaded on restart. This is also good, otherwise we lose prioritisation on restart.
- Note the removal due to block/conflict is only done when removeForBlock is called, i.e. when the block is received. If you load a mempool.dat containing mapDeltas with transactions that were mined already (e.g. the file was saved prior to the last few blocks), you don't delete them.
- Related: mapDeltas isn't always cleaned up. #4818 and Always clean up manual transaction prioritization #6464.
There is no way to query the node for not-in-mempool mapDeltas. If you add a priority and forget what the value was, the only way to get that information is to inspect mempool.dat.
Calling prioritisetransaction with an inverse value does not remove it from mapDeltas, it just sets the value to 0. It disappears on a restart (LoadMempool checks if delta is 0), but that might not happen for a while.

Added together, if a user calls prioritisetransaction very regularly and not all those transactions get mined/conflicted, mapDeltas might keep lots of entries of delta=0 around. A user should clean up the not-in-mempool prioritisations, but that's currently difficult without keeping track of what those txids/amounts are.

DrahtBot · 2023-04-20T15:01:08Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, instagibbs, ajtowns, achow101
Concept ACK	MarcoFalke, stickies-v, kevkevinpal, svanstaa
Approach ACK	ishaanam

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

maflcko

Concept ACK. Wanted to add a deletepriority RPC myself, but I guess it is easy to implement a for loop in python utilising the output of getprioritisationmap

src/rpc/mempool.cpp

dergoegge · 2023-04-20T16:11:27Z

Maybe add a short description for what mapDeltas is to the PR description under "motivation / background"? afaict mapDeltas also doesn't have any code documentation.

maflcko

lgtm ACK. Left two nits.

src/rpc/mempool.cpp

src/txmempool.cpp

stickies-v

Concept ACK

src/rpc/mempool.cpp

src/txmempool.h

ishaanam

Approach ACK

src/rpc/mempool.cpp

glozow

Addressed comments, rebased, added a release note

src/rpc/mempool.cpp

src/txmempool.cpp

src/txmempool.h

src/rpc/mempool.cpp

kevkevinpal · 2023-05-03T18:43:18Z

Concept ACK

theStack

Concept ACK

Was a bit surprised to see that the fee-delta is returned in BTC as unit rather than Satoshis, I assume our RPC guidelines (saying "Always use AmountFromValue and ValueFromAmount when inputting or outputting monetary values") have higher priority over the idea to simply use the same unit used for prioritisetransaction? Probably miners would be thankful to not need multiply the result by 100000000 first in order to delete mapDelta entries (and it would also make the tests simpler 😅), but happy to ACK either variant.

Left a few small nits below.

src/test/fuzz/rpc.cpp

src/rpc/mempool.cpp

test/functional/mempool_expiry.py

doc/release-notes-27501.md

src/txmempool.cpp

ajtowns · 2023-05-04T03:26:34Z

src/txmempool.cpp

Shouldn't these messages be returned as an RPC result?

Shouldn't the message here also show the resulting total delta, and perhaps whether the tx was already in the mempool (nTransactionsUpdated > 0)?

Would it be API-breaky to change the result of prioritisetransaction?

I've edited the logs though, to show whether the tx is in mempool and what the new delta is. Btw I don't think nTransactionsUpdated starts out at 0 when this is called.

Could add a -deprecatedrpc=prioritisetransactionresult flag to allow access to the current format of result.

src/rpc/mempool.cpp

src/txmempool.cpp

src/rpc/mempool.cpp

glozow

Addressed review comments and moved it from rpc/mempool.cpp to rpc/mining.cpp.

Probably miners would be thankful to not need multiply the result by 100000000 first in order to delete mapDelta entries (and it would also make the tests simpler 😅), but happy to ACK either variant.

I agree! I've changed it to satoshis to be the same as prioritisetransaction.

doc/release-notes-27501.md

src/rpc/mempool.cpp

src/test/fuzz/rpc.cpp

glozow · 2023-05-05T20:04:53Z

src/txmempool.cpp

Would it be API-breaky to change the result of prioritisetransaction?

I've edited the logs though, to show whether the tx is in mempool and what the new delta is. Btw I don't think nTransactionsUpdated starts out at 0 when this is called.

src/txmempool.cpp

test/functional/mempool_expiry.py

theStack

Code looks good to me, also agree with aj that getprioritisedtranactions is a better naming. Note that the first three commits still have the old names in their commit subjects and should be adapted accordingly.

src/txmempool.h

This allows the user to see prioritisation for not-in-mempool transactions.

…d expiry

It's unnecessary to keep the data around, as it doesn't do anything. If prioritisetransaction is called again, we'll make a new entry in mapDeltas. These entries are only deleted when the transaction is mined or conflicted from a block (i.e. not in replacement or eviction), are persisted in mempool.dat, and never expire. If node operators use the RPC to regularly prioritise/de-prioritise transactions, these (meaningless) map entries may hang around forever and take up valuable mempool memory.

theStack · 2023-05-11T11:59:19Z

src/txmempool.h

+        /** The modified fee (base fee + delta) of this entry. Only present if in_mempool=true. */
+        std::optional<CAmount> modified_fee;


This field is set in GetPrioritisedTransaction, but not read anywhere. Was there a plan to also return that in getprioritisedtransactions or is there any other future use-case? If not, I think it can be removed.

Correct, it is not used. I added it to address this comment: #27501 (comment).

I could add the modified fee to the RPC result? I don't feel strongly, happy to change however people want.

Have no strong opinion on that either, just wanted to make sure that there wasn't some code using modified_fee field around that was maybe forgotten to be pushed or sth alike.

Probably better to either expose it via the rpc, or not have the useless code at all?

svanstaa · 2023-05-17T13:03:01Z

Concept ACK

theStack

Code-review ACK 67b7fec

instagibbs

code review ACK 67b7fec

just test suggestions, everything looks good

instagibbs · 2023-06-06T13:32:54Z

src/rpc/mining.cpp

+            {
+                {RPCResult::Type::OBJ, "txid", "", {
+                    {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"},
+                    {RPCResult::Type::BOOL, "in_mempool", "whether this transaction is currently in mempool"},


is there a particular reason we're exposing in-mempool-ness here? No strong feelings, just curious.

Provides a good excuse to map to an object rather than just a the delta itself, which in turn might make it easier to add other info in future without having a breaking change?

100% for object for future-proofing, either way

instagibbs · 2023-06-06T13:34:42Z

test/functional/mining_prioritisetransaction.py

        assert_equal(raw_before[txid_a], raw_after[txid_a])
        assert_equal(raw_before, raw_after)
+        prioritisation_map_in_mempool = self.nodes[0].getprioritisedtransactions()
+        assert_equal(prioritisation_map_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True})


just directly construct the entire 2-entry map and assert here? means we wouldn't miss extraneous entries in future regression. Could also keep a running "tally" object that's modified along with the test cases

instagibbs · 2023-06-06T13:35:15Z

test/functional/mining_prioritisetransaction.py

        self.nodes[0].prioritisetransaction(txid=txid_c, fee_delta=int(fee_delta_c_1 * COIN))
        self.nodes[0].prioritisetransaction(txid=txid_c, fee_delta=int(fee_delta_c_2 * COIN))
+        prioritisation_map_not_in_mempool = self.nodes[0].getprioritisedtransactions()
+        assert_equal(prioritisation_map_not_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : False})


same as above?

instagibbs · 2023-06-06T13:35:27Z

test/functional/mining_prioritisetransaction.py

        assert_equal(raw_before[txid_a], raw_after[txid_a])
        assert_equal(raw_before, raw_after)
+        prioritisation_map_in_mempool = self.nodes[0].getprioritisedtransactions()
+        assert_equal(prioritisation_map_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True})


same as above?

instagibbs · 2023-06-06T13:42:18Z

src/txmempool.cpp

            }
            ++nTransactionsUpdated;
        }
+        if (delta == 0) {


maybe add a test case that shows prioritising by value of 0 doesn't add an entry, to cover all the bases

ajtowns

ACK 67b7fec code review only, some nits

ajtowns · 2023-06-06T20:37:19Z

src/txmempool.h


+    struct delta_info {
+        /** Whether this transaction is in the mempool. */
+        const bool in_mempool;


Seems strange to make these const. const auto deltas = mempool.GetPrioritisedTransactions() should be enough...

I think const in this context is mainly used to force the designated initializer to initialize the field, because no constructor exists and otherwise this may end up being uninitialized?

Seems like it would be better to explicitly specify defaults (false and 0?) in that case? If you want to delete the default constructor to force aggregate initialization, then delta_info() = delete; seems more direct?

ajtowns · 2023-06-06T20:40:49Z

src/rpc/mining.cpp

+        "Returns a map of all user-created (see prioritisetransaction) fee deltas by txid, and whether the tx is present in mempool.",
+        {},
+        RPCResult{
+            RPCResult::Type::OBJ_DYN, "prioritisation-map", "prioritisation keyed by txid",


Should just be "" rather than "prioritisation-map" ?

Yes, the string is eaten by the help generator either way

ajtowns · 2023-06-06T20:45:29Z

src/rpc/mining.cpp

+        RPCResult{
+            RPCResult::Type::OBJ_DYN, "prioritisation-map", "prioritisation keyed by txid",
+            {
+                {RPCResult::Type::OBJ, "txid", "", {


This is "transactionid" in getrawmempool and getmempoolancestors, probably better to be consistent?

ajtowns · 2023-06-06T20:48:32Z

src/rpc/mining.cpp

+            {
+                {RPCResult::Type::OBJ, "txid", "", {
+                    {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"},
+                    {RPCResult::Type::BOOL, "in_mempool", "whether this transaction is currently in mempool"},


Provides a good excuse to map to an object rather than just a the delta itself, which in turn might make it easier to add other info in future without having a breaking change?

ajtowns · 2023-06-06T20:55:46Z

src/txmempool.cpp

Could add a -deprecatedrpc=prioritisetransactionresult flag to allow access to the current format of result.

ajtowns · 2023-06-06T20:56:37Z

src/txmempool.h

+        /** The modified fee (base fee + delta) of this entry. Only present if in_mempool=true. */
+        std::optional<CAmount> modified_fee;


Probably better to either expose it via the rpc, or not have the useless code at all?

achow101 · 2023-06-07T07:22:29Z

ACK 67b7fec

…elete a…

Github-Pull: bitcoin#27501 Rebased-From: 9e9ca36

This allows the user to see prioritisation for not-in-mempool transactions. Github-Pull: bitcoin#27501 Rebased-From: 99f8046

Github-Pull: bitcoin#27501 Rebased-From: 0e5874f

…d expiry Github-Pull: bitcoin#27501 Rebased-From: c1061ac

It's unnecessary to keep the data around, as it doesn't do anything. If prioritisetransaction is called again, we'll make a new entry in mapDeltas. These entries are only deleted when the transaction is mined or conflicted from a block (i.e. not in replacement or eviction), are persisted in mempool.dat, and never expire. If node operators use the RPC to regularly prioritise/de-prioritise transactions, these (meaningless) map entries may hang around forever and take up valuable mempool memory. Github-Pull: bitcoin#27501 Rebased-From: 67b7fec

…nd delete a mapDeltas entry when delta==0 0eebd6f test: Assert that a new tx with a delta of 0 is never added (kevkevin) cfdbcd1 rpc: exposing modified_fee in getprioritisedtransactions (kevkevin) 252a867 rpc: renaming txid -> transactionid (kevkevin) 2fca6c2 rpc: changed prioritisation-map -> "" (kevkevin) 3a118e1 test: Directly constructing 2 entry map for getprioritisedtransactions (kevkevin) Pull request description: In this PR I am addressing some comments in #27501 as a followup. - changed `prioritisation-map` in the `RPCResult` to `""` - Directly constructing 2 entry map for getprioritisedtransactions in functional tests - renamed `txid` to `transactionid` in `RPCResult` to be more consistent with naming elsewhere - exposed the `modified_fee` field instead of having it be a useless arg - Created a new test that asserts when `prioritisedtransaction` is called with a fee_delta of 0 it is not added to mempool ACKs for top commit: glozow: reACK 0eebd6f, only change is the doc suggestion Tree-SHA512: e99056e37a8b1cfc511d87c83edba7c928b50d2cd6c2fd7c038976779850677ad37fddeb2b983e8bc007ca8567eb21ebb78d7eae9b773657c2b297299993ec05

glozow added RPC/REST/ZMQ Mempool labels Apr 20, 2023

maflcko approved these changes Apr 20, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

glozow force-pushed the 2023-04-clear-prioritisation branch from 2c0eee6 to d5069a4 Compare April 20, 2023 15:48

DrahtBot added the CI failed label Apr 20, 2023

glozow removed the CI failed label Apr 20, 2023

maflcko approved these changes Apr 20, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 21, 2023

fuzz: re-enable prioritisetransaction & analyzepsbt RPC #27464

Merged

stickies-v reviewed Apr 21, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/txmempool.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Apr 21, 2023

ishaanam reviewed Apr 22, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

glozow force-pushed the 2023-04-clear-prioritisation branch 2 times, most recently from 4f6ea69 to ab47d8b Compare May 3, 2023 15:54

glozow commented May 3, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

src/txmempool.h Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label May 3, 2023

theStack reviewed May 3, 2023

View reviewed changes

src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

test/functional/mempool_expiry.py Outdated Show resolved Hide resolved

ajtowns reviewed May 4, 2023

View reviewed changes

Aminkavoos mentioned this pull request May 4, 2023

lgtm ACK. Left two nits. Uniswap/default-token-list#1568

Closed

glozow force-pushed the 2023-04-clear-prioritisation branch from ab47d8b to 3e5c90e Compare May 5, 2023 20:06

glozow commented May 5, 2023

View reviewed changes

glozow changed the title ~~mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0~~ mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0 May 5, 2023

theStack reviewed May 7, 2023

View reviewed changes

src/txmempool.h Outdated Show resolved Hide resolved

glozow added 5 commits May 10, 2023 21:10

[mempool] add GetPrioritisedTransactions

9e9ca36

[rpc] add getprioritisedtransactions

99f8046

This allows the user to see prioritisation for not-in-mempool transactions.

[functional test] getprioritisedtransactions RPC

0e5874f

[functional test] prioritisation is not removed during replacement an…

c1061ac

…d expiry

glozow force-pushed the 2023-04-clear-prioritisation branch from 3e5c90e to 67b7fec Compare May 10, 2023 20:11

theStack reviewed May 11, 2023

View reviewed changes

kevkevinpal mentioned this pull request Jun 2, 2023

depriortisetransaction #27807

Closed

theStack approved these changes Jun 5, 2023

View reviewed changes

fanquake requested review from ajtowns and instagibbs June 6, 2023 09:41

instagibbs approved these changes Jun 6, 2023

View reviewed changes

ajtowns reviewed Jun 6, 2023

View reviewed changes

achow101 merged commit 1af72e7 into bitcoin:master Jun 7, 2023

glozow deleted the 2023-04-clear-prioritisation branch June 7, 2023 07:39

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 7, 2023

Merge bitcoin#27501: mempool / rpc: add getprioritisedtransactions, d…

a8559a8

…elete a…

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

[mempool] add GetPrioritisedTransactions

a0772a0

Github-Pull: bitcoin#27501 Rebased-From: 9e9ca36

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

[rpc] add getprioritisedtransactions

c045c80

This allows the user to see prioritisation for not-in-mempool transactions. Github-Pull: bitcoin#27501 Rebased-From: 99f8046

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

[functional test] getprioritisedtransactions RPC

5259331

Github-Pull: bitcoin#27501 Rebased-From: 0e5874f

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

[functional test] prioritisation is not removed during replacement an…

c3c5f53

…d expiry Github-Pull: bitcoin#27501 Rebased-From: c1061ac

glozow mentioned this pull request Nov 13, 2023

bugfix, Change up submitpackage results to return results for all transactions #28848

Merged

This was referenced Nov 13, 2023

refactor: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0 kevkevinpal/bitcoin#6

Merged

mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0 #28885

Merged

bitcoin locked and limited conversation to collaborators Nov 26, 2024

		/** The modified fee (base fee + delta) of this entry. Only present if in_mempool=true. */
		std::optional<CAmount> modified_fee;

mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0 #27501

mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0 #27501

Uh oh!

Conversation

glozow commented Apr 20, 2023

Uh oh!

DrahtBot commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dergoegge commented Apr 20, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ishaanam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevkevinpal commented May 3, 2023

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 20, 2023 •

edited

Loading