Skip to content
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

161 symmetric key pr2 #171

Merged
merged 8 commits into from
Jul 30, 2024
Merged

Conversation

dan-da
Copy link
Collaborator

@dan-da dan-da commented Jul 29, 2024

closes #161. obsoletes #169

This PR addresses review requests in #169 and goes further to simplify and improve the implementation.

Heres a list of changes since #169

code:
 * simplified `UtxoNotifyMethod` to `OnChain`, `OffChain`
 * simplified `UtxoNotifcation` to `OnChain(_)`, `OffChain(_)`
 * removed `UtxoNotifyMethodSpecifier`
 * change logic in `TxOutput::auto()`
 * add `KeyType` enum to improve abstraction
 * refactor common key logic into address::common
 * simplify WalletState::scan_for_announced_utxos()
 * simplify WalletState::find_spending_key_for_utxo()
 * add WalletState::get_all_known_spending_keys()
 * new param `owned_utxo_output_method` for `generate_tx_outputs()`
 * new param `change_key` for `create_transaction()
 * new param `key_type` for `next_receiving_address()` rpc
 * neptune-cli generate-wallet: remove display of first wallet addr
   as it should not be considered special/only.

docs:
 * document keys and addresses in mdbook
 * document utxo notifications in mdbook
 * add/update type/method doc-comments
 * add some missing module doc-comments

tests:
 * consolidate all key/addr tests into address_type::test
 * same test logic applies to both symmetric and generation keys
 * make all key tests into proptests (symmetric and generation)
 * make transaction_output tests more comprehensive

Here's how requests were addressed:

Please add a page to the documentation covering symmetric keys for change UTXOs.

Added a page about "Keys and Addresses" and another about "Utxo Notifications". Please review and feel free to edit, or send me your edits.

Avoid importing tools from generation_address in symmetric_key. Instead, factor them out to a helper module.

done. added address::common module.

UtxoNotifierMethodSpecifier and UtxoNotifierMethod seem similar and I wonder if they cannot be merged.

I got rid of UtxoNotifierMethodSpecifier. UtxoNotifyMethod and UtxoNotification are now simpler with just Onchain and OffChain variants.

Consider renaming SpendingKeyType to SpendingKey and generation_address::SpendingKey to generation_address::GenerationSpendingKey.

I forgot about this one. There is also ReceivingAddressType/ReceivingAddress. anyway, I can do that in a follow-up commit. either to this PR, or after merging.

edit: done.

Please convert the randomized tests in symmetric_key.rs into proptests. Have a look in primitive_witness.rs if you need something to pattern match on. (I realize there are plenty of tests out there that are randomized with thread_rng but I should like to convert those to proptests at some point. Let's not add to that conversion workload.)

done. all the tests for generation_address and symmetric have been moved into address_type and the same tests apply to both implementations. all tests that were in generation_address are now proptests.

Please add an integration test (not sure that's the right word) whereby you spawn two global states, one of which is a premine recipient whose sends some money to the other node and gets some change, and verify that the change is being observed and recovered correctly, even if the node in question reboots and loses everything but the seed phrase.

ok, I missed this one too. I will work on it tmw.

edit: done. please review. see module global_state_tests::restore_wallet

I'm making this a draft PR until the above two items are completed.

edit: ready for review.

Implements public announcements encrypted via symmetric keys

This is most useful for utxos that transfer funds within a single
wallet such as change addresses.

compared to on-chain pub-key public announcements:

Data encrypted with symmetric keys is smaller than data encrypted with
asymmetric keys so there is a blockchain space (and thus fee) savings.

compared to off-chain expected utxos:

symmetric-key announcements exist on the blockchain and thus are
immune to local data-loss situations.  off-chain expected utxos do not
use any blockchain space but require that the wallet holder make
ongoing backups of wallet state and never lose them.
code:
 * simplified `UtxoNotifyMethod` to `OnChain`, `OffChain`
 * simplified `UtxoNotifcation` to `OnChain(_)`, `OffChain(_)`
 * removed `UtxoNotifyMethodSpecifier`
 * change logic in `TxOutput::auto()`
 * add `KeyType` enum to improve abstraction
 * refactor common key logic into address::common
 * simplify WalletState::scan_for_announced_utxos()
 * simplify WalletState::find_spending_key_for_utxo()
 * add WalletState::get_all_known_spending_keys()
 * new param `owned_utxo_output_method` for `generate_tx_outputs()`
 * new param `change_key` for `create_transaction()
 * new param `key_type` for `next_receiving_address()` rpc
 * neptune-cli generate-wallet: remove display of first wallet addr
   as it should not be considered special/only.

docs:
 * document keys and addresses in mdbook
 * document utxo notifications in mdbook
 * add/update type/method doc-comments
 * add some missing module doc-comments

tests:
 * consolidate all key/addr tests into address_type::test
 * same test logic applies to both symmetric and generation keys
 * make all key tests into proptests (symmetric and generation)
 * make transaction_output tests more comprehensive
SpendingKey          --> GenerationSpendingKey
ReceivingAddress     --> GenerationReceivingAddress
SpendingKeyType      --> SpendingKey
ReceivingAddressType --> ReceivingAddress
we don't need to call add_expected_utxos_to_wallet() if there are not
any offchain expected_utxos.
1. Adds a Debug impl that displays to_string() value.  Useful when
reading failed assertions about coin amounts.

2. Adds From<u8>, From<u16>, From<u32> infallible conversions.
Makes it easy to do things like 42u8.into()
Adds tests for restoring a wallet from seed and verifying if balance
is correct after performing both onchain and offchain notifications.

demonstrates that offchain notifications are very risky and can
lead to loss-of-funds even when wallet-owner has the seed backed up.
@dan-da
Copy link
Collaborator Author

dan-da commented Jul 30, 2024

i think the build is failing in CI because of new rust 1.80. the time crate is generating an error. I will update to 1.80 and see if I can duplicate/fix.

1. update a couple crates in Cargo.lock where the old version was
   failing with rustc 1.80

2. workaround rust stack overflow when compiling triton-vm
fixes some indentation warnings that are new in clippy 1.80
@dan-da dan-da marked this pull request as ready for review July 30, 2024 19:37
@dan-da dan-da requested a review from aszepieniec July 30, 2024 19:41
Copy link
Contributor

@aszepieniec aszepieniec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent stuff. Very little to remark on. Green light for merging.

I added a commit for the requested description of generation addresses on asz/161_symmetric_key_pr2.

docs/src/neptune-core/utxo_notification.md Show resolved Hide resolved
src/models/state/mod.rs Show resolved Hide resolved
src/models/state/mod.rs Show resolved Hide resolved
@dan-da dan-da merged commit a8272ad into Neptune-Crypto:master Jul 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symmetric Key Announcements and Claiming
2 participants