Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Aug 25, 2018

Conversation

@dexX7
Copy link
Contributor

dexX7 commented Mar 12, 2018

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

This comment has been minimized.

Copy link
Contributor Author

dexX7 commented Mar 12, 2018

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?

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Mar 12, 2018

Member

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

This comment has been minimized.

@dexX7

dexX7 Mar 12, 2018

Author Contributor

Good idea! I updated the PR.

@dexX7 dexX7 force-pushed the dexX7:rpc-raw-replaceable-flag branch 2 times, most recently Mar 12, 2018

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Mar 12, 2018

Member

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

This comment has been minimized.

@dexX7

dexX7 Mar 12, 2018

Author Contributor

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.

This comment has been minimized.

@laanwj

laanwj Mar 12, 2018

Member

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.

This comment has been minimized.

@MarcoFalke

MarcoFalke Mar 12, 2018

Member

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

This comment has been minimized.

@conscott

conscott Mar 13, 2018

Contributor

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 dexX7 force-pushed the dexX7:rpc-raw-replaceable-flag branch Mar 13, 2018

@dexX7 dexX7 changed the title Show "replaceable" flag, when decoding raw transactions Show "bip125-replaceable" flag, when retrieving mempool entries Mar 13, 2018

@dexX7

This comment has been minimized.

Copy link
Contributor Author

dexX7 commented Mar 13, 2018

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

@instagibbs
Copy link
Member

instagibbs left a comment

src/rpc/blockchain.cpp Outdated
// Add opt-in RBF status
std::string rbfStatus = "no";
RBFTransactionState rbfState = IsRBFOptIn(tx, mempool);
if (rbfState == RBF_TRANSACTIONSTATE_UNKNOWN)

This comment has been minimized.

@instagibbs

instagibbs Mar 13, 2018

Member

please use brackets for multi-line conditionals

This comment has been minimized.

@dexX7

dexX7 Mar 13, 2018

Author Contributor

Thanks, updated.

@dexX7 dexX7 force-pushed the dexX7:rpc-raw-replaceable-flag branch Mar 13, 2018

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 13, 2018

re-ACK

@Sjors

Sjors approved these changes Mar 13, 2018

Copy link
Member

Sjors left a comment

tACK eef1b43

@@ -427,6 +427,9 @@ def test_opt_in(self):
tx1a_hex = txToHex(tx1a)
tx1a_txid = self.nodes[0].sendrawtransaction(tx1a_hex, True)

# This transaction isn't shown as replaceable

This comment has been minimized.

@Sjors

Sjors Mar 13, 2018

Member

Can you add a test for unknown?

This comment has been minimized.

@dexX7

dexX7 Mar 13, 2018

Author Contributor

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

This comment has been minimized.

@Sjors

Sjors Mar 13, 2018

Member

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

src/rpc/blockchain.cpp Outdated
std::string rbfStatus = "no";
RBFTransactionState rbfState = IsRBFOptIn(tx, mempool);
if (rbfState == RBF_TRANSACTIONSTATE_UNKNOWN) {
rbfStatus = "unknown";

This comment has been minimized.

@conscott

conscott Mar 13, 2018

Contributor

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.

This comment has been minimized.

@Sjors

Sjors Mar 13, 2018

Member

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.

This comment has been minimized.

@Sjors

Sjors Mar 13, 2018

Member

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

This comment has been minimized.

@conscott

conscott Mar 14, 2018

Contributor

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?

This comment has been minimized.

@dexX7

dexX7 Mar 14, 2018

Author Contributor

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.

This comment has been minimized.

@sipa

sipa Mar 14, 2018

Member

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.

This comment has been minimized.

@conscott

conscott Mar 20, 2018

Contributor

Any opinion on listing unknown state @MarcoFalke ?

This comment has been minimized.

@MarcoFalke

MarcoFalke Mar 21, 2018

Member

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?

This comment has been minimized.

@dexX7

dexX7 Mar 21, 2018

Author Contributor

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

This comment has been minimized.

@conscott

conscott Mar 23, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

conscott commented Mar 13, 2018

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

@dexX7

This comment has been minimized.

Copy link
Contributor Author

dexX7 commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Member

MeshCollider commented Mar 13, 2018

Concept ACK

@conscott
Copy link
Contributor

conscott left a comment

Test ACK eef1b43b5860122bd7dc9270b5b5ba3f64b1dc0e

@dexX7

This comment has been minimized.

Copy link
Contributor Author

dexX7 commented Apr 24, 2018

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

This comment has been minimized.

Copy link
Contributor

conscott commented Apr 24, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Apr 24, 2018

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

@dexX7 dexX7 force-pushed the dexX7:rpc-raw-replaceable-flag branch Apr 25, 2018

@dexX7

This comment has been minimized.

Copy link
Contributor Author

dexX7 commented Apr 25, 2018

Alright, updated. :)

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 25, 2018

Member

No need to encode a bool into a string, imo

This comment has been minimized.

@dexX7

dexX7 Apr 25, 2018

Author Contributor

Very good point.

This comment has been minimized.

@dexX7

dexX7 Apr 25, 2018

Author Contributor

Updated.

@dexX7 dexX7 force-pushed the dexX7:rpc-raw-replaceable-flag branch 2 times, most recently Apr 25, 2018

Add "bip125-replaceable" flag to mempool RPCs
This affects getrawmempool, getmempoolentry, getmempoolancestors and getmempooldescendants.

@dexX7 dexX7 force-pushed the dexX7:rpc-raw-replaceable-flag branch Apr 25, 2018

@dexX7 dexX7 force-pushed the dexX7:rpc-raw-replaceable-flag branch to 870bd4c May 22, 2018

bool rbfStatus = false;
RBFTransactionState rbfState = IsRBFOptIn(tx, mempool);
if (rbfState == RBFTransactionState::UNKNOWN) {
throw JSONRPCError(RPC_MISC_ERROR, "Transaction is not in mempool");

This comment has been minimized.

@luke-jr

luke-jr Jul 9, 2018

Member

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)

This comment has been minimized.

@sdaftuar

sdaftuar Jul 9, 2018

Member

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

This comment has been minimized.

@achow101

achow101 Jul 9, 2018

Member

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.

This comment has been minimized.

@MarcoFalke

This comment has been minimized.

@luke-jr

luke-jr Jul 10, 2018

Member

Agree with @sdaftuar

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Jul 9, 2018

utACK 870bd4c

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jul 10, 2018

utACK 870bd4c

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jul 22, 2018

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 DrahtBot closed this Jul 22, 2018

@DrahtBot DrahtBot reopened this Jul 22, 2018

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

@laanwj laanwj added the Feature label Aug 7, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 7, 2018

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

@laanwj laanwj merged commit 870bd4c into bitcoin:master Aug 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 25, 2018

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

870bd4c Update functional RBF test to check replaceable flag (dexX7)
820d31f Add "bip125-replaceable" flag to mempool RPCs (dexX7)

Pull request description:

  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.

Tree-SHA512: 1f5511957af2c20a9a6c79d80a335c3be37a2402dbf829c40cceaa01a24868eab81a9c1cdb0b3d77198fa3bb82799e3540a5c0ce7f35bbac80d73f7133ff7cbc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.