-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rust-based Backup over-REST block downloader #16762
Rust-based Backup over-REST block downloader #16762
Conversation
88ad6bf
to
44b0549
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
b3a4969
to
cf221a2
Compare
src/Makefile.am
Outdated
@@ -529,7 +537,7 @@ nodist_libbitcoin_util_a_SOURCES = $(srcdir)/obj/build.h | |||
bitcoind_SOURCES = bitcoind.cpp | |||
bitcoind_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) | |||
bitcoind_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) | |||
bitcoind_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) | |||
bitcoind_LDFLAGS = -Wl,--start-group $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICS --start-group
is not supported in clang (https://bitcoinbuilds.org/index.php?ansilog=6ccfb93d-3437-41c0-99d4-3b6653264739.log#l1705)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonasschnelli Yes ld
on macOS doesn't support --start-group
or --end-group
. This was a bit of a hack to fix a circular dependency issue Matt was seeing on his Debian machine. If you want to compile on macOS you should just be able to drop the -Wl,--start-group
additions, as that has been working for me. We'll need to fix this properly soon.
Interesting idea: cautious concept ACK -- more redundancy is good and it is hard to see how this would make us worse off in any way as long as the existing P2P logic is kept unchanged. (Intentionally not commenting on the implementation and more specifically the pros and cons of pulling in Rust code: that is an implementation detail that I'm sure will be discussed separately post concept ACK/NACK.) Censorship scenarios worth thinking about: Even if we do the over-REST block downloads over HTTPS it should be noted that the TLS handshake leaks the hostname of the connected to service to passive eavesdroppers. (Unless TLS 1.3 + Encrypted SNI is used: super rare in practice though.) The hostname is obviously also leaked when doing the DNS lookup. (Unless DoH (DNS over HTTPS) or DoT (DNS over TLS) is used: quite rare in practice.) This allows for active censorship attacks if the hostnames used are only used for Bitcoin purposes (such as in the (The same obviously goes for the IP-address used: if the IP-address connected to is only used for Bitcoin purposes then access to that IP-address could trivially be null routed without any non-Bitcoin users noticing.) I guess one way to achieve (partial) censorship resistance is to download over HTTPS URLs where the hostnames used are mostly used for non-Bitcoin purposes (such as Another route would be to use TLS 1.3 + Encrypted SNI + DoH/DoT to connect to a service whose IP-address(es) are used mostly for non-Bitcoin purposes (such as CloudFlare which luckily supports both TLS 1.3 and Encrypted SNI). That wouldn't leak the hostname and the IP-address would be hard to null route for an attacker without having non-Bitcoin users complain. |
Right, there's a few things that would need to happen to get this in mergeable state (notably buildsystem things), so no need for that just yet.
Right, two things I'd like to support in the blocks-over-REST module: domain fronting avoids most of the gnarly issues with SNI/DNS lookups breaking your privacy, though sadly many providers don't like it anymore, and Tor support, possibly even with some default onion service recommended as a backup cause privacy is less of a concern there. I'd also like to build a whole second P2P implementation (though with an explicit goal of being inefficient and simple, with the same trick as here where you wait 30 seconds before deciding to download anything to avoid wasting bandwidth), but first need to land some bit of rust downloading, and this seemed like an easier target. |
cf221a2
to
5d26d51
Compare
But why Rust not C++? |
Because we can trivially turn on compile-time checking preventing a ton of classes of vulnerabilities (especially for network-facing code), and runtime checking for many other classes (like buffer overflows, at least those the compiler can't prove aren't possible). |
I'm fairly Concept ACK on this. It's been in discussion for a little while, and I'm pretty sure it came up at the last Core dev meetup as well?
@kristapsk This PR isn't really the place to have that discussion and is only going to distract from what's actually trying to be achieved. Although Matt points out one big advantage:
If you'd like to discuss Rust inside Bitcoin Core further, I'd suggest taking a look at #15798. Here are Cory's thoughts on Rust from that thread:
I built and tested that this functions using the instructions above. The backup downloader kicked off after a 10 minute wait src/bitcoind -blockfetchrest=http://cloudflare.deanonymizingseed.com/rest/ -connect=0
2019-08-31T05:37:06Z nBestHeight = 311015
2019-08-31T05:37:06Z init message: Loading P2P addresses...
2019-08-31T05:37:06Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there
2019-08-31T05:37:06Z Loaded 63409 addresses from peers.dat 159ms
2019-08-31T05:37:06Z init message: Starting network threads...
2019-08-31T05:37:06Z DNS seeding disabled
2019-08-31T05:37:06Z net thread start
2019-08-31T05:37:06Z msghand thread start
2019-08-31T05:37:06Z addcon thread start
2019-08-31T05:37:06Z init message: Done loading
2019-08-31T05:47:10Z Synchronizing blockheaders, height: 313014 (~53.91%)
2019-08-31T05:47:13Z Synchronizing blockheaders, height: 315013 (~54.23%)
2019-08-31T05:47:15Z Synchronizing blockheaders, height: 317012 (~54.55%)
2019-08-31T05:47:17Z Synchronizing blockheaders, height: 319011 (~54.87%)
< snip >
2019-08-31T05:52:27Z Synchronizing blockheaders, height: 574883 (~97.17%)
2019-08-31T05:52:34Z Synchronizing blockheaders, height: 576882 (~97.49%)
2019-08-31T05:52:36Z Synchronizing blockheaders, height: 578881 (~97.81%)
2019-08-31T05:52:39Z Synchronizing blockheaders, height: 580880 (~98.14%)
2019-08-31T05:52:41Z Synchronizing blockheaders, height: 582879 (~98.46%)
2019-08-31T05:52:44Z Synchronizing blockheaders, height: 584878 (~98.76%)
2019-08-31T05:52:47Z Synchronizing blockheaders, height: 586877 (~99.09%)
2019-08-31T05:52:49Z Synchronizing blockheaders, height: 588876 (~99.40%)
2019-08-31T05:52:56Z Synchronizing blockheaders, height: 590875 (~99.73%)
2019-08-31T05:52:59Z Synchronizing blockheaders, height: 592551 (~100.00%)
2019-08-31T05:53:35Z Pre-allocating up to position 0x3000000 in blk00158.dat
2019-08-31T05:53:35Z Pre-allocating up to position 0x100000 in rev00158.dat
2019-08-31T05:53:35Z UpdateTip: new best=0000000000000000246039d52e90fb6b7cd442dda19861fb948c28e3a44f7a4e height=311016 version=0x00000002 log2_work=79.744641 tx=42644694 date='2014-07-16T13:51:22Z' progress=0.095646 cache=0.5MiB(3475txo)
2019-08-31T05:53:38Z UpdateTip: new best=000000000000000014c552ced25645452a0b8632753e9288e2decedd273c5828 height=311017 version=0x00000002 log2_work=79.744747 tx=42645541 date='2014-07-16T14:10:17Z' progress=0.095648 cache=1.0MiB(7067txo)
2019-08-31T05:53:40Z UpdateTip: new best=0000000000000000260ef522f4875fb0d5207e4e3ac97ce373a47209bcee52b6 height=311018 version=0x00000002 log2_work=79.744853 tx=42645990 date='2014-07-16T14:18:18Z' progress=0.095649 cache=1.2MiB(8621txo)
2019-08-31T05:53:42Z UpdateTip: new best=0000000000000000007dabdebadfde7beaef31d13133600d0d88695146e2fac6 height=311019 version=0x00000002 log2_work=79.744959 tx=42646068 date='2014-07-16T14:19:50Z' progress=0.095649 cache=1.2MiB(8821txo)
2019-08-31T05:53:45Z UpdateTip: new best=00000000000000001fc78d459e2480177ceea92d53f75cdbd856501b60cbdc3a height=311020 version=0x00000002 log2_work=79.745065 tx=42646933 date='2014-07-16T14:34:32Z' progress=0.095651 cache=1.6MiB(11938txo)
< snip >
2019-08-31T06:00:41Z UpdateTip: new best=000000000000000017de2992c0de3e98d711253582fa7ee626645e395e4fd0e5 height=311697 version=0x00000002 log2_work=79.815121 tx=42899222 date='2014-07-20T16:20:21Z' progress=0.096217 cache=67.4MiB(497634txo)
2019-08-31T06:00:44Z UpdateTip: new best=000000000000000022aeedbb76913a3666d997897c23efc8e92fe38ec79728a7 height=311698 version=0x00000002 log2_work=79.815222 tx=42899470 date='2014-07-20T16:25:14Z' progress=0.096217 cache=68.0MiB(502949txo)
2019-08-31T06:00:47Z UpdateTip: new best=000000000000000027a68e70c9c02985173c56e8c8ce210c02eb975530d77f0d height=311699 version=0x00000002 log2_work=79.815323 tx=42899561 date='2014-07-20T16:27:06Z' progress=0.096218 cache=68.3MiB(505709txo)
2019-08-31T06:00:51Z UpdateTip: new best=000000000000000038730cd937f594452e96b97a496afc03a84fc90a18119113 height=311700 version=0x00000002 log2_work=79.815424 tx=42900165 date='2014-07-20T16:40:48Z' progress=0.096219 cache=68.8MiB(509386txo) FWIW I've also rebased and added another commit for a more recent Rust onto https://github.com/fanquake/bitcoin/tree/theuni-with-rust-example-working-rebased. Will add another commit on top for the |
Concept ACK on adding more redundancy. |
…ages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes #16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in #15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on #16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
Concept ACK! |
Could we try and use something like https://github.com/dtolnay/no-panic on parts of the code? (if we can, this will give us compile time guarantees of no panics) Another Idea, checking nullness at the boundaries and then use https://doc.rust-lang.org/std/ptr/struct.NonNull.html (can even use Other than that I like this and would love to review it when it's reviewable :) |
I'd rather deliberately use panics so that we can enable overflow checking and catch the panics to kill the Rust thread(s) but leave the rest of Core running. Good call on NonNull, will update to use that! |
d81ea21
to
32d6faf
Compare
Aside from the final commit working around one last build issue, I believe this is ready for review. Doesn't make sense to land for 19, but would be nice to land early in the cycle for 20. |
32d6faf
to
bb33e04
Compare
f5e095a
to
e291a1f
Compare
e291a1f
to
30091e5
Compare
…st code. The demonstration library compiles a rust hello world example and auto-generates a header which can be included in C++ code. Co-Authored-By: Jeremy Rubin <j@rubin.io> Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com> Various changes by: Matt Corallo <git@bluematt.me>
Also, break circular dependency with a new helper lib courtesy of Cory Fields <cory-nospam-@coryfields.com>.
This starts the process of moving the last bits of validation-critical logic out of net_processing - the fallback logic of which blocks to download is incredibly critical to validation and would likely also be used for any parallel block sync systems. Further, assumeutxo will require additional ability to select blocks for download, which is a validation-specific concept and shouldn't really live in net_processing. This moves a few responsibilities to the FindNextBlocksToDownload callsite instead of passing in a CNodeId, but otherwise has no functional changes.
This makes it more general than just net_processing
30091e5
to
2c0dda3
Compare
Needs rebase |
Rust is still a concept NACK... |
Closing due to lack of interest. |
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
…og messages dcc448e Avoid unnecessary "Synchronizing blockheaders" log messages (Jonas Schnelli) Pull request description: Fixes bitcoin#16773 I'm not entirely sure why 16773 happend, but probably due to headers fallback in a compact block. However, this PR should fix it and should have been included in bitcoin#15615. ACKs for top commit: ajtowns: ACK dcc448e ; code review only, haven't compiled or tested. promag: ACK dcc448e. TheBlueMatt: utACK dcc448e. Went and read how pindexBestHeader is handled and this code looks correct (worst case it breaks a LogPrint, so whatever). I also ran into this on bitcoin#16762. fanquake: ACK dcc448e Tree-SHA512: f8cac3b6eb9d4e8fab53a535b55f9ea9b058e3ab6ade64801ebc56439ede4f54b5fee36d5d2b316966ab987b65b13ab9dc18849f345d08b81ecdf2722a3f5f5a
This is based on #15798.
I'm increasingly worried about censorship and other attacks in the P2P network (see, eg, https://twitter.com/TheBlueMatt/status/1160620919775211520). While obviously making the net_processing p2p implementation more robust should be the focus (eg #15759), adding more redundancy in every way is also key. To avoid needless review burden on such systems, I think rust is an ideal candidate: it is wholly optional so we don't just right to requiring Rust for Bitcoin Core, but building with it still provides real benefit, plus we can use compile-time checks to catch any overflows and memory corruption bugs to reduce review burden.
This PR implements a simple API for Rust code to fetch headers and blocks and then implements a sample client that can sync the chain using a REST endpoint. As a demo, I provide a rest endpoint over cloudflare at http://cloudflare.deanonymizingseed.com/rest/ so you should be able to fully sync the chain (albeit very slowly) with bitcoind -blockfetchrest=http://cloudflare.deanonymizingseed.com/rest/ -connect=0
There's a few things to do before this is mergable, but I'd like feedback on the idea, and would like to also have a fully parallel, in rust, P2P net layer that is similarly simple and provides a nice redundant implementation in the future.