This solves the [issue #19057. ](https://github.com/bitcoin/bitcoin/issues/19057)
A previous PR made the initial work but is removed due to commit history corruption from my end. Sadly the github discussion is also lost with the previous PR, though it was not extensive.
This PR makes the changes incorporating marcofalk’s last comment of implementing CAmount* inside ATMPArgs and is ready for review.
Note: testmempoolaccept only returns fee if transaction is accepted in mempool.
I am not sure how to do that. The branch for that pr in my repo is gone. That PR somehow got a ton of commit the last time i tried to push. I tried to fix it by rebasing but wasn’t working, and then it went much more messy and my local branch got deleted. So i opened a new one. It feels its better to close the previous one as i have no way to update it.
Sorry for the mess.
916@@ -916,10 +917,11 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
917
918 TxValidationState state;
919 bool test_accept_res;
920+ CAmount fee = 0;
0 CAmount fee_out;
No need to initialize. Also could clarify that this is an out parameter?
934@@ -933,6 +935,10 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
935 result_0.pushKV("reject-reason", state.GetRejectReason());
936 }
937 }
938+ // Push tx fee, only if test_accept_res == true
939+ else {
double negatives are hard to follow (probably explains why you had to add a dev comment). Easier would be to write
0if (test_accept_res) {
1 push fee
2} else {
3 puls reject reason
4}
for self-documenting code
471@@ -472,6 +472,7 @@ class MemPoolAccept
472 */
473 std::vector<COutPoint>& m_coins_to_uncache;
474 const bool m_test_accept;
475+ CAmount* fee;
0 CAmount* m_fee;
see dev notes
1062@@ -1056,10 +1063,10 @@ bool MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs
1063 /** (try to) add transaction to memory pool with a specified acceptance time **/
1064 static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
1065 int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
1066- bool bypass_limits, const CAmount nAbsurdFee, bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1067+ bool bypass_limits, const CAmount nAbsurdFee, bool test_accept, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
This is creating a problem where a non-testing AcceptToMemoryPoolWithTime doesn’t want the returning fee. One instance is in LoadMempool where the call is like below
0if (nTime + nExpiryTimeout > nNow) {
1 LOCK(cs_main);
2 AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, Time,
3 nullptr /* plTxnReplaced */, false /* bypass_limits */, 0 /* nAbsurdFee */, false /* test_accept */);
Putting CAmount* at the end worked, but now this is throwing error.
Also from initial google search it seems that c++ doesn’t allow to skip a default parameter in between the argument list.
Passing a nullptr works. Should I change this call like that, or there’s a better way?
(..., false, &fee) to use the fee while passing false in test_accept? As test_accept doesn’t have a default value, the caller always need to specify false in case of nontesting anyway. Thats what happening in the above call too.
103@@ -104,7 +104,7 @@ def run_test(self):
104 ))['hex']
105 tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_final)))
106 self.check_mempool_result(
107- result_expected=[{'txid': tx.rehash(), 'allowed': True}],
108+ result_expected=[{'txid': tx.rehash(), 'allowed': True, 'fee': (int(coin['amount']) - (0.025)) * COIN}],
int because Decimal wasn’t asserting with int type return value in fee. 0.025 is the output value in btc which is already used few lines above while constructing the tx and many other floating point amounts are used elsewhere. What exactly you are suggesting to change here?
186@@ -187,7 +187,7 @@ def run_test(self):
187 tx.deserialize(BytesIO(hex_str_to_bytes(raw_tx_reference)))
188 # Reference tx should be valid on itself
189 self.check_mempool_result(
190- result_expected=[{'txid': tx.rehash(), 'allowed': True}],
191+ result_expected=[{'txid': tx.rehash(), 'allowed': True, 'fee': (0.1 - 0.05) * COIN}],
Concept ACK, code change looks good.
Could add release note and also add/update a test without fee in the result.
928- if (!test_accept_res) {
929+ if (test_accept_res) {
930+ // Push tx fee, only if test_accept_res == true
931+ result_0.pushKV("fee", fee);
932+ }
933+ else {
683@@ -683,6 +684,12 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
684 return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), state.ToString());
685 }
686
687+ // If fee_out is passed, return the fee to the caller
688+ if (args.m_fee != nullptr){
0 if (args.m_fee) {
Thanks @luke-jr for the review. Updated and rebased. @promag I have added a release note. Let me know if any modification needed there.
Regarding the extra functional test, testmempoolaccept will always return the fee as long as the transaction is accepted in mempool. Fee will only be absent from the result if transaction acceptance check fails, which is already covered in the existing tests. So if I understand correctly, these two cases (with fee, without fee) are being checked already. Let me know if you think any more cases should need testing.
0@@ -0,0 +1,7 @@
1+RPC changes
2+------------
3+
4+### 'testmempoolaccept` imporvement
190@@ -191,10 +191,11 @@ void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune);
191 void PruneBlockFilesManual(int nManualPruneHeight);
192
193 /** (try to) add transaction to memory pool
194- * plTxnReplaced will be appended to with all transactions replaced from mempool **/
195+ * plTxnReplaced will be appended to with all transactions replaced from mempool
196+ * optionally takes an arguement to return tx fee to the caller **/
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
924- nullptr /* plTxnReplaced */, false /* bypass_limits */, max_raw_tx_fee, /* test_accept */ true);
925+ nullptr /* plTxnReplaced */, false /* bypass_limits */, max_raw_tx_fee, /* test_accept */ true, &fee);
926 }
927 result_0.pushKV("allowed", test_accept_res);
928- if (!test_accept_res) {
929+ if (test_accept_res) {
Could you elaborate on the consideration behind only passing back fee rate if the transaction is accepted? Late to the party, so I might have missed it if this was asked earlier by a different reviewer.
I think we should still pass back fee even if rejected, because In validation.h, we can see that one potential reason of rejection isTX_MEMPOOL_POLICY, which includes reasons such as mempool min fee not met, min relay fee not met, etc (in CheckFeeRate).
This RPC is also potentially improved to process more than one transaction in the future, which means that more than one trasactions could fail. At that point, it would be helpful for users to see directly the transactions that are not accepted & the exact fees they chose for the corresponding transactions.
The rationale behind not returning fees in case of rejection was mempool policy is not the only reason why a transaction might be rejected. In case of ill-formed transactions, returning a fee might cause a problem because it might try to return something that doesn’t make sense. If I am not mistaken then it will return 0 fee for ill-formed transactions. Seeing a fee=0 along with allowed=false can create false conclusion for the user that something is wrong with the fee, while the problem is somewhere else entirely. So this can create more confusion than help.
So such cases need to be separately handled.
On the other hand, CheckFeeRate already throws both the package fee and min_fee* as an error message in case of a mempool policy rejection, and other checks throws other relevant errors. So a user seeing allowed=false will need to investigate further anyway and all the relevant information are available in the log error messages, including the fee.
So it felt redundant to try to return fee in case of failure in the RPC call and handle all the edge cases.
If it needs to be done anyway for some other reasons, I think it’s better to do it in a separate PR. It will also need some added testings too to ensure what is returned actually makes sense.
If I am not mistaken then it will return 0 fee for ill-formed transactions. @rajarshimaitra would you mind pointing me to the area in source code to look more into it? I did some searching myself but wasn’t able to find where this happens.
Check here.
if CheckTxInputs fails, nFess will be 0. Look inside CheckTxInputs to see what makes it fail.
Concept ACK!
I’m wondering why you decided to return fee instead of feerate? I think we typically use feerate to decide whether a fee is reasonable, although a user might prefer to just see a fee amount. It’s a quick conversion between the two and I don’t have a strong opinion, but was wondering what your rationale was?
ACK https://github.com/bitcoin/bitcoin/pull/19093/commits/3218bba02ce6c0ae17111e340872f02a000f1dc3
I’m building on top of this in #19339, just fyi @rajarshimaitra
I’m wondering why you decided to return fee instead of feerate? I think we typically use feerate to decide whether a fee is reasonable, although a user might prefer to just see a fee amount. It’s a quick conversion between the two and I don’t have a strong opinion, but was wondering what your rationale was?
That’s a good proposal. I didn’t thought about it, but it makes sense to report the fee rate instead of the total fee.
I think its a better thing to do. Will change if others agree.
I’m building on top of this in #19339, just fyi @rajarshimaitra
It looks ready from my side. Maybe needs few more ACKs.
getmempoolentry returns (except the DEPRECATED fields and the mempool entry time). (Maybe hidden behind a verbosity switch, if someone only wants a boolean acceptance result)
690@@ -690,6 +691,12 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
691 return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), state.ToString());
692 }
693
694+ // If fee_out is passed, return the fee to the caller
695+ if (args.m_fee) {
696+ *args.m_fee = nFees;
697+ }
698+
916@@ -916,13 +917,17 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
917
918 TxValidationState state;
919 bool test_accept_res;
920+ CAmount fee; // To return transaction fee from AcceptToMemoryPool
925+ nullptr /* plTxnReplaced */, false /* bypass_limits */, max_raw_tx_fee, /* test_accept */ true, &fee);
926 }
927 result_0.pushKV("allowed", test_accept_res);
928- if (!test_accept_res) {
929+ if (test_accept_res) {
930+ // Push tx fee, only if test_accept_res == true
0@@ -0,0 +1,7 @@
1+RPC changes
2+------------
3+
4+### 'testmempoolaccept` improvement
5+
6+`testmempoolaccept` now additionally returns the transaction fee in the result.
7+Fee is only returned if transaction is accepted in the mempool.
Code looks good. A few style comments inline.
Additionally, a couple of comments on the commit log:
This commit returns 'fee' in the testmempoolaccept rpc results.
'fee' is only returned if the transaction would be accepted to the mempool.
Existing functional tests are modified to reflect changed behaviour.
Code review ACK c9781e92a211d2615a8889dede1b02913c18ecd1
s390 failure in travis is unrelated to this PR. I’ve restarted the job.
877@@ -878,6 +878,7 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
878 {
879 {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
880 {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"},
881+ {RPCResult::Type::NUM, "fee", "Fee provided in this transaction (only present when 'allowed' is true)"},
getmempoolentry returns vsize, weight, as well as a fees object.
Hmm. Now that I am thinking it makes sense to return feerate. Do you want the fees to be changed into feerate?
I am starting to think that it could even return the same info that
getmempoolinforeturns
Little confused on this one as getmepoolinfo returns the following
0Result:
1{
2 "loaded": true|false (boolean) True if the mempool is fully loaded
3 "size": xxxxx, (numeric) Current tx count
4 "bytes": xxxxx, (numeric) Sum of all virtual transaction sizes as defined in BIP 141. Differs from actual serialized size because witness data is discounted
5 "usage": xxxxx, (numeric) Total memory usage for the mempool
6 "maxmempool": xxxxx, (numeric) Maximum memory usage for the mempool
7 "mempoolminfee": xxxxx (numeric) Minimum fee rate in BTC/kB for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee
8 "minrelaytxfee": xxxxx (numeric) Current minimum relay fee for transactions
9}
Which of these do you think should be included here?
STR_AMOUNT like in all other places should be done for consistency and to protect against mistakes. I consider this a bug that should be fixed before merge.fee and replacing it with something else later is a breaking rpc change. For the sanity of our users we should keep breaking rpc changes at a minimum.
Good point about cherry picking the non-rpc changes.
Separately, I think fee is a useful value to return to the user, since it’s exact (unlike feerate which might be rounded).
getmempoolentry also returns the absolute fee (and the vsize)
474@@ -475,6 +475,7 @@ class MemPoolAccept
475 */
476 std::vector<COutPoint>& m_coins_to_uncache;
477 const bool m_test_accept;
478+ CAmount* m_fee;
m_fee_out
Code review ACK c9781e92a211d2615a8889dede1b02913c18ecd1
Ignore my nit unless you have to retouch.
vsize and fees.base, to be consistent with getmempoolentry. If we want to add other fields later (eg fees.modified), that’s not a breaking change. We shouldn’t add fee, since that’s deprecated in getmempoolentry.
I agree with @MarcoFalke.
I also had a few comments on my PR about these changes and it felt unfair to force @rajarshimaitra to follow them just because I wanted to use this code… so here’s a branch that applies the suggestions from my PR. I incorporated Marco’s comment as well, so it now returns a fees object with a STR_AMOUNT “base” field for what we get from PreChecks here.
Illustrated more clearly (just the relevant bits):
0{
1 RPCResult::Type::BOOL 'allowed'
2 RPCResult::Type::NUM 'vsize'
3 RPCResult::Type::OBJ 'fees'
4 {
5 RPCResult::Type::STR_AMOUNT 'base'
6 }
7}
Summary of the small edits if you’re interested: update release-notes.md instead of separate doc, doxygen comment for the optional fee_out, and Fabian’s naming comment above.
@rajarshimaitra please feel free to use this branch :)
It sounds like @gzhao408 has a bit more time than you to make the final changes here, so I’d suggest that she open a PR that takes your branch and makes those last minor changes to get this merged.
Yep I can do that! @rajarshimaitra you’ll be co-author of the commit (the branch adding the suggested changes is a cherry-pick from yours). Does this sound alright to you?