In short: epee's http client is garbage, standard violating, and
unreliable.
This completely removes the epee http client support and replaces it
with cpr, a curl-based C++ wrapper. rpc/http_client.h wraps cpr for RPC
requests specifically, but it is also usable directly.
This replacement has a number of advantages:
- requests are considerably more reliable. The epee http client code
assumes that a connection will be kept alive forever, and returns a
failure if a connection is ever closed. This results in some very
annoying things: for example, preparing a transaction and then waiting
a long tim before confirming it will usually result in an error
communication with the daemon. This is just terribly behaviour: the
right thing to do on a connection failure is to resubmit the request.
- epee's http client is broken in lots of other ways: for example, it
tries throwing SSL at the port to see if it is HTTPS, but this is
protocol violating and just breaks (with a several second timeout) on
anything that *isn't* epee http server (for example, when lokid is
behind a proxying server).
- even when it isn't doing the above, the client breaks in other ways:
for example, there is a comment (replaced in this PR) in the Trezor PR
code that forces a connection close after every request because epee's
http client doesn't do proper keep-alive request handling.
- it seems noticeably faster to me in practical use in this PR; both
simple requests (for example, when running `lokid status`) and
wallet<->daemon connections are faster, probably because of crappy
code in epee. (I think this is also related to the throw-ssl-at-it
junk above: the epee client always generates an ssl certificate during
static initialization because it might need one at some point).
- significantly reduces the amount of code we have to maintain.
- removes all the epee ssl option code: curl can handle all of that just
fine.
- removes the epee socks proxy code; curl can handle that just fine.
(And can do more: it also supports using HTTP/HTTPS proxies).
- When a cli wallet connection fails we know show why it failed (which
now is an error message from curl), which could have all sorts of
reasons like hostname resolution failure, bad ssl certificate, etc.
Previously you just got a useless generic error that tells you
nothing.
Other related changes in this PR:
- Drops the check-for-update and download-update code. To the best of
my knowledge these have never been supported in loki-core and so it
didn't seem worth the trouble to convert them to use cpr for the
requests.
- Cleaned up node_rpc_proxy return values: there was an inconsistent mix
of ways to return errors and how the returned strings were handled.
Instead this cleans it up to return a pair<bool, val>, which (with
C++17) can be transparently captured as:
auto [success, val] = node.whatever(req);
This drops the failure message string, but it was almost always set to
something fairly useless (if we want to resurrect it we could easily
change the first element to be a custom type with a bool operator for
success, and a `.error` attribute containing some error string, but
for the most part the current code wasn't doing much useful with the
failure string).
- changed local detection (for automatic trusted daemon determination)
to just look for localhost, and to not try to resolve anything.
Trusting non-public IPs does not work well (e.g. with lokinet where
all .loki addresses resolve to a local IP).
- ssl fingerprint option is removed; this isn't supported by curl
(because it is essentially just duplicating what a custom cainfo
bundle does)
- --daemon-ssl-allow-chained is removed; it wasn't a useful option (if
you don't want chaining, don't specify a cainfo chain).
- --daemon-address is now a URL instead of just host:port. (If you omit
the protocol, http:// is prepended).
- --daemon-host and --daemon-port are now deprecated and produce a
warning (in simplewallet) if used; the replacement is to use
--daemon-address.
- --daemon-ssl is deprecated; specify --daemon-address=https://whatever
instead.
- the above three are now hidden from --help
- reordered the wallet connection options to make more logical sense.
The conversion here was becoming an integer and thus inserting a
formatted integer rather than a char value (e.g. 0x02 would end up as
"\u00050" instead of "\u0002").
This started out with a few string_view simplifications, but unwinding
the string_view calls resulted in quite a few changes.
- use constexpr string_view instead of macros for constants
- use constexpr ints for other macros instead of constants (to be
consistent with the string_view constexprs); also eliminated a couple of
unused variables.
- convert amount parsing to string_view, and rewrite it with comments to
be far less confusing. (Previously it was remove a "." from a string,
chopping 0s off the end then putting them back on which was hard to
follow).
- fixed some not-quite-right amount parsing unit tests
- replace a bunch of string code (including lots of code allocating new
strings with .substr on a string) with std::string_view.
- avoid using C functions for strings
- convert epee http uri conversion methods to string_view (and fix one
typoed func name "conver" -> "convert")
- Add a `tools::hex_to_type` that converts hex directly into a (simple)
type, with hex and length verification for the type, and use it to
eliminate a bunch of intermediate std::string conversions.
- Convert a bunch of error-prone raw pointer string appends into
more type-safe `tools::view_guts(val)` calls. (In particular view_guts
always gets the sizeof() correct, while the individual call could easily
put the wrong type in the `sizeof()`).
This purges epee::critical_region/epee::critical_section and the awful
CRITICAL_REGION_LOCAL and CRITICAL_REGION_LOCAL1 and
CRITICAL_REGION_BEGIN1 and all that crap from epee code.
This wrapper class around a mutex is just painful, macro-infested
indirection that accomplishes nothing (and, worse, forces all using code
to use a std::recursive_mutex even when a different mutex type is more
appropriate).
This commit purges it, replacing the "critical_section" mutex wrappers
with either std::mutex, std::recursive_mutex, or std::shared_mutex as
appropriate. I kept anything that looked uncertain as a
recursive_mutex, simple cases that obviously don't recurse as
std::mutex, and simple cases with reader/writing mechanics as a
shared_mutex.
Ideally all the recursive_mutexes should be eliminated because a
recursive_mutex is almost always a design flaw where someone has let the
locking code get impossibly tangled, but that requires a lot more time
to properly trace down all the ways the mutexes are used.
Other notable changes:
- There was one NIH promise/future-like class here that was used in
example one place in p2p/net_node; I replaced it with a
std::promise/future.
- moved the mutex for LMDB resizing into LMDB itself; having it in the
abstract base class is bad design, and also made it impossible to make a
moveable base class (which gets used for the fake db classes in the test
code).
Besides being a useless idea, this variable doesn't actually do anything
anymore, even in upstream Monero since it got removed from
CRITICAL_REGION_LOCAL.
- Adds a tools::trim that operates on a string_view to replace boost
string trim usage.
- Replaces some boost::split's with tools::split's
- updates vercmp to operate on string_views instead of c strings
There are some boost::bind's left here that don't look simple to replace
with lambdas, so leave them (but add the required boost::placeholders);
remove a now unused one as well.
byte_slice was an abomination that used more than 400 lines of code to
implement what can be implemented in about 20 lines of code with a
std::shared_ptr<std::string> and a std::string_view and a couple
convenience methods.
Much of the levin noise generation using it used it gratuitously: they
always allocate a new string, but then return that wrapped it in a
byte_slice abomination because... well, there's no reason at all except
apparently that the byte_slice author wanted to push byte_slice into
places it didn't belong at all (even if you accept the overbuilt
monstrosity). Those methods now take a string_view and return a string,
which is what they should have done in the first place.
Replaces the silly cow with an ASCII Loki logo:
.
oooo
oooo
oooo . You Loki Wallet has been locked to
oooo oooo protect you while you were away.
oooo oooo
oooo oooo (Use `set inactivity-lock-timeout 0`
oooo oooo to disable this inactivity timeout)
. oooo
oooo
oooo
.
Also includes a few related miscellaneous updates to how we suspend
input and clear the screen.
Replaces the horrible callback wrapper that had to be called from each
and every command callback (and was randomly missing from some) to reset
the inactivity timer with a generic one in the console handler that
allows specifying pre-callback and post-callback commands.
Replaces reintroduced boost::chronos and posix_times with std::chrono.
Also rewrote the time printing parts of the performance_tests code to do
it better use std::chrono types instead of uint64_ts.
Replaces some boost::functions, and removes some C-style ancient
`std::function<Return(void)>` with C++-style `std::function<Return()>`.
Edit: There's also a map -> unordered_map change hidden in here because
it was in the middle of the other changes and is minor.
This removes some boost thread interruption code, but that code is
highly unlikely to have done anything: you cannot preemptively interrupt
a thread, and pretty much anywhere a thread would get stuck is in I/O,
where boost's (voluntary) thread interruption cannot follow.
It also removes thread attributes because they have really limited
usefulness. There *was* a reason to introduce them many years ago (when
musl libc ran into a stack limit problem with libunbound) but musl
increased the default stack size long since.
There's a bunch of cruft in here that isn't used anywhere; delete it,
and move the two remaining functions to a .cpp file since this header
gets included almost everywhere.
Also simplify the duration printer to avoid needing boost.
The histogram data, in particular, is not blob serializable: it is a 16
byte struct on most 64-bit architectures, and a 12 byte struct on most
32-bit architectures.
common/util.h has become something of a dumping ground of random
functions. This splits them up a little by moving the filesystem bits
to common/file.h, the sha256sum functions to common/sha256sum.h, and the
(singleton) signal handler to common/signal_handler.h.
This overhauls the (unpleasant) epee "portable storage" interface to be
a bit easier to work with. The diff is fairly large because of the
amount of type indirection that was being used, but at its core does
this simplifications:
- `hsection` was just a typedef for a `section*`, so just use `section*`
instead to not pretend it the value is something it isn't.
- use std::variant instead of boost::variant to store the portable
storage items.
- don't make the variant list recursive with itself since the
serialization doesn't support that anyway (i.e. it doesn't support a
list of lists).
- greatly simplified the variant visiting by using generic lambdas.
- the array traversal interface was a horrible mess. Replaced it with
a simpler custom iterator approach.
- replaced the `selector<bool>` templated class with templated function.
So for example,
epee::serialization::selector<is_store>::serialize_t_val_as_blob(...)
becomes:
epee::serialization::perform_serialize_blob<is_store>(bytes, stg, parent_section, "addr");
and similar for the other types of serializing.
Changes all boost mutexes, locks, and condition_variables to their stl
equivalents.
Changes all lock_guard/unique_lock/shared_lock to not specify the mutex
type (C++17), e.g.
std::lock_guard foo{mutex};
instead of
std::lock_guard<oh::um::what::mutex> foo{mutex};
Also changes some related boost::thread calls to std::thread, and some
related boost chrono calls to stl chrono.
boost::thread isn't changed here to std::thread because some of the
instances rely on some boost thread extensions.
Switch loki dev branch to C++17 compilation, and update the code with
various C++17 niceties.
- stop including the (deprecated) lokimq/string_view.h header and
instead switch everything to use std::string_view and `""sv` instead of
`""_sv`.
- std::string_view is much nicer than epee::span, so updated various
loki-specific code to use it instead.
- made epee "portable storage" serialization accept a std::string_view
instead of const lvalue std::string so that we can avoid copying.
- switched from mapbox::variant to std::variant
- use `auto [a, b] = whatever()` instead of `T1 a; T2 b; std::tie(a, b)
= whatever()` in a couple places (in the wallet code).
- switch to std::lock(...) instead of boost::lock(...) for simultaneous
lock acquisition. boost::lock() won't compile in C++17 mode when given
locks of different types.
- removed various pre-C++17 workarounds, e.g. for fold expressions,
unused argument attributes, and byte-spannable object detection.
- class template deduction means lock types no longer have to specify
the mutex, so `std::unique_lock<std::mutex> lock{mutex}` can become
`std::unique_lock lock{mutex}`. This will make switching any mutex
types (e.g. from boost to std mutexes) far easier as you just have to
update the type in the header and everything should work. This also
makes the tools::unique_lock and tools::shared_lock methods redundant
(which were a sort of poor-mans-pre-C++17 way to eliminate the
redundancy) so they are now gone and replaced with direct unique_lock or
shared_lock constructions.
- Redid the LNS validation using a string_view; instead of using raw
char pointers the code now uses a string view and chops off parts of the
view as it validates. So, for instance, it starts with "abcd.loki",
validates the ".loki" and chops the view to "abcd", then validates the
first character and chops to "bcd", validates the last and chops to
"bc", then can just check everything remaining for is-valid-middle-char.
- LNS validation gained a couple minor validation checks in the process:
- slightly tightened the requirement on lokinet addresses to require
that the last character of the mapped address is 'y' or 'o' (the
last base32z char holds only one significant bit).
- In parse_owner_to_generic_owner made sure that the owner value has
the correct size (otherwise we could up end not filling or
overfilling the pubkey buffer).
- Replaced base32z/base64/hex conversions with lokimq's versions which
have a nicer interface, are better optimized, and don't depend on epee.