Broadcast transactions through tor

[andrewtoth]

I copied your code at https://github.com/JoinMarket-Org/joinmarket/blob/master/joinmarket/peertopeer.py almost verbatim and translated to Python3, as well as https://github.com/JoinMarket-Org/joinmarket/blob/master/joinmarket/socks.py.

It takes a new tor broadcast_method in the config, and you can optionally specify tor_host and tor_port otherwise falls back to localhost:9050. The p2p network messages are different depending on network, so not sure if it'd be worth putting that in the config. Currently hardcoded mainnet and 4 attempts at connections.

Let me know what you think.

Resolves #52.

[chris-belcher]

Thanks for the PR, good job. Here's some thoughts, including some inline comments.

• the reject p2p message has been removed, so that part of the code should be removed. We can only depend on the testmempoolaccept RPC call to check whether the tx is valid https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.18.0.md#deprecated-p2p-messages
• IP addresses returned by getnodeaddresses might be unreachable, so the broadcasting threads should keep trying with new addresses until they find one which works. The RPC call should be called inside the thread so it can keep looping.
• The peertopeer.py code has this crash bug crash when broadcasting through tor JoinMarket-Org/joinmarket#736 which should be fixed. Probably what's needed is to make on_heartbeat() check the timings only after the verack message has been received. Edit: Also there needs to be some timeout in case the peer just hangs and never send verack.
• Perhaps the timeout values should maybe be increased a lot, because some nodes are just slow and that's fine. I think Bitcoin Core uses several minutes. On the other hand long timeouts mean the transaction might take long to be broadcasted.
• That file contains code to connect with the DNS seeds. That should be removed because it is unnecessary now that we have getnodeaddresses.
• When testing we should make sure that the code works for connecting to .onion addresses.
• Edit the file config.cfg_sample to add the new options and explain how to use tor broadcasting.
• I didn't understand this: The p2p network messages are different depending on network, so not sure if it'd be worth putting that in the config. Edit: Oh I get it, it's testnet vs mainnet vs regtest. I'd say the best solution is to use the RPC call getblockchaininfo under the value chain which tells you which network is being used.

[andrewtoth]

The peertopeer.py code has this crash bug JoinMarket-Org/joinmarket#736 which should be fixed. Probably what's needed is to make on_heartbeat() check the timings only after the verack message has been received. Edit: Also there needs to be some timeout in case the peer just hangs and never send verack.

I believe I fixed this with with a VERACK_TIMEOUT. https://github.com/chris-belcher/electrum-personal-server/pull/124/files#diff-f0790d116274424a09d45a4fc373217fR353

When testing we should make sure that the code works for connecting to .onion addresses.

I tested this already and it works. ipv6 does not work with this code, however.

Will update with all your other suggestions. Thanks.

[chris-belcher]

I just realized that since Electrum uses segwit we need to set NODE_WITNESS services ourselves and only connect to nodes with NODE_WITNESS services. So we should filter the results of getnodeaddresses to only obtain addresses which implement segwit.

See also: https://github.com/bitcoin/bips/blob/master/bip-0144.mediawiki#handshake Maybe add another constant at the top of the file NODE_WITNESS = (1 << 3) and use that.

Also, I'm in two minds about whether to really set the user agent to /ElectrumPersonalServer:0.1.8/ instead of just /Satoshi:0.18.0/. On the one hand we advertise that we're tor broadcasting which is a reason not to keep it (though that can be easily detected because of our behaviour and that the list of all tor exit nodes is public), on the other hand if lots of EPS user agents end up in the logs of adveraries then then they might lose faith in their analysis and stop doing it.
On balance I think it's better to set it to /Satoshi:0.18.0/ and pretend to be Bitcoin Core, because of the steganographic principle: https://en.bitcoin.it/wiki/Privacy#Steganography. Pretending to be Core means that a spy adversary has to do more work to even tell that tor broadcasting is being used, and that can only be a good thing.

[chris-belcher]

VERACK_TIMEOUT increased to 40 seconds still means that if the peer doesn't reply for 40 seconds (whether intentionally or just by being slow) then it will crash one of the threads. EDIT: I'm wrong. I'll read it more closely and test.

[andrewtoth]

I think I addressed everything with the latest commit. I can squash once you're happy with it.

[andrewtoth]

Note that it takes a while to broadcast on testnet sometimes, since there aren't many listening nodes.

[chris-belcher]

Thanks. I've added some comments.

How long does it normally take to broadcast? It's expected that it will be a bit slower, but ideally it will be as fast as possible. We could try raising TOR_CONNECTIONS to something like 8 or 12 so there are more threads concurrently searching for a working node. When this is merged I'll try to get some power users to try it out on master and see how it works for them and how slow/fast it is. It can also be tested by commenting out the part which actually uploads the transaction, and trying it on mainnet (for free, because your unconfirmed transaction never gets broadcast) Edit: maybe its worth having a log.info() message when the transaction is being uploaded, as a feedback for the user.

[andrewtoth]

When I hardcoded an onion address it worked almost instantly, since it doesn't have to go through an exit node. Too bad we can't tell listening nodes from getnodeaddresses. Upping TOR_CONNECTIONS might be a good idea.

[andrewtoth]

Updated with your feedback.

[chris-belcher]

Thanks for sticking with it. I think it looks good, I'll try testing it soon.

[andrewtoth]

Edit: maybe its worth having a log.info() message when the transaction is being uploaded, as a feedback for the user.

As soon as a transaction is uploaded it is propagated to the local node within a few seconds at most, so the feedback happens for the user like that. However, as soon as the server is done spawning all the upload threads the user gets a Payment sent information popup. I'm thinking now that maybe it would be better to refactor this to have it remain at the Broadcasting transaction... please wait popup until a successful upload works. But I'll wait for you to test and see what you think.

[andrewtoth]

I tested on mainnet with your suggestion of not actually broadcasting it, and it works much better than testnet. Usually at least 1 of the first 8 tries finds a listening node.

[chris-belcher]

Could you squish this into one commit

[andrewtoth]

Done