-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: SpendingKey self derivation, parallel iteration #304
feat: SpendingKey self derivation, parallel iteration #304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Premine distribution depends on the network. I can't see the benefit of having a premine distribution that depends on the network. If the issue is having premine allocations for testing purposes, in principle the only one that's necessary is the devnet wallet used in unit tests. Are there other reasons?
Separate Test and Non-Test Premine Distributions. If I read correctly, there are two implementations of premine_distribution
, one marked with a test
flag and one marked with a not(test)
flag. This strikes me as a really bad idea because it defeats the purpose of testing. We want to test the same thing that is going to be deployed in practice, not something else that we hope is similar.
Ensure Integrity of Existing Premine Receiver Keys. I have a concern, already articulated in #300. The context is that we have already received some public receiving addresses from investors, who ran older versions of the software to generate them. The concern is that refactoring how key pairs are managed will invalidate these premine receiver keys.
I realize that previous refactorings may have already invalidated premine receiver keys. Nevertheless, before approving this PR I want to make sure that it is not making the situation worse.
The most methodical way forward is something along these lines:
- checkout relevant commits (alphanet and betanet respectively)
- use
neptune-cli generate-wallet
,neptune-cli own-receiving-address
andneptune-cli export-seed-phrase
- building on HEAD again, add a premine allocation to the generated receiving addresses
- add a unit test that instantiates a wallet for each of the generated seed phrases
- in the test, verify that these wallets come into possession of their respective premine allocations
If addressing this concern methodically is out of scope for this PR, then a) the test case should not hinder its progress, but also b) please find another way to convince me that this PR is not making the situation worse in regards to this concern.
I haven't had time to address any comments yet because I have been chasing down a unit-test failure that is driving me crazy. this branch was passing all tests until I rebased it to master prior to pushing this PR. It now fails one test: models::state::mempool::tests::reorganization_does_not_crash_mempool. tldr; @aszepieniec I'm going to need your assistance with this. The error is:
After a lot of debugging/tracing/bisecting I discovered that if I modify WalletSecret::nth_generation_spending_key() and WalletSecret::nth_spending_key() to use the old derivation logic then the test passes. After a lot more debugging and messing around with the test case, I still did not determine a cause. I spent days on this, ruling things out, but not finding a smoking gun, or any way to make the test case pass (unmodified) with the new derivation scheme. Finally this morning I got a bright idea to do something unusual: rebase master on top of my work-in-progress branch. The commit is:
The only code changes in this commit are triton_asm additions inside From context of the modified code, the test case may be generating a coinbase with negative fee somehow, but I don't see how, or why it would be different between the two derivation schemes. The "must stay confirmable" error message provides no clue I can understand. It is also possible that the asm additions are buggy somehow. Again though I am perplexed why it would work with the old key derivation scheme, but not the new one. Something odd is going on.... |
I just wanted to say that I have not forgotten about the request for assistance but I just did not get round to it yet. Busy period. |
Primary features: 1. SpendingKey can now derive its own children without any wallet state. This enables RPC clients to derive keys themselves. 2. Derivation iterators. SpendingKeyIter and SpendingKeyParallelIter. The latter leverages rayon to utilize all cpu cores. 3. /known_keys rpc now returns a SpendingKeyRange instead of a list of addresses. This is a big perf win for large wallets. changelog: refactor spending-key types so each key can derive its own children known_keys rpcs now return iterators instead of Vec DerivationIndex: u64 -> u128. fix premine_distribution(). separate test vs non-test. make it aware of current network impl IntoIterator for SpendingKey GenerationReceivingAddress::derive_from_seed() -> from_seed() impl rayon parallel iterators SpendingKeyIter now starts at child 0, instead of parent_key. fix some DoubleEndedIter issues iteration tests add test double_ended_range_iterator_meet_middle add test double_ended_iterator_to_first_elem add tests double_ended_range_iterator_to_first_elem, range_iterator_to_last_elem, iterator_nth separate SpendingKeyParallelIter from SpendingKeyIter add par_iter tests iterator accepts any impl RangeBounds tests to validate RangeBounds conversions known_keys() rpc returns SpendingKeyRange instead of SpendingKeyIter only perform extra GenerationSpendingKey assertions for debug build. add comments
Please see #278 (comment) for motivation/rationale. In short, this fixes the total_premine_amount in Block::genesis block(). But also it is simply gross to be including addresses bech32-decoded from different networks, regardless of network. That seems self-evident to me.
We are not testing what will be deployed in practice unless/until all pre-mine addresses are listed. What we are testing is a set of stand-ins for the real pre-mine list. More importantly, note that the cfg(test) version of the function contains these lines:
In my view these should not be included in the real genesis block because:
now, other solutions are possible rather than having two different versions of the function, and feel free to toss out a suggestion, but that is a common/idiomatic way to separate things that are meant for testing from things that are not.
technically, this does not invalidate any existing priv/pub keypair. What it will do is make it so that a given wallet seed will generate different keypairs than in the past. Which is functionally equivalent to invalidating existing keys, unless we were to provide a tool for deriving the old-style keys. It's an interesting consideration. Upon a quick review, it appears we could support the old derivation. It requires storing the master-seed+derivation index in each key, instead of storing only a child seed. So the keys become larger by the size of DerivationIndex, which is presently a u128 (16 bytes). Personally, I don't think it is worth it. That is a permanent price to pay for a one-time issue. I would advocate to ask premine-recipients to generate new keys once we are fully locked in on the key format (simplest), or to provide a tool for generating the old keys, and a mechanism to import external keys into neptune-core wallet.
If they are already invalidated, then this can't make it any worse. invalidated is invalidated. Seems worth checking into.
A simpler test would just be to generate the 0th and 1st keypair as of alphanet and then verify that derived keys match those values. If the test passes in alphanet, betanet, and master without this PR, then we know this PR invalidate the keys, else if it fails in master they are already invalidated.
I will make the above simplified test. |
thx. good news (I think). I just rebased this PR on top of master and magically the failing test case is passing again. I don't know why, which concerns me, but it seems good. presently waiting for other tests to complete, as new proofs must be generated. |
24ddee3
to
89ba8a4
Compare
I just pushed this PR rebased to master and removed the minor changes inside the Arbtirary impls, which are no longer part of normal builds. After the rebase all tests are passing again (for unknown reason) so I will begin addressing the review comments. |
hmm, CI failed one of the new iterator tests:
This test passes 10/10 attempts for me on local dev machine. I will need to investigate further (later). |
A panic is thrown inside rayon when calling collect() on the parallel iterator. The test passes if run on a machine with >= 64 cores, or if run with RAYON_NUM_THREADS=n where n >= 64. This might be a rayon bug that I'm somehow triggering. Opened rayon-rs/rayon#1224 |
fixes a panic in SpendingKeyParallelIter::collect(). also adds a test assertion that count() is correct. An incorrect count was another symptom of the bug. big thanks to @cuviper for spotting the problem. rayon-rs/rayon#1224 (comment)
nope it was my own bug. the 64 thread thing was a weird red herring. fixed now thanks to @cuviper. |
Ok, I made a couple tests, and have info to report, and a question/observation. First, I checked out the
I saved these tests in a new branch alphanet-with-generation-derive-test, commit 196a8b0. Then I updated to present master 953a93c and cherry-picked the above commit. I pushed the result to branch danda/generation-derive-test, commit 426abb8. What I find is that both tests fail. This means that something has changed about the GenerationSpendingKey format and/or derivation between alphanet and now. I haven't looked into what changed or when. anyway, these tests can be updated and merged into master once we are fully decided on the derivation method.
my question is: when, and in what format did people provide their receiving addresses? I assume they were bech32m encoded, but for which network? At the or am I missing something? |
If memory serves, the only effect the network has on the bech32m encoding of an address is the single character infix (or suffix to the human-readable part). And again if memory serves, we decided that this infix should be "t" for testnet and "m" for everything else. So that if you generate an address it will be compatible with mainnet unless you specifically opt out. |
If I'm reading the bech32 crate code correctly, the HRP also affects the checksum. That is something we can test/verify. if I understand your point correctly it is that we could modify the HRP of the received addresses before hard-coding them into the code. While it may be technically possible, it strikes me as problematic to modify what we were sent by 3rd parties. Seems to me it should match exactly, so there's no cause for questions/concern. regardless of all that, the test failure I am seeing is not only the prefix. here is an example:
where: left is key with index 0, derived by code at 426abb8 (branched from present master) and right is key with index 0, derived by code branched from alphanet tag. note that:
I have not attempted to track down when/why/how the change occurs. It can be done, but is a bit tedious. But it would be helpful to know at what point we received addresses (and if prefixes are ngam or nolgam), because if they are nolgam prefixes, there is still a chance they might not have changed since then. |
The first address to be received was on 18 January 2024. All have the |
Thx, that's helpful. I found the alphanet-v5 tag at 06 Jan, 2024. So I conclude that addresses the team received were most likely generated with alphanet-v5 binaries. Given this, I created branch I then adapted these same tests, with same keys and addrs to master, passing Network::Main to ::from_bech32m(). This is in 3f4ddd1. I'm happy to report that the tests still pass. This means that the addresses we received on 18 Jan 2024 are confirmed compatible with present master, including key derivation, provided they were generated with an alphanet-v5 binary. Given this, I will next attempt to make the self-deriving keys also compatible and report back. |
Closing in favor of #317 which I believe addresses all review comments. |
obsoletes #278. See it for discussion/background.
Primary changes since #278:
changelog:
refactor spending-key types so each key can derive its own children
known_keys rpcs now return iterators instead of Vec
DerivationIndex: u64 -> u128.
fix premine_distribution(). separate test vs non-test. make it aware of current network
impl IntoIterator for SpendingKey
GenerationReceivingAddress::derive_from_seed() -> from_seed()
impl rayon parallel iterators
SpendingKeyIter now starts at child 0, instead of parent_key. fix some DoubleEndedIter issues
iteration tests
add test double_ended_range_iterator_meet_middle
add test double_ended_iterator_to_first_elem
add tests double_ended_range_iterator_to_first_elem, range_iterator_to_last_elem, iterator_nth
separate SpendingKeyParallelIter from SpendingKeyIter
add par_iter tests
iterator accepts any impl RangeBounds
tests to validate RangeBounds conversions
known_keys() rpc returns SpendingKeyRange instead of SpendingKeyIter
only perform extra GenerationSpendingKey assertions for debug build. add comments