feat: kyoto

[reez]

Description

Builds on #314 to add Kyoto to client options

Notes to the reviewers

References

#296

https://github.com/rustaceanrob/amstel

https://github.com/rustaceanrob/BDKSwiftExampleWallet/tree/kyoto

https://github.com/bitcoindevkit/BDKSwiftExampleWallet/tree/experiment/kyoto

https://github.com/reez/BDKSwiftExampleWallet/tree/kyoto

Changelog notice

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I have formatted my code with swift-format per .swift-format file

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• UI changes tested on small, medium, and large devices to ensure layout consistency

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[r1b2ns]

Maybe we could remove the network picker during onboarding when Kyoto is selected

[r1b2ns]

There’s a small bug: when we import a wallet, the block height doesn’t appear on the first run. But after killing and reopening the app, it starts showing correctly

First Execution after import a Wallet

Second

[reez]

Maybe we could remove the network picker during onboarding when Kyoto is selected

This is a good call out (since we are only supporting Signet for Kyoto right now)!

I guess I'd probably start by asking @rustaceanrob if he thinks there are any other test networks that are stable (testnet/testnet4/mutinynet/etc) for kyoto or not. Maybe there are and I could be missing it.

If it looks like we are still only going to support Signet with Kyoto, then I can see why we'd remove Signet picker (because there's nothing else to pick other than Signet) yet I can also see why we'd leave Signet picker (because while a user can't pick a different network they at least know what network they are about to be on, otherwise they wouldn't know if they are on signet or testnet until they go to SettingsView; the other solution would be to add a label for what network but then I think that opens up other questions about whether we should be adding a optional label for a specific edge case)

[reez]

There’s a small bug: when we import a wallet, the block height doesn’t appear on the first run. But after killing and reopening the app, it starts showing correctly

First Execution after import a Wallet

Second

great catch! I will investigate and try to make a nice fix. Thanks so much this is the type of thing it was nice to have your review for.

[rustaceanrob]

Kyoto supports Testnet4 as well, but I recommend only exposing Signet. Mutinynet is not supported and no plan to, as I think it will become unmaintained at some point. Just to remain flexible, I would keep the network picker.

[rustaceanrob]

I can confirm that a new wallet does not render the block height on the first sync, until restarting the app.

The block header sync feels super slow and I think it has to do with all of the string parsing from the logs. Is it possible to just rely on the Info events?

On the UI, the progress bar is a bit slow. Would it make sense to have this fill the entire top HStack? Otherwise I like the block height in the UI, and the connection icon is super useful for the user.

[reez]

There’s a small bug: when we import a wallet, the block height doesn’t appear on the first run. But after killing and reopening the app, it starts showing correctly

I can confirm that a new wallet does not render the block height on the first sync, until restarting the app.

8718349 Fix for these, haven’t done full testing yet, but I think the fix could be as simple as this so adding a commit for it right now before I forget

[reez]

c967bd6 refactor for rob correctly pointing out sync vs async

[reez]

The block header sync feels super slow and I think it has to do with all of the string parsing from the logs. Is it possible to just rely on the Info events?

381d36c removing string parsing from logs (?)

Not sure if this is what you were thinking @rustaceanrob but I think I was trying to parse logs because im not sure I could rely on Info for all the events and get the info I need. a bit fuzzy still on this.

6e4c2d0 reverted removing string parsing from logs because i do need it (might be totally missing something though, but didn't get the info I show in the UI otherwise)

[rustaceanrob]

After removing the Task.sleep call, following the node.run() recommendation I gave, and removing all string parsing, the client syncs at a reasonable speed on my end. I will leave individual review comments for each case the logs are parsed for the recommended Info field. If something is not present in the Info that you see in the logs, feel free to make an issue on FFI or Kyoto, although in this instance I found a replacement in Info for each string parse.

[reez]

An informational message may be useful to the user to inform them it will take a couple minutes to start their wallet.

I need to make sure I dont forget to loop back around on this and the pull-to-refresh thing. Still collecting my thoughts on pull-to-refresh, and then for the info message I'm thinking about having some link to Kyoto explainer somewhere in settings view and then having a TipKit https://developer.apple.com/documentation/TipKit the first time someone runs Kyoto telling them what the progress is related to

[reez]

It might also make sense to start a recovery from a height other than 0, like the Taproot activation in the case of this wallet. That would greatly speed up the initial wallet sync, but depends on your approach for supporting new descriptor types in the future.

Yeah would like to hear more thoughts from you on this, I hadn't thought about it at all yet, but right now we are supporting BIP-86 default and then recently I added support for BIP-84 bc0a485

I'm definitely open/interested in all of these details and nuances for making the wallet experience great

[reez]

I can follow up with another PR for some of these optimizations to keep the scope of this one manageable.

Yeah this is a tough balance at the moment, trying to keep as much as possible in this PR but I'm touching a lot of stuff and doing a lot in the PR, I'm hoping its the right choice though

Because I want everything to work right when the Kyoto feature is merged, but at the same time im touching some esplora stuff (and causing some regressions in both as I iterate, etc). But yeah hopefully I get this all tied up nicely in this PR.

[reez]

Pushing more UI updates and fixes, trying to snuff out all the regressions across clients (fixing one, causing another, sometimes), there's a lot of compat to do with multiple clients/networks heh.

[reez]

Adding some more logging (temporary) because having a hard time getting info from peer which is hindering some of the UI testing I'm doing across clients

[reez]

Seeing a not connected icon/color even when it is, going to figure out whats up with that...

[reez]

Seeing a not connected icon/color even when it is, going to figure out whats up with that...

hopefully fixed with this c1977a0

[rustaceanrob]

TipKit https://developer.apple.com/documentation/TipKit the first time someone runs Kyoto

Great idea. Although I think this should go into a separate pull request.

UI looks good to me. I might propose some tweaks later, but I like where this is at.

From the Kyoto perspective, I think this is a good first iteration, and in the worst case where we can't follow up, still looks good for a release. I can't review anything Esplora related, so my final ACK doesn't mean much until you give a self-review. I would rebase this into a smaller commit history as a personal preference.

I should have some follow-ups over the next couple weeks, but I will make issues for those once you are happy with this PR.

[reez]

TipKit https://developer.apple.com/documentation/TipKit the first time someone runs Kyoto

Great idea. Although I think this should go into a separate pull request.

UI looks good to me. I might propose some tweaks later, but I like where this is at.

From the Kyoto perspective, I think this is a good first iteration, and in the worst case where we can't follow up, still looks good for a release. I can't review anything Esplora related, so my final ACK doesn't mean much until you give a self-review. I would rebase this into a smaller commit history as a personal preference.

I should have some follow-ups over the next couple weeks, but I will make issues for those once you are happy with this PR.

Great. Your comments/suggestions/questions have been super helpful. I'll try to finish out my own testing/review of this to make sure it's working with no regressions, and then make any final small changes (any missing UI, remove logging, etc) and then I'll squash+merge, then that will free things up for smaller PRs.

[reez]

Adding a post-merge follow up issue #319