Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet2_basic: robust compat lib for loading/storing legacy wallets #4

Merged

Conversation

jeffro256
Copy link

@jeffro256 jeffro256 commented Aug 20, 2023

This library has no dependency on wallet2.h and gives us a way forward to move away from wallet2 in the (not-so-distant) future, while still supporting conversions of old wallet files. This lib is also useful if you have an application where you want to extract information directly from the wallet file with or without having to setup accounts and devices. This is now possible because I have split the wallet keys loading into two steps: load_from_memory and setup_account_and_devices. When one is loading a wallet keys file, the user of the API can choose whether or not to contact external devices during this process with use of the flag allow_external_devices_setup.

Reorganized for seraphis_migration.

@rbrunner7
Copy link
Member

Just to be sure and avoid misunderstandings: This is meant to be "the" commit regarding this functionality and code, right? If reviewed and merged, it would become part of the developing Seraphis enabled Monero prototype, and further development would happen here as well if needed, with further PRs to the same repo?

@jeffro256
Copy link
Author

Yes, that's the plan currently. I'll probably just add comments, tests, etc

@rbrunner7
Copy link
Member

Little comment: To successfully make a debug build of this code, a small correction is needed that @UkoeHB made in this commit.

@rbrunner7
Copy link
Member

@jeffro256: I think it would be very useful to add a high-level method to write to file because I believe it would make testing this much easier. The original PR where wallet2 used this code to write to file did of course not need it, and offered an easy way to test, but this code here does not.

@jeffro256
Copy link
Author

Little comment: To successfully make a debug build of this code, a small correction is needed that @UkoeHB made in this commit.

That should be fixed by merging those changes in a different commit/PR.

LOG_ERROR("THROW EXCEPTION: " << "error::wallet_internal_error");
tools::error::throw_wallet_ex<error::wallet_internal_error>(
std::string(__FILE__ ":" STRINGIZE(__LINE__)), "failed to load wallet cache");
}
Copy link

@DangerousFreedom1984 DangerousFreedom1984 Sep 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it will never reach this point as it will succeed or an error will be raised first but my compiler is not happy that there is not a 'return cache' here. Should it be there or should I change some config?
I'm getting that:
src/wallet/wallet2_basic/wallet2_storage.cpp:259:1: error: control reaches end of non-void function [-Werror=return-type]
259 | }
| ^
cc1plus: some warnings being treated as errors
make[3]: *** [src/wallet/wallet2_basic/CMakeFiles/obj_wallet2_basic.dir/build.make:90: src/wallet/wallet2_basic/CMakeFiles/obj_wallet2_basic.dir/wallet2_storage.cpp.o] Error 1
make[3]: Leaving directory 'build/Linux/initial_tx_history_final/debug'
make[2]: *** [CMakeFiles/Makefile2:4269: src/wallet/wallet2_basic/CMakeFiles/obj_wallet2_basic.dir/all] Error 2
make[2]: *** Waiting for unfinished jobs....
make[3]: Leaving directory 'build/Linux/initial_tx_history_final/debug'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... that seems to be a bug in the compiler. The [[noreturn]] annotations on throw_wallet_ex() should indicate to the compiler that the control flow ends there. However, I could modify it so the compiler can't mess it up.

@rbrunner7
Copy link
Member

Little comment: To successfully make a debug build of this code, a small correction is needed that @UkoeHB made in this commit.

That should be fixed by merging those changes in a different commit/PR.

I just saw that I merged the change in question already into our repository, resulting in this. So I think if you bring your code to this base the problem is already solved.

@DangerousFreedom1984
Copy link

I'm using this lib and I everything works fine for me. Just one thing to notice is that the m_daemon_rpc_mutex variable is not on the wallet2_basic (of course) so whenever this module is used, one needs to make sure to do it thread-safe.

@jeffro256
Copy link
Author

Little comment: To successfully make a debug build of this code, a small correction is needed that @UkoeHB made in this commit.

That should be fixed by merging those changes in a different commit/PR.

I just saw that I merged the change in question already into our repository, resulting in this. So I think if you bring your code to this base the problem is already solved.

That commit was merged into seraphis_lib, but also needs to be merged into branch seraphis_wallet, since seraphis_wallet is behind seraphis_lib.

@rbrunner7
Copy link
Member

Duh, yes of course. Separate branches, we don't directly use the Seraphis lib branch ...

@rbrunner7
Copy link
Member

I finally started to test this today.

By pure chance I stumbled over something that may be a not yet supported case: In the CLI wallet, there is a setting available called export-format, with the two possible values binary and ascii. If you have ascii active, as I actually had in the very first wallet I took for testing, surprisingly the setting influences the .keys of a wallet as well, which is not binary, but textual, with a first line of:

-----BEGIN MoneroAsciiDataV1-----

Maybe you encountered the code to handle this while converting it to wallet2_basic and decided to leave it out and make this edge case unsupported?

@rbrunner7
Copy link
Member

I tried with a (not yet finalized) multisig wallet and got the following error:

Error: Field multisig_derivations not found in JSON

That's expected, right? I think I saw a multisig-related "TODO" somewhere in the code.

Any plans yet when you will go back to those TODOs that are still there?

@rbrunner7
Copy link
Member

Just FYI: There was a first review of basically this code a while back by @vtnerd, as it started life as this Monero main code PR. Reviewing this here therefore does not start from zero, so to say.

@DangerousFreedom1984
Copy link

I tried with a (not yet finalized) multisig wallet and got the following error:
Error: Field multisig_derivations not found in JSON

I'm a bit confused about the level of strictness that we should aim for initially for the PRs in the seraphis_wallet. Of course we have to try to do our best but I think that it is impossible to be perfect at this moment. And that should not prevent us from moving further. The multisig stuff for example could be done later. As stated in the unit_test: // @TODO: missing fields m_multisig_signers, m_multisig_rounds_passed, m_multisig_threshold, m_multisig_derivations, so when the need comes, we do it.
Sorry, I'm confused and I have also to apologize because I have not been reviewing anything basically and this is pretty bad.

@vtnerd
Copy link

vtnerd commented Sep 24, 2023

Why not merge this into monero-project? The more this repo deviates from the main repo, the more painful it will be to merge it all back.

@rbrunner7
Copy link
Member

Why not merge this into monero-project? The more this repo deviates from the main repo, the more painful it will be to merge it all back.

I am not sure how good your overview is already regarding changes that will be needed to implement Jamtis and Seraphis. Current thinking is to make wallet2 private within the codebase, not available anymore to any wallet apps, as an especially striking example. Thus however the Monero master code and the new Seraphis based code finally find together again, it will be hard to reconcile everything, pretty much whatever we do.

That's why personally I don't worry too much about this code here in particular. It's all new, so merging back shouldn't be a big problem. Keeping it aligned with structure changes in wallet files over the next 2, maybe 3, years should be manageable.

On the other hand we can move quite a bit faster here with "our" repository as an advantage, IMHO.

But well, that's just my opinion; interested what other people think.

@rbrunner7
Copy link
Member

@DangerousFreedom1984: I also think that this can and should be merged soon, even if there as still some gaps in the functionality. I tested it today, it works solidly. I also looked over the code, ok as well. Further improvements are possible without much fuss with further PRs.

Strong opinions that this PR goes to the wrong repository altogether notwithstanding of course, see my take on @vtnerd 's question above :)

@rbrunner7
Copy link
Member

@jeffro256 : This code still contains some TODOs. What would you propose: You do those before we merge this, or we merge now and you make later separate PRs to do those?

I would be ready to merge now, to have the code easily available, but wonder how we would make sure those TODOs do not fall by the wayside.

And another question: With the code as of today, is there anything that could get lost if I load and then save a wallet because some data is not supported? E.g. a multisig wallet will not lose everythig multisig related if I load it and save it with that code?

@jeffro256
Copy link
Author

The TODOs in the unit tests are there because there's a couple of fields which aren't present in that specific wallet file, so those specific fields aren't tested. The TODOs in the key loading/storing are just me saying the code could be more conservative and assert the variant values of enums, but it isn't needed unless the user manually modifies those fields to be an invalid enum value.

And another question: With the code as of today, is there anything that could get lost if I load and then save a wallet because some data is not supported? E.g. a multisig wallet will not lose everythig multisig related if I load it and save it with that code?

It shouldn't... I tried to specifically make it a 1-to-1 matching for wallet2 files. However, if you do find a bug that removed information while storing, please let me know ;)

@rbrunner7
Copy link
Member

Thanks for the info, @jeffro256 . So from my side there probably only is this observation left to check. Maybe I will debug to have a look if it's not immediately clear to you where the problem is.

@jeffro256
Copy link
Author

jeffro256 commented Sep 29, 2023

I tried with a (not yet finalized) multisig wallet and got the following error:

That's a great find! That field shouldn't be mandatory for all multisig wallets. I think that that was a copy/paste error when I was writing the adaptor call for m_multisig_signers.

@jeffro256
Copy link
Author

Why not merge this into monero-project? The more this repo deviates from the main repo, the more painful it will be to merge it all back.

@vtnerd I'll honestly be convinced either way, but currently I'm of the mind that since this will be the most useful here, and since it may be iterated upon, we can mold it here first then pass on a more mature version to the core repo

ADAPT_JSON_FIELD(print_ring_members, Int, false);
ADAPT_JSON_FIELD(store_tx_info, Int, false);
ADAPT_JSON_FIELD(default_mixin, Uint, false);
ADAPT_JSON_FIELD(export_format, Int, false); // @TODO Check ExportFormat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think now I found a small thing that is not implemented. As reported here it looked as if loading a .keys file in ASCII format is not yet supported. Now, looking at your code, I think I confirmed that, because support for ExportFormat::Ascii is nowhere to be found.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the wallet now supports loading the keys using PEM ascii format, automatically detecting it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and storing it)

@jeffro256 jeffro256 force-pushed the wallet2_conv_sm branch 2 times, most recently from f85f1ce to 019f3f2 Compare October 22, 2023 20:47
@jeffro256 jeffro256 force-pushed the wallet2_conv_sm branch 2 times, most recently from 23e760a to c078e40 Compare October 22, 2023 21:58
@jeffro256
Copy link
Author

Only kink I'm working out now is how to load/store with non-standard KDF rounds

@jeffro256 jeffro256 changed the title wallet2_basic: robust compat lib for loading/storing legacy wallets [… wallet2_basic: robust compat lib for loading/storing legacy wallets Oct 22, 2023
…SERAPHIS]

This library has no dependency on wallet2.h and gives us a way forward to move away from `wallet2` in the (not-so-distant) future, while still supporting conversions of old wallet files.
This lib is also useful if you have an application where you want to extract information directly from the wallet file with or without having to setup accounts and devices. This is
now possible because I have split the wallet keys loading into two steps: `load_from_memory` and `setup_account_and_devices`. When one is loading a wallet keys file, the user of the API
can choose whether or not to contact external devices during this process with use of the flag `allow_external_devices_setup`.

Reorganized for `seraphis_lib`.
@jeffro256
Copy link
Author

jeffro256 commented Oct 23, 2023

Okay, I fixed the multiple KDF rounds issue. Is there any other loading/storing schemes that I could support?

@rbrunner7
Copy link
Member

Is there any other loading/storing schemes that I could support?

I can't think of any. I had no wallet older than 2017 to test, but I don't see why it would fail with even older ones, with the code being a 1-to-1 extract of the corresponding wallet2 code.

And anyway, with such things the only way to really test is using, using, using :)

If nobody opposes I intend to merge this PR tomorrow.

Copy link
Member

@rbrunner7 rbrunner7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a final look over the code. I did not systematically compare this code with the code parts in wallet2 where it comes from, but I really don't think that's necessary for this review: If the code works, i.e. loads and saves wallets correctly, we can be pretty sure the transformation was correct.

@rbrunner7 rbrunner7 merged commit 94864e8 into seraphis-migration:seraphis_wallet Oct 26, 2023
15 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants