Basic `put_block` function for the memory wallet #1298

oxarbitrage · 2024-03-21T18:40:45Z

I want to create the most basic version of a put_blocks function for the memory wallet, one where we assume that the blocks will be in order.

This PR is for @nuttycom to take a look, it has some comments and questions in the code and also review comments.

…llet_fix_build

oxarbitrage

I am pretty new to the codebase so i have a lot of unknowns and questions.

Cargo.toml

zcash_client_backend/src/data_api.rs

oxarbitrage · 2024-03-21T18:43:45Z

zcash_client_backend/src/wallet.rs

@@ -98,6 +98,7 @@ impl<AccountId, N> Recipient<AccountId, Option<N>> {
 /// The shielded subset of a [`Transaction`]'s data that is relevant to a particular wallet.
 ///
 /// [`Transaction`]: zcash_primitives::transaction::Transaction
+#[derive(Clone)]


I had to do this for my version of put_blocks as i was not able to make it otherwise, it might be not needed and not wanted.

zcash_client_backend/src/data_api/mem_wallet.rs

nuttycom

Sorry it's taken me a bit to get to commenting on this; it's been a busy time getting Zashi 1.0 out. :)

zcash_client_backend/src/data_api.rs

nuttycom · 2024-03-29T14:50:48Z

zcash_client_backend/src/data_api/mem_wallet.rs

@@ -236,7 +235,10 @@ impl WalletRead for MemoryWalletDb {
    }

    fn chain_height(&self) -> Result<Option<BlockHeight>, Self::Error> {
-        todo!()
+        match self.blocks.last_key_value() {


Something to take into consideration here is how this is handled in zcash_client_sqlite: in that context, the chain tip may not yet have been scanned, so we get the current chain height as the maximum height known to the scan queue, rather than the blocks table.

Blocks are added to the memory wallet database by put_blocks which currently receives a vector of ScannedBlock. Do we plan to change that ?

Whatever is the case, ill open a separated PR for this.

Blocks are added to the memory wallet database by put_blocks which currently receives a vector of ScannedBlock. Do we plan to change that ?

No; the main distinction here is that the chain tip height is used in computing how to prioritize the scanning queue; we scan blocks out-of-order in many cases to improve time to spendability. The important related operation is update_chain_tip; basically, chain_height should return whatever is correct as of the last call to update_chain_tip, not the last call to put_blocks.

In update_chain_tip where do you store the height that is passed by argument so you can use it in chain_height?

nuttycom · 2024-03-29T14:53:02Z

zcash_client_backend/src/data_api/mem_wallet.rs

    fn put_blocks(
        &mut self,
+        // TODO: Figure out what to do with this field.
        _from_state: &super::chain::ChainState,


This is used to ensure we have a correct "starting point" for adding to the note commitment trees in https://github.com/zcash/librustzcash/blob/main/zcash_client_sqlite/src/lib.rs#L928-L934 and https://github.com/zcash/librustzcash/blob/main/zcash_client_sqlite/src/lib.rs#L977-L983

Thanks, i added that at a4323c2

zcash_client_backend/src/data_api/mem_wallet.rs

oxarbitrage · 2024-04-02T19:44:45Z

zcash_client_backend/src/data_api/mem_wallet.rs

+
+            self.blocks.insert(block.block_height, memory_block);
+
+            // Add the sapling commitments to the sapling tree.


Is this still needed now that we added the frontier?

Yes; the frontier just provides a starting point, but the note commitments for each note in the block, and the commitment for the block-end checkpoint, must also be added.

Thanks, i added the orchard version of this at 370a4aa

nuttycom

The incorrect start to the note commitment tree position is blocking.

nuttycom · 2024-04-11T19:53:37Z

zcash_client_backend/src/data_api/mem_wallet.rs

+            if let Some(value) = from_state.final_sapling_tree().value() {
+                self.sapling_tree
+                    .batch_insert(value.position(), block_commitments.sapling.into_iter());
+            }


This is a bug, in that it doesn't handle constructing the tree starting from Sapling activation properly. I think something like this is necessary. Also, the final position of the previous frontier is one less than the starting position of new note commitments being added to the tree.

Suggested change

if let Some(value) = from_state.final_sapling_tree().value() {

self.sapling_tree

.batch_insert(value.position(), block_commitments.sapling.into_iter());

}

let start_position = from_state.final_sapling_tree().value().map_or(0.into(), |t| t.position() + 1);

self.sapling_tree.batch_insert(start_position, block_commitments.sapling.into_iter());

nuttycom · 2024-04-11T19:54:47Z

zcash_client_backend/src/data_api/mem_wallet.rs

+            // Add the Orchard commitments to the orchard tree.
+            if let Some(value) = from_state.final_orchard_tree().value() {
+                self.orchard_tree
+                    .batch_insert(value.position(), block_commitments.orchard.into_iter());
+            }


Suggested change

// Add the Orchard commitments to the orchard tree.

if let Some(value) = from_state.final_orchard_tree().value() {

self.orchard_tree

.batch_insert(value.position(), block_commitments.orchard.into_iter());

}

{

// Add the Orchard commitments to the orchard tree.

let start_position = from_state.final_orchard_tree().value().map_or(0.into(), |t| t.position() + 1);

self.orchard_tree.batch_insert(start_position, block_commitments.orchard.into_iter());

Co-authored-by: Kris Nuttycombe <[email protected]>

oxarbitrage · 2024-04-16T20:30:39Z

I fixed the clippy warnings in c9a236e

The orchard tests failures are unrelated to this PR as they are also happening in #1293

nuttycom

utACK c9a236e

oxarbitrage added 2 commits March 21, 2024 15:33

create a very basic put_blocks function for the memory wallet

69c1e68

Merge remote-tracking branch 'origin/memory_wallet_db' into memory_wa…

09e8bc9

…llet_fix_build

oxarbitrage commented Mar 21, 2024

View reviewed changes

nuttycom reviewed Mar 22, 2024

View reviewed changes

zcash_client_backend/src/data_api/mem_wallet.rs Show resolved Hide resolved

append sapling noie commitments to database

9c0c6b0

nuttycom reviewed Mar 29, 2024

View reviewed changes

oxarbitrage added 5 commits April 2, 2024 14:29

remove non needed pubs

4d3c441

remove chain_height code

7f74621

add orchard spends

0393a69

add sapling and orchard trees

a4323c2

use sapling-crypto v0.1.3

a3b4de0

oxarbitrage commented Apr 2, 2024

View reviewed changes

oxarbitrage added 7 commits April 2, 2024 17:29

populate orchard note commitment trees

370a4aa

use batch_insert

b1a0fcc

add the memos

23a2ae8

mark spent notes

2f28306

fix note commitment tree starting position

588156f

use all accounts in the wallet

f77dfc1

remove todo comment

147c033

oxarbitrage changed the title ~~WIP: Basic put_block function for the memory wallet~~ Basic put_block function for the memory wallet Apr 9, 2024

oxarbitrage marked this pull request as ready for review April 9, 2024 19:34

nuttycom requested changes Apr 11, 2024

View reviewed changes

fix incorrect start note commitment position

cbb49b4

Co-authored-by: Kris Nuttycombe <[email protected]>

oxarbitrage mentioned this pull request Apr 16, 2024

research: Explore populating in memory wallet from zebra ZcashFoundation/zebra#8358

Closed

clippy

c9a236e

nuttycom approved these changes Apr 18, 2024

View reviewed changes

nuttycom merged commit 5d2cd6f into zcash:memory_wallet_db Apr 18, 2024
20 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic `put_block` function for the memory wallet #1298

Basic `put_block` function for the memory wallet #1298

oxarbitrage commented Mar 21, 2024 •

edited

Loading

oxarbitrage left a comment

oxarbitrage Mar 21, 2024

nuttycom left a comment

nuttycom Mar 29, 2024

oxarbitrage Apr 2, 2024

oxarbitrage Apr 2, 2024

nuttycom Apr 2, 2024

oxarbitrage Apr 2, 2024

nuttycom Mar 29, 2024

oxarbitrage Apr 2, 2024

oxarbitrage Apr 2, 2024

nuttycom Apr 2, 2024

oxarbitrage Apr 2, 2024

nuttycom left a comment

nuttycom Apr 11, 2024 •

edited

Loading

nuttycom Apr 11, 2024

oxarbitrage commented Apr 16, 2024

nuttycom left a comment


		self.blocks.insert(block.block_height, memory_block);

		// Add the sapling commitments to the sapling tree.

Basic put_block function for the memory wallet #1298

Basic put_block function for the memory wallet #1298

Conversation

oxarbitrage commented Mar 21, 2024 • edited Loading

oxarbitrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

nuttycom Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxarbitrage commented Apr 16, 2024

nuttycom left a comment

Choose a reason for hiding this comment

Basic `put_block` function for the memory wallet #1298

Basic `put_block` function for the memory wallet #1298

oxarbitrage commented Mar 21, 2024 •

edited

Loading

nuttycom Apr 11, 2024 •

edited

Loading