Ledger updates #1195
Merged
Ledger updates #1195
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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.
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>
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).
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).
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).
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.
Reported by UkoeHB_ and sarang
We have a much nicer LOKI_DEFER replacement.
Like MLSAG, this code is no longer used (or usable) since HF16.
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.
- 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.
Includes Ledger implementation.
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).
The Loki ledger doesn't have account indices (those are a semi-deprecated thing in the Monero app).
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
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).
This makes it a bit nicer, and allows in-place construction rather than needing to construct-and-copy.
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.
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.
Not resetting leaves the ledger in a bad state, preventing other updates/txes/etc. from working.
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).
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.
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.
- 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.
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).
Moving 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.
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.
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.
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.
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).
device_ledger needs to call it, but can't link against cryptonote_basic, so provide an inline version.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some Ledger related updates (to go along with
devupdates in https://github.com/loki-project/loki-ledger-app/).Draft PR for now because I think there may be some more to come in this PR.