Namecoin support for onion sites in Tor Browser

added TorBrowserTeam201912R component::applications/tor browser owner::JeremyRand priority::medium resolution::fixed reviewer::gk severity::normal status::closed type::defect labels

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

Trac:
Cc: pili, GeKo, antonela, asn to pili, gk, antonela, asn
Parent: #30029 (moved) to N/A

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

NO, HE supports different streams.

Trac:
Cc: pili, gk, antonela, asn to pili, gk, antonela, asn, boklm

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:

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

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.

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:

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

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

Namecoin is enabled only in Nightly.
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.)
Namecoin is enabled only on GNU/Linux targets. (I intend to add Windows, macOS, and Android/Linux support later.)
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.)
Stream isolation is supported in this patch, but is dependent on #19859 (moved). Until #19859 (moved) 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 (moved) is merged by manually setting the tor project's Git repo/commit to use the one provided in #19859 (moved). However, while I encourage review of this patch, I do not recommend merging this patch until #19859 (moved) is merged.
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.)
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.)
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.
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.
The circuit display panel will show the .onion domain that a Namecoin domain points to. Kudos to Arthur for that patch.
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.
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.
As far as I can tell, this patch is ready for review.

Cheers!

Trac:
Status: assigned to needs_review
Keywords: N/A deleted, TorBrowserTeam201911R added

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'

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

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.

Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201911R deleted, N/A added

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.

Trac:
Keywords: N/A deleted, TorBrowserTesm201911R added
Status: needs_revision to needs_review

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.

Trac:
Keywords: TorBrowserTesm201911R deleted, TorBrowserTeam201911R added

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:

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

Trac:
Keywords: TorBrowserTeam201911R deleted, TorBrowserTeam201911 added
Status: needs_review to needs_revision

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.

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.

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!

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.

Trac:
Keywords: TorBrowserTeam201911 deleted, TorBrowserTeam201911R added

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.

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.

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.

We are in December now.

Trac:
Keywords: TorBrowserTeam201911R deleted, TorBrowserTeam201912R added

you are in December gk, I am in euphoria.

gk to review these tickets

Trac:
Reviewer: N/A to gk

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

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.
For the certifi config, see 1) above. There is no need to include the root certs conditionally.
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?
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).

Trac:
Keywords: TorBrowserTeam201912R deleted, TorBrowserTeam201912 added

Oh, I guess I forgot another point:

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.

Thanks for the review @gk.

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.

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.

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

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.

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!

Replying to JeremyRand:

Thanks for the review @gk.

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.

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.

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.

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.

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.

Trac:
Cc: pili, gk, antonela, asn, boklm to pili, gk, antonela, asn, boklm, mcs

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

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

Trac:
Keywords: TorBrowserTeam201912 deleted, TorBrowserTeam201912R added
Status: needs_revision to needs_review

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)

Trac:
Keywords: TorBrowserTeam201912R deleted, TorBrowserTeam201912 added
Status: needs_review to needs_revision

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.

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

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.

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

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.

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

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.

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.

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.

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.

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

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?

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

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?

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?

Namecoin support for onion sites in Tor Browser

Child items ...

Activity