Monero serialization reserves memory before validating the size of the
incoming data, thus allowing a remote node to send malicious data that
causes the local node to run itself out of memory.
This fixes it by limiting the amount of memory we'll reserve in advance
to a sane amount.
- Remove some useless epee functions, and add deprecated markers to ones
that have good replacements already.
- Don't use boost::lexical_cast when std::to_string or direct stream
output can be used just as well.
- Get rid of dumb epee "include_base_utils.h" header
Converts all use of boost::filesystem to std::filesystem.
For macos and potentially other exotic systems where std::filesystem
isn't available, we use ghc::filesystem instead (which is a drop-in
replacement for std::filesystem, unlike boost::filesystem).
This also greatly changes how we handle filenames internally by holding
them in filesystem::path objects as soon as possible (using
fs::u8path()), rather than strings, which avoids a ton of issues around
unicode filenames. As a result this lets us drop the boost::locale
dependency on Windows along with a bunch of messy Windows ifdef code,
and avoids the need for doing gross boost locale codecvt calls.
When targetting macos <10.14 macos won't allow use of anything from
C++17 that throws, such as:
- std::get on a variant
- std::visit
- std::optional::value()
- std::any_cast
This avoids all of these.
For std::get, we either replace with std::get_if (where appropriate), or
else use a `var::get` implementation of std::get added to lokimq (also
updated here). (This `var` namespace is just an `std` alias everywhere
*except* old target macos).
For std::visit, likewise lokimq adds an var::visit implementation for
old macos that we use.
std::optional::value() uses weren't useful anyway as everywhere it calls
them we've already checked that the option has a value, in which case we
can use `*opt` (which doesn't check for contents and throw).
std::any just has to be avoided as far as I can tell, but the one place
we used it is only ever a block, so I just replaced it with a `const
block*`.
clang doesn't like opt.emplace() when the optional's contained type
doesn't have an explicit default constructor (such as the recent
std::optional<requested_fields_t>), so construct and assign explicitly
rather than using emplace.
This changes `get_service_nodes` fields behaviour to make the fields.all
field a bit more intuitive by defaulting it to true if `fields` is
entirely omitted (as before), but now defaulting to false if `fields`
is explicitly given but `fields.all` is omitted.
This means a request such as `{"fields":{"service_node_pubkey":true}}`
now returns just pubkeys, while previously it returned everything (which
was surprising behaviour). To make it return just pubkeys you'd have to
request: `{"fields":{"all":false,"service_node_pubkey":true}}`.
`all` is still there so that you can explicitly request everything if
desired.
An std::optional lets you distinguish between a value that isn't
provided at all (std::optional will be empty) versus one that is
provided as a default value (for example, a std::string specified as
empty).
On serialization, a null optional is not serialized at all; on
deserialization the optional is reset if omitted, otherwise set to the
specified value.
This also cleans up the serialization code to use simpler C++17 `if
constexpr`s rather than SFINAE overloads for the different serialization
cases.
- Replaces the wallet RPC classes with ones like the core RPC server
(with serialization code moved into a new .cpp file).
- Restricted commands are now carried through the RPC serialization
types (by inheriting from RESTRICTED instead of RPC_COMMAND) and
restrictions are handled in one place rather than being handled in
each of the 49 restricted endpoints. This differs a little from how
the core http server works (which has a PUBLIC base class) because
for the wallet rpc server unrestricted really doesn't mean "public",
it means something closer to view-only.
- GET_TRANSFERS_CSV is now restricted (it looks like an oversight that
it wasn't before since GET_TRANSFERS is restricted)
- GET_ADDRESS_BOOK_ENTRY is now restricted. Since restricted mode is
meant to provide something like view-only access, it doesn't make
much sense that address book entries were available.
- Use uWebSockets to provide the wallet RPC server HTTP functionality.
This version is quite a bit simpler than the core RPC version since it
doesn't support multithreaded (parallel) requests, and so we don't
have to worry about queuing jobs.
- Converted all the numeric wallet rpc error codes defines to constexprs
- Changed how endpoints get called; previous this was called:
bool on_some_endpoint(const wallet_rpc::COMMAND_RPC_SOME_ENDPOINT::request& req, wallet_rpc::COMMAND_RPC_SOME_ENDPOINT::response& res, epee::json_rpc::error& er, const connection_context *ctx = NULL)
This PR changes it similarly to how core_rpc_server's endpoints work:
wallet_rpc::SOME_ENDPOINT invoke(wallet_rpc::COMMAND_RPC_SOME_ENDPOINT::request&& req);
That is:
- the response is now returned
- the request is provided by mutable rvalue reference
- the error object is gone (just throw instead)
- the connection_context is gone (it was not used at all by any wallet
rpc endpoint).
- Consolidated most of the (identical) exception handling to the RPC
method invocation point rather than needing to repeat it in each
individual endpoint. This means each endpoint's `invoke` method can
now just throw (or not catch) exceptions. Some try/catches are still
there because they are converting one type of exception into another,
but the generic ones that return a generic error are gone.
- Removed all remaining epee http code.
- DRYed out some wallet rpc code.
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.
Jason Rhinelander
Chris Buccella
Sean
Sean Darcy
doy-lee
Doyle