wallet2_basic: robust dep-free lib for loading/storing wallet2 files #8923

jeffro256 · 2023-06-30T15:59:38Z

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.

This PR is a work in progress because more testing is needed, though most of the functionality that is needed of it is already implemented.

vtnerd · 2023-07-01T00:14:38Z

What is considered a "legacy wallet" ?

jeffro256 · 2023-07-01T01:42:37Z

I mean the current wallet2 wallets. I used the term “legacy” in the context of the new wallet code being worked on in the “No Wallet Left Behind” project

vtnerd

I'm not really a fan of changing serialization formats to json. Was this discussed in the chatroom you mentioned (I'm now in that room)?

Msgpack is better for perf and size reasons, especially if string keys are used its trivial to generate a JSON tree with a generic implementation.

src/wallet/wallet2_basic/wallet2_boost_serde.h

vtnerd · 2023-07-01T15:53:00Z

src/wallet/wallet2_basic/wallet2_boost_serde.h

+namespace serialization
+{
+template <class Archive>
+inline void serialize(Archive &a, wallet2_basic::transfer_details &x, const boost::serialization::version_type ver)


This might be a good time to try boost::polymorphic_oarchive and boost::polymorphic_iarchive. It should be possible to put these implementations in a cpp - the slowdown from the stable calls should be minimal.

Realistically, there's basically only ever two archive classes that will ever be used here: {portable_}binary_iarchive so I put those definitions into a cpp file but wrote it so that this can easily be extended to other templated archives or polymorphic archives.

Why not use the polymorphic type now? Does it not work? There will be a speed difference, but its should be somewhat small.

I'm trying it out right now, but one unfortunate thing for binary bloat if we go the polymorphic route is that there is no Boost provided polymorphic route for portable_binary_iarchive so we have to define the class and generate the code for this route in the wallet binary.

vtnerd · 2023-07-01T16:00:05Z

src/wallet/wallet2_basic/wallet2_storage.cpp

+}
+
+template <typename T>
+void read_json_object_key_Int(T& out, const rapidjson::Value& json, const char* name, bool mand)


Is all this new serialization stuff? Why rapidjson all of a sudden? I have a msgpack implemenation ready with unit tests.

I answered my own question eventually, this isn't just moving around files @UkoeHB this is changing the serialization format to a JSON format backed by rapidjson.

My understanding is that this library is only intended to be used to read old wallet formats, so we can drop wallet2 while keeping support for old formats. None of this is new, .keys files use JSON today.

@tobtoht is correct. This lib defines new types cache and keys_data and has analogous deserialization code to wallet2::load. The generally point of the lib is to allow the core project to support the current wallet formats moving forwards, but ditch the vast majority of wallet2 code.

I don't see (at a quick glance) where the code is "coming from" (the corresponding delete).

I also don't see a store function for keys_data, was that left untouched?

jeffro256 · 2023-07-04T17:42:09Z

Another benefit of this PR that I contributed as a side effect of recalculating the cache key at loading/storing: the chacha key to unlock the cache file data no longer sits in memory as long as the wallet is loaded. If an attacker were to extract the cache key from memory (it was not previously locked) then they could snoop into and modify the cache file indefinitely.

EDIT: I reverted this change since the way the wallet is structured, it would be way too much refactoring to change automatic storing without m_cache_key.

vtnerd

Did a slightly longer review, but I'll still have to do another pass I think (this is somewhat big).

src/wallet/wallet2_basic/wallet2_storage.cpp

vtnerd · 2023-07-05T01:16:30Z

src/wallet/wallet2_basic/wallet2_storage.cpp

+        else
+            ASSERT_MES_AND_THROW("Field " << name << " found in JSON, but not " << "Int");
+    }
+    else if (mand)


I think the value should be default initialized when not mandatory.

I have the defaults listed in the header wallet2_storage.h for each field in the class. Since we start deserializing with a default constructed object, if the field is not present in the JSON, then the default constructed value stays.

vtnerd · 2023-07-05T12:52:20Z

src/wallet/wallet2_basic/wallet2_storage.cpp

+}
+
+template <typename T>
+void read_json_object_key_Int(T& out, const rapidjson::Value& json, const char* name, bool mand)


I don't see (at a quick glance) where the code is "coming from" (the corresponding delete).

I also don't see a store function for keys_data, was that left untouched?

src/wallet/wallet2.cpp

jeffro256 · 2023-07-05T18:06:20Z

also don't see a store function for keys_data, was that left untouched?

Yes, but I'm planning to integrate it in actually just because it will help us "prove" that the code works. The load/storing keys data code should be much more efficient as well anyways

jeffro256 · 2023-07-11T05:34:34Z

For the reviewers' sake should I split this into multiple PRs: One for adding the code, the other for integration?

jeffro256 · 2023-07-11T16:53:36Z

@vtnerd I used the polymorphic archives with this latest commit and was able to shrink the binary size by 3%

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`.

jeffro256 · 2023-08-21T00:17:46Z

I'm moving this PR to the seraphis-migration repository here: seraphis-migration#4

vtnerd · 2023-08-21T14:53:44Z

Why move it there? Isn't that repo going to eventually PR back into this repo?

jeffro256 · 2023-08-21T17:17:16Z

The reason I chose to do it this way is so that I can quickly prototype and reiterate on the code as use cases arise, since that repo needs these changes more immediately than the core repo and there's fewer review requirements. Then my plan was to PR against the main repo when the code was in a more concrete form. But honestly, I'd probably be swayed pretty easily if you convinced me to PR it here first then downstream it.

rbrunner7 · 2023-08-21T17:58:19Z

But honestly, I'd probably be swayed pretty easily if you convinced me to PR it here first then downstream it.

If you ask me you can't develop code in two places at once. Either let it flow into the main repo, master branch, and improve it further with more PRs to the master branch, or PR it to the Seraphis repo and then it lives there and gets developed further there.

Either way works for me. A decision soon would be nice however, because I wait for the moment you declare "That's it now, please review".

jeffro256 marked this pull request as draft June 30, 2023 15:59

jeffro256 force-pushed the wallet2_conv branch 2 times, most recently from 5357cc0 to ba5a9a3 Compare June 30, 2023 18:24

vtnerd reviewed Jul 1, 2023

View reviewed changes

jeffro256 force-pushed the wallet2_conv branch 6 times, most recently from 1b6e1f2 to 9433232 Compare July 4, 2023 17:25

vtnerd reviewed Jul 5, 2023

View reviewed changes

jeffro256 force-pushed the wallet2_conv branch 4 times, most recently from 66395f9 to 0ac8189 Compare July 11, 2023 05:21

jeffro256 changed the title ~~wallet2_basic: lightweight compat lib for loading legacy wallets~~ wallet2_basic: lightweight compat lib for loading/storing legacy wallets Jul 11, 2023

jeffro256 changed the title ~~wallet2_basic: lightweight compat lib for loading/storing legacy wallets~~ wallet2_basic: lightweight compat lib for loading/storing wallet2 files Jul 11, 2023

jeffro256 changed the title ~~wallet2_basic: lightweight compat lib for loading/storing wallet2 files~~ wallet2_basic: robust dep-free lib for loading/storing wallet2 files Jul 11, 2023

jeffro256 force-pushed the wallet2_conv branch from 0ac8189 to 29accd2 Compare July 11, 2023 05:24

jeffro256 force-pushed the wallet2_conv branch from 29accd2 to a8c2b20 Compare July 11, 2023 05:42

jeffro256 force-pushed the wallet2_conv branch from a8c2b20 to 468daa9 Compare July 11, 2023 17:57

jeffro256 marked this pull request as ready for review July 18, 2023 04:13

jeffro256 mentioned this pull request Aug 1, 2023

wallet2_basic: robust compat lib for loading/storing legacy wallets [… UkoeHB/monero#13

Closed

jeffro256 closed this Aug 21, 2023

rbrunner7 mentioned this pull request Sep 24, 2023

wallet2_basic: robust compat lib for loading/storing legacy wallets seraphis-migration/monero#4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet2_basic: robust dep-free lib for loading/storing wallet2 files #8923

wallet2_basic: robust dep-free lib for loading/storing wallet2 files #8923

jeffro256 commented Jun 30, 2023

vtnerd commented Jul 1, 2023

jeffro256 commented Jul 1, 2023

vtnerd left a comment

vtnerd Jul 1, 2023

jeffro256 Jul 3, 2023

vtnerd Jul 3, 2023 •

edited

Loading

jeffro256 Jul 4, 2023

vtnerd Jul 1, 2023

vtnerd Jul 1, 2023

tobtoht Jul 1, 2023

jeffro256 Jul 2, 2023

vtnerd Jul 5, 2023

jeffro256 commented Jul 4, 2023 •

edited

Loading

vtnerd left a comment

vtnerd Jul 5, 2023

jeffro256 Jul 5, 2023

vtnerd Jul 5, 2023

jeffro256 commented Jul 5, 2023

jeffro256 commented Jul 11, 2023

jeffro256 commented Jul 11, 2023

jeffro256 commented Aug 21, 2023

vtnerd commented Aug 21, 2023

jeffro256 commented Aug 21, 2023 •

edited

Loading

rbrunner7 commented Aug 21, 2023

wallet2_basic: robust dep-free lib for loading/storing wallet2 files #8923

wallet2_basic: robust dep-free lib for loading/storing wallet2 files #8923

Conversation

jeffro256 commented Jun 30, 2023

vtnerd commented Jul 1, 2023

jeffro256 commented Jul 1, 2023

vtnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtnerd Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffro256 commented Jul 4, 2023 • edited Loading

vtnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffro256 commented Jul 5, 2023

jeffro256 commented Jul 11, 2023

jeffro256 commented Jul 11, 2023

jeffro256 commented Aug 21, 2023

vtnerd commented Aug 21, 2023

jeffro256 commented Aug 21, 2023 • edited Loading

rbrunner7 commented Aug 21, 2023

vtnerd Jul 3, 2023 •

edited

Loading

jeffro256 commented Jul 4, 2023 •

edited

Loading

jeffro256 commented Aug 21, 2023 •

edited

Loading