RPC: Return transaction fee from testmempoolaccept #19093

pull rajarshimaitra wants to merge 1 commits into bitcoin:master from rajarshimaitra:fee-trial3 changing 6 files +34 −14
  1. rajarshimaitra commented at 5:30 pm on May 28, 2020: contributor

    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.

  2. jonatack commented at 6:33 pm on May 28, 2020: member
    ISTM there is no need to close #19063 and open another PR. Better to have rebased on the original PR.
  3. DrahtBot added the label RPC/REST/ZMQ on May 28, 2020
  4. DrahtBot added the label Tests on May 28, 2020
  5. DrahtBot added the label Validation on May 28, 2020
  6. rajarshimaitra commented at 7:23 am on May 29, 2020: contributor

    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.

  7. rajarshimaitra closed this on May 29, 2020

  8. rajarshimaitra reopened this on May 29, 2020

  9. MarcoFalke removed the label Tests on May 29, 2020
  10. MarcoFalke removed the label Validation on May 29, 2020
  11. in src/rpc/rawtransaction.cpp:920 in f70abce967 outdated
    916@@ -916,10 +917,11 @@ static UniValue testmempoolaccept(const JSONRPCRequest& request)
    917 
    918     TxValidationState state;
    919     bool test_accept_res;
    920+    CAmount fee = 0;
    


    MarcoFalke commented at 12:23 pm on May 29, 2020:
    0    CAmount fee_out;
    

    No need to initialize. Also could clarify that this is an out parameter?


    rajarshimaitra commented at 12:53 pm on May 29, 2020:
    Done.
  12. in src/rpc/rawtransaction.cpp:939 in f70abce967 outdated
    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 {
    


    MarcoFalke commented at 12:25 pm on May 29, 2020:

    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


    rajarshimaitra commented at 12:53 pm on May 29, 2020:
    makes sense, done.
  13. in src/validation.cpp:475 in f70abce967 outdated
    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;
    


    MarcoFalke commented at 12:25 pm on May 29, 2020:
    0        CAmount* m_fee;
    

    see dev notes


    rajarshimaitra commented at 12:55 pm on May 29, 2020:
    My mistake. Fixed.
  14. in src/validation.cpp:1070 in f70abce967 outdated
    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)
    


    MarcoFalke commented at 12:27 pm on May 29, 2020:
    please move the fee arg before the test arg, so that non-testing callers can use this as well (without having to specify that they don’t test)

    rajarshimaitra commented at 12:57 pm on May 29, 2020:

    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?


    rajarshimaitra commented at 2:06 pm on May 29, 2020:
    Also i was wondering cant a non-testing call use something like (..., 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.

    MarcoFalke commented at 2:39 pm on May 29, 2020:
    Ok, fair enough. No need to change if it is too involved.
  15. in test/functional/mempool_accept.py:107 in f70abce967 outdated
    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}],
    


    MarcoFalke commented at 12:28 pm on May 29, 2020:
    please no floating point in tests. Decimal is good enough

    rajarshimaitra commented at 1:37 pm on May 29, 2020:
    I am a little confused here, i used 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?

    MarcoFalke commented at 1:43 pm on May 29, 2020:
    Nevemind. I wrote this test and used floating point in the first place. :sweat_smile: :cold_sweat:
  16. in test/functional/mempool_accept.py:190 in f70abce967 outdated
    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}],
    


    MarcoFalke commented at 12:28 pm on May 29, 2020:
    Same
  17. MarcoFalke commented at 12:28 pm on May 29, 2020: member
    Concept ACK
  18. rajarshimaitra force-pushed on May 29, 2020
  19. rajarshimaitra commented at 4:03 pm on May 29, 2020: contributor
    Thanks for the review. Updated and rebased.
  20. promag commented at 11:45 pm on May 31, 2020: member

    Concept ACK, code change looks good.

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

  21. rajarshimaitra commented at 1:35 pm on June 1, 2020: contributor
    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?
  22. in src/rpc/rawtransaction.cpp:931 in 520ae886bb outdated
    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 {
    


    luke-jr commented at 6:09 pm on June 8, 2020:
    Should be on one line

    rajarshimaitra commented at 10:24 am on June 9, 2020:
    Done.
  23. in src/validation.cpp:688 in 520ae886bb outdated
    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){
    


    luke-jr commented at 6:10 pm on June 8, 2020:
    0    if (args.m_fee) {
    

    rajarshimaitra commented at 10:24 am on June 9, 2020:
    Done.
  24. luke-jr approved
  25. luke-jr commented at 6:11 pm on June 8, 2020: member
    utACK, with some nits
  26. rajarshimaitra force-pushed on Jun 9, 2020
  27. rajarshimaitra commented at 10:39 am on June 9, 2020: contributor

    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.

  28. in doc/release-notes-19093.md:4 in c7c3f458eb outdated
    0@@ -0,0 +1,7 @@
    1+RPC changes
    2+------------
    3+
    4+### 'testmempoolaccept` imporvement
    


    luke-jr commented at 7:14 pm on June 9, 2020:
    Typo

    rajarshimaitra commented at 7:54 am on June 10, 2020:
    Fixed.
  29. luke-jr referenced this in commit 406fb62f31 on Jun 9, 2020
  30. in src/validation.h:195 in c7c3f458eb outdated
    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 **/
    


    luke-jr commented at 0:19 am on June 10, 2020:
    Typo

    rajarshimaitra commented at 7:54 am on June 10, 2020:
    Fixed
  31. luke-jr referenced this in commit 13e889683f on Jun 10, 2020
  32. rajarshimaitra force-pushed on Jun 10, 2020
  33. DrahtBot commented at 2:14 am on June 18, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #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.

  34. in src/rpc/rawtransaction.cpp:937 in 3218bba02c outdated
    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) {
    


    glowang commented at 11:47 pm on June 20, 2020:

    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.


    rajarshimaitra commented at 6:30 am on June 21, 2020:

    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.


    glowang commented at 3:11 pm on June 24, 2020:

    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.


    rajarshimaitra commented at 6:16 pm on June 24, 2020:

    Check here.

    https://github.com/bitcoin/bitcoin/blob/205b87d2f6bd01285de50ba742e32e4ab1298b13/src/validation.cpp#L688-L691

    if CheckTxInputs fails, nFess will be 0. Look inside CheckTxInputs to see what makes it fail.

  35. glozow commented at 2:36 am on July 22, 2020: member

    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?

  36. glozow commented at 3:21 pm on July 25, 2020: member
  37. rajarshimaitra commented at 6:10 am on July 26, 2020: contributor

    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.

  38. MarcoFalke commented at 9:17 am on July 26, 2020: member
    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)
  39. in src/validation.cpp:695 in 3218bba02c outdated
    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+
    


    jnewbery commented at 2:25 pm on August 4, 2020:
    no need for two empty lines here.
  40. in src/rpc/rawtransaction.cpp:920 in 3218bba02c outdated
    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
    


    jnewbery commented at 2:27 pm on August 4, 2020:
    nit: no need for this inline comment. It’s clear from the code what this variable is doing.

    rajarshimaitra commented at 5:47 am on August 8, 2020:
    Removed this comment.
  41. in src/rpc/rawtransaction.cpp:928 in 3218bba02c outdated
    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
    


    jnewbery commented at 2:30 pm on August 4, 2020:
    No need for a comment to say what the code is doing. If you want to comment here, it should be to explain why the code is doing this, eg “if mempool acceptance failed, the returned fee may not be accurate, so only return it to the caller if the tx would be accepted.” or similar.

    rajarshimaitra commented at 5:47 am on August 8, 2020:
    Updated this comment to better express the intent.
  42. in doc/release-notes-19093.md:7 in 3218bba02c outdated
    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.
    


    jnewbery commented at 2:36 pm on August 4, 2020:
    “Fee is only returned if transaction would be accepted to the mempool.” is more accurate (testmempoolaccept doesn’t actually accept transactions to the mempool).

    rajarshimaitra commented at 5:49 am on August 8, 2020:
    updated.
  43. jnewbery commented at 2:39 pm on August 4, 2020: member

    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.”
  44. RPC: Return transaction fee from testmempoolaccept
    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.
    c9781e92a2
  45. rajarshimaitra force-pushed on Aug 8, 2020
  46. rajarshimaitra renamed this:
    RPC: testmempoolaccept returns transaction fee
    RPC: Return transaction fee from testmempoolaccept
    on Aug 8, 2020
  47. rajarshimaitra commented at 6:21 am on August 8, 2020: contributor
    Thanks @jnewbery for the review. Updated as per your suggestions and rebased.
  48. jnewbery commented at 12:26 pm on August 11, 2020: member

    Code review ACK c9781e92a211d2615a8889dede1b02913c18ecd1

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

  49. fanquake requested review from luke-jr on Aug 12, 2020
  50. in src/rpc/rawtransaction.cpp:881 in c9781e92a2
    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)"},
    


    MarcoFalke commented at 5:35 am on August 14, 2020:
    • This will put the fee in raw satoshis? I don’t think we do this anywhere else.
    • Have you seen #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.

    rajarshimaitra commented at 7:31 am on August 14, 2020:

    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

    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?


    MarcoFalke commented at 7:55 am on August 14, 2020:
    Sorry, typo. Should have been getmempoolentry in the other comment as well

    jnewbery commented at 8:59 am on August 14, 2020:
    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?

    MarcoFalke commented at 9:17 am on August 14, 2020:
    • 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.

    MarcoFalke commented at 9:20 am on August 14, 2020:
    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.

    jnewbery commented at 9:39 am on August 14, 2020:

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


    MarcoFalke commented at 9:44 am on August 14, 2020:
    Agree, since getmempoolentry also returns the absolute fee (and the vsize)

    glozow commented at 4:13 pm on August 24, 2020:
    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.
  51. MarcoFalke commented at 5:35 am on August 14, 2020: member
    Concept ACK
  52. in src/validation.cpp:478 in c9781e92a2
    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;
    


    fjahr commented at 10:16 pm on August 24, 2020:
    nit: to match your naming elsewhere you could have called this m_fee_out
  53. fjahr commented at 10:18 pm on August 24, 2020: member

    Code review ACK c9781e92a211d2615a8889dede1b02913c18ecd1

    Ignore my nit unless you have to retouch.

  54. jarrad commented at 4:42 am on August 25, 2020: none
    Concept ACK
  55. MarcoFalke commented at 1:23 pm on September 7, 2020: member
    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.
  56. MarcoFalke added the label Waiting for author on Sep 7, 2020
  57. MarcoFalke commented at 1:27 pm on September 7, 2020: member
    (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)
  58. jnewbery commented at 1:31 pm on September 7, 2020: member
    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.
  59. glozow commented at 5:06 pm on September 7, 2020: member

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

  60. rajarshimaitra commented at 5:56 am on September 10, 2020: contributor
    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.
  61. jnewbery commented at 1:16 pm on September 10, 2020: member
    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?
  62. glozow commented at 1:22 pm on September 10, 2020: member

    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?

  63. rajarshimaitra commented at 3:10 pm on September 10, 2020: contributor
    @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.
  64. jnewbery commented at 4:59 pm on September 15, 2020: member
    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.
  65. jnewbery closed this on Sep 15, 2020

  66. fanquake removed the label Waiting for author on Sep 19, 2020
  67. fanquake referenced this in commit c30f79d418 on Sep 19, 2020
  68. sidhujag referenced this in commit 4d146837e5 on Sep 20, 2020
  69. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me