Imo this should be the same key-value as the bip125-replaceable in the rpcwallet

Good idea! I updated the PR.

SignalsOptInRBF is not bip125-replacable.

• Confirmed transaction can never be replaced in the mempool
• Unconfirmed transactions that depend on a mempool-transaction that signals opt in rbf can be replaced

Would you suggest to rename the flag to something like signals-rbf? Or rather do a stateful check, whether the transaction is actually replaceable, when the call is made?

The latter would probably be very useful, but given that TxToUniv isn't stateful and currently doesn't have any mempool relation, I don't see a straight forward way for this.

If we do this I'd say rename it to signals-rbf or such, which is also the name of the function already. And not make the raw transactions utility function decoding functionality depend on the mempool.

In which case I tend to Concept NACK. Just returning if the flag is set contradicts the bip125-replacability rules, which only adds confusion.

Sorry to hijack.

I had considered adding a flag like this to the verbose output of getrawmempool. Assuming this idea were supported, based on your feedback @MarcoFalke - is it fair to say that you think it makes more sense to list a mempool transaction as replaceable (covering both explicit signaling and implicit based on ancestors) rather than just signals-rbf (just the explicit case)?

Do you think it would be appropriate to list that in getrawmempool?

please use brackets for multi-line conditionals

Thanks, updated.

Can you add a test for unknown?

Hmm.. this is going to be a bit difficult: the "unknown" case is only triggered, if the transaction isn't found in the mempool.

In the case of the RPCs this path might even get removed, given that we iterate over mempool entries, which are indeed in the mempool and thus never "unknown".

Such a test might require mocking IsRBFOptIn in blockchain.cpp, but that seems overkill here.

I think this can just be an assert(bfState != RBF_TRANSACTIONSTATE_UNKNOWN). Looking at rbf.cpp, the unknown state should only occur if the transaction in question is not found in the mempool, and this function only processes transactions that are currently in the mempool (with mempool lock held).

If the above is correct (I believe it is), the bip125-replaceable field can just become a boolean.

I'm not a fan of an assert here. It if it's really not possible, it's better to put the assert in rbf.cpp and remove this state from the enum. All this function should be concerned with is translating an enum to a string.

Actually: what about a situation where a descendant is missing from the mempool (because it somehow got evicted)?

I should have been more clear. In general it is possible to return RBF_TRANSACTIONSTATE_UNKNOWN from IsRBFOptIn(), however this call stack in particular (all mempool related rpc's) only calls IsRBFOptIn() on transactions we know to be in the mempool. So, the assert would belong here, not in rfb.cpp. I understand this still may not be the best option, I just wanted to make my language clear since my comment above was a bit ambiguous.

I am not sure I understand the problem/situation where 'a descendant is missing from the mempool'. Can you elaborate please?

I'm not a fan of using assert in non-consensus critical code, given it's massive impact, if it's hit (e.g. by accident in the future etc.).

In the case of the RPCs "unknown" isn't triggered, because they iterate over mempool entries, which are inherently part of the mempool, but I kind of tend to leave it there for the potential case entryToJSON() is called in a different context. I'd be fine with removing this path though, too.

Actually: what about a situation where a descendant is missing from the mempool (because it somehow got evicted)?

Not an issue as far as I can see. We only care about ancestors and whether an unconfirmed ancestor signals RBF, which makes the chain replaceable.

Mempool.dat is sorted in dependency order, so ancestors always appear earlier. Further, the mempool is at all times internally consistent (when observable): all mempool transactions either spend a transaction output from the chainstate (so from a confirmed transaction), or another mempool output. There are also never any double spends in the mempool.

Any opinion on listing unknown state @MarcoFalke ?

I don't have a strong opinion if the value of 'bip125-replacable' should be string (yes/no/unknown) or bool.

It seems entryToJSON can only be called when the entry is in the mempool (c.f. mempool.vTxHashes[e.vTxHashesIdx].first.ToString()). Thus, we can surely determine if it is replacable.

Instead of adding an assert, I'd prefer to throw some json exception that mentions that it never can be thrown.

I guess you could throw it in entryToJSON to make sure it is passed up to all potential callers?

But does entryToJSON really care, whether it's in the mempool or not? I think this limits it's functionality in some way.

entryToJSON specifically processes mempool transactions (CTxMemPoolEntry &e) - and I don't believe you should be able to have a CTxMemPoolEntry reference for something not currently in the mempool (it should guarantees this consistency), so in that sense, entryToJson should never be called in a "different context" (where the tx is not in mempool).

Anyhow, I am not trying to get pedantic on this point. I want this change (and requested it) and I am okay with the current implementation.

No need to encode a bool into a string, imo

Very good point.

Updated.

Throwing here seems nasty. Just don't add a key?

Eg:

    switch (IsRBFOptIn(tx, mempool)) {
        case RBFTransactionState::FINAL:
            info.push_back(Pair("bip125-replaceable", false));
            break;
        case RBFTransactionState::REPLACEABLE_BIP125:
            info.push_back(Pair("bip125-replaceable", true));
            break;
        case RBFTransactionState::UNKNOWN:
            break;
    }

(Doing it this way also gets a compile-time warning if a new state is added later)

I think it'd be nice to have this produce the same output as the wallet rpc calls that provide the same information (which appears to be "yes"/"no"/"unknown").

I don't think it is even possible to hit this case since it requires the transaction to not be in the mempool (for the unknown state) yet every entry we are processing here comes from the mempool.

Agree with @achow101

Agree with @sdaftuar