-
Notifications
You must be signed in to change notification settings - Fork 37.4k
Fetch Headers over DNS #16834
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
Fetch Headers over DNS #16834
Conversation
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. |
Concept ACK: really like these ideas/proof-of-concepts on how to improve block/block header transport diversity. Thanks for working on this @TheBlueMatt! This "headers over DNS" idea could be combined with DNS over HTTPS (DoH) using widely used DoH servers. DNS lookups are trivial to block/filter for a MITM adversary but if the user is instead doing lookups using DoH with a widely used DoH-server (think Mozilla, Google or CloudFlare or similar large scale operation) -- that would be hard to null route/block without upsetting non-Bitcoin users. |
125a50a
to
85f2418
Compare
Interesting idea! |
Tried to test this, but doesn't look like $ host 59999.5.bitcoinheaders.net
59999.5.bitcoinheaders.net has IPv6 address 2001:53ef:f60e:4c5c:670f:1c16:2d0a:300
...
$ host 60000.6.bitcoinheaders.net
Host 60000.6.bitcoinheaders.net not found: 2(SERVFAIL) Can bind (or DNS, for that matter) handle huge numbers of hosts like this? |
Right, I'm currently updating the zones. Bind can't handle huge zones so well hence why it got split up into chunks of 10k, and I'm currently slowly churning through dnssec signing for the larger zones. They should be up in a few hours. |
Should be good now. Turns out bind's inline dnssec signing has some hangs on big zones, worked fine once I manually signed them.
… On Sep 9, 2019, at 13:30, Matt Corallo ***@***.***> wrote:
Right, I'm currently updating the zones. Bind can't handle huge zones so well hence why it got split up into chunks of 10k, and I'm currently slowly churning through dnssec signing for the larger zones. They should be up in a few hours.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
85f2418
to
12372db
Compare
Concept ACK. Discussing with @theuni, there might be the possibility to simplify this even further by dropping the use of |
37c9c7b
to
a4b948b
Compare
src/rusty/src/bridge.rs
Outdated
pub fn hash(&self) -> [u8; 32] { | ||
let hashptr = unsafe { rusty_IndexToHash(self.index) }; | ||
let mut res = [0u8; 32]; | ||
unsafe { std::ptr::copy(hashptr, res.as_mut_ptr(), 32) }; |
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.
You use the pointer without checking for nullness, I know that you already asserted that on the C++ side, but I think we should be consistent on which side of the FFI checks for null. (and if the rust side then using NonNull
everywhere forces you to check)
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.
I added an if is_null() { unreachable!(); } which I think is the right approach - it clearly isn't allowed to be null, but its safe and will panic inside rust.
src/rusty/src/dns_headers.rs
Outdated
|
||
#[no_mangle] | ||
pub extern "C" fn init_fetch_dns_headers(domain: *const c_char) -> bool { | ||
let domain_str: String = match unsafe { CStr::from_ptr(domain) }.to_str() { |
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.
Check for nullness?
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.
Meh? I mean its called during init from C++, it would be kinda hard to get a bug here.
src/rusty/src/dns_headers.rs
Outdated
std::thread::spawn(move || { | ||
// Always catch panics so that even if we have some bug in our parser we don't take the | ||
// rest of Bitcoin Core down with us: | ||
THREAD_COUNT.fetch_add(1, Ordering::AcqRel); |
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.
Could this also panic somehow? (it calls atomic_xadd_acqrel
and I can't find the source code for it)
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.
I dont think so? Certainly in practice it can't panic since it just compiles down to a few instructions, but the docs even say it wraps, which seems like the only type of panic I could imagine.
Personally I really like https://doc.rust-lang.org/std/ptr/struct.NonNull.html as it gives a little bit more compile time assurance (requires you to check for nullness and after you checked you easily know you don't need to check anymore) Edit: I'm actually not sure anymore. Because with NonNull it's hard to identify const VS mut pointers. |
None of the travis jobs built this, nor gitian |
3871cbe
to
16b6727
Compare
Should be run in travis now, but waiting to see it run. |
f2564a6
to
5c50c0e
Compare
5c50c0e
to
0a3c6d2
Compare
tACK 0a3c6d2 |
0a3c6d2
to
24dfea8
Compare
24dfea8
to
c510231
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>.
c510231
to
94051ad
Compare
Needs rebase |
Closing due to lack of interest. |
Aw, sadly I've missed this at the time. As I told to Matt elsewhere this reminded me of chaindnsd, a POC I did some years ago... and I would loved that feature into the reference impl :-) |
This allows us to fetch headers from the DNS. While this isn't very useful on its own, its a somewhat cool demo of how low the review burden could be to add new fetch methods in Rust code, and could be a useful to know whether we're behind (eg for stale tip checking). You can test with -headersfetchdns=headers.bitcoinheaders.net which is served from a standard bind named with the following generation scripts: