cuprated: RPC handlers (json-rpc) #308
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
A-dependency
12 tasks
github-actions
bot
added
A-test-utils
hinto-janai
commented
Nov 27, 2024
There was a problem hiding this comment.
I'm going to open this for review for now, although there's a few places that need fixing.
RPC code structure
binaries/cuprated/src/rpc/{bin,json,other}.rs are the handlers functions which call:
binaries/cuprated/src/rpc/request/*.rswhich arefnversions of ourtower::Service'sRequest->Responseto reduce noisebinaries/cuprated/src/rpc/helper.rswhich are convenience functions that sometimes call the above functions
RPC handlers
Map of progress of the handler functions. There's more detailed comments on the handlers in other review comments.
| Color | Meaning |
|---|---|
| 🟢 | Ready |
| 🟣 | Ready but depends on other things that aren't ready |
| 🔵 | Ready but not efficient |
| fn | Status | Details |
|---|---|---|
| get_block_count | 🟢 | |
| on_get_block_hash | 🟣 | Needs access to cuprated's Chain |
| submit_block | 🟢 | |
| generate_blocks | 🟣 | Needs access to cuprated's Chain |
| get_last_block_header | 🟢 | |
| get_block_header_by_hash | 🟢 | |
| get_block_header_by_height | 🟢 | |
| get_block_headers_range | 🔵 | Would benefit from a request that allows retrieving a range of (Block, ExtendedBlockHeader) |
| get_block | 🟢 | |
| get_connections | 🟢 | |
| get_info | 🟣 | Needs access to unimplemented things; Needs correctness review |
| hard_fork_info | 🟢 | |
| set_bans | 🟢 | |
| get_bans | 🟢 | |
| banned | 🟢 | |
| flush_transaction_pool | 🟢 | |
| get_output_histogram | 🟢 | |
| get_coinbase_tx_sum | 🟢 | |
| get_version | 🟢 | |
| get_fee_estimate | 🟢 | |
| get_alternate_chains | 🟢 | |
| relay_tx | 🟢 | |
| sync_info | 🟢 | |
| get_transaction_pool_backlog | 🟢 | Requires decision on binary strings |
| get_output_distribution | 🟣 | Requires decision on binary strings; requires RpcHandler::get_output_distribution implementation |
| get_miner_data | 🟢 | |
| prune_blockchain | 🟢 | |
| calc_pow | 🟣 | CalculatePow must sanity check |
| flush_cache | 🟣 | cuprated doesn't need this call; behavior must be decided |
| add_aux_pow | 🟣 | Crypto functions |
| get_tx_ids_loose | 🟣 | This RPC call is not yet in monerod's v0.18 branch |
Comment on lines
+317
to
+321
| for height in range { | ||
| let height = usize_to_u64(height); | ||
| let header = helper::block_header(&mut state, height, request.fill_pow_hash).await?; | ||
| headers.push(header); | ||
| } |
There was a problem hiding this comment.
Is there a good way to turn async for's into iterator versions? E.g. something like:
let headers = range
.map(|height| async {
helper::block_header(
&mut state,
usize_to_u64(height),
request.fill_pow_hash
).await?
})
.collect();We could spawn a tokio::task per height too.
Comment on lines
+903
to
+906
| fn get_output_distribution() -> Result<Distribution, Error> { | ||
| todo!("https://github.com/monero-project/monero/blob/893916ad091a92e765ce3241b94e706ad012b62a/src/rpc/rpc_handler.cpp#L29"); | ||
| Err(anyhow!("Failed to get output distribution")) | ||
| } |
There was a problem hiding this comment.
This seems to be used by ZMQ too, I think it should be defined outside of cuprated. It may be changed by FCMP++ as well so I think an impl could wait.
Comment on lines
+982
to
+1003
| // let block_weight = todo!(); | ||
|
|
||
| // let median_for_block_reward = blockchain_context::context(&mut state.blockchain_context) | ||
| // .await? | ||
| // .unchecked_blockchain_context() | ||
| // .context_to_verify_block | ||
| // .median_weight_for_block_reward; | ||
|
|
||
| // if cuprate_consensus_rules::blocks::check_block_weight(block_weight, median_for_block_reward) | ||
| // .is_err() | ||
| // { | ||
| // return Err(anyhow!("Block blob size is too big, rejecting block")); | ||
| // } | ||
|
|
||
| // TODO: will `CalculatePow` do the above checks? | ||
| let pow_hash = blockchain_context::calculate_pow( | ||
| &mut state.blockchain_context, | ||
| hardfork, | ||
| block, | ||
| seed_hash, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Will BlockChainContextRequest::CalculatePow do the sanity checks here? If not, what would be the cheapest way to calculate block_weight here?
Comment on lines
+1010
to
1020
| /// <https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454/src/rpc/core_rpc_server.cpp#L3542-L3551> | ||
| async fn flush_cache( | ||
| state: CupratedRpcHandler, | ||
| request: FlushCacheRequest, | ||
| ) -> Result<FlushCacheResponse, Error> { | ||
| todo!() | ||
| // TODO: cuprated doesn't need this call; decide behavior. | ||
|
|
||
| Ok(FlushCacheResponse { | ||
| base: ResponseBase::OK, | ||
| }) | ||
| } |
There was a problem hiding this comment.
We could remove this such that calling flush_cache errors instead.
Comment on lines
+1173
to
+1213
| fn tree_hash(aux_pow_raw: &[[u8; 32]]) -> [u8; 32] { | ||
| todo!("https://github.com/serai-dex/serai/pull/629") | ||
| } | ||
|
|
||
| fn encode_mm_depth(aux_pow_len: usize, nonce: u32) -> u64 { | ||
| todo!("https://github.com/monero-project/monero/blob/893916ad091a92e765ce3241b94e706ad012b62a/src/cryptonote_basic/merge_mining.cpp#L74") | ||
| } | ||
|
|
||
| let merkle_root = tree_hash(aux_pow_raw.as_ref()); | ||
| let merkle_tree_depth = encode_mm_depth(len, nonce); | ||
|
|
||
| let block_template = { | ||
| let hex = hex::decode(request.blocktemplate_blob)?; | ||
| Block::read(&mut hex.as_slice())? | ||
| }; | ||
|
|
||
| fn remove_field_from_tx_extra() -> Result<(), ()> { | ||
| todo!("https://github.com/monero-project/monero/blob/master/src/cryptonote_basic/cryptonote_format_utils.cpp#L767") | ||
| } | ||
|
|
||
| if remove_field_from_tx_extra().is_err() { | ||
| return Err(anyhow!("Error removing existing merkle root")); | ||
| } | ||
|
|
||
| fn add_mm_merkle_root_to_tx_extra() -> Result<(), ()> { | ||
| todo!("https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454/src/rpc/core_rpc_server.cpp#L2189 | ||
| ") | ||
| } | ||
|
|
||
| if add_mm_merkle_root_to_tx_extra().is_err() { | ||
| return Err(anyhow!("Error adding merkle root")); | ||
| } | ||
|
|
||
| fn invalidate_hashes() { | ||
| // block_template.invalidate_hashes(); | ||
| // block_template.miner_tx.invalidate_hashes(); | ||
| // <https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454/src/rpc/core_rpc_server.cpp#L2195-L2196> | ||
| todo!(); | ||
| } | ||
|
|
||
| invalidate_hashes(); |
There was a problem hiding this comment.
This section has some missing functions, any suggestions for where or if these should be implemented?
Comment on lines
+174
to
+186
| pub(super) async fn key_image_spent( | ||
| state: &mut CupratedRpcHandler, | ||
| key_image: [u8; 32], | ||
| ) -> Result<KeyImageSpentStatus, Error> { | ||
| todo!("impl key image vec check responding KeyImageSpentStatus") | ||
| // if blockchain::key_image_spent(state, key_image).await? { | ||
| // Ok(KeyImageSpentStatus::SpentInBlockchain) | ||
| // } else if todo!("key image is spent in tx pool") { | ||
| // Ok(KeyImageSpentStatus::SpentInPool) | ||
| // } else { | ||
| // Ok(KeyImageSpentStatus::Unspent) | ||
| // } | ||
| } |
There was a problem hiding this comment.
TODO: not JSON-RPC but /is_key_image_spent requires something like BlockchainReadRequest::KeyImagesSpent(Vec<[u8; 32]>) -> BlockchainResponse::KeyImagesSpent(Vec<KeyImageSpentStatus>)
Comment on lines
+36
to
+38
| // TODO: if the request block is not on the main chain, | ||
| // we must get the alt block and this variable will be `true`. | ||
| let orphan_status = false; |
There was a problem hiding this comment.
Alt blocks need to be handled here, is there a cheap way this could be done?
|
Merged into #355. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Implements the JSON-RPC handlers in
cuprated; adds various types and changes some misc things as needed.