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

Support multiple namespaces in QueryablePayload::transaction_with_proof #1212

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

akonring
Copy link
Contributor

@akonring akonring commented Mar 8, 2024

closes: #1010

Description

Currently the QueryablePayload::transaction_with_proof is broken as it does not support more than one namespace with namespace id: VmId(0).
This PR fixes broken code such that both proving and verification can be done for transactions in all namespaces of the block.

Other benefits

Methods like get_tx_table_len and get_tx_table_len_proof in payload.rs were designed when only a single tx-table existed (no namespaces). These are now removed (no other methods depended on them) and the new implementation instead makes use of the TxTable inside tables.rs.

Considerations

A caching mechanism was designed when we only had a single tx-table (no namespaces). This could be revisited to regain some efficiency from re-using the proofs generated fx for tx-table lengths.

// TODO(X) Revisit caching of frequently used items
//
// TODO type should be `OnceLock<SmallRangeProofType>` instead of `OnceLock<Option<SmallRangeProofType>>`.
// We can correct this after `once_cell_try` is stabilized <https://github.com/rust-lang/rust/issues/109737>.
// #[derivative(Hash = "ignore")]
// #[derivative(PartialEq = "ignore")]
// #[serde(skip)]
// pub tx_table_len_proof: OnceLock<Option<SmallRangeProofType>>,
}

@sveitser sveitser self-requested a review March 13, 2024 13:19
Copy link
Collaborator

@sveitser sveitser left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with the payload encoding so probably wouldn't spot an error if there was one but the changes look good to me. If you're unsure and would like a more thorough review maybe @nomaxg can have a look.

@akonring
Copy link
Contributor Author

Rebasing on main to incorporate #1229 and make CI happy

@akonring akonring merged commit 49aecce into main Mar 15, 2024
16 checks passed
@akonring akonring deleted the ak/1010 branch March 15, 2024 07:19
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.

Implement QueryablePayload::transaction_with_proof for multiple namespaces
2 participants