Loki name service - Multiple owners #1048

Doy-lee · 2020-02-27T07:43:54Z

Allows a primary and backup public key (either Monero twist, public key or a standard ed25519 key) to be the owner of a LNS record. If you are the owner, you are able to produce valid signatures to update the underlying value of the LNS record.

These maybe monero, maybe ed25519 keys are stored into crypto::generic_public_key and similarly for signatures into crypto::generic_signature.

I've also added lns_make_update_mapping_signature which allows wallets to generate the required signature standalone, so now other wallets that aren't owners- can update mappings on behalf of the owner wallet- provided that the owner wallet sends them the appropriate signature and new value they wish to update to.

jagerman · 2020-03-04T18:14:39Z

src/crypto/crypto.h

+    unsigned char      data[sizeof(ed25519_public_key)];
+    static constexpr generic_public_key null() { return {0}; }
+    operator bool() const { return memcmp(data, null().data, sizeof(data)); }
+  };


This is a textbook case for using a variant.

I look at unions with skepticism in C++ code, but these ones are particularly dirty because the code is also throwing away (important!) key/signature type information, which is never a good thing. Aside from the C++ type safety, this also throws away the type safety in the encoding of the value so that we have no idea what type of public key or signature we actually have when we reconstruct it. As a result, the verify ends up trying one and then trying the other, and that feels very wrong to me.

The clean solution here is to use a variant<ed25519_public_key, public_key> (either boost or mapbox from the lokimq PR) and similarly for signatures; when encoding you then use a flag and to use a flag when serializing/deserializing to encode the type information so that you can reload the variant with the proper type on the other end. Thus you never lose the key/signature type, and you never have to "guess" by trying to do a verify against whatever type it could be: signature verification is just a visitor that does the correct signature verification call. (Plus a variant of hashable types is itself hashable so you can still use as an unordered map key, which looks like it was part of the desire here).

I'd prefer just adding a enum struct type/bool is_monero flag to the structure itself to keep the information together with the type rather than variant because the codegen for variant sucks. With the variant method you also decouple the type from the data for de/serialisation?

If that's not an issue with you then I'm ok with switching over to variant.

because the codegen for variant sucks.

Can you elaborate? AFAIK most of the codegen issues with variant are around visiting, which you wouldn't need to use here (just get_ifs for the two types).

With the variant method you also decouple the type from the data for de/serialisation?

Sure, but you always do that with serialization, deserialization should pick it up again (which means the serialization needs to carry the type as well).

Can you elaborate? AFAIK most of the codegen issues with variant are around visiting, which you wouldn't need to use here (just get_ifs for the two types).

Yes. You're somewhat right its good at -O2 for the std implementation but boost is just crazy, https://godbolt.org/z/awP_wd

Sure, but you always do that with serialization, deserialization should pick it up again (which means the serialization needs to carry the type as well).

Yeah actually not a big deal after some thought as it can easily be achieved

struct generic_key { variant<a, b> key; bool is_a; };

Loki-mq is using mapbox::variant because most things in boost are too heavy.

jagerman · 2020-03-04T18:35:41Z

src/cryptonote_core/loki_name_system.cpp

+  {
+    if (!crypto::check_signature(hash, key.monero, signature.monero))
+      return false;
+  }


As mentioned above, this should be a type-safe visitor over a variant.

Scratch that, it can just probe the types with an if/else rather than doing a full visit.

jagerman · 2020-03-04T18:55:55Z

src/cryptonote_core/loki_name_system.cpp

+        *reason = err_stream.str();
+      }
+      return false;
+    }


You have this same basic block of code (with some small variations) 33 times in this file (and also there's a bug in the "trying to renew too early" case about 150 lines above this, which isn't checking whether reason is set). You can DRY it out a lot like this:

template <typename... T> static bool check_condition(bool condition, std::string* reason, T&&... args) { if (!condition && reason) { std::ostringstream o; #ifdef __cpp_fold_expressions // C++17 (o << ... << std::forward<T>(args)); #else (void) std::initializer_list<int>{(os << std::forward<T>(args), 0)...}; #endif *reason = o.str(); } return condition; }

and then all of these check-and-set-reason blocks compact hugely to just:

if (!check_condition(entry->signature, reason, tx, ", ", *entry, ", signature to validate is the null signature")) return false;

I will fix up the typo and apply the change in the other PR since some messaging/new errors have been added.

jagerman · 2020-03-04T19:44:23Z

src/cryptonote_core/loki_name_system.cpp

+WHERE NOT EXISTS
+  (SELECT * FROM "mappings"
+   WHERE "owner"."id" = "mappings"."owner_id"
+   OR "owner"."id" = "mappings"."backup_owner_id"))";


This query is going to have to do a full table scan currently; it needs some indexes added on mappings.owner_id and mappings.backup_owner_id.

(I don't have a lot of experience with sqlite's query optimizer; it might also benefit from being rewritten as WHERE NOT EXISTS (... = owner_id) AND NOT EXISTS (... = backup_owner_id), but in either case it'll still want the indices.

I've added what I think you meant by this with

+CREATE INDEX IF NOT EXISTS "owner_id_index" ON mappings("owner_id");
+CREATE INDEX IF NOT EXISTS "backup_owner_id_index" ON mappings("backup_owner_index");

Looks good.

jagerman · 2020-03-04T19:46:59Z

src/cryptonote_core/loki_name_system.cpp

-  char constexpr SAVE_MAPPING_SQL[]                     = R"(INSERT OR REPLACE INTO "mappings" ("type", "name", "value", "txid", "prev_txid", "register_height", "owner_id") VALUES (?,?,?,?,?,?,?))";
-  char constexpr SAVE_OWNER_SQL[]                       = R"(INSERT INTO "owner" ("public_key") VALUES (?);)";
-  char constexpr SAVE_SETTINGS_SQL[]                    = R"(INSERT OR REPLACE INTO "settings" ("id", "top_height", "top_hash", "version") VALUES (1,?,?,?))";
+  std::string const get_mappings_by_owner_str            = sql_cmd_combine_mappings_and_owner_table(R"(WHERE "o1"."public_key" = ? OR "o2"."public_key" = ?)");


You can avoid needing to double-bind the value when you use this in get_mappings_by_owner by rewriting this condition as:
WHERE ? IN ("o1"."public_key", "o2"."public_key")

jagerman · 2020-03-04T20:51:35Z

src/simplewallet/simplewallet.cpp

-    PRINT_USAGE(USAGE_BUY_LNS_MAPPING);
-    fail_msg_writer() << "lns name didn't start or end with quotation marks (')";
+    PRINT_USAGE(USAGE_LNS_BUY_MAPPING);
+    fail_msg_writer() << "lns name didn't start or end with quotation marks (\"), first word in name is=\"" << local_args[first_word_index] << "\"";


Forcing quotation marks is kind of strange when the vast majority of registrations (and all lokinet registration names) are going to be single word values. Could we change this logic to:

if first_word_index == last_word_index, then don't require quotes (but remove them if present on front and back).

otherwise keep the quotes required logic.

Also the quoted value here is sort of... weird (because of how simplewallet parses arguments): trying to register

"foo bar"

will actually register

foo bar

Actually: why do we allow spaces in names at all? It seems like it is going to break a huge ton of things. For example, if someone has a tag "a b" then what happens when someone tags their user name in Session?

@a
@a b
@a b c

What if a and a b and a b c are all registered? Or what if Session sees @word another word here blah blah, will it have to look up word and then word another and then ...?

Actually looking some more - we do zero validation on session names right now, which doesn't seem right. (Control characters? Whitespace? Non-normalized UTF-8? Invalid UTF-8 sequences?)

(These comments about allowed characters in Session LNS names are aimed a bit more generally - @KeeJef @gmbnt)

I'm fine if we just following the hostname naming conventions for usernames and domain names https://en.wikipedia.org/wiki/Hostname#Restrictions_on_valid_hostnames

I brought this up earlier about the perils of utf8 whitespacing in names but no one raised an issue :(

I'll validate this as per the wiki in a new PR.

jagerman · 2020-03-04T21:06:36Z

src/simplewallet/simplewallet.cpp

+  return true;
+}
+//----------------------------------------------------------------------------------------------------
+static char constexpr NULL_KEY_STR[] = "0000000000000000000000000000000000000000000000000000000000000000";


This would be more user friendly if we just printed "(none)" instead of a bunch of 0s.

Yes, that'd be good.

jagerman · 2020-03-04T21:07:22Z

src/simplewallet/simplewallet.cpp

+    return true;
+  }
+
+  std::string const &new_value = args[args.size() - 1];


args.back()

jagerman · 2020-03-04T21:15:42Z

src/rpc/core_rpc_server.cpp

      return false;

-    std::map<crypto::ed25519_public_key, size_t> key_to_request_index;
-    std::vector<crypto::ed25519_public_key> keys;
+    std::map<crypto::generic_public_key, size_t> key_to_request_index;


Should be an unordered_map

In general the number of keys is going to be small, so I opted for a tree?

unordered_map on our pubkey types is actually really fast because we don't actually have to hash anything, we just use the value itself as a hash. But you're right, for a small container it's not going to matter anyway.

jagerman · 2020-03-04T21:22:06Z

src/wallet/wallet_rpc_server_commands_defs.h

+
+In future, support for mappings on Blockchain wallets and Lokinet addresses will be available. Tentatively,
+  - for Wallets, the recommended owner is the monero ed25519 public key of the user's wallet spend key set to loki_owner
+  - for Lokinet, the recommended default owner is the ed25519 public key of the user's Lokinet key set to ed_owner


I don't think the "for Lokinet" part here really makes sense. Lokinet doesn't really expose your keys for something like this; it makes more sense to keep it as the wallet keys here, as well: so Session recommends the session public key, and everything else recommends the wallet public key.

Okay updated

So that when adding new fields to records, updating the helper will trigger compile errors on pre-existing record checks. There have been several updates to mapping records that weren't being tested over the lifetime of implementing LNS.

Doy-lee · 2020-03-11T22:49:33Z

This was merged in: #1051

Doy-lee mentioned this pull request Mar 3, 2020

Loki Name Service: Update TX, allow individual field updates #1051

Merged

Doy-lee force-pushed the LokiNameServiceMultipleOwners branch 2 times, most recently from 2820429 to 8de1352 Compare March 4, 2020 05:25

jagerman requested changes Mar 4, 2020

View reviewed changes

Doy-lee force-pushed the LokiNameServiceMultipleOwners branch 2 times, most recently from f4822b5 to 528e995 Compare March 10, 2020 08:53

Doy-lee added 17 commits March 11, 2020 10:57

Update documentation for LNS commands

6e003e7

Add generic_[key|signature] to handle monero/ed25519 keys

b24519a

Make LNS take generic keys/sigs where it can for owner keys

9673dda

Verify owner/backup owner when validating LNS txs

34126f3

Make generic crypto be comparable

26b303a

Update SQL commands to handle backup owner keys

d6e839b

DRY combining mapping and owner table together

de054de

Bake SQL arguments into get_mappings sql

cc6ad60

Check LNS record is loaded

cd019c5

Update tests not using verify_lns_mapping_record

7e9ad55

Revise LNS documenation to reflect actual implementation

6f10a82

Prefix LNS commands with lns, instead of <verb> lns <action>

d44e78f

Add standalone LNS signature generator for CLI/RPC wallet

489deb6

Actually add make update signature to simplewallet interface

0af054e

Use prepare_tx_extra .. helper to gen signature

5063fe5

SQL injection fix, index optimize, general code warts

5e4c443

Doy-lee force-pushed the LokiNameServiceMultipleOwners branch from 528e995 to 5e4c443 Compare March 10, 2020 23:57

Doy-lee closed this Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki name service - Multiple owners #1048

Loki name service - Multiple owners #1048

Doy-lee commented Feb 27, 2020

jagerman Mar 4, 2020

Doy-lee Mar 5, 2020

jagerman Mar 5, 2020

Doy-lee Mar 5, 2020 •

edited

Loading

jagerman Mar 5, 2020

jagerman Mar 4, 2020

jagerman Mar 5, 2020

jagerman Mar 4, 2020

Doy-lee Mar 5, 2020

jagerman Mar 4, 2020

Doy-lee Mar 5, 2020

jagerman Mar 5, 2020

jagerman Mar 4, 2020 •

edited

Loading

Doy-lee Mar 5, 2020

jagerman Mar 4, 2020

jagerman Mar 4, 2020

jagerman Mar 4, 2020

jagerman Mar 4, 2020

KeeJef Mar 5, 2020

Doy-lee Mar 5, 2020

jagerman Mar 4, 2020

Doy-lee Mar 5, 2020

jagerman Mar 4, 2020

Doy-lee Mar 5, 2020

jagerman Mar 4, 2020

Doy-lee Mar 5, 2020

jagerman Mar 5, 2020

jagerman Mar 4, 2020

Doy-lee Mar 5, 2020

Doy-lee commented Mar 11, 2020

Loki name service - Multiple owners #1048

Loki name service - Multiple owners #1048

Conversation

Doy-lee commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Doy-lee Mar 5, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman Mar 4, 2020 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Doy-lee commented Mar 11, 2020

Doy-lee Mar 5, 2020 •

edited

Loading

jagerman Mar 4, 2020 •

edited

Loading