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

glozow commented at 3:01 pm on April 20, 2023: member

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 #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: #4818 and #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.

glozow added the label RPC/REST/ZMQ on Apr 20, 2023

glozow added the label Mempool on Apr 20, 2023

DrahtBot commented at 3:01 pm on April 20, 2023: contributor

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.

in src/rpc/mempool.cpp:916 in 2c0eee60c5 outdated

909@@ -910,6 +910,41 @@ static RPCHelpMan submitpackage()
910     };
911 }
912 
913+static RPCHelpMan getprioritisationmap()
914+{
915+    return RPCHelpMan{"getprioritisationmap",
916+        "\nReturns a map of all fee deltas in memory pool by txid, and whether the tx is present in mempool.\n",

maflcko commented at 3:05 pm on April 20, 2023:

0        "Returns a map of all fee deltas in memory pool by txid, and whether the tx is present in mempool.",

nit

glozow commented at 3:52 pm on April 20, 2023:

Done

in src/rpc/mempool.cpp:940 in 2c0eee60c5 outdated

935+        UniValue rpc_result{UniValue::VOBJ};
936+        const auto prioritisation_map{mempool.GetPrioritisationMap()};
937+        for (const auto& [txid, delta] : prioritisation_map) {
938+            UniValue result_inner{UniValue::VOBJ};
939+            result_inner.pushKV("fee-delta", ValueFromAmount(delta));
940+            result_inner.pushKV("in-mempool", mempool.exists(GenTxid::Txid(txid)));

maflcko commented at 3:08 pm on April 20, 2023:

Won’t this need mempool.cs to ensure an atomic result? Otherwise you may get nonsense back on a data race, like a parent missing from the mempool, when the child is in the mempool, no?

glozow commented at 3:16 pm on April 20, 2023:

Yes good point. Will add a lock-already-held exists and put this all under LOCK(mempool.cs)

glozow commented at 3:17 pm on April 20, 2023:

Oh, or would it be better to have CTxMemPool::GetPrioritisationMap take the lock and create a map from txid to pair<fee, bool>? edit: doing this one

glozow commented at 3:57 pm on April 20, 2023:

Fixed

maflcko approved

maflcko commented at 3:09 pm on April 20, 2023: member

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

glozow force-pushed on Apr 20, 2023

DrahtBot added the label CI failed on Apr 20, 2023

glozow removed the label CI failed on Apr 20, 2023

in src/rpc/mempool.cpp:944 in d5069a4f0e outdated

939+            result_inner.pushKV("fee-delta", ValueFromAmount(delta_exists_pair.first));
940+            result_inner.pushKV("in-mempool", delta_exists_pair.second);
941+            rpc_result.pushKV(txid.GetHex(), result_inner);
942+        }
943+        return rpc_result;
944+},

maflcko commented at 4:03 pm on April 20, 2023:

clang-format new code (only if you retouch)

glozow commented at 3:54 pm on May 3, 2023:

Done

in src/txmempool.cpp:904 in d5069a4f0e outdated

899+    AssertLockNotHeld(cs);
900+    LOCK(cs);
901+    std::map<uint256, std::pair<CAmount, bool>> result;
902+    for (const auto& [txid, delta] : mapDeltas) {
903+        const bool in_mempool{mapTx.count(txid) != 0};
904+        result.emplace(std::make_pair(txid, std::make_pair(delta, in_mempool)));

maflcko commented at 4:11 pm on April 20, 2023:

0        result.try_emplace(txid, delta, in_mempool);

nit: try_emplace should avoid duplicate and unneeded (move) constructor calls? Also, it is shorter.

glozow commented at 3:48 pm on May 3, 2023:

Brilliant, done

dergoegge commented at 4:11 pm on April 20, 2023: member

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 approved

maflcko commented at 4:12 pm on April 20, 2023: member

lgtm ACK. Left two nits.

in src/rpc/mempool.cpp:919 in d5069a4f0e outdated

914+{
915+    return RPCHelpMan{"getprioritisationmap",
916+        "Returns a map of all fee deltas in memory pool by txid, and whether the tx is present in mempool.",
917+        {},
918+        RPCResult{
919+            RPCResult::Type::OBJ_DYN, "prioritisation-map", "prioritisation keyed by wtxid",

stickies-v commented at 9:48 am on April 21, 2023:

0            RPCResult::Type::OBJ_DYN, "prioritisation-map", "prioritisation keyed by txid",

glozow commented at 3:48 pm on May 3, 2023:

Fixed

in src/txmempool.h:518 in d5069a4f0e outdated

514@@ -515,6 +515,8 @@ class CTxMemPool
515     void PrioritiseTransaction(const uint256& hash, const CAmount& nFeeDelta);
516     void ApplyDelta(const uint256& hash, CAmount &nFeeDelta) const EXCLUSIVE_LOCKS_REQUIRED(cs);
517     void ClearPrioritisation(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs);
518+    /** Return a copy of mapDeltas in which values include fee delta and whether the transaction is present in the mempoool. */

stickies-v commented at 10:12 am on April 21, 2023:

Slight rephrasing suggestion:

0    /** Return a map of all entries in mapDeltas with as value their fee delta and whether the transaction is present in the mempoool. */

glozow commented at 3:48 pm on May 3, 2023:

Done

stickies-v commented at 10:15 am on April 21, 2023: contributor

Concept ACK

DrahtBot added the label Needs rebase on Apr 21, 2023

in src/rpc/mempool.cpp:916 in 2b04a58972 outdated

909@@ -910,6 +910,41 @@ static RPCHelpMan submitpackage()
910     };
911 }
912 
913+static RPCHelpMan getprioritisationmap()
914+{
915+    return RPCHelpMan{"getprioritisationmap",
916+        "Returns a map of all fee deltas in memory pool by txid, and whether the tx is present in mempool.",

ishaanam commented at 2:43 pm on April 22, 2023:

In 2b04a58972e18ffd06535ad2778547db7426b05d “[rpc] add getprioritisationmap”

nit: saying that this RPC “Returns a map of all fee deltas in memory pool” might be a bit confusing for users because you can run prioritisetransaction even if a tx is not yet in the mempool. Perhaps the following would be better:

0        "Returns a map of all user-created fee deltas by txid, and whether the tx is present in mempool.",

glozow commented at 3:47 pm on May 3, 2023:

Makes sense, taken

ishaanam commented at 3:03 pm on April 22, 2023: contributor

Approach ACK

glozow force-pushed on May 3, 2023

glozow commented at 3:54 pm on May 3, 2023: member

Addressed comments, rebased, added a release note

DrahtBot removed the label Needs rebase on May 3, 2023

kevkevinpal commented at 6:43 pm on May 3, 2023: contributor

Concept ACK

in src/test/fuzz/rpc.cpp:130 in ec179560a5 outdated

126@@ -127,6 +127,7 @@ const std::vector<std::string> RPC_COMMANDS_SAFE_FOR_FUZZING{
127     "getmempoolancestors",
128     "getmempooldescendants",
129     "getmempoolentry",
130+    "getprioritisationmap",

theStack commented at 10:37 pm on May 3, 2023:

tiny nit: should keep the list alphabetically sorted (that is, add the new RPC between “getpeerinfo” and “getrawmempool”)

glozow commented at 8:03 pm on May 5, 2023:

Done

in src/rpc/mempool.cpp:937 in ec179560a5 outdated

932+        {
933+            NodeContext& node = EnsureAnyNodeContext(request.context);
934+            CTxMemPool& mempool = EnsureMemPool(node);
935+            UniValue rpc_result{UniValue::VOBJ};
936+            const auto prioritisation_map{mempool.GetPrioritisationMap()};
937+            for (const auto& [txid, delta_exists_pair] : prioritisation_map) {

theStack commented at 10:40 pm on May 3, 2023:

nit, could get rid of extra variable:

0            for (const auto& [txid, delta_exists_pair] : mempool.GetPrioritisationMap()) {

glozow commented at 8:02 pm on May 5, 2023:

Taken

in test/functional/mempool_expiry.py:22 in 55c513b526 outdated

18 from test_framework.util import (
19     assert_equal,
20     assert_raises_rpc_error,
21 )
22-from test_framework.wallet import MiniWallet
23+from test_framework.wallet import COIN, MiniWallet

theStack commented at 10:54 pm on May 3, 2023:

nit: COIN is defined in messages.py and should be imported from there

glozow commented at 8:05 pm on May 5, 2023:

Fixed

theStack commented at 11:15 pm on May 3, 2023: contributor

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 :sweat_smile:), but happy to ACK either variant.

Left a few small nits below.

in doc/release-notes-27501.md:1 in ab47d8bc1d outdated

0@@ -0,0 +1,3 @@
1+- A new `getprioritisationmap` RPC has been added. It returns a map of all fee deltas created by the

ajtowns commented at 3:06 am on May 4, 2023:

map is a C++ concept, this is returning a json object. Wouldn’t avoiding the implementation baggage by calling it something like getprioritisetransactions be better?

glozow commented at 8:01 pm on May 5, 2023:

Good point, changed to getprioritisedtransactions

in src/txmempool.cpp:902 in ab47d8bc1d outdated

894@@ -890,6 +895,18 @@ void CTxMemPool::ClearPrioritisation(const uint256& hash)
895     mapDeltas.erase(hash);
896 }
897 
898+std::map<uint256, std::pair<CAmount, bool>> CTxMemPool::GetPrioritisationMap() const
899+{
900+    AssertLockNotHeld(cs);
901+    LOCK(cs);
902+    std::map<uint256, std::pair<CAmount, bool>> result;

ajtowns commented at 3:14 am on May 4, 2023:

Having struct delta_info { uint256 txid, CAmount delta, bool in_mempool } and returning a vector<delta_info> might make this a little quicker (using reserve() upfront would avoid memory allocations on each iteration).

glozow commented at 8:05 pm on May 5, 2023:

Done

in src/txmempool.cpp:877 in ab47d8bc1d outdated

869@@ -870,8 +870,13 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD
870             }
871             ++nTransactionsUpdated;
872         }
873+        if (delta == 0) {
874+            mapDeltas.erase(hash);
875+            LogPrintf("PrioritiseTransaction: %s delta cleared\n", hash.ToString());
876+        } else {
877+            LogPrintf("PrioritiseTransaction: %s fee += %s\n", hash.ToString(), FormatMoney(nFeeDelta));

ajtowns commented at 3:26 am on May 4, 2023:

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

glozow commented at 8:04 pm on May 5, 2023:

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.

ajtowns commented at 8:55 pm on June 6, 2023:

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

in src/rpc/mempool.cpp:923 in ab47d8bc1d outdated

918+        RPCResult{
919+            RPCResult::Type::OBJ_DYN, "prioritisation-map", "prioritisation keyed by txid",
920+            {
921+                {RPCResult::Type::OBJ, "txid", "transaction txid", {
922+                    {RPCResult::Type::STR_AMOUNT, "fee-delta", "transaction fee delta in " + CURRENCY_UNIT},
923+                    {RPCResult::Type::BOOL, "in-mempool", "whether this transaction is currently in mempool"},

ajtowns commented at 3:33 am on May 4, 2023:

These should be "fee_delta" and "in_mempool" – hyphens are annoying because they often get interpreted as a subtraction, so it’s easier to say jq .[].fee_delta than jq .[]."fee-delta". Also more consistent with other code (cf #24528).

glozow commented at 8:02 pm on May 5, 2023:

Fixed

in src/rpc/mempool.cpp:921 in ab47d8bc1d outdated

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

ajtowns commented at 3:43 am on May 4, 2023:

“transaction txid” is redundant here

glozow commented at 8:01 pm on May 5, 2023:

Removed

in src/txmempool.cpp:904 in ab47d8bc1d outdated

899+{
900+    AssertLockNotHeld(cs);
901+    LOCK(cs);
902+    std::map<uint256, std::pair<CAmount, bool>> result;
903+    for (const auto& [txid, delta] : mapDeltas) {
904+        const bool in_mempool{mapTx.count(txid) != 0};

ajtowns commented at 3:50 am on May 4, 2023:

You’re already looking up the tx here, so you could use find() instead and on success also return the total modified fee.

glozow commented at 8:05 pm on May 5, 2023:

Done, added a std::optional<CAmount> modified_fee to the delta_info struct

in src/rpc/mempool.cpp:940 in ab47d8bc1d outdated

935+            UniValue rpc_result{UniValue::VOBJ};
936+            const auto prioritisation_map{mempool.GetPrioritisationMap()};
937+            for (const auto& [txid, delta_exists_pair] : prioritisation_map) {
938+                UniValue result_inner{UniValue::VOBJ};
939+                result_inner.pushKV("fee-delta", ValueFromAmount(delta_exists_pair.first));
940+                result_inner.pushKV("in-mempool", delta_exists_pair.second);

ajtowns commented at 3:54 am on May 4, 2023:

If you have txindex enabled, you could lookup the txid of any in-mempool: false transactions in the txindex here, and, if found, calculate how many confirmations the tx has, and report that. That seems like it would be fairly useful, though not sure it’s actually worth the effort (compared to writing a script that just calls getrawtransaction $txid 1 | jq .confirmations).

glozow commented at 8:03 pm on May 5, 2023:

Don’t have strong feelings, will leave this for now and perhaps we can add it in a followup

glozow force-pushed on May 5, 2023

glozow commented at 8:07 pm on May 5, 2023: member

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.

glozow renamed this:
~~mempool / rpc: add getprioritisationmap, delete a mapDeltas entry when delta==0~~
mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0
on May 5, 2023

in src/txmempool.h:529 in a5bfd2b790 outdated

524+        /** The modified fee (base fee + delta) of this entry. Only present if in_mempool=true. */
525+        std::optional<CAmount> modified_fee;
526+        /** The prioritised transaction's txid. */
527+        const uint256 txid;
528+    };
529+    /** Return a map of all entries in mapDeltas with, as value, their fee delta and whether the

theStack commented at 2:44 pm on May 7, 2023:

0    /** Return a vector of all entries in mapDeltas with, as value, their fee delta and whether the

glozow commented at 8:11 pm on May 10, 2023:

Thanks, fixed

theStack commented at 2:56 pm on May 7, 2023: contributor

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.

[mempool] add GetPrioritisedTransactions 9e9ca36c80

[rpc] add getprioritisedtransactions

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

99f8046829

[functional test] getprioritisedtransactions RPC 0e5874f0b0

[functional test] prioritisation is not removed during replacement and expiry c1061acb9d

[mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0

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.

67b7fecacd

glozow force-pushed on May 10, 2023

in src/txmempool.h:525 in 9e9ca36c80 outdated

520+        /** Whether this transaction is in the mempool. */
521+        const bool in_mempool;
522+        /** The fee delta added using PrioritiseTransaction(). */
523+        const CAmount delta;
524+        /** The modified fee (base fee + delta) of this entry. Only present if in_mempool=true. */
525+        std::optional<CAmount> modified_fee;

theStack commented at 11:59 am on May 11, 2023:

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.

glozow commented at 2:17 pm on June 2, 2023:

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

glozow commented at 3:28 pm on June 5, 2023:

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

theStack commented at 7:22 pm on June 5, 2023:

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.

ajtowns commented at 8:56 pm on June 6, 2023:

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

svanstaa commented at 1:03 pm on May 17, 2023: none

Concept ACK

theStack approved

theStack commented at 7:17 pm on June 5, 2023: contributor

Code-review ACK 67b7fecacd0489809690982c89ba2d0acdca938c

fanquake requested review from instagibbs on Jun 6, 2023

fanquake requested review from ajtowns on Jun 6, 2023

in src/rpc/mining.cpp:493 in 99f8046829 outdated

488+        RPCResult{
489+            RPCResult::Type::OBJ_DYN, "prioritisation-map", "prioritisation keyed by txid",
490+            {
491+                {RPCResult::Type::OBJ, "txid", "", {
492+                    {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"},
493+                    {RPCResult::Type::BOOL, "in_mempool", "whether this transaction is currently in mempool"},

instagibbs commented at 1:32 pm on June 6, 2023:

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

ajtowns commented at 8:48 pm on June 6, 2023:

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?

instagibbs commented at 9:03 pm on June 6, 2023:

100% for object for future-proofing, either way

in test/functional/mining_prioritisetransaction.py:88 in 0e5874f0b0 outdated

83@@ -84,6 +84,9 @@ def test_diamond(self):
84         raw_after = self.nodes[0].getrawmempool(verbose=True)
85         assert_equal(raw_before[txid_a], raw_after[txid_a])
86         assert_equal(raw_before, raw_after)
87+        prioritisation_map_in_mempool = self.nodes[0].getprioritisedtransactions()
88+        assert_equal(prioritisation_map_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True})

instagibbs commented at 1:34 pm on June 6, 2023:

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

in test/functional/mining_prioritisetransaction.py:99 in 0e5874f0b0 outdated

94@@ -92,14 +95,22 @@ def test_diamond(self):
95         self.nodes[0].prioritisetransaction(txid=txid_b, fee_delta=int(fee_delta_b * COIN))
96         self.nodes[0].prioritisetransaction(txid=txid_c, fee_delta=int(fee_delta_c_1 * COIN))
97         self.nodes[0].prioritisetransaction(txid=txid_c, fee_delta=int(fee_delta_c_2 * COIN))
98+        prioritisation_map_not_in_mempool = self.nodes[0].getprioritisedtransactions()
99+        assert_equal(prioritisation_map_not_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : False})

instagibbs commented at 1:35 pm on June 6, 2023:

same as above?

in test/functional/mining_prioritisetransaction.py:107 in 0e5874f0b0 outdated

102             self.nodes[0].sendrawtransaction(t)
103         raw_after = self.nodes[0].getrawmempool(verbose=True)
104         assert_equal(raw_before[txid_a], raw_after[txid_a])
105         assert_equal(raw_before, raw_after)
106+        prioritisation_map_in_mempool = self.nodes[0].getprioritisedtransactions()
107+        assert_equal(prioritisation_map_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True})

instagibbs commented at 1:35 pm on June 6, 2023:

same as above?

in src/txmempool.cpp:873 in 67b7fecacd

869@@ -870,8 +870,17 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD
870             }
871             ++nTransactionsUpdated;
872         }
873+        if (delta == 0) {

instagibbs commented at 1:42 pm on June 6, 2023:

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

instagibbs approved

instagibbs commented at 1:43 pm on June 6, 2023: member

code review ACK 67b7fecacd0489809690982c89ba2d0acdca938c

just test suggestions, everything looks good

in src/txmempool.h:521 in 67b7fecacd

515@@ -516,6 +516,19 @@ class CTxMemPool
516     void ApplyDelta(const uint256& hash, CAmount &nFeeDelta) const EXCLUSIVE_LOCKS_REQUIRED(cs);
517     void ClearPrioritisation(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs);
518 
519+    struct delta_info {
520+        /** Whether this transaction is in the mempool. */
521+        const bool in_mempool;

ajtowns commented at 8:37 pm on June 6, 2023:

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

maflcko commented at 1:17 pm on June 7, 2023:

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?

ajtowns commented at 1:38 pm on June 7, 2023:

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?

in src/rpc/mining.cpp:489 in 67b7fecacd

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

ajtowns commented at 8:40 pm on June 6, 2023:

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

maflcko commented at 1:24 pm on June 7, 2023:

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

in src/rpc/mining.cpp:491 in 67b7fecacd

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

ajtowns commented at 8:45 pm on June 6, 2023:

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

ajtowns commented at 8:57 pm on June 6, 2023: contributor

ACK 67b7fecacd0489809690982c89ba2d0acdca938c code review only, some nits

achow101 commented at 7:22 am on June 7, 2023: member

ACK 67b7fecacd0489809690982c89ba2d0acdca938c

achow101 merged this on Jun 7, 2023

achow101 closed this on Jun 7, 2023

glozow deleted the branch on Jun 7, 2023

sidhujag referenced this in commit a8559a8435 on Jun 7, 2023

luke-jr referenced this in commit a0772a06dc on Aug 16, 2023

luke-jr referenced this in commit c045c80812 on Aug 16, 2023

luke-jr referenced this in commit 525933197b on Aug 16, 2023

luke-jr referenced this in commit c3c5f53c64 on Aug 16, 2023

luke-jr referenced this in commit 47559b7737 on Aug 16, 2023

glozow referenced this in commit cd603361a4 on Jan 12, 2024

bitcoin locked this on Nov 26, 2024

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

Reviews

Conflicts