RPC: Return transaction fee from testmempoolaccept #19093

rajarshimaitra · 2020-05-28T17:30:05Z

This solves the issue #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.

jonatack · 2020-05-28T18:33:41Z

ISTM there is no need to close #19063 and open another PR. Better to have rebased on the original PR.

rajarshimaitra · 2020-05-29T07:23:44Z

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.

maflcko

Concept ACK

src/rpc/rawtransaction.cpp

src/validation.cpp

test/functional/mempool_accept.py

rajarshimaitra · 2020-05-29T16:03:15Z

Thanks for the review. Updated and rebased.

promag

Concept ACK, code change looks good.

Could add release note and also add/update a test without fee in the result.

rajarshimaitra · 2020-06-01T13:35:50Z

Thanks @promag for the review and comment.
I am not very familiar with release notes update protocols, is this something that I should change as part of this PR?
Also, can you elaborate a little on the testing part that you mentioned?

luke-jr · 2020-06-08T17:23:37Z

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#release-notes

https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md

luke-jr

utACK, with some nits

src/rpc/rawtransaction.cpp

src/validation.cpp

rajarshimaitra · 2020-06-09T10:39:54Z

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.

doc/release-notes-19093.md

This commit returns 'fee' in the testmempoolaccept rpc results. 'fee' is only returned if the transaction is accepted in mempool. Existing functional tests are modified to reflect changed behaviour. Github-Pull: bitcoin#19093 Rebased-From: 520ae88

src/validation.h

This commit returns 'fee' in the testmempoolaccept rpc results. 'fee' is only returned if the transaction is accepted in mempool. Existing functional tests are modified to reflect changed behaviour. Github-Pull: bitcoin#19093 Rebased-From: c7c3f45

DrahtBot · 2020-06-18T02:14:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

validation: re-delegate absurd fee checking from mempool to clients #19339 (validation: re-delegate absurd fee checking from mempool to clients by gzhao408)

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.

src/rpc/rawtransaction.cpp

glozow

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?

glozow

ACK 3218bba

I'm building on top of this in #19339, just fyi @rajarshimaitra

rajarshimaitra · 2020-07-26T06:10:50Z

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.

maflcko · 2020-07-26T09:17:20Z

I am starting to think that it could even return the same info that 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)

jnewbery

Code looks good. A few style comments inline.

Additionally, a couple of comments on the commit log:

the summary line should be in the imperative mood (i.e. "Return transaction fee from testmempoolaccept" instead of "testmempoolaccept returns transaction fee")
change "if the transaction is accepted in mempool." to "if the transaction would be accepted to the mempool."

src/validation.cpp

src/rpc/rawtransaction.cpp

doc/release-notes-19093.md

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.

rajarshimaitra · 2020-08-08T06:21:30Z

Thanks @jnewbery for the review. Updated as per your suggestions and rebased.

glozow

reACK c9781e9

jnewbery · 2020-08-11T12:26:15Z

Code review ACK c9781e9

s390 failure in travis is unrelated to this PR. I've restarted the job.

maflcko

Concept ACK

maflcko · 2020-08-14T05:35:15Z

src/rpc/rawtransaction.cpp

@@ -878,6 +878,7 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
                        {
                            {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
                            {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"},
+                            {RPCResult::Type::NUM, "fee", "Fee provided in this transaction (only present when 'allowed' is true)"},


This will put the fee in raw satoshis? I don't think we do this anywhere else.

Have you seen RPC: Return transaction fee from testmempoolaccept #19093 (comment)? Specifically, it would be better for the caller to have the feerate instead of the absolute fee. Otherwise, the caller is forced to implement a witness-aware tx-deserializer to determine the virtual size. 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 getmempoolinfo returns

Little confused on this one as getmepoolinfo returns the following

Result: { "loaded": true|false (boolean) True if the mempool is fully loaded "size": xxxxx, (numeric) Current tx count "bytes": xxxxx, (numeric) Sum of all virtual transaction sizes as defined in BIP 141. Differs from actual serialized size because witness data is discounted "usage": xxxxx, (numeric) Total memory usage for the mempool "maxmempool": xxxxx, (numeric) Maximum memory usage for the mempool "mempoolminfee": xxxxx (numeric) Minimum fee rate in BTC/kB for tx to be accepted. Is the maximum of minrelaytxfee and minimum mempool fee "minrelaytxfee": xxxxx (numeric) Current minimum relay fee for transactions }

Which of these do you think should be included here?

Sorry, typo. Should have been getmempoolentry in the other comment as well

This seems like scope creep. I think returning the fee is fine to start with, and unblocks #19339. Could we defer adding more details to a later PR?

Changing the type to 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.

Removing the field 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.

Does #19339 need the rpc changes? If not, then this can trivially be unblocked by cherry-picking this pull and undoing the RPC changes. I think an internal (non-rpc) change can go in earlier, as it doesn't have the breaking rpc change concerns.

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

Agree, since getmempoolentry also returns the absolute fee (and the vsize)

Yeah good point, I only need the fee from ATMP for #19339 so I'll just use that. @rajarshimaitra is listed as coauthor on there, and can continue iterating on the RPC stuff here.

fjahr

Code review ACK c9781e9

Ignore my nit unless you have to retouch.

fjahr · 2020-08-24T22:16:23Z

src/validation.cpp

@@ -475,6 +475,7 @@ class MemPoolAccept
         */
        std::vector<COutPoint>& m_coins_to_uncache;
        const bool m_test_accept;
+        CAmount* m_fee;


nit: to match your naming elsewhere you could have called this m_fee_out

jarrad · 2020-08-25T04:42:00Z

Concept ACK

maflcko · 2020-09-07T13:23:07Z

I still consider my comment a bug that should be fixed before merge. Also my proposed change would be a breaking RPC change, and I wouldn't want to expose users to that. So better get it right the first time and avoid another "lets do it later" like e.g. #19543.

maflcko · 2020-09-07T13:27:43Z

(Ok, just adding new fields to an object is not a breaking change. Only if the new fields were wrapped into another object, but that probably doesn't make sense here)

jnewbery · 2020-09-07T13:31:53Z

I've chatted to Marco offline and agree that this PR should be changed to add 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.

glozow · 2020-09-07T17:06:59Z

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

{
    RPCResult::Type::BOOL 'allowed'
    RPCResult::Type::NUM 'vsize'
    RPCResult::Type::OBJ 'fees' 
    {
        RPCResult::Type::STR_AMOUNT 'base'
    }
}

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

rajarshimaitra · 2020-09-10T05:56:57Z

Hi, Sorry I got busy with other works and couldn't check the updates here lately. Honestly, I am a little overwhelmed with all the discussions regarding this, couldn't really follow up on what's happening where.
So if someone can explain in simple terms what I need to do, if anything, to make this PR mergeable would be happy to make the changes and close this.

jnewbery · 2020-09-10T13:16:14Z

Hi @rajarshimaitra . Thanks for your work on this. 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. What do you think?

glozow · 2020-09-10T13:22:29Z

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?

rajarshimaitra · 2020-09-10T15:10:16Z

@jnewbery and @gzhao408 yes that sounds good to me. As I have lost touch over this it would be better if she takes over and do the last changes. ACK from me.

jnewbery · 2020-09-15T16:59:50Z

New version is at #19940 . Thanks for your hard work on this @rajarshimaitra !

@jonatack @MarcoFalke @fjahr @promag @luke-jr - mind re-reviewing the new version at #19940? It's this branch with some minor updates.

23c35bf [test] add get_vsize util for more programmatic testing (gzhao408) 2233a93 [rpc] Return fee and vsize from testmempoolaccept (codeShark149) Pull request description: From #19093 and resolves #19057. Difference from #19093: return `vsize` and `fees` object (similar to `getmempoolentry`) when the test accept is successful. Updates release-notes.md. ACKs for top commit: jnewbery: utACK 23c35bf fjahr: utACK 23c35bf instagibbs: reACK 23c35bf Tree-SHA512: dcb81b7b817a4684e9076bc5d427a6f2d549d2edc66544e718260c4b5f8f1d5ae1d47b754175e9f0c8a3bd8371ce116c2dca0583588d513a7d733d5d614f2b04

23c35bf [test] add get_vsize util for more programmatic testing (gzhao408) 2233a93 [rpc] Return fee and vsize from testmempoolaccept (codeShark149) Pull request description: From bitcoin#19093 and resolves bitcoin#19057. Difference from bitcoin#19093: return `vsize` and `fees` object (similar to `getmempoolentry`) when the test accept is successful. Updates release-notes.md. ACKs for top commit: jnewbery: utACK 23c35bf fjahr: utACK 23c35bf instagibbs: reACK bitcoin@23c35bf Tree-SHA512: dcb81b7b817a4684e9076bc5d427a6f2d549d2edc66544e718260c4b5f8f1d5ae1d47b754175e9f0c8a3bd8371ce116c2dca0583588d513a7d733d5d614f2b04

DrahtBot added RPC/REST/ZMQ Tests Validation labels May 28, 2020

rajarshimaitra closed this May 29, 2020

rajarshimaitra reopened this May 29, 2020

maflcko removed Tests Validation labels May 29, 2020

maflcko reviewed May 29, 2020

View reviewed changes

rajarshimaitra force-pushed the fee-trial3 branch from f70abce to 520ae88 Compare May 29, 2020 16:01

promag reviewed May 31, 2020

View reviewed changes

luke-jr approved these changes Jun 8, 2020

View reviewed changes

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

src/validation.cpp Outdated Show resolved Hide resolved

rajarshimaitra force-pushed the fee-trial3 branch from 520ae88 to c7c3f45 Compare June 9, 2020 10:35

luke-jr reviewed Jun 9, 2020

View reviewed changes

doc/release-notes-19093.md Outdated Show resolved Hide resolved

luke-jr reviewed Jun 10, 2020

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

rajarshimaitra force-pushed the fee-trial3 branch from c7c3f45 to 3218bba Compare June 10, 2020 11:22

DrahtBot mentioned this pull request Jun 17, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

This was referenced Jun 19, 2020

[WIP] mempool: Remove nAbsurdFee fee from AcceptToMemoryPool #15810

Closed

validation: re-delegate absurd fee checking from mempool to clients #19339

Merged

glowang reviewed Jun 20, 2020

View reviewed changes

src/rpc/rawtransaction.cpp Show resolved Hide resolved

glozow reviewed Jul 22, 2020

View reviewed changes

glozow reviewed Jul 25, 2020

View reviewed changes

jnewbery reviewed Aug 4, 2020

View reviewed changes

src/validation.cpp Show resolved Hide resolved

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

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

doc/release-notes-19093.md Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 7, 2020

Run clang-tidy -*,performance-* #19675

Closed

RPC: Return transaction fee from testmempoolaccept

c9781e9

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.

rajarshimaitra force-pushed the fee-trial3 branch from 3218bba to c9781e9 Compare August 8, 2020 06:19

rajarshimaitra changed the title ~~RPC: testmempoolaccept returns transaction fee~~ RPC: Return transaction fee from testmempoolaccept Aug 8, 2020

glozow reviewed Aug 10, 2020

View reviewed changes

fanquake requested a review from luke-jr August 12, 2020 02:27

maflcko reviewed Aug 14, 2020

View reviewed changes

fjahr reviewed Aug 24, 2020

View reviewed changes

maflcko added the Waiting for author label Sep 7, 2020

glozow mentioned this pull request Sep 11, 2020

rpc: Return fee and vsize from testmempoolaccept #19940

Merged

jnewbery closed this Sep 15, 2020

fanquake removed the Waiting for author label Sep 19, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

RPC: Return transaction fee from testmempoolaccept #19093

RPC: Return transaction fee from testmempoolaccept #19093

Conversation

rajarshimaitra commented May 28, 2020 • edited Loading

jonatack commented May 28, 2020

rajarshimaitra commented May 29, 2020 • edited Loading

maflcko left a comment

Choose a reason for hiding this comment

rajarshimaitra commented May 29, 2020

promag left a comment

Choose a reason for hiding this comment

rajarshimaitra commented Jun 1, 2020

luke-jr commented Jun 8, 2020

luke-jr left a comment

Choose a reason for hiding this comment

rajarshimaitra commented Jun 9, 2020

DrahtBot commented Jun 18, 2020 • edited Loading

Conflicts

glozow left a comment • edited Loading

Choose a reason for hiding this comment

glozow left a comment • edited Loading

Choose a reason for hiding this comment

rajarshimaitra commented Jul 26, 2020

maflcko commented Jul 26, 2020 • edited Loading

jnewbery left a comment

Choose a reason for hiding this comment

rajarshimaitra commented Aug 8, 2020

glozow left a comment

Choose a reason for hiding this comment

jnewbery commented Aug 11, 2020

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjahr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarrad commented Aug 25, 2020

maflcko commented Sep 7, 2020

maflcko commented Sep 7, 2020

jnewbery commented Sep 7, 2020

glozow commented Sep 7, 2020 • edited Loading

rajarshimaitra commented Sep 10, 2020

jnewbery commented Sep 10, 2020

glozow commented Sep 10, 2020

rajarshimaitra commented Sep 10, 2020

jnewbery commented Sep 15, 2020

rajarshimaitra commented May 28, 2020 •

edited

Loading

rajarshimaitra commented May 29, 2020 •

edited

Loading

DrahtBot commented Jun 18, 2020 •

edited

Loading

glozow left a comment •

edited

Loading

glozow left a comment •

edited

Loading

maflcko commented Jul 26, 2020 •

edited

Loading

glozow commented Sep 7, 2020 •

edited

Loading