Opened 8 months ago

Closed 6 weeks ago

Last modified 6 weeks ago

#30558 closed defect (fixed)

Namecoin support for onion sites in Tor Browser

Reported by: arthuredelstein Owned by: JeremyRand
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201912R
Cc: pili, gk, antonela, asn, boklm, mcs Actual Points:
Parent ID: Points:
Reviewer: gk Sponsor:

Description

The problem
Onion domains are generally almost impossible for humans to remember. Specifically, they are very long and consist of a series of random characters.

v2 domains look like this:

and v3 domains look like this:

So, while onion domains are secure and decentralized, they are not human-meaningful, and thus fail to satisfy all three desired properties described in Zooko's triangle.

Proposed solution
Namecoin offers a solution for Zooko's triangle. Domains are registered in a decentralized manner, can be remembered by humans, and are secure. A Namecoin (.bit) domain looks like this:

The .bit domains can be pointed to a unique .onion domain. So the user needs only to enter http://federalistpapers.bit and they will be taken to the appropriate onion site (in this case, http://7fa6xlti5joarlmkuhjaifa47ukgcwz6tfndgax45ocyn4rixm632jid.onion)

The task consists of writing patches for Tor Browser that integrates a Namecoin lookup client, such that when a user enters a .bit domain name the browser is connected to the underlying .onion site. In the address bar, the entered address including a .bit domain will continue to be shown, and the .onion domain will be indicated on the circuit display.

Initially, the patches can be integrated into Tor Browser Nightly. If testing is successful, I hope it could progress to Tor Browser alpha and eventually stable.

Comparison to other approaches
There are several promising approaches to allowing human-meaningful aliases to onion sites. However, they don't fully solve Zooko's triangle:

  • HTTPS Everywhere: Aliases are under central control by the addon maintainer.
  • Bookmarks/Petnames: Aliases are not global.
  • Alt-Svc/Onion-Location: Aliases require first connecting through a centralized ICANN domain.

I think Namecoin is especially promising because it can be globally registered and maintained securely by the onion site operator, without any centralized permission. Thus the properties of security and decentralization offered by .onion domains are shared by .bit domains.

There are some challenges:

  • Historically, Namecoin lookup has been slow and required cumbersome downloads. Jeremy has made major progress in reducing the footprint.
  • Registering a Namecoin domain requires downloading specialized software and is not anonymous without special precautions. Future work (out of scope here) could include building documentation and/or software tools to allow onion operators to easily and anonymously register a .bit domain and point it to a .onion domain.

Child Tickets

Attachments (1)

log (134.3 KB) - added by gk 6 weeks ago.

Download all attachments as: .zip

Change History (65)

comment:1 Changed 8 months ago by gk

Cc: gk added; GeKo removed
Parent ID: #30029

Note, even though this has #30029 as parent ticket, it is not related to our sponsor work. Thus, I am unparenting the ticket, to make that clear (i.e. #30029 is not blocking on this ticket).

Last edited 8 months ago by gk (previous) (diff)

comment:2 Changed 8 months ago by cypherpunks

HTTPS Everywhere: Aliases are under central control by the addon maintainer.

NO, HE supports different streams.

comment:3 Changed 7 months ago by boklm

Cc: boklm added

comment:4 Changed 7 months ago by JeremyRand

Early branch: https://notabug.org/JeremyRand/tor-browser-build/src/namecoin (you want the namecoin branch).

The 3 most major shortcomings in its current state are:

  1. ncprop279 isn't built locally, but instead is pulled from a binary release on namecoin.org. This is because ncprop279 currently needs a different version of the Go compiler than tor-browser-build uses, so it needs to be built in its own tree. The binary that's being pulled in is (in theory) reproducible with rbm (via the ncdns-repro repository), so it's not a security issue, but it makes the build workflow a lot more annoying than it should be. I'm in the process of getting a patch merged to Namecoin that will fix this issue; I expect it to be resolved in less than a month.
  2. Electrum-NMC is pulled in via the Python source code tarball on namecoin.org. That tarball contains source code from various Python dependencies of Electrum. It would be a lot better to pull in those dependencies from their upstream source (either Git or tarballs), and then combine them with Electrum-NMC's source (from Git). I'm in the process of implementing this; I expect it to be resolved in less than 2 months.
  3. This branch uses an official Electrum-NMC release rather than the Electrum-NMC branch I used in the live demo. The live demo branch has some patches that make initial syncup much faster (nearly instant), whereas the official release will probably take about 5 minutes to do initial syncup. Most of the patches for faster syncup are now undergoing review by upstream Electrum; this has already yielded much better code quality than the live demo branch (shocking, I know -- the Electrum devs know their own codebase better than I do!), but it does mean there's some delay in getting everything merged. I think it's likely that a lot of this code will be merged upstream in the next 2-3 months.

Anyway, while this isn't production-ready in any way, I figure it's a good idea to post it here for transparency purposes. If anyone wants to play around with it, build a nightly of Tor Browser (must be 64-bit Linux), and set the environment variable TOR_ENABLE_NAMECOIN=1 before you run Tor Browser. .bit and .bit.onion sites should "just work" (modulo the initial syncup time, see above). Right now .bit sites can point to IP address or onion services, and Namecoin TLS is not part of this patch. Prior to a release, I intend to disable IP addresses, so .bit can only point to a .onion, and we can evaluate how to do IP+TLS securely at a later date. Be sure to check out the awesome circuit display when you're viewing a Namecoin onion service! (Kudos to Arthur for the Torbutton patch that does this.)

Cheers!

comment:5 Changed 4 months ago by JeremyRand

ncprop279 isn't built locally, but instead is pulled from a binary release on namecoin.org.

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin (commit hash accff2aff645c7dac487e9acd0cbd43fa8372b12). ncprop279 is now built locally.

The updated branch also disables --detach if Namecoin is enabled, which allows Namecoin to be used when double-clicking the Tor Browser launcher icon. (I noticed that this was broken in the previous branch during the Stockholm meeting.)

More updates should be coming fairly soon.

comment:6 Changed 4 months ago by JeremyRand

Electrum-NMC is pulled in via the Python source code tarball on namecoin.org. That tarball contains source code from various Python dependencies of Electrum. It would be a lot better to pull in those dependencies from their upstream source (either Git or tarballs), and then combine them with Electrum-NMC's source (from Git).

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin (commit hash 4b23a0490f49e9c08c63ae5ab3c70d66be84115e). Electrum-NMC and its dependencies are now built locally from source, subject to a few exceptions:

  1. PyQt isn't built at all. This doesn't matter for Tor purposes since Tor isn't going to use the PyQt GUI from Electrum-NMC.
  2. Locales aren't built at all. Again, this doesn't matter for Tor purposes since the Electrum-NMC UI isn't going to be user-facing and therefore doesn't need to be translated.
  3. Protobuf definitions aren't built from source, instead the pre-compiled Protobuf definitions that Electrum-NMC distributes are used. I'm fairly sure this isn't a problem since Protobuf is (AFAICT) only used in Electrum-NMC for the Payment Protocol, and that's not functionality that Tor will ever need to touch. In fact, I suspect that the Protobuf dependency can be completely ripped out of Electrum-NMC for Tor distribution, which will also reduce the final binary size noticeably.

Prior to a release, I intend to disable IP addresses, so .bit can only point to a .onion

This is implemented in 4b23a0490f49e9c08c63ae5ab3c70d66be84115e as well, though I haven't tested that change very thoroughly yet.

I'm hoping to have some additional updates within the next week or two.

comment:7 Changed 3 months ago by JeremyRand

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin (commit hash 20c7d142948cad98193b50ec6ad75fc272421f1a). Here's the summary of the current status:

  1. Namecoin is enabled only in Nightly.
  2. Namecoin is enabled only if the environment variable TOR_ENABLE_NAMECOIN=1 is set when Tor Browser is executed. (At Georg's suggestion, I intend to transition this to using a Firefox pref in the future.)
  3. Namecoin is enabled only on GNU/Linux targets. (I intend to add Windows, macOS, and Android/Linux support later.)
  4. Namecoin will only work if Python 3.6 or higher is installed. Python 3.6 is pretty common nowadays, so this shouldn't be a huge barrier to testing it, although it's not ideal. (In the future, we could look at including a Python interpreter binary in the Tor Browser package, thus removing this requirement. Doing so would presumably be needed on Windows anyway.)
  5. Stream isolation is supported in this patch, but is dependent on #19859. Until #19859 is merged, this patch will still connect without errors, but stream isolation will not be functional, which has two implications: degraded privacy and degraded performance. It should be possible to review both patches in parallel, and it should be easy to build this patch before #19859 is merged by manually setting the tor project's Git repo/commit to use the one provided in #19859. However, while I encourage review of this patch, I do not recommend merging this patch until #19859 is merged.
  6. 2 of the public ElectrumX Namecoin servers are currently down for maintenance. Since Namecoin connects to multiple servers simultaneously to improve performance, the performance of this patch will be degraded until those servers come back online. It still works fine and isn't particularly annoying, but there *will* be some higher-than-typical latency while we're waiting for those servers to come back online. (It would be awesome if the Tor community decides to set up some additional ElectrumX Namecoin servers.)
  7. The 4 domain names that I demoed in Stockholm are http://federalistpapers.bit, http://onionshare.bit, http://riseuptools.bit, and http://submit.theintercept.bit. You can use either .bit or .bit.onion eTLD. (The difference between the two eTLD's was covered in the Namecoin session in Stockholm.)
  8. Currently .bit domains can only point to a .onion domain. .bit domains that point to an A, AAAA, CNAME, or other DNS record will not resolve. Adding support for other record types can be done later.
  9. Currently, the .bit eTLD is not considered a secure origin like .onion. So visiting a .bit domain will not be recognized by Firefox as secure, nor will it show the onion icon. Visiting a .bit.onion domain will work fine though. I will fix this later.
  10. The circuit display panel will show the .onion domain that a Namecoin domain points to. Kudos to Arthur for that patch.
  11. This patch includes an implementation of Prop279. My implementation differs from the spec by adding a "stream isolation ID" field to the RESOLVE command. If desired, I could submit a spec patch for Prop279 that makes it match this implementation. Let me know if you'd like me to do that.
  12. I've probably forgotten some potentially relevant notes; if anything doesn't make sense or you otherwise have some questions for me, please don't hesitate to ask.
  13. As far as I can tell, this patch is ready for review.

Cheers!

comment:8 Changed 3 months ago by gk

Keywords: TorBrowserTeam201911R added
Status: assignedneeds_review

comment:9 Changed 3 months ago by gk

Thanks for the patches! The construction with the ncdns-repro repo seems weird. I don't think this belongs into the top level of the tor-browser-build repo. Rather, looking at its contents this needs to get integrated into the projects. Or, rather, all the projects you need should get integrated into projects. But maybe I am missing something here. Could you elaborate?

FWIW: there is at least one (non-fatal) error I get during running the build that might be related to the above as you seem to try to remove a thing that cannot be removed:

git submodule update --init
Submodule 'ncdns-repro' (https://github.com/namecoin/ncdns-repro.git) registered for path 'ncdns-repro'
Cloning into '/home/gk/tor-browser-build/ncdns-repro'...
Submodule path 'ncdns-repro': checked out '1a81e71504466a518c0aa7562d393ce0e06fc22a'
make -C ncdns-repro submodule-update
make[1]: Entering directory '/home/gk/tor-browser-build/ncdns-repro'
./setup-submodule-symlinks
rm: cannot remove 'tor-browser-build': Is a directory
make[1]: Leaving directory '/home/gk/tor-browser-build/ncdns-repro'

comment:10 Changed 3 months ago by JeremyRand

Thanks for the patches! The construction with the ncdns-repro repo seems weird. I don't think this belongs into the top level of the tor-browser-build repo. Rather, looking at its contents this needs to get integrated into the projects. Or, rather, all the projects you need should get integrated into projects. But maybe I am missing something here. Could you elaborate?

My reasoning was that Namecoin already maintains rbm projects for most of the binaries we distribute, so it would reduce maintenance effort to simply reuse Namecoin's rbm projects folder (via a Git submodule) rather than require the Tor Browser devs to regularly copy Namecoin's projects folder into Tor Browser's projects folder. But, if you prefer to have Tor Browser keep its own copy of Namecoin's projects rather than reuse Namecoin's version as a Git submodule, that's fine with me and I can make that change. Let me know what you'd prefer.

FWIW: there is at least one (non-fatal) error I get during running the build that might be related to the above as you seem to try to remove a thing that cannot be removed:

Indeed, that error gets displayed the first time that the submodule is being set up. The error is harmless, but I can fix that pretty easily (though I'll wait for an answer to the above question so that I know whether the submodule is staying at all).

comment:11 in reply to:  10 Changed 3 months ago by gk

Keywords: TorBrowserTeam201911R removed
Status: needs_reviewneeds_revision

Replying to JeremyRand:

Thanks for the patches! The construction with the ncdns-repro repo seems weird. I don't think this belongs into the top level of the tor-browser-build repo. Rather, looking at its contents this needs to get integrated into the projects. Or, rather, all the projects you need should get integrated into projects. But maybe I am missing something here. Could you elaborate?

My reasoning was that Namecoin already maintains rbm projects for most of the binaries we distribute, so it would reduce maintenance effort to simply reuse Namecoin's rbm projects folder (via a Git submodule) rather than require the Tor Browser devs to regularly copy Namecoin's projects folder into Tor Browser's projects folder. But, if you prefer to have Tor Browser keep its own copy of Namecoin's projects rather than reuse Namecoin's version as a Git submodule, that's fine with me and I can make that change. Let me know what you'd prefer.

Hrm. I am not convinced yet that the maintenance effort is (so much) higher given that we want to review any changes anyway before applying them tor tor-browser-build and we might even want to cherry-pick some changes and not other which I think could be harder in the submodule setup. Thus, let's try it the way I proposed above and see it as part of the experiment we are doing.

I was not aware that Namecoin is using rbm as well, which is very nice! Keep it going. :)

FWIW: there is at least one (non-fatal) error I get during running the build that might be related to the above as you seem to try to remove a thing that cannot be removed:

Indeed, that error gets displayed the first time that the submodule is being set up. The error is harmless, but I can fix that pretty easily (though I'll wait for an answer to the above question so that I know whether the submodule is staying at all).

Thanks, yes, fixing this would be good I think.

comment:12 Changed 3 months ago by JeremyRand

Hrm. I am not convinced yet that the maintenance effort is (so much) higher given that we want to review any changes anyway before applying them tor tor-browser-build and we might even want to cherry-pick some changes and not other which I think could be harder in the submodule setup. Thus, let's try it the way I proposed above and see it as part of the experiment we are doing.

Done. Updated code is at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin (Git commit hash 88c59a9056a5c458f88e0beecd915e1cee30f2c0).

I was not aware that Namecoin is using rbm as well, which is very nice! Keep it going. :)

Yes, we're using rbm for all of our projects that aren't forks of Bitcoin codebases. We're also gently nudging upstream Electrum to move toward rbm as well (and we're using the integration of Electrum-NMC into Tor Browser as an opportunity to experiment with using rbm to build Electrum so that it's easier to justify this to upstream Electrum devs).

Thanks, yes, fixing this would be good I think.

No longer relevant, as that error message was related to the Git submodule, which is now removed.

comment:13 Changed 3 months ago by gk

Keywords: TorBrowserTesm201911R added
Status: needs_revisionneeds_review

comment:14 Changed 2 months ago by JeremyRand

2 of the public ElectrumX Namecoin servers are currently down for maintenance. Since Namecoin connects to multiple servers simultaneously to improve performance, the performance of this patch will be degraded until those servers come back online. It still works fine and isn't particularly annoying, but there *will* be some higher-than-typical latency while we're waiting for those servers to come back online. (It would be awesome if the Tor community decides to set up some additional ElectrumX Namecoin servers.)

1 of those 2 servers has completed maintenance and is back online as of yesterday, so that should yield a performance bump for anyone testing these patches.

On another note: in the hypothetical event that the patches in this ticket pass review and are merged, my recommendation would be to time the merge to coincide with a blogpost, so that users of the Nightly builds are informed of what the changes are. Otherwise, there might be unnecessary confusion by users who observe a bunch of Namecoin code being merged to tor-browser-build (or who observe its presence in the Nightly binaries) and aren't sure what the scope of it is (e.g. it's important that users know that the code is disabled by default, so that they don't think they're being exposed to new attack surface without their opt-in consent). I'm happy to contribute to such a blogpost if/when we reach the point where it's relevant. Of course, if you strongly prefer to merge without a blogpost and worry about doing a blogpost later, you're welcome to do so.

comment:15 Changed 2 months ago by gk

Keywords: TorBrowserTeam201911R added; TorBrowserTesm201911R removed

comment:16 Changed 2 months ago by gk

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201911R removed
Status: needs_reviewneeds_revision

Okay, let's start. Alas, I don't have time to review everything at once. My current plan is to go commit by commit and let you fix up things as we go. I hope this works for you.

1d46514c6be75dadcf9201961b1252edafcfa443:

+    "proxy": "socks5:127.0.0.1:9150::",

I guess that hard-coding is done by taking the Tor Browser default values? One thing to keep in mind is that not every user is using 9150 as the SOCKS port. I don't know how hard it is to make this more dynamic but it would be nice if it were possible (but it's not necessary for a nightly inclusion).

+    - patch

Are you sure this is needed? We are applying a lot of other patches without requiring patch being explicitly installed (because it seems to come at least in newer OSes installed by default).

+  #- name: electrum-nmc
+  #  project: electrum-nmc
+  #  enable: '[% c("var/namecoin") %]'
+  #- name: ncprop279
+  #  project: ncprop279
+  #  enable: '[% c("var/namecoin") %]'
+  #- name: stemns
+  #  project: stemns
+  #  enable: '[% c("var/namecoin") %]'

Please remove those comments here and just include those projects when it is time.

++        python3 TorBrowser/StemNS/poc.py &

I think we can't require users just having python3 available yet. Please add a check and somewhere so users get a notice when they need to install python3.

+      namecoin: '[% c("var/nightly") %]'

Please add a namecoin: 0 to all the other platforms given that you are not only checking for Namecoin support when dealing with Linux.

Some general remarks:
1) We try to make it possible to bisect issues in tor-browser-build in the sense that resulting builds are still running. It's hard sometimes and sometimes even not really possible, but that should be the goal anyways. With that in mind I think the commit I looked at above should be (to a large extend (that is without the changes in rbm.conf)) the *last* in your series of patches. Usually one is adding commits for all the projects and then in the final commit(s) one is "activating" those by getting them actually build once one sets the proper flags/or builds for the respective platforms/architectures. It would be nice if you could follow this general idea.

2) Please don't use the submodule approach you had in mind first and then later on discard it in favor of including all the needed projects directly. Just include those projects directly. This makes the patch set smaller, is easier to review, and is the right thing to do anyway.

comment:17 Changed 2 months ago by JeremyRand

Thanks for the feedback Georg. I've just pushed a revised set of patches (same branch as before; Git commit hash 26c5593a9230e22b52b03917ad274b8ea08a70b5) that I believe addresses your review so far.

Alas, I don't have time to review everything at once. My current plan is to go commit by commit and let you fix up things as we go. I hope this works for you.

Yes, in fact that's better than reviewing everything at once, since it lets me start addressing feedback sooner.

I guess that hard-coding is done by taking the Tor Browser default values? One thing to keep in mind is that not every user is using 9150 as the SOCKS port. I don't know how hard it is to make this more dynamic but it would be nice if it were possible (but it's not necessary for a nightly inclusion).

Doing this the "right way" will probably require patching Electrum-NMC; I'll put it on my to-do list. That said, can you elaborate on what types of users will be using a non-default SOCKS IP/port? The only cases I can think of are setups like Tails and Whonix, and those setups will need other changes to work with Namecoin because they use a control port filter that will interfere with StemNS. I definitely want to support those use cases (if nothing else because Whonix is my daily driver OS), but I'm curious if there's another set of use cases where this matters that I'm not aware of.

Are you sure this is needed? We are applying a lot of other patches without requiring patch being explicitly installed (because it seems to come at least in newer OSes installed by default).

Yes; removing that line causes the build to fail because patch isn't found. Maybe patch is only installed by default in newer versions of Debian than what's used here.

Please remove those comments here and just include those projects when it is time.

Done.

I think we can't require users just having python3 available yet. Please add a check and somewhere so users get a notice when they need to install python3.

Done. At the same time I also now check if their python3 is too old, and give them a notice for that case too.

Please add a namecoin: 0 to all the other platforms given that you are not only checking for Namecoin support when dealing with Linux.

Done.

1) We try to make it possible to bisect issues in tor-browser-build in the sense that resulting builds are still running. It's hard sometimes and sometimes even not really possible, but that should be the goal anyways. With that in mind I think the commit I looked at above should be (to a large extend (that is without the changes in rbm.conf)) the *last* in your series of patches. Usually one is adding commits for all the projects and then in the final commit(s) one is "activating" those by getting them actually build once one sets the proper flags/or builds for the respective platforms/architectures. It would be nice if you could follow this general idea.

Good point. I've rearranged the order and grouping of commits to hopefully bring it closer to what you describe. Let me know if this is an improvement, and whether there's anything remaining that I should do on this front.

2) Please don't use the submodule approach you had in mind first and then later on discard it in favor of including all the needed projects directly. Just include those projects directly. This makes the patch set smaller, is easier to review, and is the right thing to do anyway.

Done.

Thanks for the review!

comment:18 in reply to:  17 Changed 2 months ago by gk

Replying to JeremyRand:

Thanks for the feedback Georg. I've just pushed a revised set of patches (same branch as before; Git commit hash 26c5593a9230e22b52b03917ad274b8ea08a70b5) that I believe addresses your review so far.

Alas, I don't have time to review everything at once. My current plan is to go commit by commit and let you fix up things as we go. I hope this works for you.

Yes, in fact that's better than reviewing everything at once, since it lets me start addressing feedback sooner.

I guess that hard-coding is done by taking the Tor Browser default values? One thing to keep in mind is that not every user is using 9150 as the SOCKS port. I don't know how hard it is to make this more dynamic but it would be nice if it were possible (but it's not necessary for a nightly inclusion).

Doing this the "right way" will probably require patching Electrum-NMC; I'll put it on my to-do list. That said, can you elaborate on what types of users will be using a non-default SOCKS IP/port? The only cases I can think of are setups like Tails and Whonix, and those setups will need other changes to work with Namecoin because they use a control port filter that will interfere with StemNS. I definitely want to support those use cases (if nothing else because Whonix is my daily driver OS), but I'm curious if there's another set of use cases where this matters that I'm not aware of.

We have users that want to avoid using Tor Browser's tor and rather like to point to a system tor which they have running anyway. Granted, it's not the majority of users (by far) but it's an important use-case we support at least.

Last edited 2 months ago by gk (previous) (diff)

comment:19 Changed 2 months ago by gk

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed

comment:20 Changed 2 months ago by gk

Oh, and while looking at your new branch. Please avoid force-pushing if possible but just use a new branch. If you force-push during review I need to review all the code again which I already reviewed as it's hard to see what exactly changed between revisions.

comment:21 Changed 2 months ago by JeremyRand

Oh, and while looking at your new branch. Please avoid force-pushing if possible but just use a new branch. If you force-push during review I need to review all the code again which I already reviewed as it's hard to see what exactly changed between revisions.

Sorry about that. For reference, the previous branch that you reviewed on Nov 20 is now at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v0 . I'll use new branches from now on.

comment:22 Changed 2 months ago by JeremyRand

We have users that want to avoid using Tor Browser's tor and rather like to point to a system tor which they have running anyway. Granted, it's not the majority of users (by far) but it's an important use-case we support at least.

Okay yes, so that's a quite similar situation to Tails and Whonix, except that the control port filter might not be present. It should be feasible to patch Electrum-NMC to handle this use case, though it's probably not going to happen immediately.

comment:23 Changed 8 weeks ago by gk

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201911R removed

We are in December now.

comment:24 Changed 8 weeks ago by cypherpunks

you are in December gk, I am in euphoria.

comment:25 Changed 8 weeks ago by pili

Reviewer: gk

gk to review these tickets

comment:26 Changed 8 weeks ago by gk

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201912R removed

Thanks for addressing my previous points. Now let's look at commit 3fb39eeac9cb898245bc38f1444f48984a09a1a8:

1) Looking at the electrum-nmc project there are a bunch of features that are conditional. However, there are no plans right now to offer conditional Namecoin features in Tor Browser nightly builds. Please remove all the complexity here and just take what we want to ship initially.

2) For the certifi config, see 1) above. There is no need to include the root certs conditionally.

3) The build scripts for the dependencies seem to be quite complex given that they should just create a tarball of .py at the end if I see that right: you invoke first sdist to create what essentially is already a source tarball just to shuffle things around later on to create the source tarball in rbm-style which you actually use later on. Could we avoid one of those two steps here? Like, could we just do the Python source tarball creation here and use that one (maybe with some general, not per-project, post-processing later on when those sources are getting used)? If not, could we just zip up the necessary .py files ourselves avoiding the Python steps?

4) If we stick to the Python sdist process I think it is not necessary to specify --format=gztar as this should be the default on Unix-like systems (which are the only systems we use for building).

comment:27 Changed 8 weeks ago by gk

Oh, I guess I forgot another point:

5) I've not looked at later commits in depth yet but if 1)-4) affect them please fix those issues in them during that round of review as this is speeding up the whole process.

comment:28 Changed 8 weeks ago by JeremyRand

Thanks for the review @gk.

1) Looking at the electrum-nmc project there are a bunch of features that are conditional. However, there are no plans right now to offer conditional Namecoin features in Tor Browser nightly builds. Please remove all the complexity here and just take what we want to ship initially.

2) For the certifi config, see 1) above. There is no need to include the root certs conditionally.

I'm okay with making this requested change. However, please note that doing this will increase the maintenance burden on my end, because it's easier for me to maintain Electrum-NMC if the same rbm projects are used for both Namecoin's binaries and Tor's binaries. (Namecoin isn't yet using rbm to build our official Electrum-NMC binaries, but these rbm projects were written with that goal in mind, and hopefully we will transition to using them in the foreseeable future.) If needing to maintain two sets of rbm projects for Electrum-NMC is the cost of getting into Tor Browser, then I can deal with that, but it will have an impact on my maintenance costs.

3) The build scripts for the dependencies seem to be quite complex given that they should just create a tarball of .py at the end if I see that right: you invoke first sdist to create what essentially is already a source tarball just to shuffle things around later on to create the source tarball in rbm-style which you actually use later on. Could we avoid one of those two steps here? Like, could we just do the Python source tarball creation here and use that one (maybe with some general, not per-project, post-processing later on when those sources are getting used)? If not, could we just zip up the necessary .py files ourselves avoiding the Python steps?

The reason I didn't solely use the Python sdist functionality is that the tarballs it produces are nonreproducible, and there's an auditability benefit to having the outputs of those projects be reproducible (even though it shouldn't impact the final Tor Browser binaries).

I'm honestly not certain if it's feasible to skip the sdist functionality and solely use rbm's tarball creation. sdist can do interesting things that may be nontrivial to mimic on our own, but I don't know how many (if any) of the packages we're using actually use any of those interesting things. Would you like me to attempt this approach and see how well it works?

It would probably be feasible to simplify the amount of build code here by doing something similar to the build_go_lib template that's used for Go libraries. Would this be something you'd like me to attempt prior to a Nightly merge? (If it's not necessary for a Nightly merge, I will probably still do it at some point, since I like the build_go_lib structure and it seems like it would be helpful here.)

4) If we stick to the Python sdist process I think it is not necessary to specify --format=gztar as this should be the default on Unix-like systems (which are the only systems we use for building).

No objection here.

5) I've not looked at later commits in depth yet but if 1)-4) affect them please fix those issues in them during that round of review as this is speeding up the whole process.

Sorry, I wasn't able to parse this sentence with certainty (more pronouns than I can handle). Am I correct in interpreting this as "issues 1-4 should be fixed in 3fb39eeac9cb898245bc38f1444f48984a09a1a8, but issues 1-4 should not be fixed in later commits until the later commits are reviewed"?

Thanks again for the review!

comment:29 in reply to:  28 Changed 8 weeks ago by gk

Replying to JeremyRand:

Thanks for the review @gk.

1) Looking at the electrum-nmc project there are a bunch of features that are conditional. However, there are no plans right now to offer conditional Namecoin features in Tor Browser nightly builds. Please remove all the complexity here and just take what we want to ship initially.

2) For the certifi config, see 1) above. There is no need to include the root certs conditionally.

I'm okay with making this requested change. However, please note that doing this will increase the maintenance burden on my end, because it's easier for me to maintain Electrum-NMC if the same rbm projects are used for both Namecoin's binaries and Tor's binaries. (Namecoin isn't yet using rbm to build our official Electrum-NMC binaries, but these rbm projects were written with that goal in mind, and hopefully we will transition to using them in the foreseeable future.) If needing to maintain two sets of rbm projects for Electrum-NMC is the cost of getting into Tor Browser, then I can deal with that, but it will have an impact on my maintenance costs.

Yes, I can see that point. Let me explain you as well where we are coming from: this ticket is not about "getting NameCoin into Tor Browser" in the sense that it decides whether to ship this feature in actual releases or not. It's about testing optional NameCoin support on one platform in nightly builds for those that are interested.

However, including the Namecoin patches into tor-browser-build has costs far beyond just affecting a particular platform on the nightly series as it increases the general complexity of the master branch considerably. This is okay, but we should try to minimize that complexity, hence my request.

That said, in case this experiment is successful and we indeed think about shipping Namecoin support to users then we'll maintain it in our repo ourselves like we do for all the other projects, too. Thus, it's not that you are stuck with maintaining two things.

3) The build scripts for the dependencies seem to be quite complex given that they should just create a tarball of .py at the end if I see that right: you invoke first sdist to create what essentially is already a source tarball just to shuffle things around later on to create the source tarball in rbm-style which you actually use later on. Could we avoid one of those two steps here? Like, could we just do the Python source tarball creation here and use that one (maybe with some general, not per-project, post-processing later on when those sources are getting used)? If not, could we just zip up the necessary .py files ourselves avoiding the Python steps?

The reason I didn't solely use the Python sdist functionality is that the tarballs it produces are nonreproducible, and there's an auditability benefit to having the outputs of those projects be reproducible (even though it shouldn't impact the final Tor Browser binaries).

Non-reproducible in what way?

I'm honestly not certain if it's feasible to skip the sdist functionality and solely use rbm's tarball creation. sdist can do interesting things that may be nontrivial to mimic on our own, but I don't know how many (if any) of the packages we're using actually use any of those interesting things. Would you like me to attempt this approach and see how well it works?

Please do.

It would probably be feasible to simplify the amount of build code here by doing something similar to the build_go_lib template that's used for Go libraries. Would this be something you'd like me to attempt prior to a Nightly merge? (If it's not necessary for a Nightly merge, I will probably still do it at some point, since I like the build_go_lib structure and it seems like it would be helpful here.)

No, that's not needed. Right now we don't have much Python code in our projects, thus I think it's not worthwhile to spend time on that at the moment. Maybe that would change if we decided to integrate Namecoin beyond the nightly testing we plan to do, but that would still be at some point in the future which we don't need to worry about right now.

4) If we stick to the Python sdist process I think it is not necessary to specify --format=gztar as this should be the default on Unix-like systems (which are the only systems we use for building).

No objection here.

5) I've not looked at later commits in depth yet but if 1)-4) affect them please fix those issues in them during that round of review as this is speeding up the whole process.

Sorry, I wasn't able to parse this sentence with certainty (more pronouns than I can handle). Am I correct in interpreting this as "issues 1-4 should be fixed in 3fb39eeac9cb898245bc38f1444f48984a09a1a8, but issues 1-4 should not be fixed in later commits until the later commits are reviewed"?

I meant if issues 1)-4) affect later commits please fix those commits up as well while you are addressing this round of feedback.

Thanks again for the review!

No worries. Thanks for your patience and the review delay.

comment:30 Changed 8 weeks ago by mcs

Cc: mcs added

comment:31 Changed 8 weeks ago by JeremyRand

Yes, I can see that point. Let me explain you as well where we are coming from: this ticket is not about "getting NameCoin into Tor Browser" in the sense that it decides whether to ship this feature in actual releases or not. It's about testing optional NameCoin support on one platform in nightly builds for those that are interested.

However, including the Namecoin patches into tor-browser-build has costs far beyond just affecting a particular platform on the nightly series as it increases the general complexity of the master branch considerably. This is okay, but we should try to minimize that complexity, hence my request.

That said, in case this experiment is successful and we indeed think about shipping Namecoin support to users then we'll maintain it in our repo ourselves like we do for all the other projects, too. Thus, it's not that you are stuck with maintaining two things.

Sounds good to me, I'll make the requested changes.

Non-reproducible in what way?

I don't remember off the top of my head, but I do remember that upstream Electrum's official binaries are built via sdist and they are not reproducible. Might be related to datetime values getting embedded. If omitting sdist and solely using rbm's tarball generation doesn't pan out, I'll investigate this more closely.

No, that's not needed. Right now we don't have much Python code in our projects, thus I think it's not worthwhile to spend time on that at the moment. Maybe that would change if we decided to integrate Namecoin beyond the nightly testing we plan to do, but that would still be at some point in the future which we don't need to worry about right now.

Okay.

I meant if issues 1)-4) affect later commits please fix those commits up as well while you are addressing this round of feedback.

Ah, ok. Glad I asked. :)

I'll aim to have a new branch for you within a couple days. (Might be quicker than that if things go smoothly.)

comment:32 Changed 8 weeks ago by JeremyRand

Sorry this revision took so long; there was quite a bit of tinkering required, given various quirks in how the Python libs are built. But, the good news is that yes, building without sdist works fine AFAICT. I've removed the conditional variables from electrum-nmc and certifi. There were also 2 conditional variables in the ncdns and ncprop279 projects, which I've removed as well.

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v2 (Git commit hash 111d7d1447b47e5290fd0234174a7c29c1351a7b).

comment:33 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201912 removed
Status: needs_revisionneeds_review

comment:34 in reply to:  32 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201912 added; TorBrowserTeam201912R removed
Status: needs_reviewneeds_revision

Replying to JeremyRand:

Sorry this revision took so long; there was quite a bit of tinkering required, given various quirks in how the Python libs are built. But, the good news is that yes, building without sdist works fine AFAICT. I've removed the conditional variables from electrum-nmc and certifi. There were also 2 conditional variables in the ncdns and ncprop279 projects, which I've removed as well.

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v2 (Git commit hash 111d7d1447b47e5290fd0234174a7c29c1351a7b).

Thanks looks better now. commit 84888715acfda26e2735805e970ec77dd2d1f56e:

a)

You create the project dir with

+mkdir -p /var/tmp/dist/[% project %]

but then often create another project dir you then you like so

+mkdir ./[% project %]
+cp -a $rootdir/[% project %]*/[% project %]/*.py ./[% project %]/

This [% project %]/[% project %]/ setup is essentially happening in every project. Could you avoid that and just do a

+mkdir -p /var/tmp/dist/[% project %]
+cd /var/tmp/dist/

(note: you are no changing into the project dir here)
Then there is no need to create yet another project dir but you can just use that one you already created with mkdir -p.

b)

+cd /var/tmp/dist/[% project %]
+
+mkdir ./[% project %]
+cd $rootdir/dnspython*/[% project %]
+cp --parents **/*.py /var/tmp/dist/[% project %]/[% project %]/
+
+cd /var/tmp/dist/[% project %]

Do we really need to change the dir back and forth here?

c)

+tar xvf [% project %]-[% c('version') %].tar.gz
+
+mkdir -p /var/tmp/dist/[% project %]
+cd /var/tmp/dist/[% project %]
+
+cp -a $rootdir/[% project %]*/[% project %].py ./[% project %].py

For the projects that use snippets like the above it's not clear to me why you create the [% project %] dir at all if you are just using that single file directly. So, just mkdir -p /var/tmp/dist etc. should be enough.

(Again, when you make those changes, please adjust subsequent commits, too, in case they are affected)

comment:35 Changed 7 weeks ago by JeremyRand

Thanks for the review @gk.

a)

No objection here; I'll make the change.

b)
Do we really need to change the dir back and forth here?

I think it's necessary, because otherwise --parents will pick up other parts of the path (i.e. ancestor directories).

c)

No objection here; I'll make the change.

New branch should be ready within a day.

comment:36 Changed 7 weeks ago by JeremyRand

Did I say a day? I must have meant 37 minutes. :)

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v3 (Git commit hash 400bf48791c508afe8014c0fb949f61a7224b33a).

comment:37 in reply to:  36 Changed 7 weeks ago by gk

Replying to JeremyRand:

Did I say a day? I must have meant 37 minutes. :)

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v3 (Git commit hash 400bf48791c508afe8014c0fb949f61a7224b33a).

Thanks. commit 5db1d985c4339f81fb30d0fccd8f8945af3902ca looks almost good now.

In electrum-nmc's build script you have

+distdir=/var/tmp/build/[% project %]

but you don't use distdir anywhere, please remove that.

One typo in roots_of_top_10_issuers.pem s/root CA's/root CAs/.

For commit 644893e115e3d160fca5ced763a7ebbd018f1a0e

Some trailing whitespace to remove on

+    export CGO_ENABLED=[% c("var/cgo") %] 

in ncprop279/config and

+  go_lib_deps: 

in gokingpin/config.

In the same vein as comment:26 1): Please remove all the non-Linux parts and the conditional Linux includes. That's not needed for our PoC here. Some of those parts seem to be wrong as well (e.g. you have var/cgo only for Linux declared in ncdns/config; however in the corresponding build script you check for var/osx *in* the var/cgo block but that code is never reached) and I think we should not include code we currently don't test/exercise in any way.

comment:38 Changed 7 weeks ago by gk

(For the macOS parts it's worth to keep in mind that we have macosx_deployment_target: '10.9' and not 10.7 anymore. We only support macOS 10.9 and onward.)

comment:39 Changed 7 weeks ago by JeremyRand

but you don't use distdir anywhere, please remove that.

Good point; will fix that.

One typo in roots_of_top_10_issuers.pem s/root CA's/root CAs/.

AFAIK it's common practice (though not universal) in English to form plurals of initialisms, letters, and numbers by appending an apostrophe followed by an "s". DuckDuckGoing for initialisms plural apostrophe seems to indicate that this choice varies by writer, and is neither required nor erroneous. If you really want the apostrophe to be removed, let me know and I'll do it, but it wasn't unintentionally included.

Some trailing whitespace to remove on

Good catch, I'll fix that.

Please remove all the non-Linux parts and the conditional Linux includes.

Okay.

(For the macOS parts it's worth to keep in mind that we have macosx_deployment_target: '10.9' and not 10.7 anymore. We only support macOS 10.9 and onward.)

Ah, somehow I missed that. I'll fix that in Namecoin's version of the rbm scripts (even though it's not relevant for this patchset since this patchset is Linux-only), since we do build for macOS for our binaries, and we try to follow Tor's conventions on this stuff.

I'll post an updated branch shortly.

comment:40 Changed 7 weeks ago by JeremyRand

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v4 (Git commit hash a37eb03fc88f192aa02f4694468effa47de53a49).

comment:41 in reply to:  40 Changed 7 weeks ago by gk

Replying to JeremyRand:

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v4 (Git commit hash a37eb03fc88f192aa02f4694468effa47de53a49).

Commit 53279791b5a332df99215199c3040d517620a91d looks good now (I am fine with keeping CA's).
Commit a5fd87e61a53e3bceec638758f8d65ba61798468:

There are a bunch of things to note in no particular order:

a) In gobtcd: Why is there build_go_lib_pre defined? What does that do? Building ncprop279 without it works perfectly fine for me.

b) Why is gobtcd and gobtcd2 split? Is that because gobtcd depends on gobtcutil which depends on gobtcd2? How does the compilation of that work outside tor-browser-build as I can't imagine that this circular dependency is not an issue there as well? If you can't work around that (i.e. have only one gobtcd project) then please add a comment, probably in the gobtcd2 project, explaining the problem.

c) gowinsvc is not needed, please remove it.

d) ncdns: os_go_lib_deps is empty, please remove it and the respective for-loop in the build script.

e) ncdns: +#mkdir -p /var/tmp/build etc. is commented out but not needed, please remove it.

f) ncdns: what is

+[% IF c("var/linux-x86_64") -%]
+  GOPATHBIN="${GOPATH}/bin"
+[% ELSE -%]
+  GOPATHBIN="${GOPATH}/bin/${GOOS}_${GOARCH}"
+[% END -%]

doing at that place in the build script as you are not building after it anymore? The same goes for +# Build as library.

g) ncdns: the goxlog dependency does not seem to be needed, please remove it.

h) ncprop279: there is a bunch of comments in the build script which are not needed, starting
with +#mkdir -p /var/tmp/build; please remove them.

i) ncprop279: what does

+[% IF c("var/linux-x86_64") -%]
+  GOPATHBIN="${GOPATH}/bin"
+[% ELSE -%]
+  GOPATHBIN="${GOPATH}/bin/${GOOS}_${GOARCH}"
+[% END -%]
+
+ls $GOPATHBIN

do?

j) +export CGO_ENABLED=[% c("var/cgo") %] <- no need for having that exported as cgo seems to be 0. Thus, you can remove all the cgo related things.

comment:42 Changed 7 weeks ago by JeremyRand

a) In gobtcd: Why is there build_go_lib_pre defined? What does that do? Building ncprop279 without it works perfectly fine for me.

IIRC this is a historical relic that exists due to complexities involved in having Namecoin maintain a fork of btcd with minimal diff against upstream (meaning that a bunch of imports refer to upstream btcd's namespace). I think it's probably no longer needed because it was introduced before the gobtcd/gobtcd2 split, and gobtcd2 now has the same effect as this hack since it installs to upstream's namespace. I'll test to make sure that this hack is no longer needed, and remove it if testing confirms that it's okay to remove.

b) Why is gobtcd and gobtcd2 split? Is that because gobtcd depends on gobtcutil which depends on gobtcd2? How does the compilation of that work outside tor-browser-build as I can't imagine that this circular dependency is not an issue there as well? If you can't work around that (i.e. have only one gobtcd project) then please add a comment, probably in the gobtcd2 project, explaining the problem.

gobtcd builds the btcjson and rpcclient subpackages of btcd. gobtcd2 builds the btcec, chaincfg, chaincfg/chainhash, and wire subpackages of btcd. The former set depends on another project (probably gobtcutil as you say), and that project depends on the latter set. It's not a circular dependency in terms of Go packages (so it works fine outside of rbm since go get operates on the level of Go packages, not Git repos), but it is a circular dependency in terms of Git repos (meaning that it causes problems in rbm since rbm uses 1 project per Git repo). I'll add a comment explaining this.

c) gowinsvc is not needed, please remove it.

No objection.

d) ncdns: os_go_lib_deps is empty, please remove it and the respective for-loop in the build script.

No objection.

e) ncdns: +#mkdir -p /var/tmp/build etc. is commented out but not needed, please remove it.

No objection.

f) ncdns: what is [snip] doing at that place in the build script as you are not building after it anymore? The same goes for +# Build as library.

Good point, I think I forgot to remove that while I was removing the conditional support for building ncdns as an executable. I'll remove that.

g) ncdns: the goxlog dependency does not seem to be needed, please remove it.

Interesting; this was definitely required when building ncdns as an executable with TLSA support enabled. I'll check to make sure it can be safely removed when building ncdns as a library with TLSA support disabled, and remove it if that still works. (I'll also fix that in Namecoin's version of the rbm scripts, where the conditional vars are still a thing.)

h) ncprop279: there is a bunch of comments in the build script which are not needed, starting with +#mkdir -p /var/tmp/build; please remove them.

No idea how those got left in there; I'll remove them.

i) ncprop279: what does [snip] do?

The linux-x86_64 conditional is to handle the Go compiler putting binaries into a different folder depending on whether they're cross-compiled. In this case, linux-i686 binaries are considered cross-compiled. I think the ls was left there by mistake; I'll remove that line.

j) +export CGO_ENABLED=[% c("var/cgo") %] <- no need for having that exported as cgo seems to be 0. Thus, you can remove all the cgo related things.

IIRC the Go compiler defaults to CGO_ENABLED=1 in some cases, which I think includes linux targets in tor-browser-build. The intent of that line is to make sure cgo stays disabled even if the default would be to enable it. No objection to removing the variable though, since it will always be 0 here.

I'll push an updated branch later today.

comment:43 Changed 7 weeks ago by JeremyRand

g) ncdns: the goxlog dependency does not seem to be needed, please remove it.

ncprop279 imports github.com/namecoin/ncdns/backend, which imports github.com/hlandau/xlog. So I think it's appropriate to have goxlog as a dependency of ncdns. I suspect that goxlog is getting pulled in via another dependency (probably godexlogconfig) as well, which I suppose means that it will build without errors if goxlog is omitted from ncdns, but I don't think it's a good idea to omit it from ncdns's dependencies, since it's directly imported from an ncdns package. But, if you really want me to omit goxlog from ncdns's dependencies, let me know and I'll do it.

comment:44 in reply to:  43 Changed 7 weeks ago by gk

Replying to JeremyRand:

g) ncdns: the goxlog dependency does not seem to be needed, please remove it.

ncprop279 imports github.com/namecoin/ncdns/backend, which imports github.com/hlandau/xlog. So I think it's appropriate to have goxlog as a dependency of ncdns. I suspect that goxlog is getting pulled in via another dependency (probably godexlogconfig) as well, which I suppose means that it will build without errors if goxlog is omitted from ncdns, but I don't think it's a good idea to omit it from ncdns's dependencies, since it's directly imported from an ncdns package. But, if you really want me to omit goxlog from ncdns's dependencies, let me know and I'll do it.

The dependencies in the config file (more exactly, those mentioned in input_files) specify the *build* dependencies which means projects in tor-browser-build that are needed to get built before the project in question can get compiled. For goxlog you don't specify it in go_lib_deps but in input_files, yet the compilation succeeds. That is an indication that goxlog is not directly needed for building ncdns. Thus, please remove it from input_files, too.

The config file is not meant to be an universal dependency tracker in the sense that one can see any dependency, recursively down to the last one, just by looking at that file.

comment:45 Changed 7 weeks ago by JeremyRand

Updated branch at ​https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v5 (Git commit hash 76d09f1d7dd8ecc24ab9d4eb8356ac5e4d95d776).

comment:46 in reply to:  45 Changed 7 weeks ago by gk

Replying to JeremyRand:

Updated branch at ​https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v5 (Git commit hash 76d09f1d7dd8ecc24ab9d4eb8356ac5e4d95d776).

Thanks, the changes you made compared to -v4 look good to me. It feels we are getting pretty close here. In commit e32a739ea11a06880f0e411708796b2f2a2e9270 you have:

+  # Strip off the version number from the Electrum-NMC directory name
+  mv $TBDIR/TorBrowser/Electrum-NMC* $TBDIR/TorBrowser/Electrum-NMC

We should not do that here. Rahther, please include that into the respective commit where all the Electrum-NMC code gets added. Looking back at that commit I see you have

+cp -a /var/tmp/build/[% project %]/[% project %]* ./Electrum-NMC-[% c('version') %]
+
+cd ./Electrum-NMC-[% c('version') %]

Couldn't you do something like

+cp -a /var/tmp/build/[% project %]/[% project %]* ./Electrum-NMC
+
+cd ./Electrum-NMC

instead?

comment:47 Changed 7 weeks ago by JeremyRand

Good point. IIRC the reason for this is that Namecoin's rbm scripts were attempting to make the rbm-built Electrum-NMC a drop-in replacement for the Electrum-NMC built without rbm (which was intended to make it easier for upstream Electrum to migrate to rbm for their releases), and upstream Electrum's binaries had the version as part of the path inside the tar.gz file. But indeed, this motivation is irrelevant to Tor's usage, so it makes sense to make the change you suggest.

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v6 (Git commit hash 8ad40dd97c73d0319275be7c811dbc2c4d641729).

comment:48 Changed 7 weeks ago by gk

JeremyRand: Are those domains you mentioned in comment:7 still up and running? None of them works for me. If not, where are domains that are supposed to be up and working?

comment:49 Changed 7 weeks ago by JeremyRand

JeremyRand: Are those domains you mentioned in comment:7 still up and running? None of them works for me. If not, where are domains that are supposed to be up and working?

@gk All 4 of those domains work fine for me as of just now. Just to verify, you're running the GNU/Linux nightly build with the environment variable TOR_ENABLE_NAMECOIN=1, right? Are you on x86_64 or i686? What version of Python is installed? Can you provide the output that you get if you run Tor Browser from a terminal with the --verbose flag, and then try to visit one of the 4 domains I listed?

comment:50 in reply to:  49 Changed 6 weeks ago by gk

Replying to JeremyRand:

JeremyRand: Are those domains you mentioned in comment:7 still up and running? None of them works for me. If not, where are domains that are supposed to be up and working?

@gk All 4 of those domains work fine for me as of just now. Just to verify, you're running the GNU/Linux nightly build with the environment variable TOR_ENABLE_NAMECOIN=1, right? Are you on x86_64 or i686? What version of Python is installed? Can you provide the output that you get if you run Tor Browser from a terminal with the --verbose flag, and then try to visit one of the 4 domains I listed?

That's been with a custom build which is nightly-ish, yes. Python 3.7.5 with TOR_ENABLE_NAMECOIN=1. I try to reproduce the issue but meanwhile another problem I've seen is that Namecoin enabled is sometimes hammering the Tor network with stream creations like attached in the log which is very concerning. The latter at least could be related to Namecoin not properly shutting down when I close the browser e.g. by pressing Ctrl+C in my terminal. The result is that I can't stop the remaining Python process by either pressing Ctrl+C further or closing the terminal. Additionally, it has the interesting property that it messes with a newly started Tor Browser in a new terminal even though TOR_ENABLE_NAMECOIN is *not* set there, rendering that Tor Browser non-functional and one can see requests to ulrichard.ch in that terminal. That's not supposed to happen.

So, at the end my trouble connecting to a .bit domain might result in the above bug but I am not sure yet.

Changed 6 weeks ago by gk

Attachment: log added

comment:51 Changed 6 weeks ago by JeremyRand

Interesting @gk. I'll see if I can reproduce this on my end. I'm not sure if I tested quitting Tor Browser via Ctrl+C; I suspect that's likely to be the source of at least some of the breakage you're observing [1]. I'll report back shortly.

[1] Of course, the code that's likely to be responsible for that breakage will be phased out anyway once we move launching Namecoin to tor-launcher instead of the Bash script.

comment:52 Changed 6 weeks ago by JeremyRand

@gk Okay, so it was pretty easy to fix the issue where Ctrl+C didn't shut down Namecoin. Just had to trap SIGINT in start-tor-browser to terminate the Namecoin processes. Thanks for catching that. Before I push a fix to a new branch, are there any other signals I should trap besides SIGINT (so that Namecoin gets shut down properly)? I'm thinking SIGTERM would also be a good idea. Any others I'm missing?

Regarding the STREAM events that are showing up in your log: upstream Electrum periodically tries to reconnect to servers if they're down or got disconnected. I'm pretty sure that's what's happening here; the ElectrumX server at ulrichard.ch is down for maintenance at the moment, and I think Electrum-NMC is trying periodically to connect to it in case it's come back up. I'll look at the relevant code shortly and figure out whether there's a way we can reduce the impact this has. That said, some initial thoughts:

  • Not reconnecting periodically at all will have bad effects, because Electrum-NMC launches before Tor has connected, so all the ElectrumX servers will appear to be unreachable when Electrum-NMC initially launches.
  • The above condition is probably fairly easy to detect, because *none* of the servers will be currently connected when Tor isn't connected yet. So we could plausibly make the reconnect frequency high when no servers are connected, but much lower when at least 1 server is already connected.
  • We could also lower the reconnect frequency for a given server if that server has failed to connect multiple times while at least 1 other server was connected. This might help us distinguish between a server failing to connect due to a bad Tor circuit versus a server failing to connect because the server is actually unreachable.
  • I'm not sure what lower reconnect frequency we should fall back to when at least 1 connection is already established and a given server has been unreachable multiple times. There's a tradeoff here, because more server connections means it's less likely that we'll be fed a bad blockchain, but connecting too aggressively will put more load on the Tor network.
  • I'm honestly not sure why upstream Electrum is so aggressive at reconnecting (or if maybe I accidentally made it more aggressive while patching the network code for Namecoin/Tor purposes)... depending on what fixes we go with here, I might submit them upstream to Electrum too.

Curious what your take is on the above suggested changes, or if you think there's some other approach I should look into.

(FWIW, I can't reproduce the original issue where the domains were unreachable. But let's come back to that after we've got the other issues dealt with.)

comment:53 Changed 6 weeks ago by JeremyRand

Updated branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v7 (Git commit hash a1f4951bf1accf446e90423c34b5401b93f1ea3d). SIGINT and SIGTERM should now be trapped, and will result in Namecoin's processes being killed. As discussed on IRC, we'll come back to the periodic reconnect issue after we've verified that the signal trapping works as expected.

comment:54 Changed 6 weeks ago by gk

Looks better now, thanks. My previously reported issues seemed to be related to the Ctrl+C killing.

I think we can live with the fetch behavior for now. The ulrichard.ch situation is annoying but I hope this gets fixed soon. Now while further testing I stumbled on two weird behaviors:

1) On every start when first entering a *.bit address I always see a lot of

E | jsonrpclib.SimpleJSONRPCServer | Server-side exception: <Fault -32603: Server error: File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/electrum_nmc/electrum/network.py", line 972, in make_reliable_wrapper | electrum.network.BestEffortRequestFailed: no interface to do request on... gave up.
>
Traceback (most recent call last):
  File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/packages/jsonrpclib/SimpleJSONRPCServer.py", line 388, in _dispatch
    return func(*params)
  File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/electrum_nmc/electrum/commands.py", line 118, in func_wrapper
    return func(*args, **kwargs)
  File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/electrum_nmc/electrum/commands.py", line 1131, in name_show
    raise error_request_failed
  File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/electrum_nmc/electrum/commands.py", line 1120, in name_show
    return self.name_show_single_try(identifier, stream_id="Electrum-NMC name_show attempt "+str(i)+": "+stream_id)
  File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/electrum_nmc/electrum/commands.py", line 1138, in name_show_single_try
    txs = self.network.run_from_another_thread(self.network.get_history_for_scripthash(sh, stream_id=stream_id))
  File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/electrum_nmc/electrum/network.py", line 316, in run_from_another_thread
    return fut.result()
  File "/usr/lib/python3.7/concurrent/futures/_base.py", line 435, in result
    return self.__get_result()
  File "/usr/lib/python3.7/concurrent/futures/_base.py", line 384, in __get_result
    raise self._exception
  File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/electrum_nmc/electrum/network.py", line 972, in make_reliable_wrapper
    raise BestEffortRequestFailed('no interface to do request on... gave up.')
electrum.network.BestEffortRequestFailed: no interface to do request on... gave up.
20191216101540 [ERROR] ncdns.backend: failed to query namecoin: -32603: Server error: File "/home/th/30558/tor-browser_en-US/Browser/TorBrowser/Electrum-NMC/electrum_nmc/electrum/network.py", line 972, in make_reliable_wrapper | electrum.network.BestEffortRequestFailed: no interface to do request on... gave up.

errors culminating in

I/n | network | couldn't launch iface electrum-nmc.le-space.de:50002:s -- TimeoutError()
I/n | network | connecting to electrum-nmc.le-space.de:50002:s as new interface
I/n | network | couldn't launch iface ulrichard.ch:50006:s -- TimeoutError()
I/n | network | couldn't launch iface nmc.bitcoins.sk:50002:s -- TimeoutError()
I/n | network | couldn't launch secondary iface nmc.bitcoins.sk:50002:s -- TimeoutError()
I/n | network | couldn't launch secondary iface ulrichard.ch:50006:s -- TimeoutError()
I/n | network | couldn't launch secondary iface electrum-nmc.le-space.de:50002:s -- TimeoutError()

. This takes like 30 seconds for domains known to exist which is a bad user experience and wasting Tor resources. I'd expect that NameCoin is ready to get used and not spending more than 30 seconds every time after entering the first .bit domain to deal with timeout errors.

2) When entering .bit domain that does not exist I get a NameNotFoundError: Name never existed, is expired, or is unconfirmed after while. It turns out, though, that I keep getting those errors even though I corrected the domain and it got resolved. There seems to be a lot still going on in the background related to the non-existing domain (additional requests etc.), which should not happen.

comment:55 in reply to:  54 Changed 6 weeks ago by gk

Replying to gk:

[snip]

2) When entering .bit domain that does not exist I get a NameNotFoundError: Name never existed, is expired, or is unconfirmed after while. It turns out, though, that I keep getting those errors even though I corrected the domain and it got resolved. There seems to be a lot still going on in the background related to the non-existing domain (additional requests etc.), which should not happen.

In fact, even if you never got the NameNotFoundError but entered the wrong domain once (e.g. due to a typo) but corrected that you'll get a ton of NameNotFoundErrors later one, even though your corrected domain resolved and you see the contents of the website. There should not continue requests to that old domain as soon as the new one is entered.

Last edited 6 weeks ago by gk (previous) (diff)

comment:56 Changed 6 weeks ago by JeremyRand

Good to hear that the previously reported issues were related to the Ctrl+C killing.

Regarding (1): I suspect that this is because Electrum-NMC has tried to open a connection to a server while Tor was still connecting, and the result is that the server isn't reachable, so the TCP connection times out. It's possible that we could decrease the timeout period in Electrum-NMC, which might improve the behavior here. However, decreasing it too far might cause it to prematurely give up on low-quality Tor circuits. I'll get back to you on this later today. (Longer-term, it might be productive to have Electrum-NMC talk to the Tor control port so that it knows when the network is up, which would allow it to avoid trying to make connections while Tor is still connecting. This would be a more complicated fix than fiddling with the timeouts though, so I'd prefer not to attempt this prior to a Nightly merge unless further testing indicates that it's absolutely necessary.)

Regarding (2): StemNS will retry a lookup that failed with SERVFAIL for up to 1 minute before giving up completely on that lookup. This is because a SERVFAIL error is often an indication of a temporary network issue, and it will usually fix itself if retried a few times. Unfortunately, there's a bug in upstream Electrum that causes errors that should be detected as NXDOMAIN (which won't be retried) to instead be detected as SERVFAIL (specifically, Electrum doesn't properly set the JSON-RPC error variables in responses). AFAIK upstream Electrum fixed this in their master branch sometime after 3.3.8 was tagged (due to me sending in a bug report). I'm pretty sure that directly backporting Electrum's fix to 3.3.8 would be prohibitively difficult (their fix was part of a large refactor of the JSON-RPC code which will almost definitely produce merge conflicts if cherry-picked to 3.3.8), but I might be able to do a much simpler fix to the 3.3.8 branch of Electrum-NMC. Again, I'll get back to you on this later today.

comment:58 Changed 6 weeks ago by JeremyRand

I think I have fixes for both (1) and (2). For (1), decreasing the relevant timeout from 45 seconds to 15 seconds seems to improve the initial connection delay quite a lot without causing timeouts when a Tor circuit is bad. I haven't tried with pluggable transports enabled though; I wonder if the public PT bridges will be slow enough to cause 15 seconds to be too short to open a TCP connection. For (2), I applied a similar change to the one I suggested in the upstream Electrum bug. Interestingly, I just found a bug in ncdns that also causes (2) even once the Electrum-NMC JSON-RPC error code is correct. The fixes will be pushed shortly.

Thanks for catching these issues!

As an aside, note that even once the fix for (2) is applied, Electrum-NMC will try 3 times, using a different server each time, before deciding that a name doesn't exist. It does this so that a single malicious server can't hide the existence of names. (The SPV proofs returned by a server only prove inclusion, not non-inclusion; adding non-inclusion SPV proofs is something that's on our to-do list; once that's implemented, then we'll be able to drop the multiple-server lookups for nonexistent names.) This doesn't cause a significant delay on its own though.

comment:59 Changed 6 weeks ago by JeremyRand

Updated branch at ​https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v8 (Git commit hash 47abcfadb4025e9f43730ce00f116cab491039e5). This decreases the initial Electrum-NMC connection timeout from 45 to 15 seconds, and fixes the NXDOMAIN detection in both Electrum-NMC and ncdns. I've tested the modified commits manually, but haven't yet finished building the new binaries with rbm. In the interest of allowing faster testing, I'm posting the tor-browser-build branch here before my rbm build finishes. I'll report back once my rbm build has finished and I've verified that the fixes function as expected when built with rbm -- but feel free to start an rbm build and/or test the changes before I do so if you like.

comment:60 Changed 6 weeks ago by JeremyRand

I can confirm that the v8 branch fixes both issues for me. With regards to (1), it now takes circa 10 seconds for a .bit site to load if I type it into the address bar as soon as Tor Browser starts. Previously it was closer to 30 seconds. We can probably do better by having Electrum-NMC launch its connections as soon as the control port indicates Tor is connected, but for now I think v8 is "good enough" (and avoids making the invasive changes that would be needed for using the control port). I'll definitely look into using the control port for a later release though.

@gk feel free to test for more oddities. :)

comment:61 Changed 6 weeks ago by gk

Alright, this looks much better now, thanks. I think this is ready for landing. Could you do a final rebase on master and squash all the commits (having them separated was very helpful for testing and code review, though)?

comment:62 Changed 6 weeks ago by JeremyRand

Squashed and rebased branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v10 (Git commit hash dc1fa8a4b18468440e589c685eafb266211595dc). For reference, the rebased branch prior to the squash is at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v9 (Git commit hash 5237b3593bb08f2b828e8611ed3432af460624ef).

@gk Do you want to do a blogpost as described in http://ea5faa5po25cf7fb.onion/projects/tor/ticket/30558#comment:14 , or do you prefer to merge immediately and worry about whether to do a blogpost later? (Totally up to you.)

comment:63 in reply to:  62 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201912 removed
Resolution: fixed
Status: needs_revisionclosed

Replying to JeremyRand:

Squashed and rebased branch at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v10 (Git commit hash dc1fa8a4b18468440e589c685eafb266211595dc). For reference, the rebased branch prior to the squash is at https://notabug.org/JeremyRand/tor-browser-build/src/namecoin-v9 (Git commit hash 5237b3593bb08f2b828e8611ed3432af460624ef).

Thanks. Merged to master as commit 64a1bed987212218a2a04e456e1828bf135ab703. Thanks for all your hard work!

@gk Do you want to do a blogpost as described in http://ea5faa5po25cf7fb.onion/projects/tor/ticket/30558#comment:14 , or do you prefer to merge immediately and worry about whether to do a blogpost later? (Totally up to you.)

There is no blogpost planned at that point.

comment:64 Changed 6 weeks ago by JeremyRand

Great, thanks for the excellent (as usual) code review experience! Sometime after 36C3 it would be useful to ponder what the next steps are here. In the meantime, enjoy your holiday!

Note: See TracTickets for help on using tickets.