cuprated: RPC handlers by hinto-janai · Pull Request #355 · Cuprate/cuprate

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.

types/types/src/blockchain.rs

storage/blockchain/src/service/read.rs

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

@Boog900 ready for review again, although WASM CI for cuprate-rpc-types still needs fixing.

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

Boog900 · 2025-03-08T19:12:15Z

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

+async fn get_blocks(
+    mut state: CupratedRpcHandler,
+    request: GetBlocksRequest,
+) -> Result<GetBlocksResponse, Error> {


putting the comment here for a lack of a better place, but because the request type uses Bytes internally it is good to drop it as soon as you can.

What this means is we should extract all the data needed out of the request before doing any async/blocking operations

Boog900 · 2025-03-08T19:28:23Z

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

+
+    let block_hashes: Vec<[u8; 32]> = (&request.block_ids).into();
+
+    let (blocks, missing_hashes, blockchain_height) =
+        blockchain::block_complete_entries(&mut state.blockchain_read, block_hashes).await?;
+
+    if !missing_hashes.is_empty() {
+        return Err(anyhow!("Missing blocks"));
+    }


this seems to be incorrect, monerod seems to be getting the next hashes in the chain, using the incoming hashes as a representation of the clients current chain, and giving the blocks for those future hashes instead: https://github.com/monero-project/monero/blob/cc73fe71162d564ffda8e549b79a350bca53c454/src/rpc/core_rpc_server.cpp#L733

Boog900 · 2025-03-08T20:00:55Z

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"))?;


@hinto-janai can you add a link to this comment in the code please.

Boog900 · 2025-03-08T20:08:03Z

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

+/// [`cuprate_types::blockchain::BlockchainResponse::ChainHeight`] minus 1.
+pub(super) async fn top_height(state: &mut CupratedRpcHandler) -> Result<(u64, [u8; 32]), Error> {
+    let (chain_height, hash) = blockchain::chain_height(&mut state.blockchain_read).await?;
+    let height = chain_height.saturating_sub(1);


I would rather this just be a normal subtraction/checked with unwrap. Saturating is confusing IMO

Boog900 · 2025-03-08T20:20:30Z

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

+    }
+
+    let too_many_blocks = || {
+        request.end_height.saturating_sub(request.start_height) + 1 > RESTRICTED_BLOCK_HEADER_RANGE


I don't think this needs to be saturating_sub here

Boog900 · 2025-03-08T21:08:20Z

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

+    // This method can be a bit heavy, so rate-limit restricted use.
+    if state.is_restricted() {
+        tokio::time::sleep(Duration::from_millis(100)).await;
+    }


This IMO is not a great way to do rate limiting, it should be handled with a Semaphore or even better just rate restrict the whole RPC interface. When we have performance characteristics we can look at rate limiting further.

Agreed, I should have noted that this is temporary. I don't think this endpoint is high priority, is a FIXME for later ok?

it should be handled with a Semaphore or even better just rate restrict the whole RPC interface.

I would ideally prefer this to be a Layer as I would like tower-governor to be implemented at some point. So the whole RPC interface is better.

is a FIXME for later ok

yes

Boog900 · 2025-03-08T21:37:09Z

types/types/src/json/tx.rs

+impl EpeeObjectBuilder<Transaction> for () {
+    fn add_field<B: Buf>(&mut self, _: &str, _: &mut B) -> error::Result<bool> {
+        unreachable!()
+    }
+
+    fn finish(self) -> error::Result<Transaction> {
+        unreachable!()
+    }
+}
+
+#[cfg(feature = "epee")]
+impl EpeeObject for Transaction {
+    type Builder = ();
+
+    fn number_of_fields(&self) -> u64 {
+        unreachable!()
+    }
+
+    fn write_fields<B: BufMut>(self, _: &mut B) -> error::Result<()> {
+        unreachable!()
+    }
+}


is this a TODO?

No, this is a cuprate_types::json type, it only has epee impl due to:

cuprate/rpc/types/README.md

Lines 83 to 88 in 3c86c5e

# (De)serialization invariants

Due to how types are defined in this library internally (all through a single macro),

most types implement both `serde` and `epee`.

However, some of the types will panic with [`unimplemented`]

or will otherwise have undefined implementation in the incorrect context.

AFAICT only the JSON representation for Transaction is used in the (daemon) RPC API.

Added some docs on this.

Boog900 · 2025-03-08T22:11:12Z

storage/blockchain/src/ops/output.rs

+        let height = u32_to_usize(rct_output.height);
+        let tx_idx = u64_to_usize(rct_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()
+        }


I am pretty sure this isn't correct the get(tx_idx) should use th local index of the tx in the block, right? this currently used the global index.

Suggested change

let height = u32_to_usize(rct_output.height);

let tx_idx = u64_to_usize(rct_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()

}

let height = u32_to_usize(rct_output.height);

let tx_idx = u64_to_usize(rct_output.tx_idx);

let miner_tx_id = table_block_infos.get(&height)?.mining_tx_index;

if miner_tx_id == tx_idx {

let tx_blob = table_tx_blobs.get(&miner_tx_id)?;

Transaction::read(&mut tx_blob.0.as_slice())?.hash()

} else {

table_block_txs_hashes.get(&height)?[tx_idx - miner_tx_id - 1)]

}

Boog900 · 2025-03-08T22:12:18Z

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,
    })


Yes I think it should be, this is called ot get outputs when syncing so hashing miner txs/extra lookups is just wasted work.

Boog900 · 2025-03-08T22:23:01Z

rpc/types/src/other.rs


    SendRawTransaction,


I think the line numbers above this are incorrect: https://github.com/monero-project/monero/blob/8d6aff95908e029d6b131638fbbf845e8cff04fc/src/rpc/core_rpc_server_commands_defs.h#L631-L683

hinto-janai · 2025-03-27T01:09:22Z

helper/src/cast.rs

+//! # 64-bit invariant
+//! This module is available on 32-bit arches although panics
+//! will occur between lossy casts, e.g. [`u64_to_usize`] where
+//! the input is larger than [`u32::MAX`].
+//!
+//! On 64-bit arches, all functions are lossless.
+
+// TODO:
+// These casting functions are heavily used throughout the codebase
+// yet it is not enforced that all usages are correct in 32-bit cases.
+// Panicking may be a short-term solution - find a better fix for 32-bit arches.


The issue for wasm32-unknown-unknown builds was:

cuprate_rpc_types depends on cuprate_types with json feature

json feature requires cuprate_helper::cast

To fix this, I am making cuprate_helper::cast work on 32-bit targets, yet panic when the cast is lossy. I don't think this is the correct long-term solution but it does work for now.

Boog900 · 2025-04-08T16:09:23Z

Approved as this has undergone enough review + an extensive test suite is being worked on (#422) which should catch any mismatches in behavior. I would like that to be completed before any integration in cuprated, I think that is already the plan but just to make my opinion clear.

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 6 commits January 17, 2025 14:40

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

hinto-janai added 2 commits January 29, 2025 15:27

Merge branch 'main' into other

ecc21d2

output timelock fix + blockchain_context()

e6efdbb

Boog900 requested changes Mar 10, 2025

View reviewed changes

hinto-janai added 11 commits March 25, 2025 19:44

Merge branch 'main' into other

366c4a3

fix

d8f12ef

fix

d18a9da

fix

986c5c4

fix getblocks.bin

babd82e

cuprate_types doc

9009fa1

output fix

373c890

fixme

9131e91

rct output fix

daa6f63

fix cast

b87bddf

clippy

8ac347d

hinto-janai commented Mar 27, 2025

View reviewed changes

hinto-janai requested a review from Boog900 March 27, 2025 01:24

Boog900 mentioned this pull request Apr 8, 2025

32-bit potential panics #429

Open

Boog900 approved these changes Apr 8, 2025

View reviewed changes

Boog900 merged commit d3b7ca3 into Cuprate:main Apr 8, 2025
12 checks passed

hinto-janai deleted the other branch April 8, 2025 20:33

hinto-janai mentioned this pull request Jun 6, 2025

storage: fix get_txid for pre-RCT outputs #504

Merged

	/// 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 {

	# (De)serialization invariants
	Due to how types are defined in this library internally (all through a single macro),
	most types implement both `serde` and `epee`.

	However, some of the types will panic with [`unimplemented`]
	or will otherwise have undefined implementation in the incorrect context.

Conversation

hinto-janai commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Boog900 commented Apr 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hinto-janai commented Dec 5, 2024 •

edited

Loading