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.
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.
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
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 "
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));
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.
Also, avoid direct access to mapTx
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"
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.
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"
KeyError for the caller
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…
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);
Maybe with #17192 this would be updated to something like:
0 switch (state) {
1 case RBFTransactionState::REPLACEABLE_BIP125:
2 return "bip125";
3 case RBFTransactionState::FINAL:
4 return NullUniValue; // not replaceable
5 case RBFTransactionState::UNKNOWN:
6 break; // should never happen
7 }
8 CHECK_NONFATAL(false);
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.
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.
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
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());
std::optional in new code.
