Fixes lots of crappy C++ code. I strongly get the impression from these
changes that whoever wrote this code was a C programmer with very little
C++ experience. Sadly no one in the upstream Monero PR review tried to
help or seemed to care about the code quality.
- Get rid of superfluous `this->` throughout the ledger code.
- DRY: abstract away sending sequences of bytes, replacing:
memmove(buffer, this->buffer_send+offset, 32);
offset += 32
with:
send_bytes(buffer, 32, offset);
- DRY: abstract sending/receiving u32
- DRY: abstract receiving bytes/u32
- properly prefix memcpy/memmove with std::
- use std::string_view and std::string for setting/retrieving name
- rename `this->controle_device` to `debug_device`
- replace `f(void)` -> `f()` (on C++ methods, FFS!)
- DRY: replace set-length-then-exchange dance with a function
- DRY: merge nearly-identical exchange() and exchange_wait_for_input()
- remove never-used ok/mask arguments from exchange()
- Remove ASSERT_SW macro used only in one place
- Replace dumb ASSERT_X macro that was just an alias for another macro
- remove ASSERT_T0 macro that isn't used anywhere
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*`.
Also inline them into the header and simplify them.
Someone added functions for is_rct_whatever but used them almost
nowhere. This uses them (which would have saved a bunch of changes and
bugs in adding CLSAG in the first place).
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.
This moves the wallet structs that are needed elsewhere (for instance,
in the device drivers) from `tools::wallet2::whatever` to
`wallet::whatever`, allowing them to be included individually via
wallet/whatever.h without needing to include the entire wallet2.h.
What was particularly problematic here is that you couldn't even forward
declare them because they were nested inside the wallet2 class, but
instead had to include the full wallet2.h.
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.
Add number parsing and basic string splitting to common/string_util.h
and use it (replacing some regexes and boost string utilities).
Includes unit tests.
A huge amount of this is repetitive:
- `boost::get<T>(variant)` becomes `std::get<T>(variant)`
- `boost::get<T>(variant_ptr)` becomes `std::get_if<T>(variant_ptr)`
- `variant.type() == typeid(T)` becomes `std::holds_alternative<T>(variant)`
There are also some simplifications to visitors using simpler stl
visitors, or (simpler still) generic lambdas as visitors.
Also adds boost serialization serializers for std::variant and
std::optional.
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.
This adds a static dependency script for libraries like boost, unbound,
etc. to cmake, invokable with:
cmake .. -DBUILD_STATIC_DEPS=ON
which downloads and builds static versions of all our required
dependencies (boost, unbound, openssl, ncurses, etc.). It also implies
-DSTATIC=ON to build other vendored deps (like miniupnpc, lokimq) as
static as well.
Unlike the contrib/depends system, this is easier to maintain (one
script using nicer cmake with functions instead of raw Makefile
spaghetti code), and isn't concerned with reproducible builds -- this
doesn't rebuild the compiler, for instance. It also works with the
existing build system so that it is simply another way to invoke the
cmake build scripts but doesn't require any external tooling.
This works on Linux, Mac, and Windows.
Some random comments on this commit (for preserving history):
- Don't use target_link_libraries on imported targets. Newer cmake is
fine with it, but Bionic's cmake doesn't like it but seems okay with
setting the properties directly.
- This rebuilds libzmq and libsodium, even though there is some
provision already within loki-core to do so: however, the existing
embedded libzmq fails with the static deps because it uses libzmq's
cmake build script, which relies on pkg-config to find libsodium which
ends up finding the system one (or not finding any), rather than the one
we build with DownloadLibSodium. Since both libsodium and libzmq are
faily simple builds it seemed easiest to just add them to the cmake
static build rather than trying to shoehorn the current code into the
static build script.
- Half of the protobuf build system ignores CC/CXX just because Google,
and there's no documentation anywhere except for a random closed bug
report about needing to set these other variables (CC_FOR_BUILD,
CXX_FOR_BUILD) instead, but you need to. Thanks Google.
- The boost build is set to output very little because even the minimum
-d1 output level spams ~15k lines of output just for the headers it
installs.
- choice where to enter passphrase is now made on the host
- use wipeable string in the comm stack
- wipe passphrase memory
- protocol optimizations, prepare for new firmware version
- minor fixes and improvements
- tests fixes, HF12 support
The archaic (i.e. decade old) cmake usage here really got in the way of
trying to properly use newer libraries (like lokimq), so this undertakes
overhauling it considerably to make it much more sane (and significantly
reduce the size).
I left more of the architecture-specific bits in the top-level
CMakeLists.txt intact; most of the efforts here are about properly
loading dependencies, specifying dependencies and avoiding a whole pile
of cmake antipatterns.
This bumps the required cmake version to 3.5, which is what xenial comes
with.
- extensive use of interface libraries to include libraries,
definitions, and include paths
- use Boost::whatever instead of ${Boost_WHATEVER_LIBRARY}. The
interface targets are (again) much better as they also give you any
needed include or linking flags without needing to worry about them.
- don't list header files when building things. This has *never* been
correct cmake usage (cmake has always known how to wallet_rpc_headers
the headers that .cpp files include to know about build changes).
- remove the loki_add_library monstrosity; it breaks target names and
makes compiling less efficient because the author couldn't figure out
how to link things together.
- make loki_add_executable take the output filename, and set the output
path to bin/ and install to bin because *every single usage* of
loki_add_executable was immediately followed by setting the output
filename and setting the output path to bin/ and installing to bin.
- move a bunch of crap that is only used in one particular
src/whatever/CMakeLists.txt into that particular CMakeLists.txt instead
of the top level CMakeLists.txt (or src/CMakeLists.txt).
- Remove a bunch of redundant dependencies; most of them look like they
were just copy-and-pasted in, and many more aren't needed (since they
are implied by the PUBLIC linking of other dependencies).
- Removed `die` since it just does a FATAL_ERROR, but adds color (which
is useless since CMake already makes FATAL_ERRORs perfectly visible).
- Change the way LOKI_DAEMON_AND_WALLET_ONLY works to just change the
make targets to daemon and simplewallet rather than changing the build
process (this should make it faster, too, since there are various other
things that will be excluded).
The existing code just assumes it is already in the path, which fails
when it's in some prefixed place with a proper .pc file installed (such
as for a homebrew installation).
This converts the transaction type and version to scoped enum, giving
type safety and making the tx type assignment less error prone because
there is no implicit conversion or comparison with raw integers that has
to be worried about.
This ends up converting any use of `cryptonote::transaction::type_xyz`
to `cryptonote::transaction::txtype::xyz`. For version, names like
`transaction::version_v4` become `cryptonote::txversion::v4_tx_types`.
This also allows/includes various other simplifications related to or
enabled by this change:
- handle `is_deregister` dynamically in serialization code (setting
`type::standard` or `type::deregister` rather than using a
version-determined union)
- `get_type()` is no longer needed with the above change: it is now
much simpler to directly access `type` which will always have the
correct value (even for v2 or v3 transaction types). And though there
was an assertion on the enum value, `get_type()` was being used only
sporadically: many places accessed `.type` directly.
- the old unscoped enum didn't have a type but was assumed castable
to/from `uint16_t`, which technically meant there was potential
undefined behaviour when deserializing any type values >= 8.
- tx type range checks weren't being done in all serialization paths;
they are now. Because `get_type()` was not used everywhere (lots of
places simply accessed `.type` directory) these might not have been
caught.
- `set_type()` is not needed; it was only being used in a single place
(wallet2.cpp) and only for v4 txes, so the version protection code was
never doing anything.
- added a std::ostream << operator for the enum types so that they can be
output with `<< tx_type <<` rather than needing to wrap it in
`type_to_string(tx_type)` everywhere. For the versions, you get the
annotated version string (e.g. 4_tx_types) rather than just the number
4.
- Trezor: support for device address display (subaddress, integrated address)
- Wallet::API support added
- Simplewallet:
- address device [<index>]
- address new <label> // shows address on device also
- integrated_address [device] <payment_id|address> // new optional "device" arg to display also on the device
* Use the precalculated public key when trying to gen key images
Fixes failure to export key image bug and several other spending proof
mechanisms, where if you receive Loki from multiple outputs in the same
transaction, encrypted using multiple public keys- we don't fail out
prematurely by only trying the first public key.
* Rewrite find_tx_extra_field_by_typte
