p2p: Disconnect peer that send us tx INVs when we opted out of tx relay #16682

jnewbery commented at 3:33 PM on August 22, 2019: member

-blocksonly mode was introduced in 4044f07d1c5eacb0ec732f1232489aa77fb7bb3b and allows nodes to request that their peers don't relay txs to them. This is done by setting the 'relay' field in the VERSION message (introduced in BIP37) to false.

Tx INVs received from peers when running in blocksonly mode previously resulted in a log message "transaction inv sent in violation of protocol". When running in -blocksonly, it has been observed that several peers advertising as Satoshi:0.18.0 were persistently sending us tx INVs in violation of the protocol. These are suspected of being spy nodes.

Change the behaviour to disconnect nodes that send us tx INVs after we've requested no tx relay.

jnewbery commented at 3:36 PM on August 22, 2019: member

Observed while testing #15759 and suggested by @ajtowns : #15759 (comment)

dongcarl commented at 3:45 PM on August 22, 2019: member

tACK 0e7bc2fba706a8b2483e030eff137c20a61f7eb4

NicolasDorier commented at 4:04 PM on August 22, 2019: contributor

Can you check for the PF_NOBAN permissions so you don't ban whitelisted peer?

dongcarl commented at 4:07 PM on August 22, 2019: member

Can you check for the PF_NOBAN permissions so you don't ban whitelisted peer?

Misbehaving sets fShouldBan, which is checked in PeerLogicValidation::SendRejectsAndCheckIfBanned, and PF_NOBAN is checked before applying the ban.

DrahtBot added the label P2P on Aug 22, 2019

DrahtBot added the label Tests on Aug 22, 2019

instagibbs commented at 4:23 PM on August 22, 2019: member

Why would spy nodes claim to be blocksonly? To not get asked for txs?

jnewbery commented at 4:32 PM on August 22, 2019: member

Why would spy nodes claim to be blocksonly? To not get asked for txs?

Sorry, my explanation was unclear. Let me try again:

I start my node in -blocksonly
I make up to 8 outbound connections, with fRelay set to False.
Most peers (including most Satoshi:0.18.0 peers) honor that relay=False request and don't send me tx INVs. However, some of my outbound connections do send me tx INVs in violation of my request to not relay transactions. Those peers also have subver set to Satoshi:0.18.0. Even if I disconnect and reconnect to those peers, they continue to send me tx INVs.

Explanations:

there's a bug in Bitcoin Core v0.18 that under some circumstances makes them disregard fRelay=False. I've looked at the VERSION deserialization and net_processing logic and find this unlikely.
they aren't really Bitcoin Core 0.18 nodes and are just masquerading as such. They haven't implemented the relay logic properly.

The peers that I found that do this are on the spylist here: https://people.xiph.org/~greg/banlist.cli.txt

MarcoFalke commented at 4:33 PM on August 22, 2019: member

I think the nomenclature is a bit messed up. The pull request title should probably read "non-tx-relay" instead of "blocks-only"?

Or "Disconnect peer that send us tx INVs when we opted out of tx relay"

JeremyRubin commented at 5:10 PM on August 22, 2019: contributor

I have negative feelings about this as it violates the robustness principle. If at some point in the future there is a bug which causes us to relay an occasional tx, it ends up disconnecting blocks only peers.

MarcoFalke removed the label Tests on Aug 22, 2019

MarcoFalke renamed this:
~~net_processing: Disconnect blocks-only peers that send us tx INVs~~
p2p: Disconnect blocks-only peers that send us tx INVs
on Aug 22, 2019

jnewbery renamed this:
~~p2p: Disconnect blocks-only peers that send us tx INVs~~
p2p: Disconnect peer that send us tx INVs when we opted out of tx relay
on Aug 22, 2019

jnewbery commented at 5:40 PM on August 22, 2019: member

If at some point in the future there is a bug which causes us to relay an occasional tx

The main reason to run -blocksonly is to reduce bandwidth usage. If adversaries can waste our bandwidth by sending us (what is to us) garbage, then we should disconnect them.

I think your argument could be made against any disconnect behaviour: if at some point in the future there's a bug that makes us send a bad message to a peer, then we might get disconnected.

JeremyRubin commented at 6:08 PM on August 22, 2019: contributor

I don't agree. The recent blocksonly changes as presented in #15759 (comment) is as a topology privacy benefit. Modifying this logic doesn't just affect nodes running with blocksonly with that change in mind, it affects all nodes. (I'm not sure if you intend this PR to be standalone from that one or not)

I think the right thing to do would be, for example, to downgrade the connection to a non-blocksonly connection rather than disconnect and try finding a new blocksonly peer.

Disagreement nonwithstanding, IIRC there is software which will send unsolicited TX messages as well, so you may want to look into handling peers who send those as well.

jnewbery force-pushed on Aug 22, 2019

jnewbery commented at 6:59 PM on August 22, 2019: member

Modifying this logic doesn't just affect nodes running with blocksonly with that change in mind, it affects all nodes. (I'm not sure if you intend this PR to be standalone from that one or not)

Correct, but it only affects the two outbound blocks-only peers in that PR. The new behaviour would be to disconnect those blocks-only peers that are misbehaving and make new blocks-only peers. Given that the part of the justification for that PR was that the additional blocks-only peers would not cause too much additional resource usage, I think it's reasonable to disconnect them if they're wasting our bandwidth

I think the right thing to do would be, for example, to downgrade the connection to a non-blocksonly connection rather than disconnect and try finding a new blocksonly peer.

I disagree:

for a full blocksonly node, there are no non-blocksonly peers. Are you saying we should disregard the user's wishes and keep these connections as tx relay peers?
for 15759, the tx relay datastructures aren't initialized for blocks-only peers (to save memory). If we changed those blocks-only peers to be tx relay peers, we'd need to add those tx relay datastructures later. That makes the code more complex, and invalidates the resource-usage argument in that PR (since we might just end up with 10 full tx relay peers).

Disagreement nonwithstanding, IIRC there is software which will send unsolicited TX messages as well, so you may want to look into handling peers who send those as well.

Thanks! I've updated this PR to also ban peers which relay TX messages to us when we've set relay=False.

jnewbery force-pushed on Aug 22, 2019

ajtowns commented at 2:53 AM on August 23, 2019: member

Only a quick github review, but looks fine to me; maybe mark blocksonly peers as misbehaving if they send a TX/WTX GETDATA as well though? Oh, per travis you need to hold cs_main before calling Misbehaving in the NetMsgType::TX case.

I think I'd be more confident that this was a safe change to make if we merged #15759 first and could look through our logs to see how often the two blocks-only peers of any random node see this sort of misbehaviour?

I think as much as avoiding wasting bandwidth, there's an argument for this sort of change from the POV of tolerating protocol misbehaviour isn't good for the quality of the software ecosystem, a la https://tools.ietf.org/html/draft-iab-protocol-maintenance-01 . Even if that only results in higher quality spy nodes, I guess that's still an improvement...

Alternatively/longer-term, we could perhaps extend m_protect to support a "keep working if things are at all recoverable, just in case everyone's failing in the same ways" fallback behaviour. Something like:

mark your first four full outbound peers and first blocks only outbound peer as m_protect
when peers do something bad, but recoverable, if they're marked m_protect just set a questionable flag, rather than immediately disconnecting them
every ~60min on a poisson timer, if you've got a questionable==true peer, and have another m_protect=false peer of the same class that's been connected for >20min, disconnect the questionable peer and mark the other peer as m_protect

Though even that only protects you from disconnecting your outbound nodes, it doesn't prevent them from disconnecting you as an inbound node if your client is misbehaving. But if it's your client misbehaving at least you have some chance of being able to fix that directly. Could protect some of your inbound nodes too, I guess, but that doesn't seem very robust to adversarial behaviour.

fanquake requested review from sdaftuar on Aug 23, 2019

fanquake commented at 3:44 AM on August 23, 2019: member

Concept ACK - the discussion here makes sense to me, but I'm not as qualified as @ajtowns / @sdaftuar etc to comment on this.

This is failing to build on Travis macOS:

net_processing.cpp:2476:13: error: calling function 'Misbehaving' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]

            Misbehaving(pfrom->GetId(), 100, strprintf("Blocks-only peer %d sent us transaction in violation of protocol\n", pfrom->GetId()));
            ^
net_processing.cpp:2476:13: error: calling function 'Misbehaving' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
2 errors generated.
Makefile:7563: recipe for target 'libbitcoin_server_a-net_processing.o' failed

JeremyRubin commented at 4:08 AM on August 23, 2019: contributor

I think the bandwidth exhaustion argument is weak given that there's still numerous other messages (e.g., ping, pong, notfound, etc) which can be sent in blocksonly mode (unless you plan to block those too?).

I'd much rather think about bandwidth issues as a more general concept and actually track the bandwidth usage from a node and disconnect if it exceeds a limit of 'not useful' data over a certain period... We can track the nTotalBytesRecv and then count the nTotalBytesOfBlocksRecvEpoch and nTotalBytesRecvEpoch or something, and ban if we have excess non-block messages.

This is more of a whitelist approach (we count how much good stuff we're getting) v.s. a blacklist approach counting INVs as being bad while ignoring PINGs.

in src/net_processing.cpp:2262 in ae63975e9b outdated

2253 | @@ -2254,7 +2254,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2254 |              {
2255 |                  pfrom->AddInventoryKnown(inv);
2256 |                  if (fBlocksOnly) {
2257 | -                    LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol peer=%d\n", inv.hash.ToString(), pfrom->GetId());
2258 | +                    Misbehaving(pfrom->GetId(), 100, strprintf("Blocks-only peer %d sent us transaction inv (%s) in violation of protocol\n", pfrom->GetId(), inv.hash.ToString()));

luke-jr commented at 7:55 PM on August 23, 2019:

The peer isn't blocks-only, we are...

luke-jr approved

luke-jr commented at 7:55 PM on August 23, 2019: member

Concept ACK

jnewbery force-pushed on Aug 23, 2019

jnewbery commented at 10:14 PM on August 23, 2019: member

@ajtowns

per travis you need to hold cs_main before calling Misbehaving

Fixed.

I think I'd be more confident that this was a safe change to make if we merged #15759 first

Agreed. This interacts with #15759 and it's more important for that to get in, so I'll mark this as WIP for now.

maybe mark blocksonly peers as misbehaving if they send a TX/WTX GETDATA as well though?

Done. @ajtowns / @JeremyRubin

Alternatively/longer-term, we could perhaps...

I'd much rather think about bandwidth issues as a more general concept...

I don't think this small, focused fix precludes us from doing anything smarter long term. Happy to have those discussions, but I think this PR should be judged on whether it makes a small, incremental improvement (which I argue it does).

jnewbery renamed this:
~~p2p: Disconnect peer that send us tx INVs when we opted out of tx relay~~
[WIP] p2p: Disconnect peer that send us tx INVs when we opted out of tx relay
on Aug 23, 2019

DrahtBot commented at 5:41 AM on August 24, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17303 (p2p: Stop relaying non-mempool txs by MarcoFalke)
#16890 (rpc: Don't allow to 'estimatesmartfee' in blocksonly mode by darosior)

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.

jnewbery force-pushed on Aug 24, 2019

DrahtBot added the label Needs rebase on Sep 7, 2019

TheBlueMatt commented at 5:47 PM on September 7, 2019: member

#15759 implemented parts of this, tough I think a more wholesale cleanup of this logic is still required.

jnewbery force-pushed on Sep 10, 2019

jnewbery commented at 12:54 PM on September 10, 2019: member

Rebased on master, which makes the first two commits test-only changes.

Note the test change in the final commit. Previously, there was a test "Check that txs from rpc are not rejected and relayed to other peers". This is no longer possible, since when the node sends an INV to its peer, it'll be disconnected. Even if it isn't disconnected and the peer responds with a TX GETDATA, then the node will disconnect its peer for sending that GETDATA.

DrahtBot removed the label Needs rebase on Sep 10, 2019

ajtowns commented at 7:18 AM on September 11, 2019: member

Commit message for first commit needs to be updated to reflect it's not a behaviour change anymore.

Worth updating the test framework so we can have python-p2p nodes as block-relay-only outbound connections rather than just inbound connections as part of this PR? There's old commits from luke-jr that update the test framework in #10593 and I had an independant go at the same idea at https://github.com/ajtowns/bitcoin/commits/201909-p2poutbound.

I'm not sure the "txs from rpc not rejected and are relayed" change makes sense -- this means you can't use a -blocksonly node for sending your own transactions, and worse if you tried that, it'll just silently fail without giving you any obvious errors? It might make sense to move the test into the else clause -- if you're in -blocksonly you'll only have RPC-submitted tx's in mapRelay anyway, so the if path should be fine, and this way you disconnect anyone prying pretty quickly?

in test/functional/p2p_blocksonly.py:57 in cff3323a7c outdated

  55 | -        self.log.info('Check that txs from rpc are not rejected and relayed to other peers')
  56 | -        assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], True)
  57 | -        txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid']
  58 | -        with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=1'.format(txid)]):
  59 | -            self.nodes[0].sendrawtransaction(sigtx)
  60 | -            self.nodes[0].p2p.wait_for_tx(txid)

practicalswift commented at 3:17 PM on September 29, 2019:

FWIW: after this removal wait_for_tx is no longer used.

jnewbery commented at 9:24 PM on October 10, 2019:

I've made some change to this PR, so wait_for_tx is still used.

jnewbery force-pushed on Oct 10, 2019

jnewbery commented at 9:24 PM on October 10, 2019: member

@ajtowns

Commit message for first commit needs to be updated to reflect it's not a behaviour change anymore.

Done. I thought the commit log gave more useful information than commit 0ba08020c9 that was merged, but you're right that it doesn't belong on this test-change-only commit.

Worth updating the test framework so we can have python-p2p nodes as block-relay-only outbound connections rather than just inbound connections as part of this PR?

I think that's definitely worth doing, but it doesn't have to be as part of this PR. I'll happily review that change.

'm not sure the "txs from rpc not rejected and are relayed" change makes sense [...] It might make sense to move the test into the else clause

I've changed this so we only disconnect if the tx is not found in our mapRelay. Is that what you meant?

jnewbery force-pushed on Oct 11, 2019

jnewbery commented at 4:14 PM on October 11, 2019: member

The macos linter is complaining about a commit that isn't part of this PR and I have no idea why. Rebasing on master to try to resolve.

MarcoFalke commented at 5:26 PM on October 11, 2019: member

I think the linter should be removed when it is causing issues. It was added only for experimentation.

Sjors commented at 11:00 AM on October 23, 2019: member

Btcd or Lnd also does this, and bitcoind -blocksonly already disconnect in that case. ~~Even when whitelisted, which is odd.~~ (unless you give it relay whitelist, in addition to the default noban and mempool).

I noticed this while opening a channel using Lnd that was connected to my local bitcoind -blocksonly instance via p2p. It tried to broadcast the channel opening: transaction (...) inv sent in violation of protocol, disconnecting peer=10. cc @Roasbeef

The macOS linter is being removed in #17176

Roasbeef commented at 11:34 PM on October 28, 2019: none

@Sjors how is a client mean to detect that a backing node is on blocks only mode on the RPC level? On the p2p end, iirc there's no node/p2p level signalling so nodes have no idea if they're meant to relay to another node or not.

jnewbery commented at 12:58 AM on October 29, 2019: member

@roasbeef p2p nodes can use the relay field in the VERSION message to indicate that they want a peer to relay transactions. See https://btcinformation.org/en/developer-reference#version.

Roasbeef commented at 1:51 AM on October 29, 2019: none

TIL! Fixed in https://github.com/lightninglabs/neutrino/pull/190

in src/net_processing.cpp:1546 in dc84c42a69 outdated

1541 | @@ -1542,6 +1542,14 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
1542 |                  }
1543 |              }
1544 |              if (!push) {
1545 | +                if (!g_relay_txes && !pfrom->HasPermission(PF_RELAY)) {
1546 | +                    // If a blocks-only peer requests a tx and its not in our mapRelay, then disconnect them.

ajtowns commented at 8:01 AM on October 29, 2019:

But we'll have already tried relaying from the mempool and set push to true in that case, causing this code path to only work if we didn't have the tx at all? Am I missing something?

jnewbery commented at 9:00 PM on October 29, 2019:

no, you're not missing anything. You're completely right.

I think this logic needs to be moved up between the if (mi != mapRelay.end()) block and else if (pfrom->m_tx_relay->m_last_mempool_req.load().count()). I've done that in https://github.com/bitcoin/bitcoin/compare/dc84c42a69557bff3d41baea498ceee822566423..dffa26f05fca414ecd2e72c6c9a7efc953d54651. Let me know what you think.

jnewbery force-pushed on Oct 29, 2019

[test] Clean up testing for disconnecting blocks-only peers that send us tx INVs

This commit restructures the p2p_blocksonly.py test case in preparation
for adding more tests. In future commits, we'll test for disconnecting
blocks-only peers that send us TX messages or tx GETDATA messages.

c21a98a63e

[test] Add testing for disconnecting blocks-only peers that send us TXs

If a blocks-only peer sends us a TX message, we should disconnect. Add a
test for this.

2964f78e81

[net_processing] Disconnect blocks-only peers that request txs

Disconnect any blocks-only peer that sends us tx/witness tx GETDATAs for
a tx that isn't in our mapRelay.

We continue to allow GETDATAs for txs in our mapRelay so that
transactions submitted locally (by RPC/wallet) can still be relayed over
blocks-only links.

1c6fb2da65

jnewbery force-pushed on Oct 29, 2019

jnewbery commented at 9:08 PM on October 29, 2019: member

That change caused a conflict with master so I've rebased.

jnewbery renamed this:
~~[WIP] p2p: Disconnect peer that send us tx INVs when we opted out of tx relay~~
p2p: Disconnect peer that send us tx INVs when we opted out of tx relay
on Oct 30, 2019

in src/net_processing.cpp:1545 in 1c6fb2da65

1537 | @@ -1538,6 +1538,13 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
1538 |              if (mi != mapRelay.end()) {
1539 |                  connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *mi->second));
1540 |                  push = true;
1541 | +            } else if (!g_relay_txes && !pfrom->HasPermission(PF_RELAY)) {
1542 | +                // If a blocks-only peer requests a tx and it isn't in our mapRelay, then disconnect them.
1543 | +                // We allow blocks-only peers to request txs in our mapRelay so we can relay txs submitted
1544 | +                // locally when in blocks-only mode.
1545 | +                LogPrintf("Blocks-only peer %d sent us tx GETDATA in violation of protocol\n", pfrom->GetId());

sdaftuar commented at 8:43 PM on November 1, 2019:

I believe this is incorrect -- it should not be a protocol violation if we announce a transaction to a peer and it happens to wait more than 15 minutes to respond with a GETDATA. This already happens for reasons that are benign (due to us not immediately requesting a transaction from all peers who announce it).

For example: one way this can happen is if we announce a transaction to a peer, but it gets the announcement as well from other peers first and queues up a request to go to us far in the future, and in the meantime a block is found that includes the transaction as well as spends of its outputs, and then at some point when it's time for the peer to consider requesting the tx from us it has at that point forgotten about the tx completely and it'll re-request. (This type of behavior is present in all the version of Bitcoin Core I can remember.)

I think to do this right we would need to either track the tx's we've announced to a peer, or consider changing blocks-only behavior so that we never announce anything at all (which makes more logical sense to me, but I think others disagree, so probably a non-starter).

jnewbery commented at 8:52 PM on November 1, 2019: member

ok, closing this. #15759 took the more important changes from this PR (disconnecting peers that send us TXs and tx INVs), and we can't seem to reach agreement on the remaining change: @ajtowns

I'm not sure the "txs from rpc not rejected and are relayed" change makes sense -- this means you can't use a -blocksonly node for sending your own transactions, @sdaftuar changing blocks-only behavior so that we never announce anything at all (which makes more logical sense to me, but I think others disagree, so probably a non-starter).

jnewbery closed this on Nov 1, 2019

DrahtBot locked this on Dec 16, 2021