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

jnewbery · 2019-08-22T15:33:22Z

-blocksonly mode was introduced in
4044f07 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 · 2019-08-22T15:36:14Z

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

dongcarl · 2019-08-22T15:45:48Z

tACK 0e7bc2f

NicolasDorier · 2019-08-22T16:04:27Z

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

dongcarl · 2019-08-22T16:07:07Z

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.

instagibbs · 2019-08-22T16:23:51Z

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

jnewbery · 2019-08-22T16:32:42Z

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

maflcko · 2019-08-22T16:33:47Z

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 · 2019-08-22T17:10:56Z

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.

jnewbery · 2019-08-22T17:40:57Z

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 · 2019-08-22T18:08:30Z

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 · 2019-08-22T18:59:41Z

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.

ajtowns · 2019-08-23T02:53:17Z

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 · 2019-08-23T03:44:49Z

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 · 2019-08-23T04:08:38Z

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.

luke-jr

Concept ACK

luke-jr · 2019-08-23T19:55:23Z

src/net_processing.cpp

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


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

jnewbery · 2019-08-23T22:14:48Z

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

DrahtBot · 2019-08-24T05:41:17Z

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.

TheBlueMatt · 2019-09-07T17:47:04Z

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

jnewbery · 2019-09-10T12:54:01Z

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.

ajtowns · 2019-09-11T07:18:29Z

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?

practicalswift · 2019-09-29T15:17:54Z

test/functional/p2p_blocksonly.py

-        txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid']
-        with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=1'.format(txid)]):
-            self.nodes[0].sendrawtransaction(sigtx)
-            self.nodes[0].p2p.wait_for_tx(txid)


FWIW: after this removal wait_for_tx is no longer used.

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

jnewbery · 2019-10-10T21:24:21Z

@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 0ba0802 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 · 2019-10-11T16:14:37Z

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.

maflcko · 2019-10-11T17:26:25Z

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

Sjors · 2019-10-23T11:00:58Z

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 · 2019-10-28T23:34:12Z

@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 · 2019-10-29T00:58:05Z

@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 · 2019-10-29T01:51:09Z

TIL! Fixed in lightninglabs/neutrino#190

ajtowns · 2019-10-29T08:01:43Z

src/net_processing.cpp

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


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?

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.

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

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

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.

jnewbery · 2019-10-29T21:08:38Z

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

sdaftuar · 2019-11-01T20:43:45Z

src/net_processing.cpp

+                // If a blocks-only peer requests a tx and it isn't in our mapRelay, then disconnect them.
+                // We allow blocks-only peers to request txs in our mapRelay so we can relay txs submitted
+                // locally when in blocks-only mode.
+                LogPrintf("Blocks-only peer %d sent us tx GETDATA in violation of protocol\n", pfrom->GetId());


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 · 2019-11-01T20:52:59Z

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

dongcarl mentioned this pull request Aug 22, 2019

Replace -banscore with -ignoremisbehaviour #16683

Closed

DrahtBot added P2P Tests labels Aug 22, 2019

maflcko removed the Tests label Aug 22, 2019

maflcko changed the title ~~net_processing: Disconnect blocks-only peers that send us tx INVs~~ p2p: Disconnect blocks-only peers that send us tx INVs Aug 22, 2019

jnewbery changed the title ~~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 Aug 22, 2019

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch from 0e7bc2f to 6f4151f Compare August 22, 2019 18:51

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch from 6f4151f to ae63975 Compare August 22, 2019 19:01

fanquake requested a review from sdaftuar August 23, 2019 03:41

luke-jr approved these changes Aug 23, 2019

View reviewed changes

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch from ae63975 to 46caa27 Compare August 23, 2019 22:08

jnewbery changed the title ~~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 Aug 23, 2019

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch from 46caa27 to 5ff415d Compare August 24, 2019 13:42

DrahtBot added the Needs rebase label Sep 7, 2019

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch 2 times, most recently from bac862c to cff3323 Compare September 10, 2019 12:52

DrahtBot removed the Needs rebase label Sep 10, 2019

practicalswift reviewed Sep 29, 2019

View reviewed changes

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch from cff3323 to b2741ec Compare October 10, 2019 21:21

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch from b2741ec to dc84c42 Compare October 11, 2019 16:13

ajtowns reviewed Oct 29, 2019

View reviewed changes

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch from dc84c42 to dffa26f Compare October 29, 2019 20:53

jnewbery added 3 commits October 29, 2019 16:55

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

2964f78

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

jnewbery force-pushed the 2019-08-disconnect-blocksonly-violators branch from dffa26f to 1c6fb2d Compare October 29, 2019 21:08

jnewbery changed the title ~~[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 Oct 30, 2019

sdaftuar reviewed Nov 1, 2019

View reviewed changes

jnewbery closed this Nov 1, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Conversation

jnewbery commented Aug 22, 2019

jnewbery commented Aug 22, 2019

dongcarl commented Aug 22, 2019 • edited Loading

NicolasDorier commented Aug 22, 2019

dongcarl commented Aug 22, 2019 • edited Loading

instagibbs commented Aug 22, 2019

jnewbery commented Aug 22, 2019

maflcko commented Aug 22, 2019 • edited Loading

JeremyRubin commented Aug 22, 2019

jnewbery commented Aug 22, 2019

JeremyRubin commented Aug 22, 2019

jnewbery commented Aug 22, 2019

ajtowns commented Aug 23, 2019

fanquake commented Aug 23, 2019 • edited Loading

JeremyRubin commented Aug 23, 2019

luke-jr left a comment

Choose a reason for hiding this comment

luke-jr Aug 23, 2019

Choose a reason for hiding this comment

jnewbery commented Aug 23, 2019

DrahtBot commented Aug 24, 2019 • edited Loading

Conflicts

TheBlueMatt commented Sep 7, 2019

jnewbery commented Sep 10, 2019

ajtowns commented Sep 11, 2019

practicalswift Sep 29, 2019

Choose a reason for hiding this comment

jnewbery Oct 10, 2019

Choose a reason for hiding this comment

jnewbery commented Oct 10, 2019

jnewbery commented Oct 11, 2019

maflcko commented Oct 11, 2019

Sjors commented Oct 23, 2019 • edited Loading

Roasbeef commented Oct 28, 2019

jnewbery commented Oct 29, 2019

Roasbeef commented Oct 29, 2019

ajtowns Oct 29, 2019

Choose a reason for hiding this comment

jnewbery Oct 29, 2019

Choose a reason for hiding this comment

jnewbery commented Oct 29, 2019

sdaftuar Nov 1, 2019

Choose a reason for hiding this comment

jnewbery commented Nov 1, 2019

dongcarl commented Aug 22, 2019 •

edited

Loading

dongcarl commented Aug 22, 2019 •

edited

Loading

maflcko commented Aug 22, 2019 •

edited

Loading

fanquake commented Aug 23, 2019 •

edited

Loading

DrahtBot commented Aug 24, 2019 •

edited

Loading

Sjors commented Oct 23, 2019 •

edited

Loading