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: Return code for FPI #521

Merged
merged 11 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## v0.6.0 (TBD)

- Added `AccountCode` as part of `GetAccountProofs` endpoint response (#521).
- [BREAKING] Added `kernel_root` to block header's protobuf message definitions (#496).
- [BREAKING] Renamed `off-chain` and `on-chain` to `private` and `public` respectively for the account storage modes (#489).
- Optimized state synchronizations by removing unnecessary fetching and parsing of note details (#462).
Expand Down
6 changes: 5 additions & 1 deletion crates/proto/src/generated/requests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,11 @@ pub struct GetAccountProofsRequest {
/// List of account IDs to get states.
#[prost(message, repeated, tag = "1")]
pub account_ids: ::prost::alloc::vec::Vec<super::account::AccountId>,
/// Account code commitments corresponding to the last-known `AccountCode` for requested
/// accounts. Responses will include only the ones that are not known to the caller.
#[prost(message, repeated, tag = "2")]
pub code_commitments: ::prost::alloc::vec::Vec<super::digest::Digest>,
/// Optional flag to include header in the response. `false` by default.
#[prost(bool, optional, tag = "2")]
#[prost(bool, optional, tag = "3")]
pub include_headers: ::core::option::Option<bool>,
}
6 changes: 5 additions & 1 deletion crates/proto/src/generated/responses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,12 @@ pub struct AccountProofsResponse {
/// Authentication path from the `account_root` of the block header to the account.
#[prost(message, optional, tag = "3")]
pub account_proof: ::core::option::Option<super::merkle::MerklePath>,
/// Account code, returned only in the case where the request code commitment does not match
/// with the current one.
#[prost(bytes = "vec", optional, tag = "4")]
pub account_code: ::core::option::Option<::prost::alloc::vec::Vec<u8>>,
/// State header for public accounts. Filled only if `include_headers` flag is set to `true`.
#[prost(message, optional, tag = "4")]
#[prost(message, optional, tag = "5")]
pub state_header: ::core::option::Option<AccountStateHeader>,
}
#[derive(Clone, PartialEq, ::prost::Message)]
Expand Down
5 changes: 4 additions & 1 deletion crates/rpc-proto/proto/requests.proto
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ message GetAccountStateDeltaRequest {
message GetAccountProofsRequest {
// List of account IDs to get states.
repeated account.AccountId account_ids = 1;
// Account code commitments corresponding to the last-known `AccountCode` for requested
// accounts. Responses will include only the ones that are not known to the caller.
repeated digest.Digest code_commitments = 2;
// Optional flag to include header in the response. `false` by default.
optional bool include_headers = 2;
optional bool include_headers = 3;
}
5 changes: 4 additions & 1 deletion crates/rpc-proto/proto/responses.proto
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,11 @@ message AccountProofsResponse {
digest.Digest account_hash = 2;
// Authentication path from the `account_root` of the block header to the account.
merkle.MerklePath account_proof = 3;
// Account code, returned only in the case where the request code commitment does not match
// with the current one.
optional bytes account_code = 4;
// State header for public accounts. Filled only if `include_headers` flag is set to `true`.
optional AccountStateHeader state_header = 4;
optional AccountStateHeader state_header = 5;
}

message AccountStateHeader {
Expand Down
8 changes: 6 additions & 2 deletions crates/store/src/server/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,8 +490,12 @@ impl api_server::Api for StoreApi {

let account_ids = convert(request.account_ids);
let include_headers = request.include_headers.unwrap_or_default();
let (block_num, infos) =
self.state.get_account_proofs(account_ids, include_headers).await?;
let request_code_commitments = try_convert(request.code_commitments)
.map_err(|err| Status::invalid_argument(format!("Invalid code commitment: {}", err)))?;
bobbinth marked this conversation as resolved.
Show resolved Hide resolved
let (block_num, infos) = self
.state
.get_account_proofs(account_ids, request_code_commitments, include_headers)
.await?;

Ok(Response::new(GetAccountProofsResponse {
block_num,
Expand Down
30 changes: 21 additions & 9 deletions crates/store/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

use std::{
collections::{BTreeMap, BTreeSet},
ops::Not,
sync::Arc,
};

Expand All @@ -16,7 +17,7 @@ use miden_node_proto::{
};
use miden_node_utils::formatting::{format_account_id, format_array};
use miden_objects::{
accounts::{AccountDelta, AccountHeader},
accounts::{AccountCode, AccountDelta, AccountHeader},
block::Block,
crypto::{
hash::rpo::RpoDigest,
Expand Down Expand Up @@ -683,16 +684,15 @@ impl State {
pub async fn get_account_proofs(
&self,
account_ids: Vec<AccountId>,
request_code_commitments: Vec<RpoDigest>,
include_headers: bool,
) -> Result<(BlockNumber, Vec<AccountProofsResponse>), DatabaseError> {
// Lock inner state for the whole operation. We need to hold this lock to prevent the
// database, account tree and latest block number from changing during the operation,
// because changing one of them would lead to inconsistent state.
let inner_state = self.inner.read().await;

let state_headers = if !include_headers {
BTreeMap::<AccountId, AccountStateHeader>::default()
} else {
let account_data: BTreeMap<u64, (AccountStateHeader, AccountCode)> = {
let infos = self.db.select_accounts_by_ids(account_ids.clone()).await?;

if account_ids.len() > infos.len() {
Expand All @@ -709,10 +709,13 @@ impl State {
info.details.map(|details| {
(
info.summary.account_id.into(),
AccountStateHeader {
header: Some(AccountHeader::from(&details).into()),
storage_header: details.storage().get_header().to_bytes(),
},
(
AccountStateHeader {
header: Some(AccountHeader::from(&details).into()),
storage_header: details.storage().get_header().to_bytes(),
},
details.code().clone(),
),
)
})
})
Expand All @@ -724,12 +727,21 @@ impl State {
.map(|account_id| {
let acc_leaf_idx = LeafIndex::new_max_depth(account_id);
let opening = inner_state.account_tree.open(&acc_leaf_idx);
let (state_header, account_code) = account_data
.get(&account_id)
.cloned()
.expect("Function would have errored if accounts were not found");
let account_code = request_code_commitments
.contains(&account_code.commitment())
.not()
.then_some(account_code.to_bytes());

AccountProofsResponse {
account_id: Some(account_id.into()),
account_hash: Some(opening.value.into()),
account_proof: Some(opening.path.into()),
state_header: state_headers.get(&account_id).cloned(),
account_code,
state_header: include_headers.then_some(state_header),
}
})
.collect();
Expand Down
5 changes: 4 additions & 1 deletion proto/requests.proto
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ message GetAccountStateDeltaRequest {
message GetAccountProofsRequest {
// List of account IDs to get states.
repeated account.AccountId account_ids = 1;
// Account code commitments corresponding to the last-known `AccountCode` for requested
// accounts. Responses will include only the ones that are not known to the caller.
repeated digest.Digest code_commitments = 2;
bobbinth marked this conversation as resolved.
Show resolved Hide resolved
// Optional flag to include header in the response. `false` by default.
optional bool include_headers = 2;
optional bool include_headers = 3;
Comment on lines +138 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would move include_headers above code_commitments.

Also, we should update comments to make it clear that account code only is included if include_headers is true, and also that it is not associated with a specific account ID (i.e., we check code roots against all accounts).

}
5 changes: 4 additions & 1 deletion proto/responses.proto
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,11 @@ message AccountProofsResponse {
digest.Digest account_hash = 2;
// Authentication path from the `account_root` of the block header to the account.
merkle.MerklePath account_proof = 3;
// Account code, returned only in the case where the request code commitment does not match
// with the current one.
optional bytes account_code = 4;
// State header for public accounts. Filled only if `include_headers` flag is set to `true`.
optional AccountStateHeader state_header = 4;
optional AccountStateHeader state_header = 5;
bobbinth marked this conversation as resolved.
Show resolved Hide resolved
}

message AccountStateHeader {
Expand Down
Loading