p2p: Advertise `NODE_FULL_RBF` and connect to 4 outbound full-rbf peers if `-mempoolfullrbf` is set

ariard commented at 0:03 am on July 13, 2022: member

mempoolfullrbf allows a node to accept replace-by-fee transactions without requiring replaceability signaling. While the replacement transaction is locally accepted, it should be rejected by all your opt-in rbf peers, the default behavior. As such, the odds are low the replacement transaction propagates to miners, unless there are full-rbf transaction-relay paths existent between the node and those miners mempools.

This PR aims to improve this issue by enabling the setting of full-rbf transaction-relay link, in both manual and automated fashions.

The first commit introduces a NODE_REPLACE_BY_FEE service flag to advertise support of full-rbf to our peers if mempoolfullrbf is set to true. From the results of getpeerinfo or getnetworkinfo, a node operator can find full-rbf peers on the p2p network to which to manually connect to. The service flag bit select is the 26th one to be compatible with previous full-rbf experiences.

The second commit introduces a ConnectionType::FULLRBF if mempoolfullrbf is set to true. In ThreadOpenConnections, we attempt to keep opened at least 4 outbound full-rbf transaction-relay peer. An outbound full-rbf relay peer is considered as one signaling NODE_REPLACE_BY_FEE service flag.

I think if we have concerns on the network-wise resources consumption from those new outbound full-rbf relay peers, we could deduce them MAX_ADNNODE_CONNECTIONS. It should be weighted if we have other concerns about the implications on transaction-relay topology robustness or any spying peers flagging themselves as NODE_REPLACE_BY_FEE to attract early full-rbf traffic. What else ?

I don’t hold to the approach re-using the current automatic connection logic in ThreadOpenConnections, if there are others ones I’m interested to consider them. I can split the PR in two, if automatic connection requires more thinking.

TODO:

add functional tests
make MAX_FULLRBF_RELAY_CONNECTIONS configurable up to MAX_ADDNODE_CONNECTIONS ?
make automatic connection its own boolean setting ?

ariard marked this as a draft on Jul 13, 2022

ariard renamed this:
~~Advertise `NODE_REPLACE_BY_FEE` and connect to at least 1 outbound full-rbf peers if `mempoolfullrbf` sets~~
Advertise `NODE_REPLACE_BY_FEE` and connect to 1 outbound full-rbf peer if `mempoolfullrbf` sets
on Jul 13, 2022

ghost commented at 2:18 am on July 13, 2022: none

The first commit introduces a NODE_REPLACE_BY_FEE service flag to advertise support of full-rbf to our peers if #25353 is set to true. From the results of getpeerinfo or getnetworkinfo, a node operator can find full-rbf peers on the p2p network to which to manually connect to. The service flag bit select is the 26th one to be compatible with previous full-rbf experiences.

Isn’t this bad for privacy?

DrahtBot commented at 6:03 am on July 13, 2022: 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:

#26343 (p2p: Don’t require services from ADDR_FETCH peers by mzumsande)
#24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)

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.

maflcko renamed this:
~~Advertise `NODE_REPLACE_BY_FEE` and connect to 1 outbound full-rbf peer if `mempoolfullrbf` sets~~
p2p: Advertise `NODE_REPLACE_BY_FEE` and connect to 1 outbound full-rbf peer if `mempoolfullrbf` sets
on Jul 13, 2022

DrahtBot added the label P2P on Jul 13, 2022

ariard commented at 0:18 am on July 14, 2022: member

Isn’t this bad for privacy?

I think a mass-spying node could automatically connect to a node and broadcast an opt-out transaction then a higher-fee replacement one to detect if mempoolfullrbf=1 and wait if a inv(replacement_txid) is announced to a monitoring peer. So turning on bit 26 to signal NODE_REPLACE_BY_FEE sounds to me at worst makes such “fingerprint” cheaper. Though by definition policy is a source of transaction processing discrepancy between nodes, so I’m not sure we can even qualify that as a fingerprint.

The other concern could be a few spying NODE_REPLACE_BY_FEE public peers attracting inbound connection from honest peers, getting a better leverage of initial-broadcast. If it’s a qualified concern, we could announce replacement transactions only to those peers thus avoiding any privacy interferences with the node usual transaction traffic.

Though if you have more transaction-relay deanonymization attacks in mind, I’m listening.

ghost commented at 11:12 am on July 14, 2022: none

I think a mass-spying node could automatically connect to a node and broadcast an opt-out transaction then a higher-fee replacement one to detect if mempoolfullrbf=1 and wait if a inv(replacement_txid) is announced to a monitoring peer. So turning on bit 26 to signal NODE_REPLACE_BY_FEE sounds to me at worst makes such “fingerprint” cheaper.

Service flag makes it easier to make a list of nodes with full RBF policy. These nodes could be banned or this information can be used in different attacks. I don’t think it is required, requesting LN users and some miners to use full RBF would be enough.

Writing ‘help’ if lost on an island could result in 2 things:

An aircraft or chopper notices and helps you
Someone else finds you alone, looking for help, tries to exploit and kill you

The other concern could be a few spying NODE_REPLACE_BY_FEE public peers attracting inbound connection from honest peers, getting a better leverage of initial-broadcast. If it’s a qualified concern, we could announce replacement transactions only to those peers thus avoiding any privacy interferences with the node usual transaction traffic.

This is a good point which I did not consider earlier though I am not sure about announcing replacement transactions only to those peers.

luke-jr commented at 7:13 pm on July 14, 2022: member

~~At the very least, this needs a BIP. Unsure about using the temporary bit for it, and also not sure if it makes sense conceptually; it’s just a minor policy change, after all.~~

Edit: Or just mark it as a temporary service bit like existing usage does

ariard marked this as ready for review on Jul 14, 2022

mzumsande commented at 6:33 pm on July 17, 2022: contributor

Some initial general thoughts:

As I understand it, the game theory works out such that as soon as there is a connected cluster of supporting nodes and enough connected mining power, full-rbf will work for everyone who wants to do it, and non-supporting nodes don’t matter - they can’t stop it from happening or rely on full-rbf not being possible (which they already shouldn’t today, but some may do regardless)
Because of the connectivity of the p2p network, already without any preferential peering, the percentage p of nodes that would need to support full-rbf for it to work with a high reliability is rather small, way below 50% - I tried some simulations (which I need to test more, so not too confident in the exact numbers), and the threshold for a >90% chance of a full-rbf-supporting route to exist between two full-rbf-supporting nodes seems to be around p=10% for reasonably realistic network sizes (10k listening peers, 50k non-listening). See https://github.com/mzumsande/fullrbf_simulation for the code.
This PR would lower the threshold of p drastically close to zero, so that merging it would be a strong statement that we want full-rbf to be possible, even if it’s just a small minority of nodes that support it. On the other hand, it adds additional complexity to the networking code, and I’m not sure if making p2p peering decisions base on policy is something we’d want in general. But if adding support for automatic preferential peering is such a strong statement anyway - why don’t we just change the default of mempoolfullrbf to true instead and save us all the added complexity?
Finally, just mentioning the possibility of false signaling. If other nodes want to prevent or delay full-rbf they could signal full-rbf but not implement it. I’m not sure if we can detect or punish them for that.

ariard commented at 11:19 pm on July 25, 2022: member

@1440000bytes

Service flag makes it easier to make a list of nodes with full RBF policy. These nodes could be banned or this information can be used in different attacks. I don’t think it is required, requesting LN users and some miners to use full RBF would be enough.

Well that’s a fundamental trade-off between signaling policy to ease peering and retaining information. I think the relevant question is this additional information allow or significantly ease a concrete attack against node operator or funds safety. That said, even if the attack is qualified, the risk could be bearable by the operator, if the documentation is clear in that sense. E.g, in LN we recently specified out zero-conf channels, they’re coming with a lot of safety tradeoffs, though they’re a UX improvement for sure.

Writing ‘help’ if lost on an island could result in 2 things: An aircraft or chopper notices and helps you Someone else finds you alone, looking for help, tries to exploit and kill you

Beware life is risky :)

This is a good point which I did not consider earlier though I am not sure about announcing replacement transactions only to those peers.

In case of uncertain privacy/security risks, security-by-compartmentalization. @luke-jr

Edit: Or just mark it as a temporary service bit like existing usage does

Well, I used the same experimental bit 26 than the patchset from @petertodd. Let me know if you think I should use another one in the range bits 24-31 because of conflicts with existent bitcoin softwares.

in src/protocol.cpp:199 in bbc5d58e41 outdated

195@@ -196,6 +196,7 @@ static std::string serviceFlagToStr(size_t bit)
196     case NODE_WITNESS:         return "WITNESS";
197     case NODE_COMPACT_FILTERS: return "COMPACT_FILTERS";
198     case NODE_NETWORK_LIMITED: return "NETWORK_LIMITED";
199+    case NODE_REPLACE_BY_FEE:  return "FULL_RBF";

luke-jr commented at 11:40 pm on July 25, 2022:

0    case NODE_REPLACE_BY_FEE:  return "REPLACE_BY_FEE?";

petertodd commented at 11:44 am on August 1, 2022:

It’s also fine to call this service bit NODE_FULL_RBF. The NODE_REPLACE_BY_FEE name comes from code so old that the terminology hadn’t been settled yet and opt-in-RBF didn’t exist.

ariard commented at 0:01 am on August 24, 2022:

Renamed to NODE_FULL_RBF.

luke-jr changes_requested

ariard commented at 1:08 am on July 26, 2022: member

@mzumsande

For more perspective, I would like to recall that seeing full-rbf more deployed on the p2p network makes multi-party funded transactions more robust, at the price of zero-conf transactions accepting services more vulnerable to double-spend attacks. I think it raises an interesting question, in the sense of how as the Bitcoin Core project we should weight for a policy A or policy B offering different set of impacts on Bitcoin applications. In my opinion, the discussions can be tie-breaked on the hard physics of the system and recognizing there is no such thing as a global mempool in a distributed system therefore making assumptions on transactions ordering is inherently unsafe.

All of that to say I would like not to adopt a game theory reasoning framework, as ultimately game theory, at least zero-sum games, relies on the assumption of moves triggered by set of players in the pursuit of a gain, at the detriment of the other subsets. I’m thinking more of this change as a “organic” policy release engineering practice, namely giving the tools to the node operators to progressively activate full-rbf and build the full-rbf supporting routes. Even if we can assume a active subset of node operators quickly picking up the changes, social inertia being it won’t happen overnight. This should give time for zero-conf transactions accepting services to either deprecate this mode of payment or moving to safe instant payments like LN or deploy (non-perfect) distributed mempools monitoring system.

However, I acknowledge this is an ideal policy release practice, and adding automatic preferential peering might be too much a strong statement, or the timing is too short (let’s give another buffer release before to introduce automatic preferential peering ?), or we should give more time to express technical dissent from zero-conf services providers or anyone else. Posting on the mailing list was done on this purpose.

Beyond, I think this discussion is still interesting even in the lack of policy reversal like with full-rbf when we deploy new transaction-relay policy changes (e.g Erlay, package-relay, new RBF rules), we might firewall the new policy changes behind settings to observe first the performance, privacy-preserving or robustness properties of new features. We could clean up the added code complexity when we estimate the feature as mature.

I think it’s interesting to have a robust connectivity model of the p2p network, with factors like percentage p of nodes that need to support the new features and the chance threshold for success. I think we would enrich such model with the set of mempools miners among the let’s say 10k listening peers (maybe Stratum v2 might change some assumptions ?). About false signaling, I think we might reduce it to buggy signaling and includes some factor f. I believe a mitigation against false signaling could be to run a scrapper testing the effectiveness of full-rbf and noting the peers address, however the dissemination of such information of “misbehaving” peers if relied on centralized/semi-centralized sources like DNS seeds is more likely to be more harmful than beneficial.

in src/net.cpp:1065 in bbc5d58e41 outdated

1052@@ -1053,6 +1053,8 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ
1053     case ConnectionType::BLOCK_RELAY:
1054         max_connections = m_max_outbound_block_relay;
1055         break;
1056+    case ConnectionType::FULLRBF:
1057+        max_connections = m_max_outbound_fullrbf_relay;
1058     // no limit for ADDR_FETCH because -seednode has no limit either

petertodd commented at 11:36 am on August 1, 2022:

This is getting out of scope for this review. But as an aside, IIUC this function includes manually added connections in its max connections count. I’m not sure that’s correct. If I manually add a bunch of nodes that match some category, that doesn’t necessarily mean I only want my node connecting to the manually added nodes.

petertodd commented at 1:23 pm on August 3, 2022:

Oh, actually, looks like this code doesn’t do anything in practice, as AddConnection is only used by the addconnection RPC call.

ariard commented at 2:03 pm on August 3, 2022:

Oh right, that’s correct my mistake. IMHO, we do have a confusing connection type with a “flat” approach instead of an attribute based one as argued previously in #18044#pullrequestreview-442115002. I’ll fix that soon.

ariard commented at 0:13 am on August 24, 2022:

As you raised, manually adding a bunch of nodes shouldn’t necessarily mean that you only want your node to connect to manually added nodes, as you might like to profit from the automatic discovery and connecting logic. It sounds really artificial for us to constraint node operators in their peers topology when there is a deliberate choice to deviate from the spontaneous topology generated by ThreadOpenConnections.

That said, this approach of enforcing connection limits controls on the types of connection is already what is implemented by AddConnection, not subjecting outbound full-rbf peers to limits control would deviate from that behavior. So I think the real discussion we should have would be about if those checks make really sense in ThreadOpenConnections. Though as it’s a point preceding this PR, I would leave as it is.

Beyond, I wonder if that really matters as addconnection is regtest-only (cf. L347, in src/rpc/net.cpp).

in src/net.h:68 in bbc5d58e41 outdated

63@@ -64,6 +64,8 @@ static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 4 * 1000 * 1000;
64 static const unsigned int MAX_SUBVERSION_LENGTH = 256;
65 /** Maximum number of automatic outgoing nodes over which we'll relay everything (blocks, tx, addrs, etc) */
66 static const int MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8;
67+/** Maximum number of automatic fullrbf peers */
68+static const int MAX_FULLRBF_RELAY_CONNECTIONS = 1;

petertodd commented at 11:42 am on August 1, 2022:

I believe this should be higher. With just ~1 full-RBF peer per node, a full-RBF tx is propagating in a network that is “chain-like”. At each propagation step, the # of new nodes receiving the tx will be approximately one.

You should set this higher, to 4 or so, to ensure that propagation is “exponential” in nature, with each propagating step reaching more and more nodes.

mzumsande commented at 2:55 pm on August 4, 2022:

You also have to take into account that each node makes 8 automatic outgoing connections that might by chance connect to more full-RBF peers. This chance becomes higher if more peers run full-RBF. So it really comes down to how small of a minority of supporting nodes we require for full-RBF to still work reliably.

Also note that increasing automatic outbound slots too much could be problematic if full-rbf was adopted by many nodes, because there might not be sufficient inbound capacity. Studies like https://arxiv.org/pdf/2108.00815v2.pdf (Fig. 3) suggest that a significant number of nodes already operate close to their maximum inbound capacity today.

petertodd commented at 5:04 pm on August 4, 2022:

You also have to take into account that each node makes 8 automatic outgoing connections that might by chance connect to more full-RBF peers.

The chance is pretty low at small %’s of full-RBF nodes. I know none of my nodes were connected to RBF-advertising peers by accident without this patch.

Also note that increasing automatic outbound slots too much could be problematic if full-rbf was adopted by many nodes

I think that’s quite unlikely. I highly doubt more than a few % of nodes would enable this manually. Meanwhile, once a few % of nodes and miners have, any remaining arguments against enabling full-rbf by default are certainly bunk, as getting non-opt-in replacements mined will be easy.

ariard commented at 1:12 am on August 24, 2022:

I think the chances to be connected to a full-rbf subgraph by relying only on the 8 automatic outgoing connections sounds really low, even assuming you’re a listening node. If you model something with ~60k nodes, ~125 connection slots and 100 full-rbf nodes across the network, the success probability is like what ~1/60 ? This could explain the observed lack of RBF-advertising peers connections.

Thinking back to the model laid out above, rather to define success as the existence of a full-rbf route between two full-rbf supporting nodes another success definition could be the propagation of a full-rbf tx among the quasi-unanimity of the full-rbf peers. Therefore to achieve success we should aim to eradicate disconnected graphs of full-rbf peers. The exact value of MAX_FULLRBF_RELAY_CONNECTIONS would be function of the set size of peers we assume enabling full-rbf by option.

I think the concern about increased consumption of inbound capacity is qualified, however I would say best we can do is giving us a slot budget. I.e assuming a set size of full-rbf peers, a number of new automatic outbound connections and an occupation rate of the inbound slots of the listening peers and gauge if the result is acceptable.

petertodd commented at 1:40 pm on August 24, 2022:

The exact value of MAX_FULLRBF_RELAY_CONNECTIONS would be function of the set size of peers we assume enabling full-rbf by option.

No, that’s not how this works. Since we’re doing preferential peering, the total number of peers with full-rbf on the network isn’t relevant. We simply need a figure high enough that a sufficient number of full-rbf-signalling peers we connect to are in fact, actuall full-rbf peers that likewise have preferential peering. Second, by “sufficient number”, we need something greater than 1 so we end up with a well-connected group rather than disconnected chains. 4+ should be fine.

So the actual figure we care about is what % of peers with the full-rbf service bit set are running this code (or something like it)? There are a handful of nodes running Knots that advertise the service bit but don’t do preferential peering. Once a reasonable number of people (a few dozen) use this flag, they probably won’t matter as AFAICT there’s less than half a dozen such Knots-running nodes.

ariard commented at 5:19 pm on August 31, 2022:

We simply need a figure high enough that a sufficient number of full-rbf-signalling peers we connect to are in fact, actuall full-rbf peers that likewise have preferential peering. Second, by “sufficient number”, we need something greater than 1 so we end up with a well-connected group rather than disconnected chains. 4+ should be fine.

Yes, I think it would be interesting to have a consistent propagation model to determine what exact “sufficient number” would fit to ensure we have well-connected full-rbf group rather than disconnected chains. I wonder if there have been such models established in the past Core discussions about p2p modifications or extensions. Otherwise landing on the number 4+ sounds really found by empiricism and historical observations ? (not a bad method we should just be aware of the limits)

petertodd changes_requested

petertodd commented at 11:46 am on August 1, 2022: contributor

I don’t think it is required, requesting LN users and some miners to use full RBF would be enough.

It’s not enough. As @mzumsande notes, you need a significant % of nodes supporting full-RBF propagation before txs propagate reliably; once the percolation threshold is reached propagation suddenly becomes quite reliable. Also, as per my review, connecting to a single full-RBF peer is insufficient to achieve good propagation.

petertodd commented at 11:53 am on August 1, 2022: contributor

Service flag makes it easier to make a list of nodes with full RBF policy. These nodes could be banned or this information can be used in different attacks.

Banning nodes in the Bitcoin P2P network is pretty much pointless, as transactions will just propagate around the ban. In fact, preferential peering helps mitigate that (minor) attack, by making it even more likely that transactions will successfully propagate around the ban.

Re: attacking nodes with the service bit set, I had a similar version of this patch years ago, prior to the development of lightning when full-RBF was much more contentious. To the best of my knowledge no-one ever attacked any full-RBF-advertising nodes.

maflcko commented at 4:24 pm on August 4, 2022: member

Not sure if this is worth it. Now that the wallet/rpc signals for rbf by default already (to support fee bumping), this pull here would mostly benefit adversarial settings (such as lightning), where signalling isn’t possible.

So assuming that in those adversarial settings there exits a direct connection to a fullrbf miner (either by running the code here, or with a manual -connect, or by pure chance, or whatever), we might as well turn on fullrbf by default. See also the first point in #25600#pullrequestreview-1041170369 : As long as there is one route to a fullrbf miner, fullrbf txs will eventually be confirmed, regardless of the fullrbf setting of all other nodes outside the route.

petertodd commented at 5:01 pm on August 4, 2022: contributor

So assuming that in those adversarial settings there exits a direct connection to a fullrbf miner

Though this patch avoids the necessity of full-rbf miners identifying themselves, as well as make it easy to use multiple full-rbf miners at once. Without full-rbf by default, this is definitely the next best thing.

we might as well turn on fullrbf by default.

My preference as well.

petertodd commented at 5:24 pm on August 4, 2022: contributor

FYI I tried setting MAX_FULLRBF_RELAY_CONNECTIONS = 4 on three of my nodes, and if that’s all you do they don’t reliably connect to more than one full-rbf node no matter how long the node runs, even though there are a bunch of them on the network. Secondly, as I expected, transactions don’t reliably propagate between full-RBF nodes with just one full-RBF peer.

Putting the conn_type if clause higher up in precedence does fix this problem; right now connecting to full-rbf peers has lower precedence than feeler connections.

ghost commented at 10:58 pm on August 4, 2022: none

Even if its made default without rationale and usage, there will be lot of nodes that are running older versions with no full rbf. Most of the nodes are not even using v23.0 right now based on @luke-jr charts and shodan:

ariard force-pushed on Aug 24, 2022

ariard commented at 1:29 am on August 24, 2022: member

Sorry for the delay, updated at 3622534 with few changes.

From my understanding, there are few conceptual viewpoints expressed about this change. One is to argue that releasing a full-rbf service bit and corresponding automatic preferential peering is a strong statement in the wide deployment of full-rbf and a more neutral position should be adopted by the project, at least for now. Another one would be shortcut the added complexity and go directly to turn on fullrbf by default. A last viewpoint would be to release only the p2p tools for a node operator to opt and enforce the replacement policy adequate for its application, without aiming for coordinated deployment network-wide.

To be clear, I’m still favoring the latest viewpoint, as in light of a more complex ecosystem, I think we should aim for gradual deployment of new policy rules, not only to experiment the robustness/privacy/performance of the changes themselves but also to enable gradual adaptions of the node operators and Bitcoin services. I think stability and previsibility of our p2p and policy rules matter.

luke-jr commented at 1:37 am on August 24, 2022: member

One is to argue that releasing a full-rbf service bit and corresponding automatic preferential peering is a strong statement in the wide deployment of full-rbf and a more neutral position should be adopted by the project, at least for now.

This doesn’t make sense. The current de facto “position” is anti-RBF. Making it an option disabled by default is the very minimum to be neutral.

I think we should aim for gradual deployment of new policy rules

This isn’t a new rule to the network, just Core. Knots and other software have supported it for years.

I think stability and previsibility of our p2p and policy rules matter.

Policy is primarily a per-node matter, not really a per-software matter.

P.S. Don’t forget to rename the PR

petertodd commented at 1:52 pm on August 24, 2022: contributor

I think we should aim for gradual deployment of new policy rules, not only to experiment the robustness/privacy/performance of the changes themselves but also to enable gradual adaptions of the node operators and Bitcoin services.

To be clear, we always have a gradual deployment because adoption of new Bitcoin Core releases is always a gradual process. Just enabling full-rbf by default would be a gradual deployment from that point of view.

On the other hand, if you’re talking about gradual deployment in the sense of new transaction types getting to miners, we always have a non gradual deployment if preferential peering is not used. Because the math behind percolation means that there is a sharp transition between “not enough nodes to reliably propagate” and “enough nodes to reliably propagate”

in src/node/connection_types.h:82 in 36225348fe outdated

73@@ -74,6 +74,12 @@ enum class ConnectionType {
74      * AddrMan is empty.
75      */
76     ADDR_FETCH,
77+
78+    /**
79+     * Full-rbf connections are long-lived connections used to relay transactions
80+     * with peers accepting replacement without requiring replaceability signaling.
81+     */
82+    FULLRBF,

mzumsande commented at 8:20 pm on August 28, 2022:

The switch statement in ConnectionTypeToQString in guituil.cpp needs an update too (this is why the CI fails).

LarryRuane commented at 5:03 pm on August 30, 2022:

should this be FULL_RBF? (it may be better as-is but just thought I’d mention it.)

ariard commented at 4:29 pm on August 31, 2022:

Fixed.

ariard commented at 4:58 pm on August 31, 2022:

Yes this would match the service bit naming.

in src/net.cpp:1063 in 36225348fe outdated

1052@@ -1053,6 +1053,8 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ
1053     case ConnectionType::BLOCK_RELAY:
1054         max_connections = m_max_outbound_block_relay;
1055         break;
1056+    case ConnectionType::FULLRBF:
1057+        max_connections = m_max_outbound_fullrbf_relay;

mzumsande commented at 9:06 pm on August 28, 2022:

There is a break missing, so this would fallthrough.

ariard commented at 4:30 pm on August 31, 2022:

Correct.

naumenkogs commented at 7:32 am on August 29, 2022: member

I will reply to the summary of arguments from this comment.

releasing a full-rbf service bit and corresponding automatic preferential peering is a strong statement in the wide deployment of full-rbf and a more neutral position should be adopted by the project, at least for now

I think a service bit enabled by a fullrbf flag that defaults to false is not a strong statement. It just allows those nodes which chose this policy to do better peering.

Another one would be shortcut the added complexity and go directly to turn on fullrbf by default.

This is a hard part for me. The opponents of fullrbf have expectations about their zero-conf transactions. This would destroy them right away I guess. But so does this PR, if it succeeds to achieve its goals? And my expectation is that this is just a matter of time. In practice, their expectations could be destroyed even by interested parties (LN, etc.) connecting to miners directly (assuming they run fullrbf). So it seems like the main question of this alternative is how it looks politically, even though opt-in will go away one way or another.

to release only the p2p tools for a node operator to opt and enforce the replacement policy adequate for its application, without aiming for coordinated deployment network-wide.

I don’t think it really saves us much code complexity (diff is minimal).
Politically, if this PR approach saves us some bikeshedding, sure.
Do I miss anything?

in src/net.h:68 in 36225348fe outdated

63@@ -64,6 +64,8 @@ static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 4 * 1000 * 1000;
64 static const unsigned int MAX_SUBVERSION_LENGTH = 256;
65 /** Maximum number of automatic outgoing nodes over which we'll relay everything (blocks, tx, addrs, etc) */
66 static const int MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8;
67+/** Maximum number of automatic fullrbf peers */
68+static const int MAX_FULLRBF_RELAY_CONNECTIONS = 4;

mzumsande commented at 3:08 pm on August 29, 2022:

To be clear, the idea is to have 4 additional full-rbf outbound connections, in addition to the 8 full-outbound and 2 block-relay-only connections, correct? I think this isn’t currently working as intended, because m_max_outbound (which sets the number of semaphores for outbound connections) is not adjusted in CConnman::Init(), so we’d currently make just 1 full-rbf connection (and stop making feelers completely).

Also, the commit msg, OP and PR title should be updated from 1 to 4.

instagibbs commented at 5:56 pm on August 29, 2022:

rearranging the lines and accumulating the proper value results in 4 connections (to knots nodes)

ariard commented at 4:55 pm on August 31, 2022:

To be clear, the idea is to have 4 additional full-rbf outbound connections, in addition to the 8 full-outbound and 2 block-relay-only connections, correct?

Right, the idea is to have 4 additional full-rbf outbound connections, in addition to the 8 full-outbound and 2 blocks-relay-only connections. Modified m_max_outbound_fullrbf_relay in init.cpp to account for nMaxConnections and number of full-outbound and block-relay-only connections, and modified m_max_outbound computation in CConman::Init(). The feelers 1 slot connection should be preserved.

Thanks for the catchup.

jonatack commented at 1:33 pm on September 2, 2022:

To be clear, the idea is to have 4 additional full-rbf outbound connections, in addition to the 8 full-outbound and 2 block-relay-only connections, correct?

Right, the idea is to have 4 additional full-rbf outbound connections, in addition to the 8 full-outbound and 2 blocks-relay-only connections.

The second commit title and message body still needs to be updated (as would docs like, offhand, reduce-memory.md and reduce-traffic.md and perhaps elsewhere), and other affected p2p logic and behavior reviewed carefully.

ariard commented at 4:59 pm on September 20, 2022:

Addressed, modifying reduce-memory.md, reduce-traffic.md to notify the change in number of outbound connections.

instagibbs commented at 3:53 pm on August 29, 2022: member

This would destroy them right away I guess.

It really depends on user uptake, which is the primary difference here. If preferential peering ends up destroying whatever is left of zero-conf security, it would be done by some critical mass of users and miners opting into the new behavior. If it’s universally rejected by users and miners, then it’s a no-op. If it has significant uptake, we can revisit ripping out the additional complexity in follow-on release.

luke-jr commented at 8:04 pm on August 29, 2022: member

“zero-conf security”, outside of L2 solutions, has always been non-existent.

instagibbs commented at 8:10 pm on August 29, 2022: member

Argument is that even if it existed, it evaporates on users opting into this feature. I’m taking the strongest position possible to work through the politics :)

in src/protocol.h:360 in 36225348fe outdated

350@@ -349,6 +351,14 @@ static inline bool MayHaveUsefulAddressDB(ServiceFlags services)
351     return (services & NODE_NETWORK) || (services & NODE_NETWORK_LIMITED);
352 }
353 
354+/**
355+ * Checks if a peer with the given service flags enables fullrbf.
356+ */
357+static inline bool HasFullRBFServiceFlag(ServiceFlags services)
358+{
359+    return (services & NODE_FULL_RBF);

LarryRuane commented at 4:50 pm on August 30, 2022:

nit you could eliminate the parenthesis.

ariard commented at 4:56 pm on August 31, 2022:

Personally, I prefer when code style matches logical unit, even if it’s slightly more verbose.

LarryRuane commented at 5:03 pm on August 30, 2022: contributor

Suggestion for additional testing – verify that when started with -mempoolfullrbf=1, the node advertises the NODE_FULL_RBF services bit. This doesn’t test the outbound direction, that the node actually tries to add 4 fullrbf peers, I’m not sure how difficult that would be.

 0diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
 1index 7603248ae..27673c639 100755
 2--- a/test/functional/feature_rbf.py
 3+++ b/test/functional/feature_rbf.py
 4@@ -9,6 +9,7 @@ from decimal import Decimal
 5 from test_framework.messages import (
 6     MAX_BIP125_RBF_SEQUENCE,
 7     COIN,
 8+    NODE_FULLRBF,
 9     SEQUENCE_FINAL,
10 )
11 from test_framework.test_framework import BitcoinTestFramework
12@@ -18,6 +19,7 @@ from test_framework.util import (
13 )
14 from test_framework.wallet import MiniWallet
15 from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE
16+from test_framework.p2p import P2PInterface
17 
18 MAX_REPLACEMENT_LIMIT = 100
19 class ReplaceByFeeTest(BitcoinTestFramework):
20@@ -705,7 +707,14 @@ class ReplaceByFeeTest(BitcoinTestFramework):
21 
22         confirmed_utxo = self.make_utxo(self.nodes[0], int(2 * COIN))
23         self.restart_node(0, extra_args=["-mempoolfullrbf=1"])
24+
25+        self.log.info("Verify getmempoolinfo and getnetworkinfo report fullrbf")
26         assert self.nodes[0].getmempoolinfo()["fullrbf"]
27+        assert 'FULL_RBF' in self.nodes[0].getnetworkinfo()["localservicesnames"]
28+
29+        self.log.info("Verify that the node advertises the NODE_FULLRBF service bit")
30+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
31+        assert peer.nServices & NODE_FULLRBF
32 
33         # Create an explicitly opt-out transaction
34         optout_tx = self.wallet.send_self_transfer(
35diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
36index 4e757b64c..96a1514fc 100755
37--- a/test/functional/test_framework/messages.py
38+++ b/test/functional/test_framework/messages.py
39@@ -51,6 +51,7 @@ NODE_BLOOM = (1 << 2)
40 NODE_WITNESS = (1 << 3)
41 NODE_COMPACT_FILTERS = (1 << 6)
42 NODE_NETWORK_LIMITED = (1 << 10)
43+NODE_FULLRBF = (1 << 26)
44 
45 MSG_TX = 1
46 MSG_BLOCK = 2

ariard commented at 4:18 am on August 31, 2022: member

Updating the PR and answering comments soon.

glozow renamed this:
~~p2p: Advertise `NODE_REPLACE_BY_FEE` and connect to 1 outbound full-rbf peer if `mempoolfullrbf` sets~~
p2p: Advertise `NODE_FULL_RBF` and connect to 4 outbound full-rbf peers if `-mempoolfullrbf` is set
on Aug 31, 2022

ariard commented at 3:56 pm on August 31, 2022: member

@luke-jr

This doesn’t make sense. The current de facto “position” is anti-RBF. Making it an option disabled by default is the very minimum to be neutral.

In a decentralized ecosystem environment like Bitcoin, I think the measurement of stakeholders positions about consensus or policy rules changes should rely on a wide (non-exhaustive) variety of factors: compatible softwares, reasoned opinions on technical trade-offs, economic actors adoptions, enabled use-cases, etc. The weight attached to each of this factor to determine ecosystem position or consensus is left to everyone’s appreciation. Therefore your statement about anti-RBF being the de facto “position” is the reflect of your personal judgement and I don’t believe it should be interpreted as the ecosystem position. Other developers, entities and node operators might have a diverging appreciation than yours. At the very least in the LN community only, the advantages of full-rbf sound to be well-understood and appreciated. In my opinion, this data point would make it hard to qualify there is a solidified anti-RBF sentiment across the ecosystem.

This isn’t a new rule to the network, just Core. Knots and other software have supported it for years.

The interest of this work is not only to offer the tools to node operators to profit from full-rbf propagation paths at their own choice, but also to interrogate how the project is deploying new changes across the ecosystem, in light of the increasing complexity of Bitcoin applications (lightning, coinjoins, swaps, etc) with significant stakes. In that regard, while I agree that full-rbf isn’t a new rule, the “new” qualification was aiming to encompass future p2p/mempool policy changes like package relay or ephemeral output, still at the design stage and not yet fully-implemented in any other Bitcoin software to the best of my knowledge.

Policy is primarily a per-node matter, not really a per-software matter.

While I acknowledge the subjective ability of node operator to use, copy, study and change their Bitcoin softwares in any way suiting their preferences, as contributors to this project we’re still left with the question on what default which should pickup among a set of policy options. The choice of a default might be far from neutral in term of privacy, economic efficiency, safety or any other dimensions that matters. Moreover, as the RBF signaling case is illustrating, a default choice might impact negatively a use-case (here zero-conf) while offering positive impact on another use-case (here multi-party funding transactions). I think we should build gradually over time a common decision-making framework in how Bitcoin Core adopts a default policy rule, that way guaranteeing stability and previsibility to our users. @petertodd

To be clear, we always have a gradual deployment because adoption of new Bitcoin Core releases is always a gradual process. Just enabling full-rbf by default would be a gradual deployment from that point of view.

Yes, I agree we always had a gradual deployment because adoption of new Bitcoin Core versions isn’t uniform and instantaneous across the ecosystem. While enabling full-rbf by default would still be a gradual deployment from that viewpoint, the position of offering service bit and automatic preferential peering would allow the “market” of node operator to suit what is best for their use-cases, without Bitcoin Core, the project have to adopt a strong position on this policy rule. In case of controversial policy rules, the best we might be able to do is document clearly the trade-offs presented by every policy rule options.

On the other hand, if you’re talking about gradual deployment in the sense of new transaction types getting to miners, we always have a non gradual deployment if preferential peering is not used. Because the math behind percolation means that there is a sharp transition between “not enough nodes to reliably propagate” and “enough nodes to reliably propagate”

In that regard, hopefully we could do a better knowledge transmission work towards the ecosytem node operators to explain that percolation theory being what it is, we’re left with a binary situation w.r.t to deployment of new transaction types getting to miners. I think this is opening an interesting question, as some choice and deployment of policy rules might offer security properties emerging from holistic behaviors. E.g, if a LN node security and safety funds is increased with the deployment of package relay, one could argue with the number of full-nodes supporting and enabling that change across the ecosystem, the security is in proportion increased, in a quantitative fashion. @naumenkogs

This is a hard part for me. The opponents of fullrbf have expectations about their zero-conf transactions. This would destroy them right away I guess. But so does this PR, if it succeeds to achieve its goals? And my expectation is that this is just a matter of time. In practice, their expectations could be destroyed even by interested parties (LN, etc.) connecting to miners directly (assuming they run fullrbf). So it seems like the main question of this alternative is how it looks politically, even though opt-in will go away one way or another.

About the “zero-conf transactions” security, while I think this assumption has always been weak due to the distributed nature of the Bitcoin p2p network and practical “zero-conf” double-spend have been demonstrated in the past, enabling full-rbf by default is lowering the technical and knowledge bar to enact such double-spend. This “zero-conf” attacks lowering bar could be gauged as an encroachment on some operators applications or services interests. At the really same time, we have demonstrated DoS against the reliability of multi-party funded transactions due to opt-in RBF. In that “split” situation regarding policy change, I would say the best we might be able to do as a project is to offer tools to the market of node operators, to let them express there is demand for fullrbf (even if due to percolation theory, the operators threshold to disaggregate the “zero-conf” security notion is low).

Zooming out, I think there is a concern of us establishing stable policy release engineering practices, and setting a good precedent towards the community, in anticipation of more complex “Bitcoin-applications-and-policy-rules-interactions” cases in the future. @instagibbs

If it’s universally rejected by users and miners, then it’s a no-op. If it has significant uptake, we can revisit ripping out the additional complexity in follow-on release.

I’m converging on that, we shouldn’t substitute in the users and miners roles about expressing demand for full-rbf. While we might still do a knowledge transmission work in explaining the trade-offs, we might miss subtle trade-offs due to lack of informations on their use-cases. If there is a significant uptake, for sure we can rip out the additional complexity in follow-on releases.

ariard force-pushed on Aug 31, 2022

ariard commented at 5:08 pm on August 31, 2022: member

Updated at 36225348 with comments addressed.

Thanks for the reviews.

instagibbs commented at 6:17 pm on August 31, 2022: member

0./net.h: In member function ‘bool CNode::ExpectServicesFromConn() const’:
1./net.h:471:34: error: ‘FULLRBF’ is not a member of ‘ConnectionType’; did you mean ‘FULL_RBF’?
2  471 |             case ConnectionType::FULLRBF:
3      |                                  ^~~~~~~
4      |                                  FULL_RBF
5./net.h:463:16: warning: enumeration value ‘FULL_RBF’ not handled in switch [-Wswitch]
6  463 |         switch (m_conn_type) {
7      |                ^

ariard force-pushed on Aug 31, 2022

ariard force-pushed on Sep 1, 2022

ariard commented at 6:49 pm on September 1, 2022: member

Updated at 35d33402

0./net.h: In member function ‘bool CNode::ExpectServicesFromConn() const’:
1./net.h:471:34: error: ‘FULLRBF’ is not a member of ‘ConnectionType’; did you mean ‘FULL_RBF’?
2  471 |             case ConnectionType::FULLRBF:
3      |                                  ^~~~~~~
4      |                                  FULL_RBF
5./net.h:463:16: warning: enumeration value ‘FULL_RBF’ not handled in switch [-Wswitch]
6  463 |         switch (m_conn_type) {
7      |                ^

Thanks, in fact the builds failure was assignable to guiutil.cpp.

ariard commented at 7:06 pm on September 1, 2022: member

There was a number of relevant discussions about this PR during yesterday review club session: https://bitcoincore.reviews/25600

The increased resource (inbound/outbound slots) usage of the new 4 outbound-full-rbf peers was raised as a concern. However, as this is the automatic preferential peering is only enable if the node operator turns mempoolfullrbf=1, and as such opt-in for the resource consumption this could be acceptable (at the condition there is proper disconnection when the service bit doesn’t match between addr and version).

The non-reliability of the service execution as advertised by the service bit, in the present case full-rbf acceptance of a non-signaling replacement transaction, has been raised as another concern. A peer can never be “trusted” to relay transactions or accomplish the p2p request sent to.

The rational of introducing automatic preferential peering instead of turning on by default mempoolfullrbf has been discussed. On one side, there is a code complexity increased to support peering. on the other hand we have no certainty that this policy default would be the best trade-off for all the applications relying on Bitcoin Core for their operations, especially as this change could have safety impact. Delegating the default setting to node operators could also set a good precedent in the way policy are released, by removing uncertainty.

There is also simulations for the number of nodes required to enable mempoolfullrbf for the propagation paths to be effective: https://github.com/mzumsande/fullrbf_simulation

unknown changes_requested

unknown commented at 7:08 pm on September 1, 2022: none

NACK https://github.com/bitcoin/bitcoin/pull/25600/commits/35d33402f0ee13b431766b0159878b94955d3218

I think I had this idea to spoof before any reviewers although it doesn’t matter: https://bitcoin.stackexchange.com/questions/114491/how-does-bitcoin-core-handle-service-flag-spoofing

TL;DR: This service flag improves nothing but increases opportunity for attackers

instagibbs commented at 7:16 pm on September 1, 2022: member

@1440000bytes you should probably elaborate on what additional powers this gives attackers otherwise it sounds like your complaint is that service flags are just bad in general

luke-jr commented at 7:28 pm on September 1, 2022: member

Policy and transaction relay in general is inherently unreliable, but this still improves the situation in a normal scenario. I don’t think it helps attackers meaningfully.

ariard commented at 8:18 pm on September 1, 2022: member

@1440000bytes

I think I had this idea to spoof before any reviewers although it doesn’t matter: https://bitcoin.stackexchange.com/questions/114491/how-does-bitcoin-core-handle-service-flag-spoofing

About the non-reliability of the service bit, and the subject of spoofing attacks, I think today from a local node viewpoint there is no strong guarantee that the “services” announced (e.g bloom-filters or BIP157 support, block archive) or implicitly assumed to be supported (e.g block-relay) by our peers are going to be executed faithfully, i.e compatible with the working logic on our side or in compliance with a BIP or standard.

In that regard, I think a malicious peer is really akin to a buggy peers, and the best we can do is to have numerous connections to the network, to palliate attacks or failures from a single peer. As long as our peer selection strategy is robust, I think we should be able to discover the most-work valid chain and have our transactions relayed to the miners.

Apart of the peer selection strategy, we could implement more invading logic to monitor the quality of our peers, in analogy on how LN is selecting routing hops to build payment path over which to relay HTLC. However, this would raise a number of other trade-offs, especially in term of network-wise resource consumption and privacy, and I’m not sure it’s a relevant direction.

On the other side, a service bit, I think save a lot of time to full-rbf node operators and should increase efficiency of full-rbf transaction propagation, under the assumption that at least one of our full-rbf peers selected by our connection logic is honest.

That said, if we can come up with an efficient solution to service bit spoofing attack, that could be interesting to think about implementing it.

jonatack commented at 1:24 pm on September 2, 2022: contributor

Not sure if this is worth it… we might as well turn on fullrbf by default.

I agree.

in src/node/connection_types.h:82 in 35d33402f0 outdated

73@@ -74,6 +74,12 @@ enum class ConnectionType {
74      * AddrMan is empty.
75      */
76     ADDR_FETCH,
77+
78+    /**
79+     * Full-rbf connections are long-lived connections used to relay transactions
80+     * with peers accepting replacement without requiring replaceability signaling.
81+     */
82+    FULL_RBF,

jonatack commented at 1:40 pm on September 2, 2022:

0 * If adding or removing types, please update CONNECTION_TYPE_DOC in
1 * src/rpc/net.cpp and src/qt/rpcconsole.cpp, as well as the descriptions in
2 * src/qt/guiutil.cpp and src/bitcoin-cli.cpp::NetinfoRequestHandler. */
3enum class ConnectionType {

If this pull moves forward would need to update these with the added connection type.

ariard commented at 5:16 pm on September 20, 2022:

Added the missing updates with the new connection type, including netinfo documentation normally.

jonatack commented at 1:56 pm on September 2, 2022: contributor

On this branch, I’m seeing the same issue localy as the failing CI tests: p2p_eviction.py --timeout-factor 0 stalls indefinitely at “Create 8 peers and protect them from eviction by having faster pings”.

ghost commented at 5:38 am on September 3, 2022: none

@1440000bytes you should probably elaborate on what additional powers this gives attackers otherwise it sounds like your complaint is that service flags are just bad in general @instagibbs Sorry I could not understand your question at that moment however related things are mentioned in these 2 comments:

#25600 (comment)

#25600#pullrequestreview-1041170369 (last point)

ghost commented at 6:39 am on September 3, 2022: none

On the other side, a service bit, I think save a lot of time to full-rbf node operators and should increase efficiency of full-rbf transaction propagation, under the assumption that at least one of our full-rbf peers selected by our connection logic is honest.

That said, if we can come up with an efficient solution to service bit spoofing attack, that could be interesting to think about implementing it. @ariard How would transaction propagation work properly if only one peer is honest about service flag added in this pull request? Isn’t full RBF added to make some projects secure?

Agree we should find a solution for false signaling before adding another service flag that aims to improve security. Else it would create more problems rather than solving something.

petertodd commented at 12:41 pm on September 3, 2022: contributor

On Fri, Sep 02, 2022 at 11:39:47PM -0700, 1440000bytes wrote:

On the other side, a service bit, I think save a lot of time to full-rbf node operators and should increase efficiency of full-rbf transaction propagation, under the assumption that at least one of our full-rbf peers selected by our connection logic is honest.

That said, if we can come up with an efficient solution to service bit spoofing attack, that could be interesting to think about implementing it.

@ariard How would transaction propagation work properly if only one peer is honest about service flag added in this pull request? Isn’t full RBF added to make some projects secure?

Because you only need one honest peer actually implementing full-RBF that in turn propagates your transaction to others. In the exact same way that in general, you only need one honest peer for tx propagation to work at all.

Agree we should find a solution for false signaling before adding another service flag that aims to improve security. Else it would create more problems rather than solving something.

The situation with this patch is the same as prior to this patch: you need a minimum number of honest peers implementing your needed tx propagation policy. Adding service bits doesn’t change the fundamental nature of the security problem.

ghost commented at 3:39 pm on September 3, 2022: none

“Because of the connectivity of the p2p network, already without any preferential peering, the percentage p of nodes that would need to support full-rbf for it to work with a high reliability is rather small, way below 50% - I tried some simulations (which I need to test more, so not too confident in the exact numbers), and the threshold for a >90% chance of a full-rbf-supporting route to exist between two full-rbf-supporting nodes seems to be around p=10% for reasonably realistic network sizes (10k listening peers, 50k non-listening). See https://github.com/mzumsande/fullrbf_simulation for the code”

#25600#pullrequestreview-1041170369

If less than 50% nodes would allow any transaction to be replaced, why do we need this service flag? Particularly when there are 15k LN nodes, thousands of coinjoin users and maybe a few using other projects who are affected by opt-in RBF. There would be lot of bitcoin core nodes supporting this even if its not default?

Because you only need one honest peer actually implementing full-RBF that in turn propagates your transaction to others. In the exact same way that in general, you only need one honest peer for tx propagation to work at all.

The situation with this patch is the same as prior to this patch: you need a minimum number of honest peers implementing your needed tx propagation policy. Adding service bits doesn’t change the fundamental nature of the security problem. @petertodd

If we need just one honest peer, this pull request is not required.

What changes after this pull request:

You can ban peers that are using full RBF
You can false signal that your node is using full RBF

Note: Different version of nodes used: #25600 (comment)

ghost commented at 4:18 pm on September 3, 2022: none

TBH I would be excited if this pull request gets merged, white or grey or black hat.

mzumsande commented at 4:03 am on September 5, 2022: contributor

On this branch, I’m seeing the same issue localy as the failing CI tests: p2p_eviction.py --timeout-factor 0 stalls indefinitely at “Create 8 peers and protect them from eviction by having faster pings”.

I think this might be fixed if we only set m_max_outbound_fullrbf_relay and increase m_max_outbound in the -mempoolfullrbf case, not always.

You can ban peers that are using full RBF

Why would anyone do that? That would be help the peers with the preferential peering. If you want to prevent full-RBF in the preferential-peering scenario you should signal RBF yourself, get as many full-RBF peers as you can, and discard any replacement transactions from them.

petertodd commented at 1:51 pm on September 9, 2022: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/25600/commits/35d33402f0ee13b431766b0159878b94955d3218

I’m running this on two full time nodes, and one part-time node, and it’s working fine. Full-RBF peers are quickly found during startup, and at least 4 outgoing full-RBF peers are maintained.

instagibbs commented at 2:37 pm on September 9, 2022: member

tested ACK https://github.com/bitcoin/bitcoin/pull/25600/commits/35d33402f0ee13b431766b0159878b94955d3218

I’ve been running this for a few weeks now on listening/private nodes.

Connects to outbound peers in the expected order: full block-only rbf

Easily finding Knots-based peers.

If this ends up making mempool double-spends trivial(let’s coordinate some testing?), then we can remove the complexity and opt-in additional connections in favor of unconditional full rbf.

edit: Obviously needs some tests before thinking of merging.

Maybe expose the details in -netinfo ?

petertodd commented at 3:35 pm on September 9, 2022: contributor

I’ve temporarily modified my https://alice.btc.calendar.opentimestamps.org/ OTS calendar to use full RBF, and to attempt to do one timestamp transaction per block. This means that you can both use the current timestamp tx to see if full-RBF replacements are being propagated. And you can check if they’re getting mined by looking at the historical list of mined txs at the bottom of that page. Anything without the min fee is a replacement.

On September 9, 2022 4:38:05 PM GMT+02:00, Gregory Sanders @.***> wrote:

tested ACK https://github.com/bitcoin/bitcoin/pull/25600/commits/35d33402f0ee13b431766b0159878b94955d3218

Connects to outbound peers in the expected order: full block-only rbf

Easily finding Knots-based peers.

If this ends up making mempool double-spends trivial(let’s coordinate some testing?), then we can remove the complexity and opt-in additional connections in favor of unconditional full rbf.

– Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/25600#issuecomment-1242056927 You are receiving this because you were mentioned.

Message ID: @.***>

ariard force-pushed on Sep 20, 2022

ariard commented at 5:28 pm on September 20, 2022: member

Updated at 0112763

Review comments about reduce-traffic.md, reduce-memory.md and CONNECTION_TYPE_DOC update addressed, netinfo updated, p2p_eviction.py should be fixed.

Still pending on more tests.

ariard commented at 5:49 pm on September 20, 2022: member

@jonatack

I agree.

Thanks for the review, note the proposal of introducing automatic preferential peering with a limited number of full-rbf signaling peers is a procedural novation itself in the way we release policy change, imo. Rather than turning on by default mempoolfullrbf, and therefore ignoring the implications on Bitcoin applications/services arguing “zero-conf” security or any advantage from optin-rbf, we give the transaction-relay tooling for the node operators seeking to benefit from full-rbf. While acknowledging that due to percolation theory, the equilibrium is unstable, we as a project remove the necessity in our decision-making to have to arbiter between class of applications. I think by setting this precedent we make the project decision-making process in matters of policy with holistic implications on the ecosystem more lisible and predictable for our users. I’m concerned in the future of more complex future policy changes affecting the safety or security of L2 applications, due to design undersight from our side. @1440000bytes

You can ban peers that are using full RBF You can false signal that your node is using full RBF

In the former case, it’s unclear to me what exact interest of a honest node is damaged by a malicious peer refusing connection, as another more valuable peer will be sought. In the latter case, while it sounds correct to me you will waste an outbound slot of a honest node, this can be already achieved for any regular connection. I don’t think we’re making strong assumptions of peer reliability in our p2p stack, therefore the redundancy of connections. As laid out by other contributors, as long as there is 1 honest full-rbf peers, there is already a full-rbf propagation efficiency gain.

Add NODE_REPLACE_BY_FEE service flag

If `mempoolfullrbf` config option is set, announce to our peers
we support fullrbf by advertising NODE_REPLACE_BY_FEE in our service
flag. Ensure the service name is displayed by net-related RPC calls.

79c69777b2

Add automatic connection to 4 outbound-fullrbf-relay

If `mempoolfullrbf` config option is set, try to connect to at
least 4 outbound-fullrbf-relay (`ConnectionType::FULLRBF`) in
`ThreadOpenConnections`. An outbound-fullrbf-relay peer is considered
as one signaling NODE_REPLACE_BY_FEE service flag.

501be3aa8e

Update with full-rbf resources controls documentation 0112763071

ariard force-pushed on Sep 20, 2022

in src/protocol.h:297 in 79c69777b2 outdated

292@@ -293,6 +293,8 @@ enum ServiceFlags : uint64_t {
293     // collisions and other cases where nodes may be advertising a service they
294     // do not actually support. Other service bits should be allocated via the
295     // BIP process.
296+
297+    NODE_FULL_RBF = (1 << 26),

naumenkogs commented at 9:37 am on September 21, 2022:

I assume the comment should be added?

in src/bitcoin-cli.cpp:441 in 501be3aa8e outdated

437@@ -437,6 +438,7 @@ class NetinfoRequestHandler : public BaseRequestHandler
438         if (conn_type == "block-relay-only") return "block";
439         if (conn_type == "manual" || conn_type == "feeler") return conn_type;
440         if (conn_type == "addr-fetch") return "addr";
441+        if (conn_type == "fullrbf") return "full-rbf";

naumenkogs commented at 9:40 am on September 21, 2022:

This gets kinda ugly: “fullrbf” is also “outbound-full-relay”, (well, so is “manual”). Perhaps “outbound-full-relay” should be renamed to “generic-outbound-full-relay” or something, I dunno.

in src/net.cpp:1681 in 501be3aa8e outdated

1677@@ -1675,13 +1678,15 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1678         // Only connect out to one peer per network group (/16 for IPv4).
1679         int nOutboundFullRelay = 0;
1680         int nOutboundBlockRelay = 0;
1681+        int nOutboundFullRBF = 0;

naumenkogs commented at 9:48 am on September 21, 2022:

nit: why using ugly variable name in new code?

yancyribbens commented at 9:14 pm on November 3, 2022:

What do you suggest? The variable matches with the naming in the previous two lines.

in src/rpc/net.cpp:45 in 501be3aa8e outdated

40@@ -41,7 +41,8 @@ const std::vector<std::string> CONNECTION_TYPE_DOC{
41         "inbound (initiated by the peer)",
42         "manual (added via addnode RPC or -addnode/-connect configuration options)",
43         "addr-fetch (short-lived automatic connection for soliciting addresses)",
44-        "feeler (short-lived automatic connection for testing addresses)"
45+        "feeler (short-lived automatic connection for testing addresses)",
46+        "fullrbf (long-lived automatic connection for full-rbf peers)"

naumenkogs commented at 9:57 am on September 21, 2022:

This seems like a new slang: what you call “full-rbf” is really “outbound full-rbf”. I’d expand this, but as i mention in a prior comment, our categorization of peers is not that consistent anyway.

in src/net_processing.cpp:3182 in 501be3aa8e outdated

3176@@ -3177,6 +3177,13 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3177             return;
3178         }
3179 
3180+        if (pfrom.IsFullRBF() && !HasFullRBFServiceFlag(nServices))
3181+        {
3182+            LogPrint(BCLog::NET, "peer=%d does not offer fullrbf as expected; disconnecting\n", pfrom.GetId());

naumenkogs commented at 9:59 am on September 21, 2022:

According to the current code, this is impossible? Because the only way to become IsFullRBF() is if the latter condition is true. In that case, should it be Assume instead?

petertodd commented at 9:43 pm on September 22, 2022: contributor

This is an optional, opt-in, feature that only a subset of nodes will ever use. We do not need to analyze to death every single possibility, and in the event of these unlikely attacks, the minority affected can just turn the flag off (potentially with getting a new IP to disassociate from their old “identity”).

Anyway, the phrasing “it’s unclear to me what exact interest of a honest node” is usually just a polite way of saying “no one has convinced me this is a problem”

I don’t believe your continued discussion here is either productive or in good faith.

On September 22, 2022 5:27:54 PM GMT+00:00, 1440000bytes @.***> wrote:

In the former case, it’s unclear to me what exact interest of a honest node is damaged by a malicious peer refusing connection, as another more valuable peer will be sought. In the latter case, while it sounds correct to me you will waste an outbound slot of a honest node, this can be already achieved for any regular connection. I don’t think we’re making strong assumptions of peer reliability in our p2p stack, therefore the redundancy of connections. As laid out by other contributors, as long as there is 1 honest full-rbf peers, there is already a full-rbf propagation efficiency gain.

Did you make all those assumption in last core CVE where it was not documented about something?

Anyway, I am serious this change will affect bitcoin. I will report every change. If it still gets igngored, we cant’t do anything.

– Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/25600#issuecomment-1255332319 You are receiving this because you were mentioned.

Message ID: @.***>

ghost commented at 9:55 pm on September 22, 2022: none

Thanks for the review, note the proposal of introducing automatic preferential peering with a limited number of full-rbf signaling peers is a procedural novation itself in the way we release policy change, imo. Rather than turning on by default mempoolfullrbf, and therefore ignoring the implications on Bitcoin applications/services arguing “zero-conf” security or any advantage from optin-rbf, we give the transaction-relay tooling for the node operators seeking to benefit from full-rbf. While acknowledging that due to percolation theory, the equilibrium is unstable, we as a project remove the necessity in our decision-making to have to arbiter between class of applications. I think by setting this precedent we make the project decision-making process in matters of policy with holistic implications on the ecosystem more lisible and predictable for our users. I’m concerned in the future of more complex future policy changes affecting the safety or security of L2 applications, due to design undersight from our side.. @ariard I am surprised someone who guided me with lot of RBF attacks and CVE-2022-35913 wants to go ahead with this. There are some some issue which I don’t want to repeat and been mentioned in the pull request. We can get our head in the sand and go ahead merging this because we cannot read anything negative from new contributors.

I would keep testing this after merging and please don’t blame me if there are any CVE in any bitcoin projects.

ghost commented at 10:08 pm on September 22, 2022: none

Note: This is the first time bitcoin network will have multiple replacement policies afaik. We should be careful and make something default later that is used most by network. Not by developers.

petertodd commented at 6:57 pm on September 23, 2022: contributor

Bitcoin has always had different replacement policies: each release with different policies means there are different replacement policies. And of course, there have been full-rbf nodes for ages, including ones with preferential peering.

Re: CVE-2022-35913, that’s an example that would be less of a problem with full-rbf, not more.

On September 22, 2022 10:08:57 PM GMT+00:00, 1440000bytes @.***> wrote:

Note: This is the first time bitcoin network will have multiple replacement policies afaik. We should be careful and make something default later that is used most by network. Not by developers.

– Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/25600#issuecomment-1255605845 You are receiving this because you were mentioned.

Message ID: @.***>

ariard commented at 7:59 pm on November 8, 2022: member

Closing this PR for now. I think there have been a lot of high-level conceptual and procedural issues not addressed during the last weeks, on which I’m aiming to work more on.

ariard closed this on Nov 8, 2022

willcl-ark commented at 1:33 pm on November 10, 2022: member

If this does get re-opened in the future, I noticed that the output of -netinfo is misaligned because full-rbf is 8 chars, which can be fixed with this patch

 0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1index 18aeea779..81a078e5c 100644
 2--- a/src/bitcoin-cli.cpp
 3+++ b/src/bitcoin-cli.cpp
 4@@ -525,7 +525,7 @@ public:
 5         // Report detailed peer connections list sorted by direction and minimum ping time.
 6         if (DetailsRequested() && !m_peers.empty()) {
 7             std::sort(m_peers.begin(), m_peers.end());
 8-            result += strprintf("<->   type   net  mping   ping send recv  txn  blk  hb %*s%*s%*s ",
 9+            result += strprintf("<->     type   net  mping   ping send recv  txn  blk  hb %*s%*s%*s ",
10                                 m_max_addr_processed_length, "addrp",
11                                 m_max_addr_rate_limited_length, "addrl",
12                                 m_max_age_length, "age");
13@@ -534,7 +534,7 @@ public:
14             for (const Peer& peer : m_peers) {
15                 std::string version{ToString(peer.version) + peer.sub_version};
16                 result += strprintf(
17-                    "%3s %6s %5s%7s%7s%5s%5s%5s%5s  %2s %*s%*s%*s%*i %*s %-*s%s\n",
18+                    "%3s %8s %5s%7s%7s%5s%5s%5s%5s  %2s %*s%*s%*s%*i %*s %-*s%s\n",
19                     peer.is_outbound ? "out" : "in",
20                     ConnectionTypeForNetinfo(peer.conn_type),
21                     peer.network,
22@@ -559,7 +559,7 @@ public:
23                     IsAddressSelected() ? peer.addr : "",
24                     IsVersionSelected() && version != "0" ? version : "");
25             }
26-            result += strprintf("                     ms     ms  sec  sec  min  min                %*s\n\n", m_max_age_length, "min");
27+            result += strprintf("                       ms     ms  sec  sec  min  min                %*s\n\n", m_max_age_length, "min");
28         }
29 
30         // Report peer connection totals by type.

petertodd commented at 4:28 am on December 6, 2022: contributor

@willcl-ark

I added your patch and a subsequent fix to my full-rbf tree: https://github.com/petertodd/bitcoin/tree/full-rbf-v24.0

If anyone has any further improvements for it, feel free to open a pull-req on my repo.

ariard commented at 3:35 am on December 8, 2022: member

@petertodd

This comment can be valuable to be addressed: #25600 (review), the other ones if I remember are not functional.

ghost commented at 10:09 am on January 16, 2023: none

Why would anyone do that? That would be help the peers with the preferential peering. If you want to prevent full-RBF in the preferential-peering scenario you should signal RBF yourself, get as many full-RBF peers as you can, and discard any replacement transactions from them.

ariard commented at 1:45 am on June 26, 2023: member

In the context of #27926, I think we might have to reconsider automatic transaction-relay preferential peering on the table, therefore allowing full-node operators to opt-in with annexrelay=true.

Beyond, such use-case, I think the preferential peering could be abstracted like we have for consensus-deployment code in /consensus/params.h and DeploymentHeight() in src/validation.cpp. This could be re-used not only for mempoolfullrbf and future replacement logic upgrades, but also for nVersion=3 and beyond transaction formats.

bitcoin locked this on Jun 25, 2024

p2p: Advertise NODE_FULL_RBF and connect to 4 outbound full-rbf peers if -mempoolfullrbf is set #25600

Conflicts

p2p: Advertise `NODE_FULL_RBF` and connect to 4 outbound full-rbf peers if `-mempoolfullrbf` is set #25600