Address comments remaining from #25353

ariard commented at 11:51 pm on July 8, 2022: member

This PR should address the remaining comments from #25353.

in doc/policy/mempool-replacements.md:18 in 6a547cbf8a outdated

14@@ -15,8 +15,8 @@ other consensus and policy rules, each of the following conditions are met:
15 
16    *Rationale*: See [BIP125
17    explanation](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki#motivation).
18-   The Bitcoin Core implementation offers a node setting (`mempoolfullrbf`) to allow transaction
19-   replacement without enforcement of the opt-in signaling rule.
20+   Use the (`mempoolfullrbf`) setting to allow transaction replacement without enforcement of the

jonatack commented at 10:13 am on July 9, 2022:

6a547cb

0   Use the (`-mempoolfullrbf`) configuration option to allow transaction replacement without enforcement of the

in doc/release-notes.md:91 in 6a547cbf8a outdated

86@@ -87,7 +87,7 @@ New settings
87 ------------
88 
89 - A new `mempoolfullrbf` option has been added, which enables the mempool to
90-  accept transaction replacement without enforcing the opt-in replaceability
91+  accept transaction replacement without enforcing BIP125 replaceability
92   signal. (#25353)

jonatack commented at 10:13 am on July 9, 2022:

6a547cbf8a10ac057b07d5f143dc40f15f2955d6

0  signaling. (#25353)

sdaftuar commented at 10:23 am on July 9, 2022: member

A opt-out child of a replaceable parent should be from now on seen as non-replaceable.

I think there’s some confusion about why this RPC was implemented this way in the first place. The purpose was to allow a node operator to be able to determine whether a given transaction might be replaceable due to BIP 125 RBF semantics. Since replacing a parent causes all descendants to be removed from the mempool, this logic checks all the unconfirmed ancestors of a transaction to see if they are replaceable.

I understand the confusion this has created for developers who may have understood the BIP to mean that it should be sufficient to conflict with such a non-signaling child transaction and be able to achieve RBF semantics. However, I think that is an oversight in documentation and maybe our release notes for what is intended with this logic, and not a mistake in the logic itself. I think as long as we support BIP 125 RBF, this RPC is doing what is intended (and hopefully we will eventually drop BIP 125 altogether and we can eliminate this sort of confusion!).

in test/functional/feature_rbf.py:667 in 6a547cbf8a outdated

661@@ -662,8 +662,9 @@ def test_no_inherited_signaling(self):
662             fee_rate=Decimal('0.01'),
663         )
664 
665-        # Reports true due to inheritance
666-        assert_equal(True, self.nodes[0].getmempoolentry(optout_child_tx['txid'])['bip125-replaceable'])
667+        # Our RPC code is in sync with our mempool code.
668+        # The opt-out child is reported as non-repleaceable.
669+        assert_equal(False, self.nodes[0].getmempoolentry(optout_child_tx['txid'])['bip125-replaceable'])

jonatack commented at 10:39 am on July 9, 2022:

It looks like the first commit 70d4e2147bda67f6f4b22c1fd323d1993ffd3c7c would change the behavior of several RPCs, like getmempoolentry that was added six years ago in b09b8135ae1, getmempoolinfo, and listtransactions. Whether it is a good idea to break or change user space with API behavior changes here may be a question worth considering.

ariard commented at 10:46 pm on July 11, 2022:

Dropped the commit.

jonatack commented at 11:19 am on July 9, 2022: contributor

Some feedback

MarcoFalke commented at 7:52 am on July 11, 2022: member

NACK. A bool has two states, and the confusion around this is based on a misinterpretation of at least three states (tx can be replaced “by itself”, tx can be replaced “via parent”, tx can’t be bip125 replaced). Toggling the bool doesn’t help anything in fixing the underlying misunderstanding. If you really want to fix this you may offer the consumer a way to determine the state. I think it should be sufficient to just return the two boolean values SignalsOptInRBF and IsRBFOptIn, alternatively you can return a new field (something like #16490).

glozow commented at 8:19 am on July 11, 2022: member

@ariard Perhaps consider limiting the scope of this PR to “Address comments remaining from #25353” and open a different PR/issue for “fix getmempoolentry inaccurate inheritance signaling status” ?

Address comments remaining from #25353 1056bbdfcd

ariard renamed this:
~~Follow-ups #25353 + fix `getmempoolentry` inaccurate inheritance signaling status~~
Follow-ups #25353
on Jul 11, 2022

ariard commented at 11:07 pm on July 11, 2022: member

Updated at 1056bbdfc with comments addressed and dropped the fix getmempoolentry commit.

So from reviewing back #22698, it wasn’t clear to me what was the state of the thinking on fixing the discrepancy between the BIP 125 documentation and our replacement logic. Effectively, if a non-signaling child transaction marked as “bip125-replaceable” should be interpreted as “tx can be replaced via parent’, I agree it can be an acceptable-though-non-perfect result. It might still confuses developers attempting to replace the non-signaling child via an assumed “inherited signaling” and I believe it would be good to update or refresh BIP125 on that regard (though I don’t plan to do so). It should be noted “inherited signaling” is implemented by other implementation so I think any update to BIP125 might require coordination beyond the scope of the project. As suggested, we could enrich getmempoolentry with a new field on that subtlety but I don’t have an interest there.

ariard force-pushed on Jul 11, 2022

ariard renamed this:
~~Follow-ups #25353~~
Address comments remaining from #25353
on Jul 11, 2022

in test/functional/feature_rbf.py:714 in 1056bbdfcd

713         # Create an explicitly opt-out transaction
714         optout_tx = self.wallet.send_self_transfer(
715             from_node=self.nodes[0],
716             utxo_to_spend=confirmed_utxo,
717-            sequence=SEQUENCE_FINAL,
718+            sequence=BIP125_SEQUENCE_NUMBER + 1,

MarcoFalke commented at 5:46 am on July 12, 2022:

Is there an explanation why the arithmetic is easier to understand than a named constant?

glozow commented at 7:07 am on July 12, 2022:

Not extremely opinionated about this, but I interpret SEQUENCE_FINAL as primarily for disabling locktime, and the fact that it doesn’t signal rbf is a side effect. bip125signal + 1 literally means no signal. But fine with whatever is conventionally used / easiest to understand.

MarcoFalke commented at 7:19 am on July 12, 2022:

Ah right. I wonder why BIP125_SEQUENCE_NUMBER isn’t called MAX_BIP125_RBF_SEQUENCE?

MarcoFalke commented at 7:20 am on July 12, 2022: member

cr ACK 1056bbdfcd40b6bc4e16844c6da5cf666e87b1bc

MarcoFalke assigned glozow on Jul 12, 2022

glozow commented at 8:24 am on July 12, 2022: member

ACK 1056bbdfcd40b6bc4e16844c6da5cf666e87b1bc

Thanks for splitting the PRs. Confirmed this resolves:

CI error in wallet_encryption.py is unrelated, since this only touches feature_rbf.py and docs.

glozow requested review from jonatack on Jul 12, 2022

w0xlt approved

w0xlt commented at 1:38 pm on July 12, 2022: contributor

cr ACK https://github.com/bitcoin/bitcoin/pull/25575/commits/1056bbdfcd40b6bc4e16844c6da5cf666e87b1bc

glozow merged this on Jul 12, 2022

glozow closed this on Jul 12, 2022

MarcoFalke referenced this in commit 31c6309cc6 on Jul 13, 2022

fanquake referenced this in commit c5f0cbefa3 on Aug 22, 2022

bitcoin locked this on Jul 13, 2023

Address comments remaining from #25353 #25575