cuprated: RPC handlers #355

hinto-janai · 2024-12-05T01:45:06Z

What

Implements the {json-rpc, binary, json} handlers in cuprated; adds various types and changes some misc things as needed.

How

See below review.

hinto-janai

@Boog900 I think this mostly needs review on:

general direction and broader changes
viability of new tower::Service request/response types

I don't think you have to review each handler fn individually in-depth since there will eventually be the testing harness that proves input/output matches monerod. With that said, here's some info:

RPC code structure

Directory	Contains
`binaries/cuprated/src/rpc/handlers/`	The RPC handler functions, shared functions, and helper functions
`binaries/cuprated/src/rpc/service/`	`fn` versions of our `tower::Service`s to reduce noise

RPC handlers

Color	Meaning
🟢	Ready
🟡	Ready but could be more efficient
🟣	Ready but callstack depends on other things
🟠	Depends on other things
🔴	Waiting on binary strings
🔵	Unsupported (for now)
⚪	Unsupported (probably forever)
⚫	I think these could be deprecated or have different behavior

JSON-RPC `fn`	Status	Details
get_block_count	🟢
get_last_block_header	🟢
get_block_header_by_hash	🟢
get_block_header_by_height	🟢
get_block	🟢
hard_fork_info	🟢
on_get_block_hash	🟢
get_block_headers_range	🟡	Would benefit from a request that allows retrieving a range of `(Block, ExtendedBlockHeader)`
get_connections	🟣	Waiting on address book `Service` impl
set_bans	🟣	^
get_bans	🟣	^
banned	🟣	^
get_version	🟣	Waiting on blockchain context `Service` impl
get_output_histogram	🟣	^
get_fee_estimate	🟣	^
calc_pow	🟣	^
flush_transaction_pool	🟣	Waiting on txpool manager `Service` impl
relay_tx	🟣	^
get_coinbase_tx_sum	🟣	Waiting on blockchain `Service` impl
get_alternate_chains	🟣	^
sync_info	🟣	Waiting on multiple `Service` impls
get_miner_data	🟣	Waiting on txpool `Service` impl
submit_block	🟣	^
get_info	🟣	Needs access to unimplemented things from various places
generate_blocks	🟣	Needs access to `cuprated`'s `Chain` and `--regtest`
add_aux_pow	🟣	Waiting on crypto functions
get_transaction_pool_backlog	🔴
get_output_distribution	🔴
get_tx_ids_loose	🔵	Not implemented in `monerod` release branch yet
flush_cache	⚪	`cuprated` does not need this
prune_blockchain	⚫	I don't think an always available RPC method is necessary for something that is done once. This could return if the chain is pruned + the pruning seed but not actually prune if that makes things more complex. Pruning itself should be done with the equivalent of `--prune-blockchain`.

Other JSON `fn`	Status	Details
get_height	🟢
get_outs	🟢
is_key_image_spent	🟣	Waiting on txpool `Service` impl
get_transaction_pool_hashes	🟣	^
get_transaction_pool	🟣	^
get_transaction_pool_stats	🟣	^
save_bc	🟣	Waiting on blockchain manager `Service` impl
stop_daemon	🟣	^
pop_blocks	🟣	^
get_peer_list	🟣	Waiting on address book `Service` impl
get_public_nodes	🟣	^
get_alt_blocks_hashes	🟣	Waiting on blockchain `Service` impl
send_raw_transaction	🟣	Waiting on txpool manager `Service` impl
get_transactions	🟣	Waiting on JSON representation of `Transaction`
get_limit	🟠	Waiting on P2P interface
set_limit	🟠	^
out_peers	🟠	^
in_peers	🟠	^
get_net_stats	🟠	^
set_log_level	🔵	Will use `tracing` levels
set_log_categories	🔵	Could be re-purposed to use `tracing` filters
set_bootstrap_daemon	🔵	Needs bootstrap implementation
start_mining	⚪	`cuprated` does not mine
stop_mining	⚪	^
mining_status	⚪	^
set_log_hash_rate	⚪	^
update	⚫	This could return if an update is available and related info but not actually self-update. Software upgrades should be done by the user and/or a package manager.

Binary `fn`	Status	Details
get_blocks_by_height	🟢
get_hashes	🟢
get_output_indexes	🟢
get_outs	🟢
get_blocks	🟣	Waiting on txpool `Service` impl
get_transaction_pool_hashes	🟣	^
get_output_distribution	🔴	Although this is binary, the internal fn is shared with JSON-RPC's `get_output_distribution`

Cargo.toml

hinto-janai · 2024-12-25T19:40:04Z

rpc/types/src/macros.rs

@@ -291,6 +291,7 @@ macro_rules! define_response {
        }
    ) => {
        $( #[$attr] )*
+        #[cfg_attr(feature = "serde", serde(default))] // TODO: link epee field not serializing oddity


For the time being, I'm making response types in cuprate-rpc-types have serde(default) since:

monerod will not serialize fields with empty containers

monerod's response types sometimes change depending on branches

serde(default) is a bit of a blunt tool to use but I think it works for now. I will think about this more when integrating RPC into cuprated and with more tests.

As I said in the other issue I am ok with this for now, I don't like the idea of this being permanent though

types/types/src/rpc/types.rs

rpc/types/src/from.rs

rpc/types/src/json.rs

binaries/cuprated/src/rpc/handlers/json_rpc.rs

helper/src/fmt.rs

hinto-janai · 2024-12-25T21:37:53Z

storage/blockchain/src/ops/output.rs

+    let txid = {
+        let height = u32_to_usize(output.height);
+        let tx_idx = u64_to_usize(output.tx_idx);
+        if let Some(hash) = table_block_txs_hashes.get(&height)?.get(tx_idx) {
+            *hash
+        } else {
+            let miner_tx_id = table_block_infos.get(&height)?.mining_tx_index;
+            let tx_blob = table_tx_blobs.get(&miner_tx_id)?;
+            Transaction::read(&mut tx_blob.0.as_slice())?.hash()
+        }
+    };
+
    Ok(OutputOnChain {
        height: output.height as usize,
        time_lock,
        key,
        commitment,
+        txid,
    })


This function now returns the txid for each output due to https://www.getmonero.org/resources/developer-guides/daemon-rpc.html#get_outs.

This is more expensive, the txid is not required by default as well. Should this be something like txid: Option<[u8; 32]> instead?

hinto-janai · 2024-12-25T21:43:27Z

storage/blockchain/src/service/read.rs

-fn key_images_spent(env: &ConcreteEnv, key_images: HashSet<KeyImage>) -> ResponseResult {
+fn key_images_spent(env: &ConcreteEnv, key_images: Vec<KeyImage>) -> ResponseResult {


This was changed to Vec due to https://www.getmonero.org/resources/developer-guides/daemon-rpc.html#is_key_image_spent behavior.

curl http://127.0.0.1:18081/is_key_image_spent -d '{"key_images":["8d1bd8181bf7d857bdb281e0153d84cd55a3fcaa57c3e570f4a49f935850b5e3","8d1bd8181bf7d857bdb281e0153d84cd55a3fcaa57c3e570f4a49f935850b5e3"]}' -H 'Content-Type: application/json'

{ "credits": 0, "spent_status": [1,1], "status": "OK", "top_hash": "", "untrusted": false }

If cuprated used HashSet then it would respond with something like "spent_status": [1]. Small detail but I think it could matter with wallets due to indexing code depending on this.

I kinda want to say this should be a separate request, I can imagine it being more efficient to check if any one KI is spent rather than always needing to check every KIs spent status in the blockchain.

#355 (comment)

Boog900

Just leaving the review in the current state will probably do another pass. Looks good so far but there are defiantly areas where we could improve performance when the inner services get the required requests filled in.

Boog900 · 2025-01-06T18:36:36Z

binaries/cuprated/src/rpc/handlers/bin.rs

+    let Some((index, start_height)) =
+        blockchain::find_first_unknown(&mut state.blockchain_read, hashes).await?
+    else {
+        return Err(anyhow!("Failed"));
+    };
+
+    let m_blocks_ids = bytes.split_off(index);
+
+    Ok(GetHashesResponse {
+        base: helper::access_response_base(false),
+        m_blocks_ids,
+        start_height,
+        current_height,
+    })


this seems to only be doing part of the request - finding the split point, we also need to give the next hashes in the chain back. This can be all done with BlockchainReadRequest::NextChainEntry

Boog900 · 2025-01-06T18:43:42Z

binaries/cuprated/src/rpc/handlers/helper.rs

+    // FIXME: is there a cheaper way to get this?
+    let difficulty = blockchain_context::batch_get_difficulties(
+        &mut state.blockchain_context,
+        vec![(height, hardfork)],
+    )
+    .await?
+    .first()
+    .copied()
+    .ok_or_else(|| anyhow!("Failed to get block difficulty"))?;


// FIXME: is there a cheaper way to get this?

yes - get the cumulative difficulty of this block take away the cumulative difficulty of the block before it will give you the difficulty of this block. Cumulative difficulty should be in the extended header. Also this current method is incorrect as this request takes in a timestamp not height.

Separate but related: I think we should create a specific DB request for as much data as we can as we currently need to call the service multiple times for this request.

This can all be a TODO, no need to do in this PR.

Boog900 · 2025-01-06T19:10:51Z

binaries/cuprated/src/rpc/handlers/json_rpc.rs

+
+    // TODO: this is hardcoded for the current address scheme + mainnet,
+    // create/use a more well-defined wallet lib.
+    let parse_wallet_address = || {
+        if request.wallet_address.len() == 95 {
+            Ok(())
+        } else {
+            Err(())
+        }
+    };
+    let is_correct_address_type = || !request.wallet_address.starts_with("4");


I recommend monero-address: https://github.com/serai-dex/serai/tree/develop/networks/monero/wallet/address

Boog900 · 2025-01-08T02:37:56Z

binaries/cuprated/src/rpc/handlers/json_rpc.rs

+    mut state: CupratedRpcHandler,
+    request: GetBlockHeaderByHeightRequest,
+) -> Result<GetBlockHeaderByHeightResponse, Error> {
+    helper::check_height(&mut state, request.height).await?;


As a TODO, this check should be done in the DB to prevent needing to do multiple DB requests.

(there are lots of places where we could improve performance, could just be left as a wider todo)

Boog900 · 2025-01-08T02:54:04Z

binaries/cuprated/src/rpc/handlers/json_rpc.rs

+    let block_weight_limit = usize_to_u64(c.effective_median_weight);
+    let block_weight_median = usize_to_u64(c.median_weight_for_block_reward);


Suggested change

let block_weight_limit = usize_to_u64(c.effective_median_weight);

let block_weight_median = usize_to_u64(c.median_weight_for_block_reward);

let block_weight_limit = usize_to_u64(c.effective_median_weight * 2);

let block_weight_median = usize_to_u64(c.effective_median_weight);

that matches monerod behavior but technically it should be this:

Suggested change

let block_weight_limit = usize_to_u64(c.effective_median_weight);

let block_weight_median = usize_to_u64(c.median_weight_for_block_reward);

let block_weight_limit = usize_to_u64(c.median_weight_for_block_reward * 2);

let block_weight_median = usize_to_u64(c.median_weight_for_block_reward);

Both will be the same for the current HF although past HFs these values are different. Should we copy monerod's bad behavior here?

Copy for now, submit patch to monerod eventually, then update cuprated?

Boog900 · 2025-01-11T02:22:32Z

binaries/cuprated/src/rpc/handlers/json_rpc.rs

+    let major_version = c.current_hf.as_u8();
+    let height = usize_to_u64(c.chain_height);
+    let prev_id = Hex(c.top_hash);
+    let seed_hash = Hex(c.top_hash);


This is the Rx seed hash, it'll have to be a todo, I'll add it to the context in a future PR.

Boog900 · 2025-01-11T02:25:22Z

binaries/cuprated/src/rpc/handlers/json_rpc.rs

+    let prev_id = Hex(c.top_hash);
+    let seed_hash = Hex(c.top_hash);
+    let difficulty = c.next_difficulty.hex_prefix();
+    let median_weight = usize_to_u64(c.median_weight_for_block_reward);


Same as the other comment, this is actually the correct value but if we are trying to match Monero, the value should be effective_median_weight.

Boog900 · 2025-01-11T02:29:42Z

binaries/cuprated/src/rpc/handlers/json_rpc.rs

+        tokio::time::sleep(Duration::from_millis(100)).await;
+    }
+
+    tokio::task::spawn_blocking(|| add_aux_pow_inner(state, request)).await?


this should spawn on a rayon thread, using rayon_spawn_async

Boog900 · 2025-01-11T02:41:02Z

binaries/cuprated/src/rpc/handlers/other_json.rs

+    blockchain::key_images_spent(&mut state.blockchain_read, key_images.clone())
+        .await?
+        .into_iter()
+        .for_each(|ki| {
+            if ki {
+                spent_status.push(KeyImageSpentStatus::SpentInBlockchain.to_u8());
+            } else {
+                spent_status.push(KeyImageSpentStatus::Unspent.to_u8());
+            }
+        });
+
+    txpool::key_images_spent(&mut state.txpool_read, key_images, !restricted)
+        .await?
+        .into_iter()
+        .for_each(|ki| {
+            if ki {
+                spent_status.push(KeyImageSpentStatus::SpentInPool.to_u8());
+            } else {
+                spent_status.push(KeyImageSpentStatus::Unspent.to_u8());
+            }
+        });


This seems to be returning a list with double the length of the key images? I don't think this is correct. We should have a list the exact same length right

Boog900 · 2025-01-14T01:43:48Z

binaries/cuprated/src/rpc/handlers/shared.rs

+            index_map.into_values().map(|out| OutKeyBin {
+                key: out.key.map_or([0; 32], |e| e.compress().0),
+                mask: out.commitment.compress().0,
+                unlocked: matches!(out.time_lock, Timelock::None),


this should return true even if the output is locked but the lock time has passed right?

#355 (comment)

Co-authored-by: Boog900 <boog900@tutanota.com>

types/types/src/blockchain.rs

hinto-janai · 2025-01-17T19:48:48Z

storage/blockchain/src/service/read.rs

+/// [`BlockchainReadRequest::KeyImagesSpentVec`].
+fn key_images_spent_vec(env: &ConcreteEnv, key_images: Vec<KeyImage>) -> ResponseResult {
+    // Prepare tx/tables in `ThreadLocal`.
+    let env_inner = env.env_inner();
+    let tx_ro = thread_local(env);
+    let tables = thread_local(env);
+
+    // Key image check function.
+    let key_image_exists = |key_image| {
+        let tx_ro = tx_ro.get_or_try(|| env_inner.tx_ro())?;
+        let tables = get_tables!(env_inner, tx_ro, tables)?.as_ref();
+        key_image_exists(&key_image, tables.key_images())
+    };
+
+    // Collect results using `rayon`.
+    Ok(BlockchainResponse::KeyImagesSpentVec(
+        key_images
+            .into_par_iter()
+            .map(key_image_exists)
+            .collect::<DbResult<_>>()?,
+    ))
+}


3/3

DB operation, same as before just added _vec.

hinto-janai · 2025-01-17T19:51:59Z

binaries/cuprated/src/rpc/handlers/shared.rs

+    for (_, index_vec) in outputs {
+        for (_, out) in index_vec {
+            let out_key = OutKeyBin {
+                key: out.key.map_or([0; 32], |e| e.compress().0),
+                mask: out.commitment.compress().0,
+                unlocked: helper::timelock_is_unlocked(&mut state, out.time_lock).await?,
+                height: usize_to_u64(out.height),
+                txid: if request.get_txid { out.txid } else { [0; 32] },
+            };
+
+            outs.push(out_key);
+        }
+    }


1/2

Fixed timelock check, although now it is for due to async.

TODO: optimized async iterators?

Outputs unlock time is not just done by looking at the timestamp/block height, it has specific rules, here is the function in Cuprate:

cuprate/consensus/rules/src/transactions.rs

Lines 220 to 223 in 9842535

/// Checks if an outputs unlock time has passed.

///

/// <https://monero-book.cuprate.org/consensus_rules/transactions/unlock_time.html>

pub const fn output_unlocked(

You will need to get the blockchian context for the time for unlock & chain height:

cuprate/consensus/context/src/lib.rs

Lines 138 to 141 in 9842535

/// Returns the timestamp the should be used when checking locked outputs.

///

/// ref: <https://cuprate.github.io/monero-book/consensus_rules/transactions/unlock_time.html#getting-the-current-time>

pub fn current_adjusted_timestamp_for_time_lock(&self) -> u64 {

That only has to be done once, you could reuse the context for each output

hinto-janai · 2025-01-17T19:52:17Z

binaries/cuprated/src/rpc/handlers/helper.rs

+/// Returns `true` if the `timelock` is unlocked.
+pub(super) async fn timelock_is_unlocked(
+    state: &mut CupratedRpcHandler,
+    timelock: Timelock,
+) -> Result<bool, Error> {
+    let unlocked = match timelock {
+        Timelock::None => true,
+        Timelock::Block(height) => {
+            let (top_height, _) = top_height(state).await?;
+            top_height > usize_to_u64(height)
+        }
+        Timelock::Time(timestamp) => cuprate_helper::time::current_unix_timestamp() > timestamp,
+    };
+
+    Ok(unlocked)
+}


2/2

Timelock check function.

Maybe TODO: should this be a BlockchainReadRequest? I feel like this could be used elsewhere.

hinto-janai added 30 commits October 9, 2024 20:47

import diffs

d170b46

small fixes, hardfork changes

02e950b

lints

c18ba6e

hard_fork

93be3be

apply diffs

c590bbb

review fixes

cec746b

Merge branch 'hf' into json

e57c887

Merge branch 'main' into json

ac6360a

binaries/cuprated/src/rpc/request: pub(super) -> pub(crate)

2522bb8

add BlockChainContextService, on_get_block_hash

fd136b2

Merge branch 'main' into json

9459d59

map tower::BoxError to anyhow::Error

b82ff27

get_block

8a3229b

connection_info

9422c07

hard_fork_info

50bd576

set_bans

f677b1a

get_bans

37bcbb5

banned

c7ae795

flush_transaction_pool

6af977f

get_output_histogram

1a76934

get_coinbase_tx_sum

1d435cc

get_version

d7fef15

get_fee_estimate

1e0ab56

get_alternate_chains

294df3f

relay_tx

08b5ff2

response_base: fn -> const

2daf255

get_transaction_pool_backlog

a10ed59

prune

d7d8b35

calc_pow

5926f24

add_aux_pow

7148ba1

hinto-janai added 7 commits December 19, 2024 18:42

fix conflict

114a5db

json-rpc fixes

b2b7e93

get_transaction_pool_hashes fix

d41014b

rpc/interface: fix cargo hack

ecf5d66

review fixes

85ebc67

Merge branch 'main' into other

04ade52

cargo hack fix

c56ecd3

hinto-janai mentioned this pull request Dec 25, 2024

cuprated: RPC handlers (temporary draft) #272

Closed

hinto-janai commented Dec 25, 2024

View reviewed changes

hinto-janai marked this pull request as ready for review December 25, 2024 23:51

hinto-janai requested a review from Boog900 December 25, 2024 23:51

hinto-janai mentioned this pull request Jan 7, 2025

rpc/types: add missing defaults to fields #361

Merged

Boog900 requested changes Jan 14, 2025

View reviewed changes

Merge branch 'main' into other

1c3f1b1

hinto-janai mentioned this pull request Jan 16, 2025

book/architecture: RPC handlers and related #341

Draft

hinto-janai and others added 13 commits January 16, 2025 11:44

Merge branch 'main' into other

13f5ed9

use monero_address

1d3c807

Update binaries/cuprated/src/rpc/handlers/json_rpc.rs

bc6fc0a

Co-authored-by: Boog900 <boog900@tutanota.com>

Update binaries/cuprated/src/rpc/handlers/json_rpc.rs

c7443c9

Co-authored-by: Boog900 <boog900@tutanota.com>

Merge branch 'other' of github.com:hinto-janai/cuprate into other

4118af2

review fixes

0b3ff4e

fix get_hashes

78b3ff1

fix is_key_image_spent

95ab976

fix key image types

416efcb

fixes

2c32e02

Merge branch 'main' into other

405db20

Merge branch 'main' into other

783ca05

fix book

384ac54

hinto-janai commented Jan 17, 2025

View reviewed changes

hinto-janai requested a review from Boog900 January 17, 2025 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cuprated: RPC handlers #355

cuprated: RPC handlers #355

hinto-janai commented Dec 5, 2024 •

edited

Loading

hinto-janai left a comment

hinto-janai Dec 25, 2024

Boog900 Jan 14, 2025

hinto-janai Dec 25, 2024

hinto-janai Dec 25, 2024

Boog900 Jan 14, 2025

hinto-janai Jan 17, 2025

Boog900 left a comment

Boog900 Jan 6, 2025

Boog900 Jan 6, 2025 •

edited

Loading

Boog900 Jan 6, 2025

Boog900 Jan 8, 2025

Boog900 Jan 14, 2025

Boog900 Jan 8, 2025

hinto-janai Jan 17, 2025

Boog900 Jan 11, 2025

Boog900 Jan 11, 2025

Boog900 Jan 11, 2025

Boog900 Jan 11, 2025

Boog900 Jan 14, 2025

hinto-janai Jan 17, 2025

hinto-janai Jan 17, 2025

hinto-janai Jan 17, 2025

Boog900 Jan 22, 2025

hinto-janai Jan 17, 2025

		fn key_images_spent(env: &ConcreteEnv, key_images: HashSet<KeyImage>) -> ResponseResult {
		fn key_images_spent(env: &ConcreteEnv, key_images: Vec<KeyImage>) -> ResponseResult {

		let block_weight_limit = usize_to_u64(c.effective_median_weight);
		let block_weight_median = usize_to_u64(c.median_weight_for_block_reward);

	/// Checks if an outputs unlock time has passed.
	///
	/// <https://monero-book.cuprate.org/consensus_rules/transactions/unlock_time.html>
	pub const fn output_unlocked(

	/// Returns the timestamp the should be used when checking locked outputs.
	///
	/// ref: <https://cuprate.github.io/monero-book/consensus_rules/transactions/unlock_time.html#getting-the-current-time>
	pub fn current_adjusted_timestamp_for_time_lock(&self) -> u64 {

cuprated: RPC handlers #355

Are you sure you want to change the base?

cuprated: RPC handlers #355

Conversation

hinto-janai commented Dec 5, 2024 • edited Loading

What

How

hinto-janai left a comment

Choose a reason for hiding this comment

RPC code structure

RPC handlers

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

Boog900 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Boog900 Jan 6, 2025 • edited Loading

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

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

hinto-janai commented Dec 5, 2024 •

edited

Loading

Boog900 Jan 6, 2025 •

edited

Loading