rpc: Report reason for replaceable txpool transactions #16490

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:1907-rpcMempoolWhyReplacable changing 7 files +36 −8
  1. MarcoFalke commented at 8:51 PM on July 29, 2019: member

    Currently, the reason for the boolean bip125-replaceable is not given. However, future versions of Bitcoin Core (or software forks of Bitcoin Core) might offer different mempool (replacement) policies to their users. So instead of a flat boolean, it would be nice to return the reason why bip125-replaceable is true or false. This pull does that among a refactoring commit.

  2. DrahtBot added the label Mempool on Jul 29, 2019
  3. DrahtBot added the label RPC/REST/ZMQ on Jul 29, 2019
  4. DrahtBot added the label TX fees and policy on Jul 29, 2019
  5. DrahtBot added the label Validation on Jul 29, 2019
  6. MarcoFalke removed the label TX fees and policy on Jul 29, 2019
  7. MarcoFalke removed the label Validation on Jul 29, 2019
  8. MarcoFalke added the label TX fees and policy on Jul 29, 2019
  9. MarcoFalke force-pushed on Jul 29, 2019
  10. DrahtBot commented at 10:16 AM on July 31, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #17809 (rpc: Auto-format RPCResult by MarcoFalke)
    • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

    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.

  11. MarcoFalke force-pushed on Aug 6, 2019
  12. laanwj commented at 11:27 AM on August 14, 2019: member

    Concept ACK

  13. MarcoFalke force-pushed on Aug 14, 2019
  14. MarcoFalke closed this on Aug 14, 2019
  15. MarcoFalke reopened this on Aug 14, 2019
  16. in src/policy/rbf.h:16 in fa12ae20da outdated 
      14 | +    FINAL,
  15 |  };
  16 |  
  17 | -// Determine whether an in-mempool transaction is signaling opt-in to RBF
  18 | -// according to BIP 125
  19 | +// Determine whether a transaction is replacable
    practicalswift commented at 9:27 PM on August 18, 2019:

    Should be "replaceable" :-)

  17. in src/rpc/blockchain.cpp:401 in fa12ae20da outdated 
     396 | @@ -397,7 +397,11 @@ static std::string EntryDescriptionString()
 397 |             "    \"spentby\" : [           (array) unconfirmed transactions spending outputs from this transaction\n"
 398 |             "        \"transactionid\",    (string) child transaction id\n"
 399 |             "       ... ]\n"
 400 | -           "    \"bip125-replaceable\" : true|false,  (boolean) Whether this transaction could be replaced due to BIP125 (replace-by-fee)\n";
 401 | +           "    \"bip125-replaceable\" : true|false,  (boolean) Whether this transaction could be replaced according to BIP125 (replace-by-fee) rules\n"
 402 | +           "    \"replacable-reason\" :   (string) Why this transaction is or is not replaceable. Reasons are "
    practicalswift commented at 9:27 PM on August 18, 2019:

    Same :-)

  18. in src/rpc/blockchain.cpp:465 in fa12ae20da outdated 
     461 | @@ -458,6 +462,7 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool
 462 |          rbfStatus = true;
 463 |      }
 464 |  
 465 | +    info.pushKV("replacable-reason", RBFTransactionStateToString(rbfState));
    practicalswift commented at 9:27 PM on August 18, 2019:

    And same :-)

  19. MarcoFalke force-pushed on Aug 19, 2019
  20. MarcoFalke force-pushed on Aug 20, 2019
  21. MarcoFalke force-pushed on Aug 20, 2019
  22. MarcoFalke commented at 6:28 PM on August 20, 2019: member

    Added test

  23. luke-jr commented at 11:09 PM on September 2, 2019: member

    BIP 125 is pretty straightforward... Any other policy isn't BIP 125.

    It would be incorrect for "bip125-replacable" to show true/false based on anything other than BIP 125.

    If we want to be clearer with regard to different policies, we need a new boolean too. Or just a string/null field. A string that doesn't answer the main "is this replacable?" question isn't very helpful.

  24. policy: Avoid duplicate mempool lookup
    Also, avoid direct access to mapTx
    fa46f68211
  25. MarcoFalke force-pushed on Sep 5, 2019
  26. MarcoFalke force-pushed on Sep 5, 2019
  27. MarcoFalke force-pushed on Sep 5, 2019
  28. MarcoFalke commented at 1:35 AM on September 5, 2019: member

    Ok, made it a new optional string, without modifying existing return values

  29. MarcoFalke renamed this:
    rpc: Report reason for 'bip125-replaceable' value
    rpc: Report reason for replaceable txpool transactions
    on Sep 5, 2019
  30. laanwj added this to the "Blockers" column in a project
  31. in src/rpc/blockchain.cpp:417 in fa0bead965 outdated 
     412 | @@ -399,7 +413,12 @@ static std::string EntryDescriptionString()
 413 |             "    \"spentby\" : [           (array) unconfirmed transactions spending outputs from this transaction\n"
 414 |             "        \"transactionid\",    (string) child transaction id\n"
 415 |             "       ... ]\n"
 416 | -           "    \"bip125-replaceable\" : true|false,  (boolean) Whether this transaction could be replaced due to BIP125 (replace-by-fee)\n";
 417 | +           "    \"bip125-replaceable\" : true|false,  (boolean) Whether this transaction could be replaced due to BIP125 (replace-by-fee)\n"
 418 | +           "    \"replaceable\" :         (string) Whether this this transaction is replaceable.\n"
    fjahr commented at 2:28 PM on October 11, 2019:

    typo: this this

    MarcoFalke commented at 6:41 PM on October 11, 2019:

    Thanks, fixed typo

  32. fjahr commented at 2:54 PM on October 11, 2019: member

    tested and code review ACK

    Conceptually, I think I would have liked it a little more if replaceable was a boolean and then there was another string field replaceable-reason, similar to reject-reason in the testmempoolaccept RPC. Is it realistic that we could remove bip125-replaceable then in the future? If that is an option I would prefer it but I don't know how many users might rely on bip125-replaceable.

  33. MarcoFalke force-pushed on Oct 11, 2019
  34. rpc: Report reason for replaceable txpool transactions fa5d55d4ed
  35. MarcoFalke force-pushed on Oct 11, 2019
  36. MarcoFalke commented at 6:57 PM on October 11, 2019: member

    Ok, changed to what @fjahr suggested

  37. fjahr commented at 2:46 PM on October 12, 2019: member

    tested ACK fa5d55d

  38. in src/rpc/blockchain.cpp:419 in fa5d55d4ed 
     412 | @@ -399,7 +413,13 @@ static std::string EntryDescriptionString()
 413 |             "    \"spentby\" : [           (array) unconfirmed transactions spending outputs from this transaction\n"
 414 |             "        \"transactionid\",    (string) child transaction id\n"
 415 |             "       ... ]\n"
 416 | -           "    \"bip125-replaceable\" : true|false,  (boolean) Whether this transaction could be replaced due to BIP125 (replace-by-fee)\n";
 417 | +           "    \"bip125-replaceable\" : true|false,  (boolean) Whether this transaction could be replaced due to BIP125 (replace-by-fee)\n"
 418 | +           "    \"replaceable\" :         (bool) Whether this transaction is replaceable.\n"
 419 | +           "    \"replaceable-reason\" :  (string) Why this transaction is replaceable.\n"
 420 | +           "                              If the transaction is not replaceable, this is Null.\n"
    promag commented at 3:32 PM on October 12, 2019:

    Have you considered not adding the key at all?

    MarcoFalke commented at 7:24 PM on October 13, 2019:

    The caller doesn't have to use it. I think it might be helpful to see the reason why something is replaceable, no?

    promag commented at 7:33 PM on October 13, 2019:

    Ops, I mean when it's null.

    MarcoFalke commented at 7:36 PM on October 13, 2019:

    A missing key will throw KeyError for the caller

    luke-jr commented at 9:49 PM on October 13, 2019:

    That depends on the caller's programming language and API of choice.

    If they choose a language that gives KeyError, then they should expect it...

    MarcoFalke commented at 2:26 PM on October 14, 2019:

    I think it is not a good api design to make parsing depend on the result

    promag commented at 6:40 AM on October 16, 2019:

    No strong feeling but it was already suggested (and implemented) elsewhere to omit keys when not applicable. For instance, search for "present" in rpcwallet.cpp.

    MarcoFalke commented at 12:21 PM on October 16, 2019:

    Lets discuss this in #17155

    MarcoFalke commented at 8:44 PM on October 18, 2019:

    Closing this discussion

  39. promag commented at 3:33 PM on October 12, 2019: member

    Concept ACK and from a quick look fa5d55d4eda6f88e7424e58c33b7522a33cad30e LGTM.

  40. in src/rpc/blockchain.cpp:384 in fa5d55d4ed 
     379 | +        return "bip125";
 380 | +    case RBFTransactionState::FINAL:
 381 | +        return NullUniValue; // not replaceable
 382 | +        // no default case, so the compiler can warn about missing cases
 383 | +    }
 384 | +    assert(false);
    ryanofsky commented at 8:21 PM on October 18, 2019:

    Maybe with #17192 this would be updated to something like:

        switch (state) {
    case RBFTransactionState::REPLACEABLE_BIP125:
        return "bip125";
    case RBFTransactionState::FINAL:
        return NullUniValue; // not replaceable
    case RBFTransactionState::UNKNOWN:
        break; // should never happen
    }
    CHECK_NONFATAL(false);
    MarcoFalke commented at 8:36 PM on October 18, 2019:

    Indeed. Will adjust the second of the pulls that gets merged accordingly.

  41. ryanofsky approved
  42. ryanofsky commented at 8:26 PM on October 18, 2019: member

    Code review ACK fa5d55d4eda6f88e7424e58c33b7522a33cad30e. Current travis error seems like spurious lint error.

  43. MarcoFalke commented at 8:45 PM on October 18, 2019: member

    This has three ACKs, so I will probably merge it in the next few days unless there are objections.

  44. sdaftuar commented at 3:37 PM on October 22, 2019: member

    This change seems premature to me -- it only seems to make sense in a world where we've updated our actual policy rules to include other-than-bip-125 replacement, which we've not done (yet).

    In the meantime, having an extra "replaceable" field on top of "bip-125-replaceable" seems like it will confuse our users. I don't think we should make a change like this in a release where bip 125 replacement is the only thing we support.

  45. ryanofsky commented at 5:12 PM on October 22, 2019: member

    seems like it will confuse our users

    Might be less confusing to mark the old field as deprecated, and maybe start the process of removing it with deprecation flags.

    If policy rules are likely to be updated in the future, updating the RPC interface sooner that could allow writing more robust RPC client code that won't have to change later. If I were writing RPC client code and knew there were going to be changes to the interface, I'd want more time to develop and test against those changes rather than less.

  46. sdaftuar commented at 5:44 PM on October 22, 2019: member

    @ryanofsky If we had consensus to change our policy rules, then I'd agree that marking the old field as deprecated would make sense to allow people to prepare. But I think the issue has been that we haven't reached that consensus yet (I believe several PRs in the last year or two that proposed new replacement policies have stalled out or been closed).

  47. ryanofsky commented at 6:25 PM on October 22, 2019: member

    Achieving consensus about changing policy rules and achieving consensus about future-proofing an RPC seem like things that could be done separately. That'd probably be my inclination, so I'd keep my previous ACK here. But I don't have a very strong opinion, and really just wanted to add the practical suggestion to document the less general field as deprecated to avoid confusion, or at least mention the difference between the two fields in the documentation.

  48. laanwj removed this from the "Blockers" column in a project
  49. in src/txmempool.cpp:863 in fa46f68211 outdated 
     859 | @@ -860,7 +860,7 @@ const CTransaction* CTxMemPool::GetConflictTx(const COutPoint& prevout) const
 860 |      return it == mapNextTx.end() ? nullptr : it->second;
 861 |  }
 862 |  
 863 | -boost::optional<CTxMemPool::txiter> CTxMemPool::GetIter(const uint256& txid) const
 864 | +Optional<CTxMemPool::txiter> CTxMemPool::GetIter(const uint256& txid) const
    promag commented at 10:12 AM on December 17, 2019:

    Please rebase as this change is already in master.

  50. DrahtBot commented at 2:28 PM on March 4, 2020: member

    <!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

  51. DrahtBot added the label Needs rebase on Mar 4, 2020
  52. in src/policy/rbf.cpp:21 in fa5d55d4ed 
      17 | @@ -18,15 +18,16 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
  18 |  
  19 |      // If this transaction is not in our mempool, then we can't be sure
  20 |      // we will know about all its inputs.
  21 | -    if (!pool.exists(tx.GetHash())) {
  22 | +    const Optional<CTxMemPool::txiter> it = pool.GetIter(tx.GetHash());
    fanquake commented at 2:07 AM on March 15, 2021:

    Please use std::optional in new code.

  53. mjdietzx commented at 2:39 PM on November 29, 2021: contributor

    Concept ACK fa46f68211ae6616a42c0fd1467fdd5710cd3b72

    I think I am a concept NACK fa5d55d4eda6f88e7424e58c33b7522a33cad30e, along the lines of @luke-jr 's initial comment here

  54. MarcoFalke closed this on Feb 25, 2022
  55. MarcoFalke deleted the branch on Feb 25, 2022
  56. DrahtBot locked this on Feb 25, 2023
Contributors
MarcoFalkeDrahtBotlaanwjpracticalswiftluke-jrfjahrpromagryanofskysdaftuarfanquakemjdietzx
Labels
TX fees and policyRPC/REST/ZMQMempoolNeeds rebase
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: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me