New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ledger updates #1195
Ledger updates #1195
Commits on Nov 30, 2020
-
Loki-side updates for Ledger Nano S support: - Add a get-network command (and bump protocol version) so that we can verify that the Ledger is set to the correct network type (i.e. mainnet or testnet). Previously there was no check at all, so you could have a testnet wallet on desktop using mainnet keys on the Ledger. Now they get checked and an error occurs on mismatch. - Reset required version to 0.9.0
-
-
Overhaul tx prefix comms; rename some overloaded methods
Tx prefix communication was missing some needed information on the tx type, and was a little inefficient. This redoes the protocol to send the tx type info and then the entire prefix (rather than starting from a few bytes in). It also changes how we number requests and signal the final piece of a multi-piece transmission.
-
- Adds "create_wallet" rpc arguments to allow creating a hardware wallet - Adds a "create-hwdev" option to both rpc and cli wallets that creates a "[wallet].hwdev.txt" file containing an optional comment to flag the wallet as a hardware wallet. Although this data is present in the wallet file itself, that isn't available without a password description and the the GUI wallet needs to know this *before* it opens the wallet to properly display a wallet list with some indicators for hardware wallets. The comment also lets the user record some optional comment such as "Jason's Ledger Nano S" to serve as a reminder in the GUI wallet list. - Fixes some unicode EN DASHes that got added to documentation comments in place of the intended hyphens.
-
Allow prompting for restore height for hw wallet
For no good reason the hardware wallet creation didn't allow you to specify a restore height from the cli wallet for no good reason. This commit lets it use the same prompt, and improves the prompt somewhat: - you can now specify `curr` to use the current blockchain height (and this is the default for the hardware wallet). - added a note about the purpose of the restore height (i.e. that transactions won't be seen if before the restore height). - put example values in the description. - show the default in the prompt.
-
-
The blockchain doesn't accept MLSAG txes anymore (since HF16 + 10 blocks), so there is no need to keep the generation code around. Also renames mlsag_prehash to clsag_prehash since that is where it is primarily used now.
-
-
The capital Z here fails an assert in loki_name_system.cpp; lower-case it to fix the test.
-
This is dead code as the previous if is always entered for CLSAG. The body of this is applied later in the function, so this looks like code that got moved but the original didn't get deleted?
-
- Replace <cinttypes> with more limited <cstdint> - avoid epee for hex conversion - don't include <iostream>
-
-
Split up and DRY Modernize and DRY ringct serialization tests
One big blob with 50 tests in it was stupid because you have no clue what failed when something fails.
-
The blockchain only accepts CLSAG txes now, so no need to keep the MLSAG generation code around. (MLSAG verification stays, of course).
-
Rename serialization test category for consistency
The vast majority of the tests use lower_case_category.lower_case_test, but Serialization did it differently for no reason (and even then wasn't consistent with the test names). Fix that.
-
This one file used 2, 4, and 8 space indenting in different parts. Fix it. (Only whitespace changes).
-
ringct tests: Update ringct type to CLSAG
We can't construct pre-CLSAG anymore.
-
single 0 input with no outputs is not acceptable in post-Borromean.
-
Passing in a true/false value is dumb, we should just call EXPECT_TRUE/EXPECT_FALSE rather than `EXPECT_TRUE(...(true, ...` or `EXPECT_TRUE(...(false, ...` Passing the fee in the output array and then taking it out of the output array is similarly stupid, so don't do that either.
-
- 0 outputs is not valid post-Borromean. This was passing before because previously the tests were using Borromean ringct construction. - Remove and fix non-bulletproof/clsag tests which can't run anymore (since we can't construct pre-clsag).
-
Add helpful message to error string
A few times I've connected my test Ledger but forgotten to open the app, and then had connection failures without any indicating of what went wrong (just "No device found"). This adds "(Is the device running with the wallet app opened?)" to that error message as that is likely often the culprit.
-
use memwipe on secret k/alpha values
Reported by UkoeHB_ and sarang
-
Remove epee scope handler from core code
We have a much nicer LOKI_DEFER replacement.
-
Like MLSAG, this code is no longer used (or usable) since HF16.
-
-
Reformat length function signatures
Function signatures (especially in headers) should be readable! Also removes useless "const" on pass-by-value parameters from headers, and pass the bool argument by value instead of by const lvalue reference.
-
Ledger: Revise/fix/optimize data chunk transmission
- Sending one 32-byte key at a time is noticeably slower than sending in larger chunks. - Sending in 256-byte chunks was broken because the size field is only 254 bytes. Since we are actually sending these for Keccak, it makes some sense to chunk it at 136 bytes (i.e. keccak block size). - Change how multipart works to send as parts 1->2->...->0. Previously 0xff (rather than 0) marked the last chunk. - Allow multi-part chunks to wrap by wrapping the part after 255 to 1 (skipping 0 since that now means "last"). - Use multi-part chunk scheme for CLSAG in addition to prefix hashing.
-
The hard wallet debug code had no way to enable it, and if you did manually add the define, didn't compile. It was also nasty, gross, disgusting code that someone slopped into the file. This fixes it, adds a cmake option for it, and significantly cleans it up--just because code is for debugging doesn't mean it should be nasty and broken.
-
-
Don't calculate CLSAG
c
on the deviceIt's slow and unnecessary and depends on no keys in the hardware wallet (c is public info in CLSAG). If the wallet was to provide a changed c value then verification would simply fail.
-
-
-
- Show the instruction name (in debug builds) - annotate the byte values
-
-
-
Ledger: don't send nulls for no reason
don't send 32 null bytes for no reason in INS_GEN_TXOUT_KEYS when there is no additional txkey (this doesn't even match the case when there is one since we send it encrypted, requiring 64 bytes).
-
Improve crappy ledger C++ code
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
-
-
Monero's codes are extremely broken (relative to Monero's own ledger app code) with wrong codes, omissions, and status codes that don't exist at all.
-
Commits on Dec 2, 2020
Commits on Dec 3, 2020
-
Fix awful random scalar generating code
This code is nasty: - every time through the loop requires a mutex acquire and release rather than holding the mutex around the loop. - the comment about the limit is confusing af: eventually I figured out that the "limit" equals 15L, and that this code is basically trying to generate a value in (0, 15L) then mod L the number to get an unbiased value in (0, L). - However, the code is broken in two ways: - it can return 0 because (nL % L == 0) for integer n > 0 - the while condition is broken af so that the while loop can *never* repeat, and thus the entire function is just always returning (random % L) which thus has the slight bias. This commit fixes all these issues by moving the mutex outside the loop and borrowing libsodium's approach for the generation: generate a random value, mask off the 3 most significant bits, then repeat until this yields a value in (0, L), which happens slightly more than 1/2 the time. This is a bit slower than the 15L approach, but much simpler and generating random scalars is not a performance bottleneck.
-
- A static function inside an anonymous namespace is pointless. - Add comments above the sheer nastiness that is overloading `&` to avoid having to reinterpret_cast a char to an unsigned char. This horror needs to die. - Replace nasty C99 flexible array members reinterpret_cast'ed into a value and then stuffed into a shared_ptr with a proper struct. - Don't predeclare a bunch of crap long before the crap is used; this is C++, not C.
-
-
Annotate and split up ring signature code
We use generate_ring_signature and check_ring_signature somewhat inappropriately to sign and check a signature of a single key image. While it works for that, the full ring signature algorithm adds quite a bit of complexity that we don't need (and simply doesn't run) for the key image proof included in stake transactions and exported key images from the wallet. This splits it up, makes the key image interface considerably simpler, and adds annotation comments through it (and also adds comments into the "main" signature code). This is a necessary step to getting stake transactions and key image exports working with Ledger, without implementing the full ring signature (because that is quite involved, and not needed for most of these cases). Also remove unused gen/check_ring_signatures interfaces: The raw pointer code is never called, except through the vector version and one place in the test suite, so just remove it and make the vector version the main implementation.
-
Add random_scalar returning function
Allows slightly cleaner calling code in a few places.
-
Changing crypto random functions broke the test code horribly (because it depends on a deterministic random order), but it has no nice way to reproduce it! This restructures it a bit and significantly improves it, also updating it for the new generated crypto values, and adding support for setting a `REGEN=1` environment variable to generate a new test file.
-
Commits on Dec 4, 2020
-
Move key image signature generation into device
We don't have access to output private keys, so without this we can't generate staking transactions.
-
Add tx secret key via device layer
We add the tx secret key to the tx_extra in staking transactions so that values can be decoded, but the tx secret key value that we have on hand is encrypted and so we can't access it. This moves the call that adds the secret key into the device code so that devices can provide this. It also adds the tx version/type earlier in the process (into `open_tx`) so that the device can know early on that this is a stake transaction and therefore that leaking the tx secret key is okay (and can also apply other stake-specific behaviour).
-
Remove nonce (replace with 0) from stake unlock
There is no reason at all to sign a *different* message in every stake unlock; signatures already have their own nonce. Having something that serves no purpose is worse than not having it (because it leads to questions about why such a thing is there), so this commit removes it by always using 0 as a nonce and comments about it. Removing this from the broadcast tx would require a new tx extra field so that isn't worth doing for now (but can be done in the future if we change the tx extra structure for unlocks). This also simplifies the nonce-to-hash code and fixes an endian bug in it.
Commits on Dec 7, 2020
-
-
Move txversion/txtype outside of cryptonote_basic.h
This is simply enough that it can be used from device (which is above cryptonote_basic in the deps hierarchy).
-
-
-
Move LNS signature generation into device code
Includes Ledger implementation.
-
-
Revert wallet-side clsag c generation
Being able to pass the hash to the Ledger might be abusable (e.g. if it passed a different hash, with a different secret key to try to sign something else using the device's secret keys).
-
Remove "wrong account index" from error message
The Loki ledger doesn't have account indices (those are a semi-deprecated thing in the Monero app).
Commits on Dec 8, 2020
-
Properly display SW_WRONG_LENGTH errors
SW_WRONG_LENGTH is a range of errors: the least significant two bytes carry the failed length.
-
- rename INS_STEALTH to INS_ENCRYPT_PAYMENT_ID - remove no-longer-valid (and unused) INS_MANAGE_SEEDWORDS - hard-code CLSAG rct type prehashing and remove pre-CLSAG code paths - remove unused decrypt(rct key vector) - use a constexpr rather than memset & loop for dummy view/spend key values - fix speeling mistacks - fix shitty code formatting
-
Commits on Dec 9, 2020
-
Disable uninitialized value warning in jh.c
This is *probably* a false positive, but the code is such a mess that it's hard to be sure. Just switch off the warning and hope for the best.
-
Moves a bunch of inline methods out into a new cryptonote_basic.cpp compilation unit. (Given how widely cryptonote_basic.h gets included it seems desirable to have as little code needing compilation as possible).
-
Replace
keypair::generate
with akeypair
constructor taking a hwdevThis makes it a bit nicer, and allows in-place construction rather than needing to construct-and-copy.
-
Remove pre-HF16 CLSAG check and gen CLSAG txes in core test
Core tests were breaking because of the removal of pre-CLSAG transaction generation support. This fixes it by allowing and using CLSAG transactions before HF16 (which is safe to do now that we are well past the hard fork).
-
Changed this while debugging something else, but it's slightly better so keep it.
Commits on Dec 10, 2020
-
Rename hw::reset_mode to hw::mode_resetter
This isn't *doing* something, it is an RAII class that does the thing on destruction. Class names as verbs is confusing, so fix it.
-
Reset mode after making LNS signature
Not resetting leaves the ledger in a bad state, preventing other updates/txes/etc. from working.
-
Stake unlock: catch and show exceptions properly
Exceptions (e.g. because you denied the tx on the Ledger) were printing and being immediately (mostly) overwritten by the wallet prompt. This fixes them to be returned as proper errors (and thus bright red, consistent with other returned simplewallet errors).
-
-
Test suite: add big junk tx generator
When we need to fill a block we are currently generating a ton of transactions, but that is fairly slow: much faster to generate a small number of huge txes.
-
Test fix: confirm SNs before adding a dereg
Updating this test to the latest HF broke it because it was relying on a pre-HF13 bug that allowed deregs in the same height as the registration.
Commits on Dec 11, 2020
-
Test fixes for old tx construction removal
- updates tests to work properly with current HF - makes loki_generate_sequential_hard_fork_table jump 7->14....->15->16 rather than intermediate 8-9-10-11-12-13 blocks. (The 14 sequence is the generate block rewards before 15 lowers and 16 eliminates mining rewards). - remove test relying on the old 30-day expiry; that only worked on old HFs, but also required old (pre-v4) txes which don't work anymore, so I just removed the test.
-
Make hf table generation non-sequential
There's no reason we need intermediate blocks, so make it just generate v7@0 (for genesis), v14s to make funds, and the the target. (Or just v7@0 + target for <v14 hard fork versions).
Commits on Dec 14, 2020
-
Make
deferred
properly movableMoving it does what you'd expect: moves the lambda, copies the current cancel status, and cancels the old one. The implicit move constructor could malfunction: the old one wouldn't necessarily end up cancelled.
-
Test suite: add tx version hack and remove broken tests
This adds a variable hack into loki-core that lets us disable the transaction hard fork requirement so that the test suite can still generate transactions under older tx rules even though the transactions will be modern CLSAG txes. These are sort of bastardized txes that can never occur on the proper mainnet, but let us keep tests that apply to v2/v3 transactions even though we can't actually generate proper v2/v3 transactions anymore. A few tests got removed here because they are testing for old invalid bulletproof formats that don't matter anymore because they will never be accepted on the current chain anyway.
-
-
Remove invalid Borromean test case
What we are passing here is invalid, and so raises an exception, but the test structure here doesn't have a nice way to catch that, so just disable the test.
-
Annotate and fix double spend test
For some reason we aren't keeping the old chain as an alt chain anymore, but that shouldn't be a problem: fix the test as well as make some improvements in the tests it does.
-
Don't invoke cryptonote_core functions from device code
We can't call cryptonote::add_tx_secret_key_to_tx_extra from `device` code because that isn't necessarily available in `device` (though for some odd reason this only actually showed up on the i386 build). This amends the call to just get the secret key, leaving the actual job of adding it to tx.extra to the caller (which is a cleaner way to do it anyway).
-
Inline get_unlock_time to resolve build dep issue
device_ledger needs to call it, but can't link against cryptonote_basic, so provide an inline version.
-