In this PR I am addressing some comments in #27501 as a followup.
prioritisation-map in the RPCResult to ""txid to transactionid in RPCResult to be more consistent with naming elsewheremodified_fee field instead of having it be a useless argprioritisedtransaction is called with a fee_delta of 0 it is not added to mempool
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | glozow |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
507@@ -507,7 +508,12 @@ static RPCHelpMan getprioritisedtransactions()
508 for (const auto& delta_info : mempool.GetPrioritisedTransactions()) {
509 UniValue result_inner{UniValue::VOBJ};
510 result_inner.pushKV("fee_delta", delta_info.delta);
511+ CAmount modified_fee;
512 result_inner.pushKV("in_mempool", delta_info.in_mempool);
513+ if(delta_info.in_mempool){
514+ modified_fee = *delta_info.modified_fee;
515+ result_inner.pushKV("modified_fee", (uint64_t)modified_fee);
516+ }
These might make more sense in their own pr? #27501 (review) #27501 (review) #27501 (review)
Unsure if this is wanted #27501 (review)
Directly constructing the map in the assertion instead of indexing by txid
so that we wouldn't miss new entries in future regressions.
110@@ -111,9 +111,8 @@ def test_diamond(self):
111 raw_after = self.nodes[0].getrawmempool(verbose=True)
112 assert_equal(raw_before[txid_a], raw_after[txid_a])
113 assert_equal(raw_before, raw_after)
114- prioritisation_map_in_mempool = self.nodes[0].getprioritisedtransactions()
115- assert_equal(prioritisation_map_in_mempool[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True})
116- assert_equal(prioritisation_map_in_mempool[txid_c], {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True})
117+ assert_equal(self.nodes[0].getprioritisedtransactions()[txid_b], {"fee_delta" : fee_delta_b*COIN, "in_mempool" : True})
118+ assert_equal(self.nodes[0].getprioritisedtransactions()[txid_c], {"fee_delta" : (fee_delta_c_1 + fee_delta_c_2)*COIN, "in_mempool" : True})
I don’t understand the first commit message:
0 test: Directly constructing 2 entry map for getprioritisedtransactions
1
2Directly constructing the map so that we wouldn't miss extraneous
3entries in future regressions.
What does it mean and why is the code changed? Can you give an example or explanation?
I think I may have miss read the initial comment but I have updated the message and the tests.
I am now testing the whole object we get back from self.nodes[0].getprioritisedtransactions() so in the future if there are new transactions added this assertion will fail
looks like the “test each commit” git action failed but doesn’t seems related to the changes proposed
I see the following in the logs
0E: Failed to fetch mirror+file:/etc/apt/apt-mirrors.txt/pool/main/a/avahi/libavahi-core7_0.8-5ubuntu5.1_amd64.deb 404 Not Found [IP: 52.252.75.106 80]
1E: Failed to fetch mirror+file:/etc/apt/apt-mirrors.txt/pool/main/a/avahi/avahi-daemon_0.8-5ubuntu5.1_amd64.deb 404 Not Found [IP: 52.252.75.106 80]
488@@ -489,7 +489,7 @@ static RPCHelpMan getprioritisedtransactions()
489 RPCResult{
490 RPCResult::Type::OBJ_DYN, "", "prioritisation keyed by txid",
491 {
492- {RPCResult::Type::OBJ, "txid", "", {
493+ {RPCResult::Type::OBJ, "transactionid", "", {
updated the commit message in 9a918a4
I also fixed the other ones that had refactor in the commit message since they weren’t really refactors either
Oh, ok (it would be clearer to me if it was presented a little differently, along with the other instances).
0- {RPCResult::Type::OBJ, "transactionid", "", {
1+ {RPCResult::Type::OBJ, "<transactionid>", "", {
507@@ -507,7 +508,12 @@ static RPCHelpMan getprioritisedtransactions()
508 for (const auto& delta_info : mempool.GetPrioritisedTransactions()) {
509 UniValue result_inner{UniValue::VOBJ};
510 result_inner.pushKV("fee_delta", delta_info.delta);
511+ CAmount modified_fee;
512 result_inner.pushKV("in_mempool", delta_info.in_mempool);
513+ if(delta_info.in_mempool){
514+ modified_fee = *delta_info.modified_fee;
515+ result_inner.pushKV("modified_fee", (uint64_t)modified_fee);
262@@ -263,6 +263,12 @@ def run_test(self):
263 if (x != high_fee_tx):
264 assert x not in mempool
265
266+
267+ self.log.info("Assert that prioritised free transaction with 0 delta is never added to mapDeltas")
268+ tx_id_zero_del = self.wallet.create_self_transfer(fee_rate=0)['txid']
in a1fb3eddc91db708727fc48309d3bb6182858bbd
I don’t think it matters what the feerate of this tx is?
0 self.log.info("Assert that 0 delta is never added to mapDeltas")
1 tx_id_zero_del = self.wallet.create_self_transfer()['txid']
508@@ -508,6 +509,9 @@ static RPCHelpMan getprioritisedtransactions()
509 UniValue result_inner{UniValue::VOBJ};
510 result_inner.pushKV("fee_delta", delta_info.delta);
511 result_inner.pushKV("in_mempool", delta_info.in_mempool);
512+ if(delta_info.in_mempool){
git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v, with HEAD~<number of commits in your change>
491+ RPCResult::Type::OBJ_DYN, "", "prioritisation keyed by txid",
492 {
493- {RPCResult::Type::OBJ, "txid", "", {
494+ {RPCResult::Type::OBJ, "transactionid", "", {
495 {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"},
496+ {RPCResult::Type::NUM, "modified_fee", /*optional=*/true, "new fee for this transactionid if in mempool"},
0 {RPCResult::Type::NUM, "modified_fee", /*optional=*/true, "new fee for this transaction. Only returned if transaction is in mempool"},
As modified_fee is the only optional field in the result and depends on the value of the in_mempool field, would it be better to put it after the always-present fields, i.e. after in_mempool?
494- {RPCResult::Type::OBJ, "txid", "", {
495- {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"},
496- {RPCResult::Type::BOOL, "in_mempool", "whether this transaction is currently in mempool"},
497- }}
498- },
499+ RPCResult::Type::OBJ_DYN,
44@@ -45,15 +45,15 @@ def test_replacement(self):
45 self.nodes[0].prioritisetransaction(tx_replacee["txid"], 0, 100)
46 assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : 100, "in_mempool" : False}})
47 self.nodes[0].sendrawtransaction(tx_replacee["hex"])
48- assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : 100, "in_mempool" : True}})
49+ assert_equal(self.nodes[0].getprioritisedtransactions(), { tx_replacee["txid"] : { "fee_delta" : 100, "in_mempool" : True, "modified_fee": 1140}})
262@@ -263,6 +263,12 @@ def run_test(self):
263 if (x != high_fee_tx):
264 assert x not in mempool
265
266+
267+ self.log.info("Assert that 0 delta is never added to mapDeltas")
268+ tx_id_zero_del = self.wallet.create_self_transfer()['txid']
269+ self.nodes[0].prioritisetransaction(txid=tx_id_zero_del, fee_delta=0)
270+ assert tx_id_zero_del not in self.nodes[0].getrawmempool()
in 0d3a4abd805e3d4deec25f9902e762a7e693ce6b
This is just testing that you didn’t submit the transaction. Did you mean to use the getprioritisedtransactions RPC instead?
Still working on this? I’ll be available to re-review quickly after comments are addressed.
yes sorry I can try and make some followup changes to the comments soon
prioritisation-map gets eaten by the help generator to be "" so we are
setting to "" to begin with
renamed to transactionid because it is named this way in getrawmempool
and getmempoolancestors
41@@ -42,7 +42,8 @@ def test_transaction_expiry(self, timeout):
42
43 # Add prioritisation to this transaction to check that it persists after the expiry
44 node.prioritisetransaction(parent_txid, 0, COIN)
45- assert_equal(node.getprioritisedtransactions()[parent_txid], { "fee_delta" : COIN, "in_mempool" : True})
46+ # Fee rate found in test/functional/test_framework/wallet.py create_self_transfer
47+ assert_equal(node.getprioritisedtransactions()[parent_txid], { "fee_delta" : COIN, "in_mempool" : True, "modified_fee": COIN + COIN * (0.003 * 104 / 1000) })
These are still magic numbers, even if they are accurate. The comment is helpful, but doesn’t change the fact that this test relies on the implementation of create_self_transfer, making it brittle. If we were to change the implementation so that the virtual size could be something other than 104, or if we adjusted the default feerate to something other than 0.003, this test would break, even though the mempool/RPC logic hasn’t changed.
I was suggesting that you assign variables and reuse them in this test:
fee_delta to use in prioritisetransactionparent_fee meaning the absolute fees of the tx. If you’re having trouble getting the fee, it’ll be one of the results returned from send_self_transfer.modified_fee is equal to parent_fee + fee_delta
create_self_transfer fixed in dc189d3
491@@ -492,6 +492,7 @@ static RPCHelpMan getprioritisedtransactions()
492 {RPCResult::Type::OBJ, "<transactionid>", "", {
493 {RPCResult::Type::NUM, "fee_delta", "transaction fee delta in satoshis"},
494 {RPCResult::Type::BOOL, "in_mempool", "whether this transaction is currently in mempool"},
495+ {RPCResult::Type::NUM, "modified_fee", /*optional=*/true, "new fee for this transaction. Only returned if transaction is in mempool"},
dc189d3ec5c4af9bca02928dfeb4770570060982: I’m not sure that “new fee” would be understood by people. It would also be helpful to add the units here, i.e.
0 {RPCResult::Type::NUM, "modified_fee", /*optional=*/true, "modified fee in satoshis. Only returned if in_mempool=true"},
Instead of having modified_fee be hidden we are now exposing it to avoid
having useless code