LN-Zap / zap-iOS

Zap Wallet - Native iOS lightning wallet focused on user experience and ease of use ⚡️
http://zaphq.io
MIT License
181 stars 47 forks source link

Bugfix suggestion: Fix Tor connection issues #342

Closed ghost closed 4 years ago

ghost commented 4 years ago

Description

Recent versions of lnd have introduced new fields for the RPC GetInfoResponse message. Zap's Lnrpc_GetInfoResponse struct seems to have been generated using lnd version 0.8.0.

A diff for GetInfoResponse version v0.8.0-beta and v0.10.1-beta (most recent version) can be viewed here: v0.8.0-beta...v0.10.1-beta.

In short, the new fields that have been introduced to GetInfoResponse are:

When connecting to a Tor node, Zap fails to parse the GetInfoResponse JSON, because its Lnrpc_GetInfoResponse struct does not know about those new fields. This results in a connection error - making it impossible to connect to Tor enabled nodes. I believe this is the reason for the connection problems that numerous people in #333 are reporting.

An obvious quick fix for this problem is to ignore unknown fields when decoding lnd's JSON responses. This is what this pull requests proposes. By ignoring unknown fields, I can successfully connect to my node again using the latest version of Zap from master.

👉 Obviously a better fix would be to regenerate Lnrpc_GetInfoResponse. I figured before I start diving into that I'll ask for your guys' opinion:

  1. whether this makes sense generally
  2. what you suggest as a way forward for this issue

Thanks for the great work you put into Zap so far and let me know what you think! 🙏

Motivation and Context

How Has This Been Tested?

  1. I tried connecting to a Tor enabled node using a Zap build from master.
  2. I received what seems to be the same errors reported in #333.
  3. I placed error breakpoints in Xcode to verify the source of the connection problem.
  4. JSON parsing errors reporting unknown fields for GetInfoResponse were thrown.
  5. I enabled the option to ignore unknown fields for JSON decoding
  6. I could successfully connect to the same Tor enabled node from step 1.

I do not believe that this has effects on other parts of the code, however as discussed above, a more durable fix compared to ignoring unknown fields would maybe be to regenerate the decoding code, adding the newest fields.

Screenshots

Click for screenshot 📸

Types of changes

Checklist:

ferologics commented 4 years ago

Bump ☝️ when is this going to get merged?

ottosuess commented 4 years ago

LGTM, thanks a lot 🎉