[seraphis_wallet]: key container #15

ghostway0 · 2023-11-03T21:04:26Z

depends on #14
would love feedback
cc @rbrunner7

…is_lib_hist_05_15_23 branch for commit history

rbrunner7

For me this class leads to a surprising number of quite "deep" questions about the architecture of the wallet and the schemes that the wallet code will use. I will invite other team members to leave opinions and comments in the Matrix room.

In any case I think it may take some time to disuss this all and get this into a merge-ready state ...

rbrunner7 · 2023-11-05T09:03:15Z

src/seraphis_wallet/key_container.cpp

+    sp::jamtis::make_jamtis_ciphertag_secret(test_keys.s_ga, test_keys.s_ct);
+    sp::make_seraphis_spendkey(test_keys.k_vb, test_keys.k_m, test_keys.K_1_base);
+    sp::jamtis::make_jamtis_unlockamounts_pubkey(test_keys.xk_ua, test_keys.xK_ua);
+    sp::jamtis::make_jamtis_findreceived_pubkey(test_keys.xk_fr, test_keys.xK_ua, test_keys.xK_fr);


Already one more of these quite general questions that I ask myself when going through this code, but can't arrive at an answer: If it's so easy to derive all keys from the one base one, why would we still read and write all of them? Why not just write the spend key and then re-generate all the other keys when deserializing?

rbrunner7 · 2023-11-05T09:05:40Z

src/seraphis_wallet/key_container.cpp

+{
+}
+//-------------------------------------------------------------------------------------------------------------------
+boost::optional<KeyContainer> KeyContainer::load_from(const std::string &path, const crypto::chacha_key &key)


Right now I don't think that we will ever have to support this use case in the final code, i.e. reading and writing a key container alone in and out of files. Maybe it's a good idea to move such capabilities into the unit tests.

rbrunner7 · 2023-11-05T09:08:37Z

src/seraphis_wallet/key_container.cpp

+    return write_encrypted_file(path, key, view_only_keys);
+}
+//-------------------------------------------------------------------------------------------------------------------
+bool KeyContainer::write_view_balance(const std::string &path, const crypto::chacha_key &key)


I think it's a quite difficult question to decide how to implement different wallet types, like full wallet, read-only wallet, address-generation wallet, multisig wallet, whatever. I am not sure at all whether finally the way to go will be to have a set of different read and write methods in the key container class here. Maybe leave those away for the time being? But seems to me I should not decide this alone, eager to read what other people are thinking.

I think these are convenient for the tiers that make sense to share around

src/seraphis_wallet/jamtis_keys.h

rbrunner7 · 2023-11-05T09:47:51Z

src/seraphis_wallet/jamtis_keys.h

+///
+struct jamtis_keys
+{
+    crypto::secret_key k_m;           //master


I don't know the Seraphis lib that closely yet. Are very short and at first sight quite cryptic names like k_m the norm there? Do tons of comments refer to this key as k_m so it would not be a good idea to give this a more "speaking" name like master_key?

Paging @jeffro256, @jberman and @DangerousFreedom1984 for their opinions as well.

I agree with this. Imo the key names should be more descriptive (and not abbreviated like this) everywhere they're used.

it's from jamtis.md iirc

I'm still more a proponent of descriptive names in the code; imo it makes it easier to follow without keeping so much in your head. I think it's fine for the spec (math papers usually use one letter notation). I also think matching the spec is ok, so I'm fine with either way.

I'm still more a proponent of descriptive names in the code

+1

After thinking about it again, one more vote to switch to the usual descriptive names for things. Devs shouldn't have to deal with names like xk_fr or even mixed-case K_1_base for so fundamental things. Descriptive names trump having names that are identical with the spec, IMHO.

src/seraphis_wallet/jamtis_keys.h

rbrunner7 · 2023-11-05T09:58:00Z

src/seraphis_wallet/key_container.cpp

+{
+//-------------------------------------------------------------------------------------------------------------------
+// INTERNAL
+static jamtis_keys ser_keys_to_jamtis(ser_jamtis_keys const &keys)


As far as I remember we never properly discussed this issue I wrote back in summer whether we really use the Seraphis library scheme of pairs of classes, one "normal" and one ser_ class, for serialization of wallet classes. Maybe now it's the time to come back to this?

I don't think they should be here

DangerousFreedom1984

The idea and implementation of the key container looks like going in the right direction to me. I like the idea of not being dependent on the generate_chacha_key function in these classes. But maybe would be better to have something to replace it?

src/seraphis_wallet/jamtis_keys.h

src/seraphis_wallet/jamtis_keys.cpp

DangerousFreedom1984 · 2023-11-09T11:59:09Z

src/seraphis_wallet/jamtis_keys.h

+///
+struct jamtis_keys
+{
+    crypto::secret_key k_m;           //master


I'm still more a proponent of descriptive names in the code

+1

src/seraphis_wallet/key_container.h

DangerousFreedom1984 · 2023-11-09T12:55:21Z

src/seraphis_wallet/jamtis_keys.cpp

+        user_address_out);
+}
+//-------------------------------------------------------------------------------------------------------------------
+void jamtis_keys::encrypt(const crypto::chacha_key &key, const crypto::chacha_iv &iv)


Wallet2 is doing that to (re)store a private key in this order:

from password uses cn_slow_hash to get chacha key.

from chacha key use encrypt/decrypt function.

from encrypt/decrypt function uses cn_slow_hash to get derived chacha key.

from derived chacha key uses chacha20 to encrypt data.

Here you are proposing to encrypt/decrypt the keys directly using chacha20 without using a derived key or another round of the generate_chacha_key which is based on a hashing algorithm (cn_slow_hash) in this case. So from my understanding we would be removing one layer of security here. Maybe it could be added later when generating the chacha key that will feed these functions. Do you have any comments here?

I don't like the idea of using cn_slow_hash but I also dont like to remove this another layer of hashing.

add comments and modify some functions in the key_container

SNeedlewoods

This is my first review, so take it with a grain of salt. My intention was to be diligent, sorry if it comes off as nitpicking.

From my POV, except the small things I commented on, this looks complete.

tests/unit_tests/encrypted_file.cpp

SNeedlewoods · 2024-01-18T12:18:12Z

src/seraphis_wallet/encrypted_file.h

+    BEGIN_SERIALIZE_OBJECT()
+    VERSION_FIELD(0)
+    FIELD(encrypted_data)
+    FIELD(iv)
+    END_SERIALIZE()


I'd say indent fields between BEGIN_SERIALIZE and END_SERIALIZE for consistency, but then realized this file was already merged, so not sure.

sure. it's just that clang-format doesn't understand this format, so I'm hesitant

src/seraphis_wallet/jamtis_keys.cpp

src/seraphis_wallet/jamtis_keys.h

SNeedlewoods · 2024-01-18T14:00:27Z

src/seraphis_wallet/key_container.h

+    FIELD(k_m)
+    FIELD(k_vb)
+    FIELD(xk_ua)
+    FIELD(xk_fr)
+    FIELD(s_ga)
+    FIELD(s_ct)
+    FIELD(K_1_base)
+    FIELD(xK_ua)
+    FIELD(xK_fr)


SNeedlewoods · 2024-01-18T14:00:48Z

src/seraphis_wallet/key_container.h

+    FIELD(keys)
+    FIELD(encryption_iv)
+    FIELD(encrypted)


SNeedlewoods · 2024-01-18T14:03:38Z

tests/unit_tests/encrypted_file.cpp

+    BEGIN_SERIALIZE()
+    FIELD(data)
+    END_SERIALIZE()
+
+    BEGIN_KV_SERIALIZE_MAP()
+    KV_SERIALIZE_VAL_POD_AS_BLOB_FORCE(data)
+    END_KV_SERIALIZE_MAP()


Indent fields.

tests/unit_tests/encrypted_file.cpp

tests/unit_tests/key_container.cpp

ghostway0 · 2024-01-22T16:50:56Z

last question: to indent, or not to indent?
indenting those FIELDs is what happened until now, but it also means that we're going against the new clang-format file. wdyt?

DangerousFreedom1984

Could you rebase your branch with the most updated seraphis_wallet branch and then force push your last changes?

DangerousFreedom1984 · 2024-03-28T22:45:00Z

src/seraphis_wallet/jamtis_keys.cpp

+#include "seraphis_wallet/key_container.h"
+#include <cassert>
+#include <openssl/crypto.h>
+#include <sodium/crypto_verify_32.h>


What functions do you need from these libraries?

crypto.h from openssl I don't think I need
I think we should include a clang-tidy to tell us about these issues.

DangerousFreedom1984 · 2024-03-28T22:48:02Z

src/seraphis_wallet/jamtis_keys.cpp

+        return seraphis_wallet::WalletType::PaymentValidator;
+    }
+
+    if (is_one(keys.d_fa) && is_one(keys.s_ga))


Why they have to be equal to the identity? They should be different from zero, right?
You can use keys.d_fa == x25519_secret_key{} for example to compare the private x25519 keys to zero.

right, will fix
that will not be constant time :) if that matters even

src/seraphis_wallet/jamtis_keys.cpp

src/seraphis_wallet/jamtis_keys.h

rbrunner7 · 2024-04-14T14:15:33Z

@ghostway0, will you have opportunity to bring this to a conclusion anytime soon, what do you think?

UkoeHB and others added 6 commits May 16, 2023 15:34

add key exchange round booster to multisig_account

e99b422

seraphis mega squash commit [initial commit - 05/15/2023]; see seraph…

0b10b72

…is_lib_hist_05_15_23 branch for commit history

async: explicit task constructors

dbd8c9c

rebase fixes

f3dda41

[seraphis_wallet]: encrypted file

62b29e0

[seraphis_wallet]: key_container

46e5a6c

rbrunner7 reviewed Nov 5, 2023

View reviewed changes

DangerousFreedom1984 reviewed Nov 9, 2023

View reviewed changes

DangerousFreedom and others added 2 commits January 11, 2024 07:10

add comments and modify some functions in the key_container

d013693

Merge pull request #3 from DangerousFreedom1984/ghostway/key_container

2aaaf33

add comments and modify some functions in the key_container

SNeedlewoods reviewed Jan 18, 2024

View reviewed changes

fix

032928d

rbrunner7 force-pushed the seraphis_wallet branch from e2ca1aa to bf95191 Compare February 25, 2024 09:17

rbrunner7 force-pushed the seraphis_wallet branch from bf95191 to e651e0b Compare March 24, 2024 18:47

jamtis v2

7cca5f9

DangerousFreedom1984 reviewed Mar 28, 2024

View reviewed changes

This was referenced Apr 17, 2024

Deprecate wallet2 and replace it with the Seraphis lib seraphis-migration/strategy#3

Closed

Deprecate wallet2 and replace it with the Seraphis lib seraphis-migration/wallet3#64

Open

ghostway0 and others added 7 commits May 3, 2024 13:49

fix

887a27d

jamtis v2

e8e9797

add comments and modify some functions in the key_container

df4b3a8

fix

e88668a

jamtis v2

6b637a0

fix

772bf14

jamtis v2

bee5ae2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[seraphis_wallet]: key container #15

[seraphis_wallet]: key container #15

ghostway0 commented Nov 3, 2023 •

edited

Loading

rbrunner7 left a comment

rbrunner7 Nov 5, 2023

rbrunner7 Nov 5, 2023

rbrunner7 Nov 5, 2023

ghostway0 Nov 6, 2023

rbrunner7 Nov 5, 2023

j-berman Nov 6, 2023 •

edited

Loading

ghostway0 Nov 6, 2023

j-berman Nov 6, 2023

DangerousFreedom1984 Nov 9, 2023

rbrunner7 Jan 19, 2024

rbrunner7 Nov 5, 2023

ghostway0 Nov 6, 2023

DangerousFreedom1984 left a comment

DangerousFreedom1984 Nov 9, 2023

DangerousFreedom1984 Nov 9, 2023

SNeedlewoods left a comment

SNeedlewoods Jan 18, 2024

ghostway0 Jan 22, 2024

SNeedlewoods Jan 18, 2024

SNeedlewoods Jan 18, 2024

SNeedlewoods Jan 18, 2024

ghostway0 commented Jan 22, 2024

DangerousFreedom1984 left a comment

DangerousFreedom1984 Mar 28, 2024

ghostway0 Apr 8, 2024

DangerousFreedom1984 Mar 28, 2024

ghostway0 Apr 8, 2024

rbrunner7 commented Apr 14, 2024

[seraphis_wallet]: key container #15

Are you sure you want to change the base?

[seraphis_wallet]: key container #15

Conversation

ghostway0 commented Nov 3, 2023 • edited Loading

rbrunner7 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-berman Nov 6, 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

DangerousFreedom1984 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SNeedlewoods 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghostway0 commented Jan 22, 2024

DangerousFreedom1984 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

Choose a reason for hiding this comment

rbrunner7 commented Apr 14, 2024

ghostway0 commented Nov 3, 2023 •

edited

Loading

j-berman Nov 6, 2023 •

edited

Loading