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

feat: forester batched ops #1357

Draft
wants to merge 5 commits into
base: jorrit/test-rebase-audit-branch
Choose a base branch
from

Conversation

sergeytimoshin
Copy link
Contributor

No description provided.

@@ -35,6 +37,17 @@ fn process_state_account(account: &Account, pubkey: Pubkey) -> Result<TreeAccoun
))
}

fn process_batch_state_account(account: &mut Account, pubkey: Pubkey) -> Result<TreeAccounts> {
check_discriminator::<BatchedMerkleTreeAccount>(&account.data)?;
let tree_account = ZeroCopyBatchedMerkleTreeAccount::from_bytes_mut(&mut account.data)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

from_bytes_mut checks the discriminator

Comment on lines 178 to 188
if i == merkle_tree.get_account().queue.batch_size / 2 {
instruction_data = Some(
create_append_batch_ix_data(
&mut e2e_env.rpc,
&mut e2e_env.indexer.state_merkle_trees[0],
state_merkle_tree_pubkey,
output_queue_pubkey,
)
.await,
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

best remove this it was just to test concurrency in the other test

.ok_or(AccountCompressionErrorCode::InclusionProofByIndexFailed)?;

if element == value {
*element = [0; 32];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*element = [0; 32];

We need to remove this so that we can use the value from output_queue.value_vec[full_batch_index] directly instead from the indexer.

@sergeytimoshin sergeytimoshin force-pushed the sergey/forester-batched-ops-v2 branch 3 times, most recently from 2db5b62 to c9952ce Compare November 27, 2024 18:31
@ananas-block ananas-block changed the base branch from main to jorrit/test-rebase-audit-branch November 29, 2024 00:26
.indexer
.lock()
.await
.get_proof_by_index(self.merkle_tree, i as usize);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a batched call to the indexer for this?

let mut merkle_proofs = Vec::new();

let batch = {
let mut merkle_tree_account = rpc.get_account(self.merkle_tree).await.unwrap().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate get_account call its already fetched in line 331.

.indexer
.lock()
.await
.get_leaf_indices_tx_hashes(self.merkle_tree, zkp_batch_size as usize);
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering whether it would make sense to return get_proof_by_index(self.merkle_tree, *index as usize); with this call as well so that we avoid the second call to the indexer.

@@ -271,7 +267,6 @@ impl ZeroCopyBatchedQueueAccount {
.ok_or(AccountCompressionErrorCode::InclusionProofByIndexFailed)?;

if element == value {
*element = [0; 32];
Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately we need to revert this because it leads to some issues onchain.
This means that we can't get the leaves for the batch append circuit inputs from the batch Merkle tree account value array because some values have been zeroed out.
Instead we need to get the data from the indexer. I think it makes sense to add a get_queue_elements method to the Indexer trait like I have for the batch address tree see here.

@sergeytimoshin sergeytimoshin force-pushed the sergey/forester-batched-ops-v2 branch 3 times, most recently from fa35b58 to e1e2ed9 Compare December 3, 2024 15:06
@sergeytimoshin sergeytimoshin force-pushed the sergey/forester-batched-ops-v2 branch from e1e2ed9 to 0f8eae2 Compare December 3, 2024 15:21
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.

2 participants