Show "bip125-replaceable" flag, when retrieving mempool entries #12676

dexX7 commented at 9:56 AM on March 12, 2018: contributor

This pull request adds a flag "bip125-replaceable" to the mempool RPCs getrawmempool, getmempoolentry, getmempoolancestors and getmempooldescendants, which indicates whether an unconfirmed transaction might be replaced.

Initially the flag was added to the raw transaction RPCs, but thanks to @conscott, it was moved to the mempool RPCs, which actually have access to the mempool.

~~This pull request adds a flag "bip125-replaceable" to the RPCs "getrawtransaction" and "decoderawtransaction", which indicates, whether a transaction signals BIP 125 replaceability.~~

There was some discussion in #7817, whether showing replaceability in the UI could lead to the false assumption that transactions that don't signal BIP 125 are truely non-replaceable, but given that this PR tackles the raw transaction interface, which is a rather low level tool, I believe having this extra piece of information isn't bad.

dexX7 commented at 9:56 AM on March 12, 2018: contributor

~~Please note, I copied SignalsOptInRBF() from policy/rbf.cpp into core_write.cpp.~~

This is probably not a good way to do, but it was done for now, because rbf.cpp isn't avaliable in LIBBITCOIN_COMMON. However, simply moving rbf.cpp from LIBBITCOIN_SERVER to LIBBITCOIN_COMMON doesn't work, because IsRBFOptIn() in rbf.cpp has mempool dependencies, which are also not available.

As alternative I could imagine to split rbf.cpp into something like rbf.cpp with SignalsOptInRBF(), which becomes part of LIBBITCOIN_COMMON and rbfstate.cpp with IsRBFOptIn(), which becomes part of LIBBITCOIN_SERVER?

fanquake added the label RPC/REST/ZMQ on Mar 12, 2018

in src/core_write.cpp:175 in 6436b6b863 outdated

 171 | @@ -162,6 +172,7 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry,
 172 |      entry.pushKV("size", (int)::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION));
 173 |      entry.pushKV("vsize", (GetTransactionWeight(tx) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR);
 174 |      entry.pushKV("locktime", (int64_t)tx.nLockTime);
 175 | +    entry.pushKV("replaceable", SignalsOptInRBF(tx));

MarcoFalke commented at 12:21 PM on March 12, 2018:

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

dexX7 commented at 1:54 PM on March 12, 2018:

Good idea! I updated the PR.

dexX7 force-pushed on Mar 12, 2018

in src/core_write.cpp:175 in e62c505f7e outdated

 171 | @@ -162,6 +172,7 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry,
 172 |      entry.pushKV("size", (int)::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION));
 173 |      entry.pushKV("vsize", (GetTransactionWeight(tx) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR);
 174 |      entry.pushKV("locktime", (int64_t)tx.nLockTime);
 175 | +    entry.pushKV("bip125-replaceable", SignalsOptInRBF(tx));

MarcoFalke commented at 2:11 PM on March 12, 2018:

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

dexX7 commented at 2:48 PM on March 12, 2018:

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.

laanwj commented at 4:51 PM on March 12, 2018:

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.

MarcoFalke commented at 4:56 PM on March 12, 2018:

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

conscott commented at 5:27 AM on March 13, 2018:

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?

dexX7 force-pushed on Mar 13, 2018

dexX7 renamed this:
~~Show "replaceable" flag, when decoding raw transactions~~
Show "bip125-replaceable" flag, when retrieving mempool entries
on Mar 13, 2018

dexX7 commented at 11:10 AM on March 13, 2018: contributor

@conscott brought up the idea of adding the flag to mempool entries and mempool RPCs. I really like the idea, because these calls are actually mempool aware and provide full access to the mempool and thus also allow to check, whether a transaction is really replaceable instead of simply signaling replaceablity.

I updated the PR accordingly, because I believe this is the way to go. @conscott if you want to submit this as PR, feel free to do so though, and I'll close this one.

in src/rpc/blockchain.cpp:427 in f215f99b90 outdated

 419 | @@ -418,6 +420,16 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
 420 |      }
 421 |  
 422 |      info.pushKV("spentby", spent);
 423 | +
 424 | +    // Add opt-in RBF status
 425 | +    std::string rbfStatus = "no";
 426 | +    RBFTransactionState rbfState = IsRBFOptIn(tx, mempool);
 427 | +    if (rbfState == RBF_TRANSACTIONSTATE_UNKNOWN)

instagibbs commented at 2:32 PM on March 13, 2018:

please use brackets for multi-line conditionals

dexX7 commented at 4:01 PM on March 13, 2018:

Thanks, updated.

instagibbs approved

instagibbs commented at 2:35 PM on March 13, 2018: member

utACK, minus style nits https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

in test/functional/feature_rbf.py:430 in 788885ffd0 outdated

 426 | @@ -427,6 +427,9 @@ def test_opt_in(self):
 427 |          tx1a_hex = txToHex(tx1a)
 428 |          tx1a_txid = self.nodes[0].sendrawtransaction(tx1a_hex, True)
 429 |  
 430 | +        # This transaction isn't shown as replaceable

Sjors commented at 3:06 PM on March 13, 2018:

Can you add a test for unknown?

dexX7 commented at 6:22 PM on March 13, 2018:

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

Sjors commented at 7:07 PM on March 13, 2018:

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

dexX7 force-pushed on Mar 13, 2018

instagibbs commented at 4:10 PM on March 13, 2018: member

re-ACK

Sjors approved

Sjors commented at 4:16 PM on March 13, 2018: member

tACK eef1b43

in src/rpc/blockchain.cpp:428 in eef1b43b58 outdated

 419 | @@ -418,6 +420,17 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
 420 |      }
 421 |  
 422 |      info.pushKV("spentby", spent);
 423 | +
 424 | +    // Add opt-in RBF status
 425 | +    std::string rbfStatus = "no";
 426 | +    RBFTransactionState rbfState = IsRBFOptIn(tx, mempool);
 427 | +    if (rbfState == RBF_TRANSACTIONSTATE_UNKNOWN) {
 428 | +        rbfStatus = "unknown";

conscott commented at 6:24 PM on March 13, 2018:

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.

Sjors commented at 7:09 PM on March 13, 2018:

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.

Sjors commented at 7:12 PM on March 13, 2018:

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

conscott commented at 2:27 AM on March 14, 2018:

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?

dexX7 commented at 7:29 AM on March 14, 2018:

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.

conscott commented at 9:06 AM on March 14, 2018:

Agree about the 'descendant is missing from the mempool' not being an issue, or relevant, as far as I can see.

I also see your point that in general the function could be called in a context where the tx is no longer in the mempool, but in cases strictly calling down from a mempool rpc, it seems like we should always 'know' rbf state in that context. In that sense, it may not make sense for the rpc itself to list unknown as an option - unless there actually is a case I am missing where we would not know the rbf state of a tx in our own mempool.

Having said that, whether or not an assert is the right thing to do, I am also an open about.

I will happily wait for someone more qualified than myself to chime in with their opinion :)

Sjors commented at 1:58 PM on March 14, 2018:

this call stack in particular (all mempool related rpc's) only calls IsRBFOptIn() on transactions we know to be in the mempool

There's no guarantee all ancestors are in the mempool (someone might have deleted mempool.dat and it's still in the process of syncing, unless we guarantee order there). If a transaction in the mempool doesn't signal RBF, but its ancestors do, then it's replaceable. So IsRBFOptIn would return UNKNOWN in that case, I think..

sipa commented at 4:42 PM on March 14, 2018:

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.

conscott commented at 12:55 PM on March 20, 2018:

Any opinion on listing unknown state @MarcoFalke ?

MarcoFalke commented at 1:32 PM on March 21, 2018:

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?

dexX7 commented at 2:24 PM on March 21, 2018:

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

conscott commented at 6:43 AM on March 23, 2018:

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.

conscott commented at 6:28 PM on March 13, 2018: contributor

Concept ACK. Thanks for updating @dexX7 - just left a comment about the unknown state. Will test this out tomorrow.

dexX7 commented at 6:31 PM on March 13, 2018: contributor

Regarding checking the "unknown" case: in the case of the RPCs this 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.

meshcollider commented at 9:00 PM on March 13, 2018: contributor

Concept ACK

conscott commented at 6:44 AM on March 23, 2018: contributor

Test ACK eef1b43b5860122bd7dc9270b5b5ba3f64b1dc0e

dexX7 commented at 11:12 AM on April 24, 2018: contributor

Hey guys, just to clarify: given there were mixed reactions, do you insist on removing the branch for the "unknown" case and replace it with an exception or is this PR good to go as it is?

conscott commented at 11:40 AM on April 24, 2018: contributor

@dexX7 - I was arguing over the unknown, but I am happy with it as is.

MarcoFalke commented at 2:44 PM on April 24, 2018: member

I just fail to see why it makes sense to burden rpc users with a third value that is never actually set.

dexX7 force-pushed on Apr 25, 2018

dexX7 commented at 5:38 AM on April 25, 2018: contributor

Alright, updated. :)

in src/rpc/blockchain.cpp:380 in 5434aaaee5 outdated

 375 | @@ -375,7 +376,8 @@ std::string EntryDescriptionString()
 376 |             "       ... ]\n"
 377 |             "    \"spentby\" : [           (array) unconfirmed transactions spending outputs from this transaction\n"
 378 |             "        \"transactionid\",    (string) child transaction id\n"
 379 | -           "       ... ]\n";
 380 | +           "       ... ]\n"
 381 | +           "    \"bip125-replaceable\": \"yes|no\",  (string) Whether this transaction could be replaced due to BIP125 (replace-by-fee)\n";

MarcoFalke commented at 3:19 PM on April 25, 2018:

No need to encode a bool into a string, imo

dexX7 commented at 3:57 PM on April 25, 2018:

Very good point.

dexX7 commented at 4:03 PM on April 25, 2018:

Updated.

dexX7 force-pushed on Apr 25, 2018

Add "bip125-replaceable" flag to mempool RPCs

This affects getrawmempool, getmempoolentry, getmempoolancestors and getmempooldescendants.

820d31f95f

dexX7 force-pushed on Apr 25, 2018

Update functional RBF test to check replaceable flag 870bd4c73d

dexX7 force-pushed on May 22, 2018

in src/rpc/blockchain.cpp:429 in 870bd4c73d

 420 | @@ -419,6 +421,17 @@ void entryToJSON(UniValue &info, const CTxMemPoolEntry &e)
 421 |      }
 422 |  
 423 |      info.pushKV("spentby", spent);
 424 | +
 425 | +    // Add opt-in RBF status
 426 | +    bool rbfStatus = false;
 427 | +    RBFTransactionState rbfState = IsRBFOptIn(tx, mempool);
 428 | +    if (rbfState == RBFTransactionState::UNKNOWN) {
 429 | +        throw JSONRPCError(RPC_MISC_ERROR, "Transaction is not in mempool");

luke-jr commented at 10:16 AM on July 9, 2018:

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)

sdaftuar commented at 1:56 PM on July 9, 2018:

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

achow101 commented at 10:28 PM on July 9, 2018:

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.

MarcoFalke commented at 5:18 PM on July 10, 2018:

Agree with @achow101

luke-jr commented at 5:32 PM on July 10, 2018:

Agree with @sdaftuar

luke-jr changes_requested

achow101 commented at 10:29 PM on July 9, 2018: member

utACK 870bd4c73ddf494dc23c658bf0fb672ee0109158

MarcoFalke commented at 5:18 PM on July 10, 2018: member

utACK 870bd4c

DrahtBot commented at 11:50 PM on July 22, 2018: member

The last travis run for this pull request was 61 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

DrahtBot closed this on Jul 22, 2018

DrahtBot reopened this on Jul 22, 2018

laanwj added this to the milestone 0.18.0 on Aug 7, 2018

laanwj added the label Feature on Aug 7, 2018

laanwj commented at 12:26 PM on August 7, 2018: member

This seems mergeable, should do so after 0.17 branch as this is a feature

laanwj merged this on Aug 25, 2018

laanwj closed this on Aug 25, 2018

laanwj referenced this in commit 6516b36731 on Aug 25, 2018

Munkybooty referenced this in commit c0e8721372 on Jun 30, 2021

Munkybooty referenced this in commit 8f0f802c48 on Jun 30, 2021

Munkybooty referenced this in commit 9fc66137f7 on Jun 30, 2021

MarcoFalke locked this on Sep 8, 2021