Loki name service backend #922

Doy-lee · 2019-11-01T06:52:34Z

Adds SQLite and we start storing loki_name_system txs into the database. We currently only store LNS entries for blocks that are checkpointed.

We support third party LNS entries by using a different type in the extra
Renewal/expiration of Lokinet name entries
Acquiring Messenger and Wallet names

This is my to-do list, or things that are missing from this PR and can come later in subsequent PRs

- [x] Burning amounts via fee
- [x] On db init, we should scan the necessary blocks for getting LNS running (right now it gets the correct amount of blocks on block receipt)
- [x] Wallet client side LNS commands
- [x] RPC endpoint "get_lns_mappings"
- [ ] Handle pop_blocks for LNS db

This PR goes ontop of #913

…function Call service_node_lists's block added hook function manually instead of hooking the hook. There's a common operation between the 2 subsystems, Loki Name Service and Service Node List, in which they generate state based off incoming blocks from the blockchain. Upon profiling, the slowest factor of this is retrieving the blocks from the DB in batches of a 1000 rather than the processing of the blocks in the subsystems. So flatten out the hierarchy so that we can retrieve the batch and simultaneously process the blocks in both subsystems on the same fetch request. A complete removal of the hook system so we have more flexibility at the callsite can be done in a later PR, to avoid too many unneccesary changes in the LNS PR.

jagerman · 2019-12-10T02:41:50Z

external/CMakeLists.txt

@@ -93,3 +93,4 @@ endif()
 add_subdirectory(db_drivers)
 add_subdirectory(easylogging++)
 add_subdirectory(randomx EXCLUDE_FROM_ALL)
+add_subdirectory(sqlite)


This feels like something that should try linking against the system sqlite3 when not doing a static build, like we do with libunbound and miniupnpc, and then use the local copy only if that fails (or it's STATIC). It looks like sqlite3 includes a pkg-config file, so this could basically just add

pkg_check_modules(SQLITE3 sqlite3)

with the libunbound/miniupnpc pkg_check_modules, then use that if found, otherwise add the subdirectory for the local static copy.

On second thought: perhaps we should just require libsqlite3 rather than bundling it? It's a pretty ubiquitous library.

I'm on the vendor everything train. Builds are more reliable and everyone builds the same thing that we've tested the software on.

I'm on the vendor everything train. Builds are more reliable and everyone builds the same thing that we've tested the software on.

This is a major problem in multiple ways.

Bundling dependencies is a sometimes necessary evil to deal with unavailable or difficult to install dependencies, cases where the upstream source has an unstable API, or dealing with cases where the software needs to be locally modified. None of those seem to apply to a mature, stable project like sqlite3. If our software doesn't work with particular versions of sqlite3 then we should be expressing that through a version dependency, not by forcing someone compiling the software to use a statically compiled bundled version. There are a lot of downsides to bundling software; sometimes the pros outweigh the cons, but given the ubiquity of sqlite3 as a standard system library with a long-standing stable API (pledged by upstream to remain stable until at least 2050) I'm not seeing them at all here.

I also don't want to worry about needing to track security updates for sqlite3, boost, libunbound, openssl, and anything else that we might want to vendor, and issue new loki software releases every time there is any security update in any of them. But those are the implications of vendor everywhere, and that's why it is such a software development anti-pattern in an open source project. That additional work of needing to monitor upstream projects and issue new releases whenever something critical comes along is development time wasted with little benefit when that process is already being done on pretty much every sane OS (i.e. everywhere except Windows), and typically with faster reactions than we will ever be able to manage.

Furthermore "everyone builds the same thing that we've tested the software on" is simply not possible: admission into pretty much any linux distribution basically requires that bundled software be unbundled in the distribution package. Downstream distributors strongly recommend that upstream projects don't forcibly bundle to save them extra work of having to unbundle when packaging for a distribution. See Fedora, Gentoo, Debian policies on the topic: they all amount to the same requirement, with the same justifications. (Since I'm maintaining the debs in such a way that they are acceptable for inclusion into Debian, that also means I'm going to have to write a patch to unbundle the debs before we release, so we won't even be getting that exact sqlite3 version requirement for our official binaries).

The Gentoo section on what upstream software devs should do is particularly on-point: if an upstream software include a bundled dependency for convenience, please provide some way to defer to using system libraries instead.

But this doesn't only apply to linux distributions. If I compile software on my system I strongly, strongly prefer that the software link to system libraries so that when I install system updates I get those updates in my compiled software without having to do anything. You want to take that away from people compiling the source as-is for themselves, and I am not even remotely convinced by it being justified in this case.

Yet another disadvantage is that it will make the wallet api library unusable in any other project that uses sqlite because you will have symbol conflicts between the two versions. This dependency hell doesn't happen if you dynamically link because there's only one sqlite3 library.

Bundling a static library for static build system binaries is totally fine: we already do that with all of our dependencies, like boost, but that belongs as part of the build inside contrib/depends that is downloaded and built on demand, not a forced bundled build in the main repository. The change here doesn't merely add it only into our static builds, it also forces it on all unmodified compilations of the software, and that is the part that is not acceptable.

Quite aside from all of that, the way sqlite3 is packaged here is problematic. First, we should not be sticking a copy of any subproject into our repository; rather we should either be using a submodule to fetch the upstream code (as we do with unbound, miniupnpc, unbound), or downloading it (with checksum verification) when needed as part of the build (as we do in contrib/depends, or as lokinet does with libsodium). That saves needlessly adding 9MB (!) of C code to the loki repository.

Second, the way the sqlite3 code that has been included here is the amalgamated version of the sqlite3 code. This is problematic for licensing reasons: it makes loki core non-free software because we are not actually including the source code (in the sense of being the preferred form of modification); rather we have derived code generated from the actual source code (i.e. the preferred form of modification) that we aren't including. That would make the loki source code non-free software by most definitions, including the Debian Free Software Guidelines and FSF. That's unacceptable.

My whole premise with static and vendoring is to remove friction, build ease, nothing changing behind our backs, but that is not realistic here for compliance with *nix ecosystem. Some of the arising code/workflow issues are questionable but compliance/user expectation trumps the latter.

I'll change it to dynamic linking and submodule otherwise.

jagerman · 2019-12-10T02:43:58Z

src/cryptonote_basic/account.cpp

@@ -101,6 +107,8 @@ DISABLE_VS_WARNINGS(4244 4345)
      for (size_t i = 0; i < sizeof(crypto::secret_key); ++i)
        k.data[i] ^= *ptr++;
    }
+    for (size_t i = 0; i < sizeof(m_spend_ed25519_secret_key); ++i)
+      m_spend_ed25519_secret_key.data[i] ^= *ptr++;


This looks like it is going to run off the end of key_stream; I think it needs a + sizeof(m_spend_ed25519_secret_key) added to the get_key_stream() argument.

Have removed storing the secret key from account since I only need the public key at the moment.

Yeah this might've been the bug I came across earlier, couldn't figure out why I was getting different key results.

jagerman · 2020-01-15T18:44:53Z

src/cryptonote_core/blockchain.h

+      loki_subsystem_lns = 1 << 0,
+      loki_subsystem_snl = 1 << 1,
+      loki_subsystem_all = (loki_subsystem_lns | loki_subsystem_snl),
+    };


The loki_ prefix seems unnecessary; the entire code is loki already.

Making the caller use a bitfield is adding some unnecessary verbosity at the call site and is a bit C rather than C++; an abstraction here such as the following (or perhaps something better?) would provide a nicer caller interface:

struct subsystem { bool lns = false; bool snl = false; static constexpr subsystem loki_name_service() { subsystem s; s.lns = true; return s; } static constexpr subsystem service_node_list() { subsystem s; s.snl = true; return s; } static constexpr subsystem all() { subsystem s; s.lns = true; s.snl = true; return s; } };

(which will be a 2-byte, trivially copyable struct and so still just as efficient to pass around by value as the 4-byte int). But the main point is that the caller gets cleaned up to if (subsys.lns && whatever) instead of if ((subsystems & loki_subsystem_lns) && whatever).

I've removed subsystem actually. I used to conditionally update one but not the other, but that is not the case anymore in the wallet PR

jagerman · 2020-01-15T18:54:08Z

src/cryptonote_basic/tx_extra.h

@@ -35,8 +35,6 @@
 #include "serialization/variant.h"
 #include "crypto/crypto.h"
 #include <boost/variant.hpp>
-
-


This looks unintentional? The whitespace between the headers and the defines seemed nice.

Have reverted in wallet PR on a cleanup pass.

jagerman · 2020-01-15T19:01:58Z

src/cryptonote_core/loki_name_system_db.cpp

+  return true;
+}
+
+constexpr char BUILD_TABLE_SQL[] = R"FOO(


The "FOO" delimiters seem pretty unnecessary here compared to just R"(...)" which doesn't seem like it would conflict with anything in the queries here.

jagerman · 2020-01-27T21:16:20Z

src/cryptonote_core/loki_name_system_db.cpp

+  return true;
+}
+
+enum struct db_version : int { v1, };


No need to specify int here, it's implied for an enum struct (unlike a non-struct enum).

Removed int

jagerman · 2020-01-27T21:21:12Z

src/cryptonote_core/loki_name_system_db.cpp

+  if (!validate_lns_entry(tx, tx_extra))
+  {
+    assert("Failed to validate acquire name service. Should already have failed validation prior" == nullptr);
+    LOG_PRINT_L1("LNS TX: Failed to validate for tx: " << get_transaction_hash(tx) << ". This should have failed validation earlier");


This should probably be an L0 as this seems like an internal logic error.

jagerman · 2020-01-27T21:57:06Z

src/cryptonote_core/loki_name_system_db.cpp

+  }
+
+  return true;
+}


I am a bit skeptical of the robustness of this SQL transaction mechanism.

First, an RAII class here would be a better approach: BEGIN on construction, have a commit(); that commits the transaction, and ROLLBACK on destruction if commit() wasn't called. That way if some unanticipated error case gets hit you'll get the rollback and not leave the database with an active transaction.

Second I'm quite concerned about the nesting of transactions. Currently the code is just trying to issue a BEGIN and if it fails for any reason (which might not be transaction-related) it just carries on and then doesn't call a COMMIT or ROLLBACK. But this seems like we could get in a case where we do what would be, with nesting, BEGIN; BEGIN; /*... */; ROLLBACK; COMMIT; and end up committing things that shouldn't be.

There are two solutions to this that I can see:

don't nest transactions: track whether we already have one open, and if so, raise an exception if we try to take out another, and then update the code so that we don't nest transactions. It doesn't seem to me like we actually need nested transactions in the code, so this might be simplest.

use SAVEPOINTs instead of transactions, which can be nested (I think as long as they have unique savepoint names). This is only really needed if we need to be able to support a case where we want to partially rollback but then continue with adding into an existing transaction, and I don't see us needing that complexity here.

error detection is effectively being disabled on the transaction creation. Unfortunately I don't see anything in the API that actually lets you distinguish between a transaction that fails because a transaction is already open versus a failure for any other reason (the error code is generic, sadly). Either of the above solutions (with propagation of failures to open a transaction/savepoint) will deal with this.

thread safety. SQLite doesn't seem thread-safe with respect to transactions, which means effectively that anywhere taking out a transaction needs to be sure it is called in a context that is blocking out all other threads. (This is nasty, but I'm not sure there's a nice way to fix this).

This has been robustified with RAII

jagerman · 2020-01-27T22:11:10Z

src/cryptonote_core/loki_name_system_db.h

+{
+constexpr uint64_t BURN_REQUIREMENT                 = 100 * COIN;
+constexpr uint64_t BLOCKCHAIN_NAME_MAX              = 95;
+constexpr uint64_t BLOCKCHAIN_WALLET_ADDRESS_LENGTH = 69;


Can you rewrite this value in terms of primitives? i.e. 2*sizeof(public_key) + ... or, if the header inclusions would be tricky, at least comment why the value is 69 instead of 64? (A regular wallet address can be represented in 64, so what are the other 5 bytes doing?)

Cleaned up in Wallet PR

jagerman · 2020-01-27T22:29:52Z

src/cryptonote_basic/account.cpp

+  {
+    crypto::secret_key const &spend_key          = keys.m_spend_secret_key;
+    crypto::ec_scalar const &spend_key_unwrapped = unwrap(unwrap(keys.m_spend_secret_key));
+    crypto_sign_ed25519_seed_keypair(keys.m_spend_ed25519_public_key.data, keys.m_spend_ed25519_secret_key.data, reinterpret_cast<unsigned char const *>(spend_key_unwrapped.data));


You don't need to do any type unwrapping here; secret_key is required to be directly copyable:

crypto_sign_ed25519_seed_keypair( keys.m_spend_ed25519_public_key.data, keys.m_spend_ed25519_secret_key.data, reinterpret_cast<unsigned char const *>(&keys.m_spend_secret_key));

I'll update this where its now being done in wallet2::create_buy_lns_mapping_tx

Doy-lee · 2020-03-11T22:51:15Z

Merged in #1036

Doy-lee force-pushed the LokiNameServiceBackend branch from 297d809 to 523396d Compare December 5, 2019 04:32

Doy-lee force-pushed the LokiNameServiceBackend branch from 523396d to 1db931b Compare January 8, 2020 23:32

Doy-lee force-pushed the LokiNameServiceBackend branch from 1db931b to 8eeeed3 Compare January 16, 2020 01:01

Doy-lee added 7 commits January 21, 2020 16:04

Distinguish staking transactions and revamp construct_tx

45c71d7

Add sqlite 3.30.1

72dfa3e

Add support for Loki Name Service on the backend

b77ce25

Implement expiration/renewal of lokinet entries

0dabb40

Add logging macros, add IF NOT EXIST qualifier to unique index

ed3488a

Update tests for rebasing on blink changes

58797a7

jagerman reviewed Jan 27, 2020

View reviewed changes

Doy-lee force-pushed the LokiNameServiceBackend branch from 8eeeed3 to 58797a7 Compare January 29, 2020 22:15

This was referenced Feb 14, 2020

Loki Name System - Name Hashing/Mapping Value Encryption/Decryption Doy-lee/loki#8

Closed

Loki Name System - Name Hashing/Mapping Value Encryption/Decryption #1036

Merged

Doy-lee closed this Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loki name service backend #922

Loki name service backend #922

Doy-lee commented Nov 1, 2019 •

edited

jagerman Dec 10, 2019

Doy-lee Jan 31, 2020

jagerman Feb 2, 2020 •

edited

Doy-lee Feb 3, 2020

jagerman Dec 10, 2019

Doy-lee Jan 31, 2020

jagerman Jan 15, 2020

Doy-lee Jan 31, 2020

jagerman Jan 15, 2020

Doy-lee Jan 31, 2020

jagerman Jan 15, 2020

Doy-lee Jan 31, 2020

jagerman Jan 27, 2020

Doy-lee Jan 31, 2020

jagerman Jan 27, 2020

Doy-lee Jan 31, 2020

jagerman Jan 27, 2020

Doy-lee Jan 31, 2020

jagerman Jan 27, 2020

Doy-lee Jan 31, 2020

jagerman Jan 27, 2020

Doy-lee Jan 31, 2020

Doy-lee commented Mar 11, 2020

Loki name service backend #922

Loki name service backend #922

Conversation

Doy-lee commented Nov 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagerman Feb 2, 2020 • edited

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

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 commented Nov 1, 2019 •

edited

jagerman Feb 2, 2020 •

edited