policy/rbf: don't return "incorrect" replaceability status

darosior commented at 4:13 pm on August 8, 2021: member

We were still reporting transactions that inherited signaling as being replaceable in IsRBFOptIn. In addition, the second commits does a bunch of refactoring by getting rid of IsRBFOptIn entirely and removing the check for RBF from the node interface since we actually don’t need access to the mempool.

Related to #22209, kinda followup of #21946.

DrahtBot added the label Build system on Aug 8, 2021

DrahtBot added the label interfaces on Aug 8, 2021

DrahtBot added the label RPC/REST/ZMQ on Aug 8, 2021

DrahtBot added the label TX fees and policy on Aug 8, 2021

DrahtBot added the label Utils/log/libs on Aug 8, 2021

DrahtBot added the label Wallet on Aug 8, 2021

in src/wallet/rpcwallet.cpp:1393 in eba9666122 outdated

1389@@ -1397,7 +1390,7 @@ static const std::vector<RPCResult> TransactionDescriptionString()
1390            {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."},
1391            {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."},
1392            {RPCResult::Type::STR, "comment", "If a comment is associated with the transaction, only present if not empty."},
1393-           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") Whether this transaction could be replaced due to BIP125 (replace-by-fee);\n"
1394+           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no\") Whether this transaction could be replaced due to BIP125 (replace-by-fee);\n"

mjdietzx commented at 6:41 pm on August 8, 2021:

Note: because bip125-replaceable can no longer be “unknown”, we can probably improve the api here in a later PR by changing the type of this field from STR to BOOL

mjdietzx commented at 7:43 pm on August 8, 2021:

On second thought, I think we should change the type of this ret val in this PR. Because the underlying behavior changes introduced warrant a deprecation process imo, and changing the type here will have the added benefit of forcing any applications/users affected by this change to at least look at it (instead of having something change beneath their feet w out knowing it)

darosior commented at 8:35 am on August 9, 2021:

It could be improved, but would be a breaking change. I’d like to keep this PR only a bugfix and not break the API there.

mjdietzx commented at 7:18 pm on August 8, 2021: contributor

I like the simplification this PR provides and think it’s nicely done. But, my main concern is that existing applications/users might currently rely on bip125-replaceable for this type of behavior:

0// Applications relying on first-seen mempool behavior should
1// check all unconfirmed ancestors; otherwise an opt-in ancestor
2// might be replaced, causing removal of this descendant.

which after this PR won’t be the case.

Based on the description of bip125-replaceable:

0Whether this transaction could be replaced due to BIP125 (replace-by-fee)

even though the transaction can’t be directly replaced in the mempool, it can effectively be replaced by replacing one of its ancestors that does explicitly signal RBF.

What are your thoughts on this? I’m just wondering what direction this PR moves us in long-term as I saw some good discussion/ideas from @ariard and @ajtowns in #21946

DrahtBot commented at 11:14 pm on August 8, 2021: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22798 (doc: Fix RPC result documentation by MarcoFalke)
#22796 (RBF move (1/3): extract BIP125 Rule 5 into policy/rbf by glozow)
#22788 (scripted-diff: Use generate* from TestFramework by MarcoFalke)
#22698 (Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status by mjdietzx)
#22689 (rpc: deprecate top-level fee fields in getmempool RPCs by josibake)
#22677 ([RFC] cut the validation <-> txmempool circular dependency by glozow)
#22675 (MOVEONLY policy: extract RBF logic into policy/rbf by glozow)
#22650 (Remove -deprecatedrpc=addresses flag and corresponding code/logic by mjdietzx)
#22290 (Package Mempool Submission with Package Fee-Bumping by glozow)
#21260 (wallet: indicate whether a transaction is in the mempool by danben)
#21245 (rpc: Add level 3 verbosity to getblock RPC call. by fyquah)
#19792 (rpc: Add dumpcoinstats by fjahr)

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.

darosior commented at 8:32 am on August 9, 2021: member

my main concern is that existing applications/users might currently rely on bip125-replaceable for this type of behavior

If they rely on this, then they are already broken (as demonstrated by CVE-2021-31876). This is the point of this PR actually, and i would argue it is actually a bugfix.

meshcollider removed the label Build system on Aug 11, 2021

meshcollider removed the label RPC/REST/ZMQ on Aug 11, 2021

meshcollider removed the label Utils/log/libs on Aug 11, 2021

meshcollider removed the label interfaces on Aug 11, 2021

mjdietzx commented at 3:38 am on August 11, 2021: contributor

If they rely on this, then they are already broken (as demonstrated by CVE-2021-31876). This is the point of this PR actually, and i would argue it is actually a bugfix.

I’m specifically referring to this comment in MemPoolAccept::PreChecks:

// Applications relying on first-seen mempool behavior should // check all unconfirmed ancestors; otherwise an opt-in ancestor // might be replaced, causing removal of this descendant.

Before your PR, applications could do this check by just reading the result of {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") because isRBFOptIn went through all the transaction’s ancestors and checked if they are opt-in.

But after your PR, this is no longer the case. I see that your PR fixes the bug from the perspective of “I know I can replace this transaction based on the value of bip125-replaceable, and it wont get unexpectedly rejected”. But I do see it as a change in behavior from the perspective of “I’m an application that relies on first-seen mempool behavior, and I’m looking at the value of bip125-replaceable to know if this transaction might be removed due to an opt-in unconfirmed parent transaction being replaced somewhere up the ancestry tree”

in src/node/interfaces.cpp:42 in eba9666122 outdated

38@@ -40,6 +39,7 @@
39 #include <uint256.h>
40 #include <univalue.h>
41 #include <util/check.h>
42+#include <util/rbf.h>

glozow commented at 1:00 pm on August 11, 2021:

Don’t think this needs to be included?

glozow commented at 1:05 pm on August 11, 2021: member

Concept ACK. Technically, we are returning the correct BIP125 replaceability of a transaction, but it’s not useful for a client to get this information since it doesn’t actually reflect what would happen in their mempool. I also agree with the cleanup of RBFTransactionState and other no-longer-used code.

I think it would be better to add a new RPC field “replaceable” (since it’s not really BIP125) to return this as a bool result, and remove “bip125-replaceable” or make it an alias of “replaceable” and remove it later.

Talkless commented at 2:16 pm on August 11, 2021: none

Is this commit message: since we don't need the mempool anymore since we don't need the mempool anymore.. OK..?

in src/wallet/rpcwallet.cpp:175 in eba9666122 outdated

178-        if (rbfState == RBFTransactionState::UNKNOWN)
179-            rbfStatus = "unknown";
180-        else if (rbfState == RBFTransactionState::REPLACEABLE_BIP125)
181-            rbfStatus = "yes";
182-    }
183+    std::string rbfStatus = confirms <= 0 && SignalsOptInRBF(*wtx.tx) ? "yes" : "no";

Talkless commented at 2:23 pm on August 11, 2021:

nit: rbfStatus now can be const due to PR changes.

darosior force-pushed on Aug 20, 2021

darosior commented at 9:44 am on August 20, 2021: member

I think it would be better to add a new RPC field “replaceable” (since it’s not really BIP125) to return this as a bool result

Good call. Done in a50a332921c74a0f1fc51b097f8b5beca09a1b07.

Also applied your and @Talkless ’s comments.

in doc/release-notes.md:70 in a50a332921 outdated

63@@ -64,6 +64,11 @@ P2P and network changes
64 Updated RPCs
65 ------------
66 
67+- The `bip125_replaceable` field output by mempool and wallet RPC commands was
68+  misleading (see CVE-2021-31876). It was deprecated in favour of a `replaceable`
69+  boolean field which does not check for the inheritance rule defined by BIP125,
70+  sticking to what is currently enforced by the mempool.

glozow commented at 10:37 am on August 20, 2021:

Include PR number?

0  sticking to what is currently enforced by the mempool. (#22665)

in src/wallet/rpcwallet.cpp:1401 in a50a332921 outdated

1395@@ -1399,8 +1396,9 @@ static const std::vector<RPCResult> TransactionDescriptionString()
1396            {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."},
1397            {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."},
1398            {RPCResult::Type::STR, "comment", "If a comment is associated with the transaction, only present if not empty."},
1399-           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") Whether this transaction could be replaced due to BIP125 (replace-by-fee);\n"
1400-               "may be unknown for unconfirmed transactions not in the mempool"}};
1401+           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no\") Whether this transaction could be replaced due to BIP125 (replace-by-fee) (DEPRECATED, use 'replaceable' field instead. Returned only if config option 'deprecatedrpc=bip125' is set);\n"
1402+               "may be unknown for unconfirmed transactions not in the mempool"},
1403+           {RPCResult::Type::BOOL, "replaceable", "Whether this transaction could be replaced by a conflicting one"}};

glozow commented at 10:40 am on August 20, 2021:

0           {RPCResult::Type::BOOL, "replaceable", "Whether this transaction could be replaced by a conflicting one using replace-by-fee"}};

in src/wallet/rpcwallet.cpp:179 in a50a332921 outdated

181-            rbfStatus = "unknown";
182-        else if (rbfState == RBFTransactionState::REPLACEABLE_BIP125)
183-            rbfStatus = "yes";
184+    const bool signals_rbf = confirms <= 0 && SignalsOptInRBF(*wtx.tx);
185+    entry.pushKV("replaceable", signals_rbf);
186+    if (IsDeprecatedRPCEnabled("bip125")) {

glozow commented at 10:42 am on August 20, 2021:

nit in a50a332921: perhaps calling it bip125-signaling would be clearer

darosior commented at 10:15 pm on August 21, 2021:

I prefer bip125, it’s shorter and i think clear enough: bip125 is only about signaling (it’s even in the title) so it seems redundant

glozow commented at 11:49 am on August 23, 2021:

Fair!

glozow commented at 11:01 am on August 20, 2021: member

Approach ACK

have a few tiny nits :sweat_smile: could also use a functional test for the deprecation behavior, e.g. something like this

glozow added the label RPC/REST/ZMQ on Aug 20, 2021

in src/wallet/rpcwallet.cpp:1400 in a50a332921 outdated

1395@@ -1399,8 +1396,9 @@ static const std::vector<RPCResult> TransactionDescriptionString()
1396            {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."},
1397            {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."},
1398            {RPCResult::Type::STR, "comment", "If a comment is associated with the transaction, only present if not empty."},
1399-           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") Whether this transaction could be replaced due to BIP125 (replace-by-fee);\n"
1400-               "may be unknown for unconfirmed transactions not in the mempool"}};
1401+           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no\") Whether this transaction could be replaced due to BIP125 (replace-by-fee) (DEPRECATED, use 'replaceable' field instead. Returned only if config option 'deprecatedrpc=bip125' is set);\n"
1402+               "may be unknown for unconfirmed transactions not in the mempool"},

MarcoFalke commented at 1:21 pm on August 20, 2021:

This is no longer true?!

darosior commented at 10:15 pm on August 21, 2021:

Wow, thanks. Should have paid more attention to the other part of the help line..

MarcoFalke commented at 1:22 pm on August 20, 2021: member

Funny that this bugfix removes more than 100 lines of code

darosior force-pushed on Aug 21, 2021

darosior commented at 10:15 pm on August 21, 2021: member

@glozow thanks for the test, included it. Also addressed (2 of) your nits.

in src/rpc/blockchain.cpp:480 in aa865b2160 outdated

476@@ -477,7 +477,8 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
477         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "parent transaction id"}}},
478     RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction",
479         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}},
480-    RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee)"},
481+    RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee) (DEPRECATED, use 'replaceable' field instead. Returned only if config option 'deprecatedrpc=bip125' is set)"},

MarcoFalke commented at 7:19 am on August 22, 2021:

0    RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Identical to 'replacable' field. Does *not* return the BIP125 (replace-by-fee) status (DEPRECATED. Returned only if config option 'deprecatedrpc=bip125' is set)"},

the field does not return the bip125 status, so at least the docs should say that (or maybe the field be removed completely)

darosior commented at 8:33 am on August 22, 2021:

Removed the mention in the doc, i think we should still keep the deprecation period.

 0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 1index 337a10294b..18ab787f46 100644
 2--- a/src/rpc/blockchain.cpp
 3+++ b/src/rpc/blockchain.cpp
 4@@ -477,7 +477,7 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
 5         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "parent transaction id"}}},
 6     RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction",
 7         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}},
 8-    RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee) (DEPRECATED, use 'replaceable' field instead. Returned only if config option 'deprecatedrpc=bip125' is set)"},
 9+    RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to replace-by-fee (DEPRECATED, use 'replaceable' field instead. Returned only if config option 'deprecatedrpc=bip125' is set)"},
10     RPCResult{RPCResult::Type::BOOL, "replaceable", "Whether this transaction could be replaced by a conflicting one using replace-by-fee"},
11     RPCResult{RPCResult::Type::BOOL, "unbroadcast", "Whether this transaction is currently unbroadcast (initial broadcast not yet acknowledged by any peers)"},
12 };}
13diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
14index 8339ff63d9..b66a77ea3d 100644
15--- a/src/wallet/rpcwallet.cpp
16+++ b/src/wallet/rpcwallet.cpp
17@@ -1396,7 +1396,7 @@ static const std::vector<RPCResult> TransactionDescriptionString()
18            {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."},
19            {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."},
20            {RPCResult::Type::STR, "comment", "If a comment is associated with the transaction, only present if not empty."},
21-           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no\") Whether this transaction could be replaced due to BIP125 (replace-by-fee) (DEPRECATED, use 'replaceable' field instead. Returned only if config option 'deprecatedrpc=bip125' is set)"},
22+           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no\") Whether this transaction could be replaced due to replace-by-fee (DEPRECATED, use 'replaceable' field instead. Returned only if config option 'deprecatedrpc=bip125' is set)"},
23            {RPCResult::Type::BOOL, "replaceable", "Whether this transaction could be replaced by a conflicting one using replace-by-fee"}};
24 }

glozow commented at 11:49 am on August 23, 2021:

Is it worth keeping bip125-replaceable returning the correct bip125 status for 1 release and then removing all the logic later?

darosior commented at 11:51 am on August 23, 2021:

I think we should get rid of the misleading logic as soon as possible.

darosior force-pushed on Aug 22, 2021

in src/policy/rbf.h:17 in 2bb244ea95 outdated

 7-#include <txmempool.h>
 8-
 9-/** The rbf state of unconfirmed transactions */
10-enum class RBFTransactionState {
11-    /** Unconfirmed tx that does not signal rbf and is not in the mempool */
12-    UNKNOWN,

glozow commented at 11:33 am on August 25, 2021:

Nit: it would be nice if e4c04c0409cd734d5b02997cb190e6c383d39f11 were split into 2 commits (1) eliminate “unknown” and (2) refactor to remove RBFTransactionState.

darosior commented at 2:02 pm on August 26, 2021:

I tried to do that in the first place but it’s messy and makes both commits non-atomics :/

glozow commented at 11:43 am on August 25, 2021: member

ACK 2bb244ea95ceeb0b955ca015291fddb1d4ce96d9

IIUC, with this change, replaceability status returned for transactions is changing from yes/no/unknown -> True/False. Some transactions will go from yes -> no (i.e. 87dc011a95d00038fd5c69655a2518e324c94d05). All the transactions that are unknown -> no (e4c04c0409cd734d5b02997cb190e6c383d39f11).

The deprecation approach looks good to me - everyone using bip125-replaceable right now will be notified of the change, and switching to using replaceable means some incorrectly labeled “yes” transactions will now be correctly labeled “no,” and nothing is “unknown” now.

in src/rpc/blockchain.cpp:480 in 2bb244ea95 outdated

476@@ -477,7 +477,8 @@ static std::vector<RPCResult> MempoolEntryDescription() { return {
477         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "parent transaction id"}}},
478     RPCResult{RPCResult::Type::ARR, "spentby", "unconfirmed transactions spending outputs from this transaction",
479         {RPCResult{RPCResult::Type::STR_HEX, "transactionid", "child transaction id"}}},
480-    RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to BIP125 (replace-by-fee)"},
481+    RPCResult{RPCResult::Type::BOOL, "bip125-replaceable", "Whether this transaction could be replaced due to replace-by-fee (DEPRECATED, use 'replaceable' field instead. Returned only if config option 'deprecatedrpc=bip125' is set)"},

MarcoFalke commented at 12:20 pm on August 25, 2021:

I’d still prefer if this said that the two fields return exactly the same info. This will hopefully encourage users to use the non-deprecated without using deprecatedrpc at all.

Also, it will avoid confusion as to what the difference is between the two fields.

rajarshimaitra commented at 1:38 pm on August 25, 2021: contributor

Code changes looks good and clear. But I am unsure about the approach.

Whats the rationale for fixing the RPC output to match the current code behavior instead of fixing the behavior discrepancy with BIP125 in the first place? If Core doesn’t follow BIP125, I think that can cause downstream trouble for wallets (especially LN) who rely on core’s result on transaction replacements.

Also, with this PR, a transaction not signalling RBF, but having one of its ancestor signal for RBF, will be removed from mempool, while at the same time a RBF status query will return “false”. This seems confusing to me.

The original logic of BIP25: “if any ancestor is replaceable, descendant is implicitly replaceable” sounds logical to me.

This also breaks compatibility between core and other software who follows BIP125. The purpose of having a standard is that everybody can follow same behavior. So why not just fix the behaviour instead of trying to fix the reporting?

mjdietzx commented at 3:52 pm on August 25, 2021: contributor

+1 to @rajarshimaitra - I think this approach has merit, but wouldn’t we want to update the BIP (and have a review/discussion there) before we make this implementation change (which in my view cements Bitcoin core’s current behavior that is inconsistent with BIP 125)? To me it seems we need to establish if RBF inherited signaling is a worthwhile feature - if it’s not why wouldn’t we update the BIP accordingly first? But isn’t there a discussion to be had before we just go ahead and do this? If it’s an unnecessary feature, great, this PR simplifies the code and has good performance guarantees.

Keep in mind, we also have the option of supporting inherited signaling and making bitcoin core adhere to BIP 125 exactly, which I do in #22698

jnewbery commented at 1:43 pm on August 26, 2021: member

The purpose of having a standard is that everybody can follow same behavior. So why not just fix the behaviour instead of trying to fix the reporting? @rajarshimaitra - as discussed in PR review club yesterday (https://bitcoincore.reviews/22665), BIP125 is not a “standard”. It was intended to document the implemented behaviour in Bitcoin Core, but that behaviour was misdocumented and review didn’t catch the mistake.

MarcoFalke commented at 1:58 pm on August 26, 2021: member

Keep in mind, we also have the option of supporting inherited signaling and making bitcoin core adhere to BIP 125 exactly, which I do in #22698

I think long term there is no prospect that BIP 125 will be adhered to exactly, so I think removing the replacable-125 field (replacing it with a replaceable field) is the right move regardless.

Longer term we can hopefully do full-rbf or a full-rbf after timeout option, with or without #22698.

MarcoFalke commented at 1:59 pm on August 26, 2021: member

nit: Maybe remove the 125 line from https://github.com/bitcoin/bitcoin/blob/master/doc/bips.md in this pr?

darosior force-pushed on Aug 26, 2021

darosior commented at 2:06 pm on August 26, 2021: member

Addressed the documentation comment to say that the two fields return exactly the same info, and removed BIP125 from the list of implemented BIPs.

in src/test/fuzz/rbf.cpp:5 in 2280b31042 outdated

1@@ -2,13 +2,13 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5-#include <policy/rbf.h>

jnewbery commented at 2:28 pm on August 26, 2021:

lots more unused includes here:

 0diff --git a/src/test/fuzz/rbf.cpp b/src/test/fuzz/rbf.cpp
 1index 7c5d9bb95d..dcc1d23b8a 100644
 2--- a/src/test/fuzz/rbf.cpp
 3+++ b/src/test/fuzz/rbf.cpp
 4@@ -3,17 +3,11 @@
 5 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 6 
 7 #include <primitives/transaction.h>
 8-#include <sync.h>
 9 #include <test/fuzz/FuzzedDataProvider.h>
10-#include <test/fuzz/fuzz.h>
11 #include <test/fuzz/util.h>
12-#include <txmempool.h>
13 #include <util/rbf.h>
14 
15-#include <cstdint>
16 #include <optional>
17-#include <string>
18-#include <vector>
19 
20 FUZZ_TARGET(rbf)
21 {

MarcoFalke commented at 2:47 pm on August 26, 2021:

#include <test/fuzz/fuzz.h> is needed for FUZZ_TARGET

jnewbery commented at 2:52 pm on August 26, 2021:

ah oops. Thanks @MarcoFalke.

in src/wallet/rpcwallet.cpp:1399 in 2280b31042 outdated

1395@@ -1399,8 +1396,8 @@ static const std::vector<RPCResult> TransactionDescriptionString()
1396            {RPCResult::Type::NUM_TIME, "time", "The transaction time expressed in " + UNIX_EPOCH_TIME + "."},
1397            {RPCResult::Type::NUM_TIME, "timereceived", "The time received expressed in " + UNIX_EPOCH_TIME + "."},
1398            {RPCResult::Type::STR, "comment", "If a comment is associated with the transaction, only present if not empty."},
1399-           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no|unknown\") Whether this transaction could be replaced due to BIP125 (replace-by-fee);\n"
1400-               "may be unknown for unconfirmed transactions not in the mempool"}};
1401+           {RPCResult::Type::STR, "bip125-replaceable", "(\"yes|no\") Whether this transaction could be replaced due to replace-by-fee (DEPRECATED, returns the same value as 'replaceable'. Use this field instead. Returned only if config option 'deprecatedrpc=bip125' is set)"},

jnewbery commented at 2:31 pm on August 26, 2021:

returns the same value as ‘replaceable’

This isn’t true. bip125-replaceable returns “yes”/“no”. replaceable returns “true”/“false”.

in doc/bips.md:35 in 2280b31042 outdated

31@@ -32,7 +32,6 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v22.0**):
32 * [`BIP 111`](https://github.com/bitcoin/bips/blob/master/bip-0111.mediawiki): `NODE_BLOOM` service bit added, and enforced for all peer versions as of **v0.13.0** ([PR #6579](https://github.com/bitcoin/bitcoin/pull/6579) and [PR #6641](https://github.com/bitcoin/bitcoin/pull/6641)).
33 * [`BIP 112`](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki): The CHECKSEQUENCEVERIFY opcode has been implemented since **v0.12.1** ([PR #7524](https://github.com/bitcoin/bitcoin/pull/7524)), and has been *buried* since **v0.19.0** ([PR #16060](https://github.com/bitcoin/bitcoin/pull/16060)).
34 * [`BIP 113`](https://github.com/bitcoin/bips/blob/master/bip-0113.mediawiki): Median time past lock-time calculations have been implemented since **v0.12.1** ([PR #6566](https://github.com/bitcoin/bitcoin/pull/6566)), and has been *buried* since **v0.19.0** ([PR #16060](https://github.com/bitcoin/bitcoin/pull/16060)).
35-* [`BIP 125`](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki): Opt-in full replace-by-fee signaling honoured in mempool and mining as of **v0.12.0** ([PR 6871](https://github.com/bitcoin/bitcoin/pull/6871)). Enabled by default in the wallet GUI as of **v0.18.1** ([PR #11605](https://github.com/bitcoin/bitcoin/pull/11605))

jnewbery commented at 2:34 pm on August 26, 2021:

Would it be better to document the difference between the implementation and BIP125, rather than remove this entirely?

MarcoFalke commented at 2:46 pm on August 26, 2021:

Maybe a separate doc/transaction-relay-policy.md would be better suited?

jnewbery commented at 3:25 pm on August 26, 2021:

I think it’d be better to have it outside the main bitcoin core repository, since the review/maintenance of documentation is different from code. https://github.com/bitcoin-core/bitcoin-devwiki/wiki seems like the right place to me.

MarcoFalke commented at 3:38 pm on August 26, 2021:

Policy is versioned, as each major version of Bitcoin Core ships with different policy rules, so it would be good to also see how policy changed over time. Obviously this is also possible in the wiki, but doing it in source code would make it possible to atomically update the docs with the code changes.

glozow commented at 3:51 pm on August 26, 2021:

doing it in source code would make it possible to atomically update the docs with the code changes.

Oh good point :O

jnewbery commented at 4:11 pm on August 26, 2021:

Let’s take the discussion elsewhere. It seems quite tangential to this PR.

MarcoFalke commented at 4:29 pm on August 26, 2021:

#22806

michaelfolkson commented at 2:39 pm on August 26, 2021: contributor

BIP125 is not a “standard”

The BIP type is “Type: Standards Track”. So to say the BIP was not intended as a “standard” is dubious. Removing references to BIP125 in Core makes sense if BIP125 is considered final and Core comes to the collective judgement that not following the wording of the BIP is the optimal path.

If Core doesn’t follow BIP125, I think that can cause downstream trouble for wallets (especially LN) who rely on core’s result on transaction replacements.

Re the optimal path I think @rajarshimaitra’s above consideration is worth discussing. I lean towards #22698 because I think the rationale for inherited signaling is stronger than the rationale for not implementing inherited signaling (ignoring the BIP, code discussion and the downstream project discussion)

intended to document the implemented behaviour in Bitcoin Core

Even if this was the intention at the time, the Core documentation and Core repo should document the behavior of Bitcoin Core. The act of writing and formalizing a BIP (at least to me) suggests that a broader discussion and decisions were required beyond merely documenting what a Core PR does. Especially today with not only alternative Bitcoin implementations but higher layer implementations relying on decisions made I think it is important to explain the rationale behind these decisions rather than just say “You should have ignored what the BIP says and just looked at the code in the PR”.

This is a tricky one though.

michaelfolkson commented at 3:03 pm on August 26, 2021: contributor

If Core doesn’t follow BIP125, I think that can cause downstream trouble for wallets (especially LN) who rely on core’s result on transaction replacements.

Perhaps worth informing the bitcoin-dev mailing list before this or #22698 is merged? Either Core isn’t following the BIP (this PR) or the behavior within Core is going to change (#22698).

Interesting observation from @pinheadmz on IRC that not only did he identify this problem in 2019 but at least on Bcoin he followed what was in the Core code rather than the BIP.

MarcoFalke commented at 3:16 pm on August 26, 2021: member

Perhaps worth informing the bitcoin-dev mailing list

This was already done as part of announcing the CVE

michaelfolkson commented at 3:24 pm on August 26, 2021: contributor

This was already done as part of announcing the CVE

The approach that Core would be taking to address the discrepancy between the BIP and the Core implementation surely wasn’t?

michaelfolkson commented at 4:54 pm on August 26, 2021: contributor

I don’t think this is tricky at all.

Under the assumption that Core will maintain its current mempool behavior (versus changing it to meet the BIP wording because that is deemed superior logic) I agree it isn’t tricky. But making that assumption in the first place seems tricky to me. A short term stopgap with minimal code changes in anticipation of full RBF or a more invasive code change for what some deem superior logic in the meantime as full RBF might never land. (I think that’s where we are anyway). Let’s at least be transparent about that rather than glossing over it.

MarcoFalke commented at 6:06 pm on August 26, 2021: member

Fixes #22209, kinda followup of #21946.

Does it really fix #22209? The new replacable field still doesn’t accurately reflect whether a transaction can be replaced by a higher fee tx. See #22809. Both fields (the newly added one and the removed one in this pull) are needed to say that. And for users to interpret the fields for practical purposes, they will also need to know who controls the keys that were used to sign the inputs of the transactions.

darosior commented at 11:28 am on August 27, 2021: member

Does it really fix #22209? The new replacable field still doesn’t accurately reflect whether a transaction can be replaced by a higher fee tx.

The replaceable field does reflect whether a transaction signals for RBF. Maybe it’d be wiser that I update the documentation to explicitly state it, rather than maybe implying (i don’t parse it like that but maybe it could be confused as such, idk) that !replaceable is an assurance that “your transaction won’t be evicted not matter what” which we of course can’t provide. However i’m curious what should be done to fix #22209. But happy to remove the link from the OP (updated it).

Both fields (the newly added one and the removed one in this pull) are needed to say that

The newly added one is a subset of the removed one here, so i’m not sure to get how having both would help.

MarcoFalke commented at 11:55 am on August 27, 2021: member

!replaceable is an assurance that “your transaction won’t be evicted not matter what” which we of course can’t provide.

I know that the new field doesn’t do that. My point was that with the old field, you could see !bip125-replaceable and deduce that with the state of the blockchain at the time of the call and the policy rules of your node, the tx won’t be replaced by another transaction with a higher fee in the mempool. (Obviously that doesn’t mean you or your wallet should trust such a tx, but this discussion seems unrelated to returning the information for the raw mempool).

MarcoFalke commented at 12:00 pm on August 27, 2021: member

However i’m curious what should be done to fix #22209.

I have no idea what is needed to fix the issue. Maybe the reason should be reported as an enum? #16490?

The states could be:

opt-in-signal
rbf-inheritance
(full-rbf)
(timeout-rbf)
…

The ones in braces are for not-(yet) Bitcoin Core policies.

darosior force-pushed on Aug 29, 2021

darosior commented at 12:40 pm on August 29, 2021: member

Fixed the bad english in the bip125-replaceable doc and removed needless includes in the fuzz target after @jnewbery ’s review. Thanks!

michaelfolkson commented at 10:23 am on August 30, 2021: contributor

I think the discussion on the Approach (N)ACK part of this PR and comparing the approaches of #22665 versus #22698 has been weak and almost dismissive (the BIP isn’t a standard etc). In my view @mjdietzx @rajarshimaitra have a valid perspective including in @mjdietzx’s case coding up and opening an alternative PR. @laanwj said on IRC:

There are no other mempool policy implementations in wide use

There’s a lot of inertia involved in trying to change the implementation, if something gets merged it will take a long time before that version is widely spread on the network to be behavior other software can rely on

From the above I’m an Approach ACK on this PR meeting shorter term user needs and think a rebased #22698 can/should be considered post the merging of this PR. But my gosh, unless we want to ditch the Concept ACK, Approach ACK part of the PR review process I’d hope we can do better in these kinds of scenarios where someone presents a competing PR….

michaelfolkson commented at 10:36 am on August 30, 2021: contributor

Great comment too earlier from @glozow on this PR (which was deleted but I don’t think it should have been)

I don’t think this is tricky at all. Not thinking about the BIP while reviewing this PR:

In our mempool code, a transaction that doesn’t signal RBF is not opting in. IsRBFOptIn returns a result that is inconsistent with what our mempool will do. Since the RPC code uses IsRBFOptIn(), it returns bip125-replaceable as “yes” or “unknown” for mempool transactions that are not actually RBFable. This can be very confusing and frustrating for users. We should not just change the result of bip125-replaceable, as this can also be confusing. So this PR adds a new field, replaceable, which tells users whether or not our mempool code thinks this transaction opts in to RBF.

DrahtBot added the label Needs rebase on Aug 31, 2021

rbf: don't report txs that don't explicitly signal replaceability

As disclosed by CVE-2021-31876, the mempool acceptance logic doesn't
implement BIP125's inheritance rule.
However, we were still mistakenly reporting transactions that don't
explicitly signal for RBF as being replaceable.

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

d425bc006f

rbf: remove RBFTransactionState enum

The enum was only needed for the "unknown" RBF state of a transaction
when we can't fetch its ancestors. Since we don't actually implement
inherited RBF signaling, the replaceability of a transaction is only a
function of itself and cannot be unknown.

This also does a bit of cleanup, as there is no need for the RBF check
to be part of the node interface since we don't need the mempool anymore..

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

ca54223954

rpc: move IsDeprecatedRPCEnabled to util/

To avoid linking against server. For example, using it to deprecate an
RPC in wallet/rpcwallet would require libbitcoin_wallet to link against
rpc/server and all its dependencies.

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

27103f577d

rpc: deprecate 'bip125-replaceable' in favour of 'replaceable'

This doesn't need to be a string anymore. And mentioning BIP125 is
confusing, as we technically don't return the replaceability as defined
by BIP125 (which includes the inheritance rule).

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

26c7fc0c38

[functional test] deprecated bip125-replaceable field 0220b8275f

doc: remove BIP125 from the list of implemented BIPs

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>

cc091fa2e2

darosior force-pushed on Sep 1, 2021

darosior commented at 7:30 am on September 1, 2021: member

Rebased after #22796 merge.

DrahtBot removed the label Needs rebase on Sep 1, 2021

in src/wallet/rpcwallet.cpp:179 in 26c7fc0c38 outdated

173@@ -174,8 +174,12 @@ static void WalletTxToJSON(interfaces::Chain& chain, const CWalletTx& wtx, UniVa
174     entry.pushKV("timereceived", (int64_t)wtx.nTimeReceived);
175 
176     // Add opt-in RBF status
177-    const std::string rbfStatus = confirms <= 0 && SignalsOptInRBF(*wtx.tx) ? "yes" : "no";
178-    entry.pushKV("bip125-replaceable", rbfStatus);
179+    const bool signals_rbf = confirms <= 0 && SignalsOptInRBF(*wtx.tx);
180+    entry.pushKV("replaceable", signals_rbf);
181+    if (IsDeprecatedRPCEnabled("bip125")) {

glozow commented at 12:07 pm on September 1, 2021:

In 26c7fc0c38 rpc: deprecate ‘bip125-replaceable’ in favour of ‘replaceable’

Could you grab this via the Chain interface, i.e. call chain.rpcEnableDeprecated("bip125") instead of moving IsDeprecatedRPCEnabled() to util? Or is it better to do it this way and take the opportunity to remove rpcEnableDeprecated by changing this one too? (maybe scope creepish, sorry)

glozow commented at 12:27 pm on September 1, 2021: member

ACK cc091fa

My assumption was that this RPC result would be used to see if the inputs can be used in an RBF transaction (I don’t think anything can be expected to stay in the mempool)? And since a transaction can always be evicted due to higher fee transactions squeezing it out of the mempool, I thought of replaceable as “does this transaction signal for rbf” rather than “can this transaction be replaced” which is always yes.

I can also see how replaceable could naturally be defined as “can this transaction be evicted due to RBF,” so maybe calling the result “signals-rbf” would be more accurate. @MarcoFalke’s suggestion of an enum of reasons makes sense to me too.

MarcoFalke commented at 2:28 pm on September 1, 2021: member

I think there is a difference between “This tx can be evicted due to mempool size if there are 300 MB worth of txs on top of it” and “This tx can be replaced by a single tx that pays a higher fee than the tx(s) it evicts”.

I think it makes sense to distinguish the mempool removal reasons, as it is done internally in the code as well:

0enum class MemPoolRemovalReason {
1    EXPIRY,      //!< Expired from mempool
2    SIZELIMIT,   //!< Removed in size limiting
3    REORG,       //!< Removed for reorganization
4    BLOCK,       //!< Removed for block
5    CONFLICT,    //!< Removed for conflict with in-block transaction
6    REPLACED,    //!< Removed for replacement
7};

NACK from me on the changes here.

darosior commented at 2:35 pm on September 1, 2021: member

Sigh. Removal reason has nothing to do with this PR.

I have no intention to continue tweaking the RPC code nor keeping up with your nitpicks, i initially picked this up because the current interface is wrong and misleading hence potentially dangerous for downstream users. But as you wish, i guess.

darosior closed this on Sep 1, 2021

darosior commented at 2:40 pm on September 1, 2021: member

Thanks to everyone who helped moving this forward. This is up for grabs, i guess.

MarcoFalke commented at 2:59 pm on September 1, 2021: member

Sigh. Removal reason has nothing to do with this PR.

This was a reply to the previous comment (https://github.com/bitcoin/bitcoin/pull/22665#pullrequestreview-743825789), which mentioned that txs can be evicted from the mempool for reaching the size limit.

But as you wish, i guess.

I hope you didn’t close this pull request because I left a comment with a concern (which is not a nitpick). I want to apologise for leaving nitpicks before reviewing the concept of the pull.

the current interface is wrong and misleading

I agree. Though, I also think that the interface proposed in this pull request isn’t crystal clear about the fee replacability status of the tx, which is why I raised the concern.

darosior commented at 12:17 pm on September 2, 2021: member

@MarcoFalke sorry for overreacting.

I also think that the interface proposed in this pull request isn’t crystal clear about the fee replacability status of the tx

Ok, then we could go for the minimal fix (which i initially proposed, to stop returning "yes" or "unknown" if the transaction is not replaceable), and introduce the new interface in a following PR?

Regarding the interface, you proposed:

I have no idea what is needed to fix the issue. Maybe the reason should be reported as an enum? #16490? The states could be: - opt-in-signal - rbf-inheritance - (full-rbf) - (timeout-rbf) - … The ones in braces are for not-(yet) Bitcoin Core policies.

I think of those only opt-in-signal makes sense to have: reporting that it signals by inheritance while we don’t implement this rule sounds confusing to me. However i understand the future-proofing argument raised in #16490 and we now have the opportunity to introduce it. Maybe we can at least introduce the enum with a single variant for now?

So, how about going first going for the minimal fix here and then proceed with something like #16490 to deprecate bip125-replaceable in favour of a new field more complete about the replaceability status of the transaction?

MarcoFalke commented at 1:17 pm on September 2, 2021: member

reporting that it signals by inheritance while we don’t implement this rule sounds confusing to me.

The rule isn’t implemented fully, but surely is implemented partially in Bitcoin Core. Bitcoin Core simply has the additional requirement that an rbf-signalling parent must be replaced in the same instance.

Not implementing the rule (or rather implementing the inverse of the rule) would mean: A non-signalling child tx could remove the signalling status from a parent, and thus prevent replacement. This is what a plain no for the non-signalling child would imply (this pr), which is my concern.

michaelfolkson commented at 1:02 pm on November 29, 2021: contributor

For the benefit of reviewers this PR was NACKed by @MarcoFalke and it appears that @glozow is now comfortable with the approach taken in #22698 which was earlier seen as a competing PR.

This PR is currently closed but I think it is more important we get the approach right than just blindly picking whichever PR is kept open by its author. However, it does seem that @MarcoFalke, @glozow @mjdietzx @ariard etc are comfortable with the approach taken in #22698.

Thanks for the work on this @darosior, sorry for the frustration you’ve experienced. I’ve certainly wavered on this and seems like others have been unsure too.

bitcoin locked this on Jul 11, 2023

policy/rbf: don’t return “incorrect” replaceability status #22665

Conflicts