[RFC] Add TorRPC.swift

[brett-doffing]

A consolidation of MakeRPCCall.swift and Reducer.swift.

[brett-doffing]

Initially, upon Concept ACK (if applicable), I’d like to simply add the TorRPC.swift file. After adding the file I would like to go through each file it would touch individually as a WIP, create some tests, clean-up, document, refactor to use the new TorRPC, then create a PR for that individual file. In this way I think there could be a smooth incremental transition for improved quality of a fundamental piece of code. I have already done a dry run of sorts and assessed in general what needs to be done. While there are some lingering questions, I believe I can address them as I go through each case.

What TorRPC does so far is consolidate MakeRPCCall.swift and Reducer.swift into one file as Reducer seems to be an unnecessary abstraction. TorRPC begins by removing some properties, namely: torClient (unnecessary reference to a singleton), ud (unused reference to UserDefaults), errorBool, errorDescription, and objectToReturn, which can be handled by, and returned with, the new Swift Result type, and the implemented TorRPCError enum for error handling. Essentially, files making use of this call simply need to refactor their handling of the returned Result type, and perhaps be cleaned up in the process.

With respect to the TorRPC currently, I would like to refactor it more, but I would also like to determine a few more things:

• Is it necessary to call getNode() every call? It doesn’t look like nodes are updated too often.
• Where exactly is it necessary to retry the call 20 times upon failure, and could it not be handled there? As it stands, potentially every call could be called 20 times, and I’d like to remove that?
• Is creating a queue necessary? It looks like it might be redundant as TorClient possibly creates the same queue already?
• Should param be of type Any rather than String? I only found it to be of type String, introducing an unnecessary conversion from Any to String.

Anyway, I think this is something that could be done incrementally, not necessarily done as priority yet keep me busy, could be a good start in improving code quality, and I’d like to get some feedback on it.

[Fonta1n3]

To answer your direct questions:

• Is it necessary to call getNode() every call? It doesn’t look like nodes are updated too often.

Yes. The app allows users to add multiple nodes, so it is necessary to check which one is active, I would rather do that in one central place than everywhere else. The user can at anytime go to NodeManager and switch nodes/add a node/delete a node or switch wallets which may be on a different node. In a normal situation it is quite likely a user would want to run a testnet and a mainnet node simultaneously, as far as the app is concerned this will be treated as two separate nodes as each hidden service will be representing by one individual rpcport. Not to mention that many enthusiasts have multiple nodes.

• Where exactly is it necessary to retry the call 20 times upon failure, and could it not be handled there? As it stands, potentially every call could be called 20 times, and I’d like to remove that?

This is absolutely essential. If you build the app in XCode and watch the console output you will see many failed calls to the node returning an Internet connection appears to be offline or request timed out etc. This is due to the slowness of the onion network. Unless we want the app to seem totally broken we need to automate these attempts for the user. 20 attempts has never failed for me in my thousands of tests. One thing that needs to be improved is to detect if it is the tor V3 authentication that is failing and if that is the case return a specific error to the user rather than continue on making 20 failed authentication attempts. Maybe after 10 attempts we should warn the user that the request is taking longer than expected. Tor is slow and getting that initial connection to the node is not instant. If you are not seeing these failed attempts in the output it could be that my wifi is just bad here in Indonesia, to me it is beneficial that the app will work even on poor connections.

• Is creating a queue necessary? It looks like it might be redundant as TorClient possibly creates the same queue already?

Could be redundant I am not sure.

• Should param be of type Any rather than String? I only found it to be of type String, introducing an unnecessary conversion from Any to String.

Yes this could be changed to string but may have implications throughout the app. The codebase is not optimized, I am soon approaching the point of optimization but not there yet.

Generally:

The Reducer.swift file literally reduces the total number of lines of code throughout the app by a large portion, to me this justifies its existence as it makes the codebase cleaner and offers a lot of nuanced benefits that are not obvious. When we make a specific rpccall to the node we know what type it will return so rather then having a hundreds of if let statements for node responses throughout the app I can have one large switch statement that checks if the command was successful or not, filters the commands and if it was successful we will know the type has already been cast by Reducer.swift (see NodeLogic.swift for an example of what I mean).

I do want to keep Reducer.swift separate from MakeRPCCall.swift as they do two different things. One makes the request and one parses/filters the response and handles a number of special cases. There is some funny business happening with bitcoind whereby it may not return any type of response for certain rpccalls, so if you have an if let statement it will simply fail even though the rpc call may have been successful, these cases need to be handled. We need to check if one of those null return type calls was made to the node and instead return a custom type of response that we assign rather than bitcoind's null response. Granted some of the calls that return null types are not yet used in the app but will be soon. It is also a convenient place to do things like check for specific errors whereby the wallet may not be loaded and we can reload it. Reducer.swift is handy because the file handles all request responses throughout the app and allows us to handle weird stuff in one place rather than for every single rpc call.

[Fonta1n3]

Please do not take this the wrong way but I just see this as opening a can of worms and I do not see any practical benefit so for now concept NACK.

Of course once things settle down we should optimize this code and make some changes here, but for now it is battle tested, works great and reduces the size of the overall codebase.

For higher priority items that can keep you busy would you please have a look at the Kanban TODO list, it is kept up to date. Anything you are working on please move it into the "In progress" section by dragging/dropping it that way we can avoid over writing/duplicating each others work :)

[brett-doffing]

I am entirely open to critical review, no need to worry about that. I do however think there is more to discuss, which is what this is. I quite literally went through every piece of code that Reducer.swift touches and replaced it in a dry run (no build), as you could find in my ["network" branch],(https://github.com/doffing81/FullyNoded-2/commit/e120052e0226285f2f1eaff54767cf7a5d60462d) and my conclusion was that Reducer is essentially a redundant call to executeRPCCommand() with an added check to return a sort of status completion message if a specific command was made (which I believe I handled), and a couple other errors to handle (which I believe I can handle), although the info you have laid out about them is interesting and really just gets me curious. In any event, I consider the consolidation itself to be altogether insignificant.

WRT all of those files that Reducer touches, although I did more than was necessary in my dry run in refactoring and cleaning up the code (with 716 additions and 1,469 deletions i.e. reducing the code), the only significant change (which is why I was confidently able to do so much) is making use of Swift's Result type, for which returned values are then simply cast locally, merely replacing how they are already assigned, rather than Reducer needing 5 different properties for possible responses. If I were to not consolidate, this would probably be the one change I would propose, alongside better error handling, as this is specifically what the Result type is for, but this would require a copy of the method in order to not be immediately disruptive, which is essentially all TorRPC.swift is.

I have more questions and things to say about the implementation of getNode(), the 20 tries, and a whole host of other implementation details, but as this doesn't seem to be a favorable proposal, I need to clarify my perspective.

The critical nature of this proposal lies in its concern for the code. This codebase needs to be cleaned up, and I see no more a perfect place to start than with quite possibly the most fundamental piece. I'd argue that the more you build off of an unkept codebase, the harder it will be to deal with. This is my concern over adding features and the like, which I can still and will do along the way, while fortifying the longevity of the codebase with tests, documentation, and refactoring, and I'd hope, in the very least, this not be viewed as opening a can of worms. I don't think I could have made this proposal in any less an intrusive way, and it is hard for me to see it as anything but beneficial. Be things as they may, I have no problem not doing this, except of course concern for the codebase, and excusing "not right now", which will probably creep up more.

[brett-doffing]

I don't think I'm looking at merging quite yet, but more breaking down, discussing, and understanding each part to see what could incrementally be done. I just commented out the attempts for emphasis really.

What I am looking to do is merely pull the attempts and getNode() functionality out of the method, if I can, and I suppose in general, any nested functionality.

Also, I'm not particularly looking to use TorRPC immediately, but rather work on an individual file, one at a time, and write tests for it, document, clean it up, then make a PR for that individual file. In this way, everything else just stays the same, while the code incrementally gets better.

[Fonta1n3]

Understood, that approach is great, I agree with what you are trying to accomplish but it is critical that existing functionality is not broken and non obvious edge cases with bitcoind are taken into account. I did not mean to imply what you are trying to do will open a can of worms, I think it is an improvement, I just meant that making changes there could have unintended consequences. As far as knock on effects are concerned I have not seen your other branch so do not know what has been done. I do not like to see the code commented out for important stuff like attempts and wallet not loaded error handling. FWIW I would suggest starting with NodeLogic.swift to ensure the home screen builds as it should, if the new TorRPC file works there it should be fine everywhere else.

[brett-doffing]

Well, that sounds like a perfect place to start. I’ll have to see what I come up with.

[ChristopherA]

I would recommend that you to agree on a separate branch name for this, and do your work there separate from main and other branches. Once functioning it will still need to be re-based into main but I think that that’s the best solution.

[brett-doffing]

I wonder if there could be a more general pipeline for such work? Essentially, this is all I am interested in doing, namely write tests, document, and clean up.

[ChristopherA]

@doffing81 wrote:

I wonder if there could be a more general pipeline for such work? Essentially, this is all I am interested in doing, namely write tests, document, and clean up.

First, we should consider setting up more of a "git flow" methodology. For instance, addition to main (which eventually will be the current app in the App Store), there could be testflight-testnet-dev, which is what Peter will use to create Testflight versions, then testflight-testnet-dev-feature-X testflight-testnet-dev-refactor-X, testflight-testnet-dev-test-X, for any branch that two or more people are working on together, or on developer forks, testflight-testnet-dev-doffing81-feature-X for something that has been assigned to a particular developer.

There are other top-level approaches I'm ok with, such as having main be the "current development" branch, and "stable" be the App Store branch.

Peter, as lead, I suggest that you take to a search "git flow" and "git naming branches" — you'll find a lot of strategies, and suggest one you are comfortable to use. You may also want to look at bitcoin-core branching and tagging strategies.

-- Christopher Allen

[brett-doffing]

So, just for kicks, I threw in the code from my dry run into NodeLogic to see how far it could get, and it worked right out of the gate! That doesn't really say much other than I was pleasantly surprised. I still have the attempts logic commented out, so I tried some refreshes, and sure enough it likes to think it is "offline" or something, but I am noticing that once it originally kicks in, then moves to the successive calls in loadNodeData(), it doesn't seem to have any problems with those, which is making me wonder if there is some session issue where a session may be kept alive. Anyway, that's what I am looking into.

[Fonta1n3]

Nice! I very much doubt it is a URLSession issue and am quite positive it is a Tor network issue (eg getting that initial response from your hidden service), Tor itself is not designed for optimum speed/connectivity. But if you can prove me wrong I will be very happy. This is stuff I have been banging my head against the wall on for well over a year.

[brett-doffing]

The curious thing is how unreliably a refresh will work, yet how reliably the successive calls work, and I am absolutely curious to explore why the successive calls work, or at least to see if they will ever fail.

And I am now noticing a counter that is counting "Connections". It increments on both fail and success, yet noticeably it does not increment on successive successes lol, which seems to indicate something is being kept alive and allowing for uninterrupted queries.

[Fonta1n3]

I think it is all about the initial connection/authentication to the V3 hidden service, in the beginning I envision the circuit being made to the correct HSDir (it can take time to find that path) then there is a lot of crypto stuff happening with key exchanges etc on the Tor protocol level. Even if the V3 hidden service is unauthenticated as far as Tor is concerned all V3 hidden services are authenticated as they have some dummy authentication info put in place for unauthenticated V3 services. Worth doing a deep dive into V3 hidden services and how they work to see what I mean. Very keen to hear if you gain any insights.

[Fonta1n3]

FWIW there are a few commented out lines in the TorClient.swift file which if you uncomment them will reveal a ton more detailed log info regarding the Tor process. Probably worth you while to uncomment these lines out:

Line 177/178: //TORInstallEventLogging() //TORInstallTorLogging()

Worth noting I keep them commented out as they have caused crashes in the past.

[brett-doffing]

TORInstallTorLogging() seems to make the app crash, and TORInstallEventLogging() doesn't seem to be giving me any added info wrt failed attempts, but while the initial connection is absolutely something to look into, at this point I think I am more focused on the connection being manageable, or rather that it is, and can be, kept alive.

[Fonta1n3]

Hmmm, the connection being manageable and staying alive is not an issue. Unless you put the app into the background, lock the app and then unlock and bring the app back into the foreground. There is a known issue, would be helpful if you can assist in resolving that.

[brett-doffing]

If you know how to keep it alive, then there is no reason to attempt a dataTask() 20 times, which is the issue I am looking to resolve, however when it comes to the initial connection, which could itself be a better solution, I think I just have to figure out what "[] nw_protocol_read_connect Received SOCKS connect status 5, failing" means.

[Fonta1n3]

I think you are looking at it backwards. IMO If there is no need to call it 20 times then it won't be called 20 times :-)

As you have already seen we need to call it multiple times, or break the app.

[brett-doffing]

You understand that I am looking for a solid solution to an arbitrary band-aid?

[Fonta1n3]

Again, I would be really keen to know if you gain any insights there. Wish you luck. For stuff like this I have found raising issues on the Tor.framework and being highly persistent helps :)

[brett-doffing]

Well, I opened an issue with the project just to see if I could gain some insight there, but not really expecting much, and as such, I will still be looking to remove the recursive calls from the method itself and place them elsewhere, and/or see if I can maintain a connection, but I won't dwell too much on this. More just interesting than a priority.

[Fonta1n3]

When you say maintain a connection or stay connected really what is happening is the tor node in the app will constantly be connected to the tor network and is literally a node in and of itself, I believe we can even host a hidden service from the device. The connection to the bitcoind hidden service though is not some constant thing, you make individual request and get individual responses back, there is not an ongoing connection (per say) to the hidden service that needs to be managed. For example you can switch out that url with another nodes url and it will speak with that node instead yet you have not altered your Tor connection in anyway. Hope I am making sense here.

[brett-doffing]

Well that would be contrary to what I am finding, where the “Connection” I speak of looks to be related to NSURLSession, and not Tor. Can you verify that the Tor library is responsible for incrementing the Connections in the errors/failures?

[Fonta1n3]

I would say the URLSession is independent of any specific connection to your bitcoind node's hidden service, again the reason I say that is you can switch between hidden services without effecting or reinitiating the URLSession config in TorClient.swift.

At least that is how I perceive it.

I think a lot of the errors you talk about really have to do with the server side of things on the tor network itself. If you are using our testing node it could be multiple people are making calls to it at the same time, it is by no means a production server so may not be able to handle multiple simultaneous calls very well.

You can experiment by disabling ExcludeExitNodes from the tor config options and then attempt to visit normal urls over the clearnet via the same URLSession, or better yet use our StandUp.app to run your own bitcoind hidden service. I have found different servers are easier/faster to connect to than others.

I wonder if there is some server side settings we can tweak to reduce failed attempts? I have not dug into this, I do know that the server time zone must be set to UTC for Tor to work properly but thats about it :)

I am by no means an expert on any of this and have been learning as I go. If you have input it is greatly appreciated.

[brett-doffing]

I'd love to use my own node, but the Linux scripts didn't work for me, and there isn't a project in the repo.

[Fonta1n3]

What linux distro did you try it on? They must be run as root. We have only tested on Debian 10 and Ubuntu 18.04. Would you mind please raising an issue on the repo below with some info about what went wrong and which distro/version you tried? Here is the project's repo.

Also if you have mac you can try out StandUp.app which we also would appreciate contributions on :)

[ChristopherA]

Supposedly BTCPay works with QuckConnect, and it does a full node in Docker. So that could also work for you.

[brett-doffing]

Ah, didn’t realize that the Mac app had moved (or at least I was looking in the wrong spot), but I already raised an issue days ago for the scripts. It was on Ubuntu 18.04. I sudo’d the script the first time, then actually su’d to root the next. I also had a node with Tor before the scripts, so not sure if that means anything. Essentially no host is recognized for the hidden service. Regardless, I’ll look into the Mac app, or maybe even trying out BTCPay.

[Fonta1n3]

Ah sorry I forgot to watch the new repo so did not get a notification. Have replied there ;) More testing needs to be done with the scripts on machines that already have nodes running etc. Would be ideal to provide stand alone scripts to only set up tor or bitcoind or simply configure the hidden service itself. Right now its more geared for a fresh VPS. Too much to do, too little time.

[brett-doffing]

@Fonta1n3, do you know if "Requested wallet does not exist or is not loaded" and "Could not connect to server" errors/exceptions result from a successful response with the node, whereby they could be found at if let errorMessage = errorCheck["message"] as? String in MakeRPCCall? I haven't come across them yet. Also, would you know if it is necessary to handle "Requested wallet does not exist or is not loaded" in both Reducer's nested getResult() and NodeLogic's loadWalletData()?

[Fonta1n3]

Yes they work, but they should only be in the MakeRPCCall.swift or Reducer.swift file. Having it in both of those files is redundant. Please do test though, you can test that by issuing an bitcoin-cli unloadwallet command to the node for your active wallet.

The "Could not connect to server" error check only works when app reenters from background if you have not locked/unlocked your device. This is an ongoing issue on how to deal with tor dying in the background, a lot of testing has been done on this and basically the current method is a good trade off between a lot of different approaches/nuances.

[brett-doffing]

I am now going to close this PR as I think we’ve exhausted its communicative purpose. I’ll be taking a horribly timed trip this week, so hope I don’t get Covid-19, but after that I’m hoping to have written some tests and made some worthwhile changes to TorRPC and NodeLogic and can perhaps push them to a branch set up for a better flow.