Deprecate wallet2 and replace it with the Seraphis lib

[j-berman]

Goals

• Unload the technical debt of wallet2 and replace it with a cleaner structured wallet using the Seraphis lib internally.
• Don't lose the existing features of the GUI/CLI/RPC wallets.
• Do this in the most efficient manner possible.

Challenges

• wallet2 does the heavy lifting for wallet logic, so replacing all its functionality is going to take a lot of work.
  • Manages wallet state, keys, and settings.
  • Handles wallet scanning, tx construction, signing messages, constructing proofs, and more.
• The CLI and RPC wallets are tightly woven with the wallet2 object (see m_wallet in simplewallet.cpp and wallet_rpc_server.cpp).
• The GUI uses the wallet API, and the wallet API is tightly woven with the wallet2 object (see m_wallet in wallet/api/wallet.cpp).

Proposed plan

Step 1: wallet API

Stop using the wallet2 object directly in the wallet API, and instead use the Seraphis lib, except to construct legacy txs.

• Benefits:
  • Takes a heavy load off reliance on wallet2 to manage state, keys, settings, and scanning.
  • Establishes a clean path to deprecate wallet2 once we no longer need to construct legacy txs.
  • Maintains the wallet API's existing functionality that wallet devs currently rely on.
  • Wallet devs that currently use the wallet API can start using the Seraphis lib sooner rather than later without needing to overhaul their wallets.
• Challenges:
  • The wallet API is tightly woven with the wallet2 object (references m_wallet 193 times).
  • Will still need a "house" for wallet state and settings.
  • Will still need to be able to read a wallet2 keys file and wallet cache and migrate it over to a new file structure (related to 1 and 2).
  • Needs care to avoid turning the wallet API into another wallet2.
• Estimated full-time work to complete: 3-6 months.

Step 2: CLI and RPC wallets

Stop using the wallet2 object directly in the CLI and RPC wallets, and instead use the wallet API.

• Benefits:
  • Cuts off major dependencies on wallet2.
  • Minimizes time and maintenance burden to develop features for all of the GUI/CLI/RPC wallets in the future.
  • Enables reusing the suite of functional tests for the RPC wallet for the Seraphis lib.
  • Charts a path for wallets like Feather that also use the wallet2 object directly to use the wallet API exclusively.
• Challenges:
  • Both CLI and RPC wallets are tightly woven with the wallet2 object (CLI references m_wallet 724 times, RPC 368 times).
  • Will likely need to extend functionality of the wallet API to allow for some of the behavior that the CLI and RPC wallets need.
    • e.g. the wallet API doesn't support wipeable strings when passing secrets around, which the CLI relies on to keep sensitive key material out of RAM. On this note, the wallet API actually keeps the wallet password in memory too which needs to change as @tobtoht proposed here.
• Estimated full-time work to complete: 2 months.
  • There is a lot of logic in CLI and RPC that the wallet API duplicates, so it should mostly be a matter of replacing high level calls, rather than deep changes.

Note: technically the above 2 steps can be done in parallel, but I figured it would be a cleaner process to prioritize 1 first.

Why keep the wallet API?

• Significantly reduces time to develop functional GUI/CLI/RPC wallets that maintain existing functionality.
• Makes it significantly easier for wallet devs who currently use the wallet API to migrate to Seraphis.
• Gets the Seraphis lib used by devs sooner rather than later.
  • Helps squash bugs in the Seraphis lib.
  • Gets faster scanning into user's hands sooner.
• The wallet API is ok, not perfect, but ok. Dropping wallet2 would be a major win.

[j-berman]

@r4v3r23 proposed an interesting idea as well to me in DM's.

Before replacing the entirety of wallet2 in the wallet API in Step 1, Step 0.5 could be to replace just the scanner. My knee-jerk reaction was it might cause more headache later, but it should be smooth if done like this pseudocode:

wallet_api::refresh()
	seraphis_lib::refresh_enote_store()
	populate_wallet2_instance_from_seraphis_enote_store()

Since we'd want to be able to use wallet2 "in a box" to construct a tx, then we'd need populate_wallet2_instance_from_seraphis_enote_store to work correctly anyway. Starting with the scanner like this could be a solid incremental step. That's already in the thousands of lines of wallet2 code replaced by the Seraphis lib.

[rbrunner7]

Thanks for moving this, appreciated.

Interesting proposal, and I guess an almost logical consequence of all now quite substantial "delays" that we have with implementing our new wallet, for various reasons: The ideal, a brand-new API designed out on a green field, is probably a luxury we can't afford anymore timewise.

A question: Why take wallet_api as base for the whole endeavor, and not directly wallet2.h itself? Does wallet_api just add the right amount of abstraction on top of that to make the switching of more or less all code underneath feasible in the first place? Strikes me as a bit improbable, but I don't know that API in detail.

If we can pull off your trick even with wallet2.h itself, compatibility could be even better, no?

[j-berman]

I don't see much benefit to getting rid of wallet2.cpp going that route. I figure if we were to keep wallet2.h, we might as well just extend wallet2.cpp for Seraphis reusing individual components from the Seraphis lib.

I do think the wallet API offers a solid base of abstraction to build from that makes it easier to drop wallet2.h and wallet2.cpp in favor of a cleaner wallet structure.

This is generally postulation on my part though from eyeing the code and working with it.

I'm strongly warming to this idea to start an incremental approach with the scanner.

[rbrunner7]

Hmm, you are not afraid that if you add absolutely everything that wallet apps need for their fully current functionality, wallet_api starts to look a lot like wallet2.h?

[j-berman]

Ya it's a concern, I included that as a challenge to this approach. I think following similar coding practices as the Seraphis lib (clean class separation, shorter functions, measured design decisions to name a few), it's avoidable.

Example: the Seraphis lib's modular scanner shows how we can replace a major component of wallet2 with cleaner structured code. That's why I'm thinking the scanner is a good place to start.

[r4v3r23]

i support this modular approach

since according to rbrunner7 a new api is out of the question, reusing the current one and making the seraphis essentially a drop-in replacement update for current wallets is a massive plus

will be helping @j-berman test his upcoming scanner once its ready

[DangerousFreedom1984]

I agree that now we have so many Seraphis components that integrating into the existing code starts to be the main challenge. I like your organizational proposal as it tries to set a clear path to use some seraphis features though I dont know if it is the best approach to build wallet3 since we cant add all features into the wallet2_api and it constrains us with wallet2 design. But at least looks like we have an approach now. So let me rephrase some of your ideas to try to understand it and let me try to frame what I can do to contribute.

Also, I disagree with @rbrunner7 here. Looking at how the seraphis_lib work, Koe did such an excellent job that it is already doing a great deal of the work that the wallet2 functions do. Therefore implementing a wallet and an api for seraphis should be much easier than implementing wallet2 again.

Interesting proposal, and I guess an almost logical consequence of all now quite substantial "delays" that we have with implementing our new wallet, for various reasons: The ideal, a brand-new API designed out on a green field, is probably a luxury we can't afford anymore timewise.

Anyway, moving forward with this thought.

Step 1: wallet API - stop using wallet2 (m_wallet) but keep using the API

How exactly would we do that?

I can imagine taking the following path:

We create a wallet3.h(.cpp) under src/wallet/api/ to implement the wallet2_api.h interface.

So it means implementing the methods from: WalletListener, Wallet, WalletManager and so on.

wallet3 here would call the src/seraphis_wallet/* functions.

Is that what you think?

I think it would be nice to initially target a high degree of compatibility with the GUI therefore using the wallet2_api.h as the interface would be a good first approach in that sense. I believe if that is achieved, all the other wallets wouldnt have big troubles to migrate (given that the interface is very similar). But of course it can't be achieved 100% since jamtis_seraphis have many new features that are not contemplated in that interface. But adding new features could be done incrementally and the old methods could be replaced with the new wallet3 methods progressively (as well as the style since we would be migrating from camelCase to snake_case :p).

In the same way, since the CLI uses wallet2 directly. We could also target creating a new CLI that uses the wallet2_api.h interface initially and then migrate to wallet3_api.h to fully support jamtis_seraphis if/when needed. I believe I could target that in my CCS (basically I'm trying to have the basic functions of a wallet using the PoC CLI that I created and with the help of @jeffro256 we hopefully will be able to create demonstrator that is integrated with the LMDB database.) so in the end we would have a demonstrator of a CLI wallet3 that uses the wallet2_api.h / wallet3_api.h that does the basic functions of the wallet. What do you think?

[j-berman]

I was thinking something a bit different.

How exactly would we [stop using the wallet2 object]?

I was imagining systematically going through each m_wallet-> function and replacing with Seraphis lib-style structured code.

I loosely categorize what m_wallet is used for in src/wallet/api/wallet.cpp as follows:

• read wallet keys / key data
• read wallet settings
• mutate wallet settings
• read wallet state
• mutate wallet state
• use wallet state to do something else
• what are essentially static functions

I think most functions could be structured better and "housed" into a better bin. I was thinking something along these lines...

The keys can get their own class referenced via m_wallet_keys which has its own readers and modifiers.

The settings can get their own class referenced via m_wallet_settings which has its own readers and modifiers.

The enote store can capture wallet state. We'd rewrite wallet2 functions into Seraphis-style functions (refresh_enote_store being a perfect example as replacement for m_wallet->refresh) to read/mutate/use wallet state.

Static functions can be refactored.

[DangerousFreedom1984]

Interesting. So basically the bug free seraphis_wallet is the non existing seraphis_wallet ? :p
Basically you would 1) refactor wallet2 and 2) add new functionalities to it, right?

[j-berman]

Would eliminate dependencies on wallet2.cpp and wallet2.h, and add new functionalities to src/wallet/api/{wallet.cpp, wallet.h, wallet2_api.h, wallet_manager.cpp, wallet_manager.h} as needed to replace lost functionality

[rbrunner7]

I have been reading through the discussion so far and tried to understand the various ideas and approaches mentioned here, and I am not sure whether I fully understood what everbody tried to say and whether also people fully understood each other so far. This is perhaps not surprising, because it's a difficult endeavor.

Faced with a difficult and complex programming task I always try to radically simplify it first and find a way to do it using steps of manageable size. IMHO in this case, all on my own, I would probably try the following first:

Put away any Seraphis library code for the time being. Work strictly only within the frame of existing Monero code. Basically, just complete at long last what that "Wallet API" originally set out to do but did not fully accomplish: Create a new, cleaner and better structured wallet API that supersedes wallet2.h.

End goal: You can write any wallets and wallet-like apps like the RPC server, even fully featured ones like the CLI wallet, by using the "Wallet API" only, without using anything from wallet2.h directly.

That would mean going systematically going through wallet2.h and the "Wallet API" header files in parallel, make a "gap analysis" i.e. identify what functionality of wallet2 is not yet available through the "Wallet API", add carefully designed new classes, methods, structs, enums etc. for that missing functionality, and then implement those, based on wallet2.h, possibly other existing lower Monero classes, or new code.

Hopefully the amount of new code needed would be pretty small, with most code just use one thing or another from wallet2.h to do the "heavy lifting" - just with a better API, better structure, and better naming.

Result would be something that we could PR to the existing Monero codebase without problems, because, again, it would not yet include any parts of the Seraphis lib, reviews should be possible in a short timeframe, with many people being able to review in the first place, and wallet devs could start to use it anytime.

Of course, once that's out of the door, you can continue and almost immediately start to use e.g. @j-berman 's new scanner together with the Seraphis lib parts that it needs.

[SNeedlewoods]

End goal: You can write any wallets and wallet-like apps like the RPC server, even fully featured ones like the CLI wallet, by using the "Wallet API" only, without using anything from wallet2.h directly.

FWIW no one disagreed with the end goal.

That would mean going systematically going through wallet2.h and the "Wallet API" header files in parallel, make a "gap analysis" i.e. identify what functionality of wallet2 is not yet available through the "Wallet API" ...

I would even break it down further and add an intermediate goal:
Get rid of all wallet2.h code in api/wallet.h and replace it, without adding new functionality first.

Here is an incomplete list of "API Tasks" that I see so far:

1. Get rid of wallet2.h in api/wallet.h

• Seperate changes into new classes/files if suitable:
  • WalletSettings
  • WalletKeys
  • WalletState* (like EnoteStore)
    • Maybe add a Blockchain class, which would replace functionality from hashchain class in wallet2.h.
  • api/utils.cpp (imo some functions don't need to directly be part of WalletImpl)
    • e.g. uint64_t wallet2::estimate_blockchain_height()
• Serialization should get seperated* (maybe into wallet_api_serialization.h following the design from jeffro256 here https://github.com/UkoeHB/monero/pull/39/files).

2. Add all necessary features needed for RPC/CLI wallet.
3. Add a layer for multisig (e.g. a class which inherits from WalletImpl from api/wallet.h), there shouldn't be any multisig code directly in api/wallet.h.*

* I'm not sure about this.

First I planned to handle WalletSettings only and make a PR, then make another PR for WalletKeys and so on. But after spending some time implementing things for WalletSettings I figured, how much everything is intertwined, so now I'm leaning towards "bite-sized" PRs where just a single function is targeted, but with the whole rattail (every change needed to make it work), I assume this would lay out a better foundation for later work.