cuprated: internal signatures required for RPC #297

hinto-janai · 2024-09-23T22:42:02Z

What

Adds the {function, request, response} signatures needed by cuprated's RPC system to the {blockchain, txpool, p2p, blockchain_context} APIs.

Closes #279.

This covers most (but not all) signatures needed for all RPC requests (JSON-RPC, binary, other).

hinto-janai

This review is split like such:

Title	Purpose
Misc	Miscellaneous changes added and/or needed in the future
Signatures	New requests/responses added to our `tower::Service`s
Types	New types added (for usage in the request/responses)
TODO	Things to do after this PR

Questions that need answering:

Are any of the requests/responses impossible to fulfill?
Can any of the requests/responses be done in a better way?
Do any of the requests/responses belong to a different Service?

binaries/cuprated/src/rpc/handler.rs

binaries/cuprated/src/rpc/request.rs

consensus/src/context.rs

hinto-janai · 2024-10-03T00:29:02Z

storage/blockchain/src/service/mod.rs

-//! The `DatabaseReadHandle` can be shared as it is cheaply [`Clone`]able, however,
-//! the `DatabaseWriteHandle` cannot be cloned. There is only 1 place in Cuprate that
-//! writes, so it is passed there and used.
-//!


Misc

Not true anymore, we need Clone on CupratedRpcHandler and thus the DatabaseWriteHandle.

This does need to be removed for the tx-pool handle, however the database is still only written to in 1 place, the blockchain manager.

You shouldn't need to hold a write handle to the blockchain in the RPC

hinto-janai · 2024-10-03T00:33:20Z

binaries/cuprated/src/rpc/request/address_book.rs

+/// [`AddressBookRequest::PeerlistSize`]
+pub(super) async fn peerlist_size<Z: NetworkZone>(
+    address_book: &mut impl AddressBook<Z>,
+) -> Result<(u64, u64), Error> {
+    let AddressBookResponse::PeerlistSize { white, grey } = address_book
+        .ready()
+        .await
+        .expect("TODO")
+        .call(AddressBookRequest::PeerlistSize)
+        .await
+        .expect("TODO")
+    else {
+        unreachable!();
+    };
+
+    Ok((usize_to_u64(white), usize_to_u64(grey)))
+}


Misc

The services that use tower::BoxError don't work with anyhow::Error when using ?, can we swap them to anyhow?

The services are:

address book

blockchain context

transaction pool

Yeah, there are other as well, probably should be done in a separate PR

hinto-janai · 2024-10-03T00:44:10Z

p2p/p2p-core/src/ban.rs

+//! Data structures related to bans.
+
+use std::time::{Duration, Instant};
+
+use crate::NetZoneAddress;
+
+/// Data within [`crate::services::AddressBookRequest::SetBan`].
+pub struct SetBan<A: NetZoneAddress> {
+    pub address: A,
+    pub ban: bool,
+    pub duration: Duration,
+}
+
+/// Data within [`crate::services::AddressBookResponse::GetBans`].
+pub struct BanState<A: NetZoneAddress> {
+    pub address: A,
+    pub banned: bool,
+    pub unban_instant: Instant,
+}


Types (address book)

Some of the requests/responses in this PR require defining some types.

A lot of these are 1-1 with the RPC types but I decided on making more specific versions because:

Pulling in cuprate-rpc-types everywhere seems wrong

The RPC types are JSON-like, so a [u8; 32] field will be something like String, adding these types lets us use the canonical type internally

Some of the types are defined in the crate where the request/response is defined, many are defined in cuprate-types.

Yeah separating them is a good idea IMO

Those types seem weird though, wouldn't this make more sense:

Suggested change

//! Data structures related to bans.

use std::time::{Duration, Instant};

use crate::NetZoneAddress;

/// Data within [`crate::services::AddressBookRequest::SetBan`].

pub struct SetBan<A: NetZoneAddress> {

pub address: A,

pub ban: bool,

pub duration: Duration,

}

/// Data within [`crate::services::AddressBookResponse::GetBans`].

pub struct BanState<A: NetZoneAddress> {

pub address: A,

pub banned: bool,

pub unban_instant: Instant,

}

//! Data structures related to bans.

use std::time::{Duration, Instant};

use crate::NetZoneAddress;

/// Data within [`crate::services::AddressBookRequest::SetBan`].

pub struct SetBan<A: NetZoneAddress> {

pub address: A,

pub ban: Option<Duration>,

}

/// Data within [`crate::services::AddressBookResponse::GetBans`].

pub struct BanState<A: NetZoneAddress> {

pub address: A,

pub banned: Option<Instant>,

}

hinto-janai · 2024-10-03T00:44:52Z

storage/txpool/src/tx.rs

+//! Transaction metadata.
+
+/// Data about a transaction in the pool.
+///
+/// Used in [`TxpoolReadResponse::Backlog`](crate::service::interface::TxpoolReadResponse::Backlog).
+#[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq, Hash)]
+pub struct TxEntry {
+    /// The transaction's weight.
+    pub weight: u64,
+    /// The transaction's fee.
+    pub fee: u64,
+    /// How long the transaction has been in the pool.
+    pub time_in_pool: std::time::Duration,
+}


Types (txpool)

hinto-janai · 2024-10-03T00:45:39Z

types/src/types.rs

+/// Input required to generate an output histogram.
+///
+/// Used in RPC's `get_output_histogram`.
+#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct OutputHistogramInput {
+    pub amounts: Vec<u64>,
+    pub min_count: u64,
+    pub max_count: u64,
+    pub unlocked: bool,
+    pub recent_cutoff: u64,
+}
+
+/// A single entry in an output histogram.
+///
+/// Used in RPC's `get_output_histogram`.
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct OutputHistogramEntry {
+    pub amount: u64,
+    pub total_instances: u64,
+    pub unlocked_instances: u64,
+    pub recent_instances: u64,
+}
+
+/// Data of summed coinbase transactions.
+///
+/// Used in RPC's `get_coinbase_tx_sum`.
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct CoinbaseTxSum {
+    pub emission_amount: u128,
+    pub fee_amount: u128,
+    pub wide_emission_amount: u128,
+    pub wide_fee_amount: u128,
+}
+
+/// Data to create a custom block template.
+///
+/// Used in RPC's `get_miner_data`.
+#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct MinerData {
+    pub major_version: u8,
+    pub height: u64,
+    pub prev_id: [u8; 32],
+    pub seed_hash: [u8; 32],
+    pub difficulty: u128,
+    pub median_weight: u64,
+    pub already_generated_coins: u64,
+    pub tx_backlog: Vec<MinerDataTxBacklogEntry>,
+}
+
+/// A transaction in the txpool.
+///
+/// Used in [`MinerData::tx_backlog`].
+#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct MinerDataTxBacklogEntry {
+    pub id: [u8; 32],
+    pub weight: u64,
+    pub fee: u64,
+}
+
+/// Information on a [`HardFork`].
+///
+/// Used in RPC's `hard_fork_info`.
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct HardForkInfo {
+    pub earliest_height: u64,
+    pub enabled: bool,
+    pub state: u32,
+    pub threshold: u32,
+    pub version: u8,
+    pub votes: u32,
+    pub voting: u8,
+    pub window: u32,
+}
+
+/// Estimated fee data.
+///
+/// Used in RPC's `get_fee_estimate`.
+#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct FeeEstimate {
+    pub fee: u64,
+    pub fees: Vec<u64>,
+    pub quantization_mask: u64,
+}
+
+/// Information on a (maybe alternate) chain.
+///
+/// Used in RPC's `get_alternate_chains`.
+#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub struct ChainInfo {
+    pub block_hash: [u8; 32],
+    pub block_hashes: Vec<[u8; 32]>,
+    pub difficulty: u128,
+    pub height: u64,
+    pub length: u64,
+    pub main_chain_parent_block: [u8; 32],
+    pub wide_difficulty: u128,
+}


Types (everything else)

hinto-janai · 2024-10-03T00:49:15Z

storage/blockchain/src/service/read.rs

+/// [`BlockchainReadRequest::Block`]
+fn block(env: &ConcreteEnv, block_height: BlockHeight) -> ResponseResult {
+    Ok(BlockchainResponse::Block(todo!()))
+}


TODO

All the new request/responses are fully typed but they will all panic with todo!().

The exception is AddressBookRequest::GetBan which I added a new function for.

Handlers can be added in a different PR.

hinto-janai · 2024-10-03T00:50:15Z

consensus/src/context.rs

+    ///
    /// A map of seed height to `RandomX` VMs.
    RxVms(HashMap<usize, Arc<RandomXVm>>),


TODO

Why does this return a HashMap when the request is CurrentRxVm?

I might be misremembering but I think somewhere in RPC we need access to a RandomX VM, would calling .last() on this HashMap always give us the latest in a Some?

Why does this return a HashMap when the request is CurrentRxVm?

It should probably be CurrentRxVms, it needs to return more than one so we can compute PoW concurrently.

I might be misremembering but I think somewhere in RPC we need access to a RandomX VM

You will probably just need to generate a new VM - I don't think you will need the current active one, I may be wrong though I just can't see why you would.

hinto-janai · 2024-10-03T01:03:18Z

binaries/cuprated/src/rpc/handler.rs

+    pub txpool_read: TxpoolReadHandle,
+
+    /// Write handle to the transaction pool database.
+    pub txpool_write: TxpoolWriteHandle,


@Boog900 some questions about #279 (comment):

The transaction pool is going to have a specific interface for requests that mutate the pool, just to keep in mind, it wont directly use the write handle.

What should be here in txpool_write?

Is "specific interface" just going to be a different tower::Service?

Why does the txpool need this special interface?

What should be here in txpool_write?
Is "specific interface" just going to be a different tower::Service?

Yes, so a handle to that other service.

Why does the txpool need this special interface?

To synchronise tx-pool state so we don't end up verifying the same tx across multiple connections before adding them to the pool. There are other operations that require more synchronization than a database write handle can provide.

Boog900 · 2024-10-03T22:22:15Z

binaries/cuprated/src/rpc/handler.rs

+    /// Relay a block to the network.
+    RelayBlock(Block),


This should be add block, this also happens to be the one request I already have in the blockchain manager PR (handle_incoming_block).

Does it have the same semantics though? It adds a block to our local chain and broadcasts it?

The usecase is submit_block.

Boog900 · 2024-10-03T22:27:15Z

binaries/cuprated/src/rpc/handler.rs

+    pub txpool_read: TxpoolReadHandle,
+
+    /// Write handle to the transaction pool database.
+    pub txpool_write: TxpoolWriteHandle,


What should be here in txpool_write?
Is "specific interface" just going to be a different tower::Service?

Yes, so a handle to that other service.

Why does the txpool need this special interface?

To synchronise tx-pool state so we don't end up verifying the same tx across multiple connections before adding them to the pool. There are other operations that require more synchronization than a database write handle can provide.

Boog900 · 2024-10-03T22:29:14Z

binaries/cuprated/src/rpc/request/address_book.rs

+/// [`AddressBookRequest::PeerlistSize`]
+pub(super) async fn peerlist_size<Z: NetworkZone>(
+    address_book: &mut impl AddressBook<Z>,
+) -> Result<(u64, u64), Error> {
+    let AddressBookResponse::PeerlistSize { white, grey } = address_book
+        .ready()
+        .await
+        .expect("TODO")
+        .call(AddressBookRequest::PeerlistSize)
+        .await
+        .expect("TODO")
+    else {
+        unreachable!();
+    };
+
+    Ok((usize_to_u64(white), usize_to_u64(grey)))
+}


Yeah, there are other as well, probably should be done in a separate PR

Boog900 · 2024-10-03T22:34:31Z

consensus/src/context.rs

+    ///
    /// A map of seed height to `RandomX` VMs.
    RxVms(HashMap<usize, Arc<RandomXVm>>),


Why does this return a HashMap when the request is CurrentRxVm?

It should probably be CurrentRxVms, it needs to return more than one so we can compute PoW concurrently.

I might be misremembering but I think somewhere in RPC we need access to a RandomX VM

You will probably just need to generate a new VM - I don't think you will need the current active one, I may be wrong though I just can't see why you would.

Boog900 · 2024-10-03T22:36:00Z

p2p/p2p-core/src/ban.rs

+//! Data structures related to bans.
+
+use std::time::{Duration, Instant};
+
+use crate::NetZoneAddress;
+
+/// Data within [`crate::services::AddressBookRequest::SetBan`].
+pub struct SetBan<A: NetZoneAddress> {
+    pub address: A,
+    pub ban: bool,
+    pub duration: Duration,
+}
+
+/// Data within [`crate::services::AddressBookResponse::GetBans`].
+pub struct BanState<A: NetZoneAddress> {
+    pub address: A,
+    pub banned: bool,
+    pub unban_instant: Instant,
+}


Yeah separating them is a good idea IMO

Boog900 · 2024-10-03T22:38:05Z

p2p/p2p-core/src/ban.rs

+//! Data structures related to bans.
+
+use std::time::{Duration, Instant};
+
+use crate::NetZoneAddress;
+
+/// Data within [`crate::services::AddressBookRequest::SetBan`].
+pub struct SetBan<A: NetZoneAddress> {
+    pub address: A,
+    pub ban: bool,
+    pub duration: Duration,
+}
+
+/// Data within [`crate::services::AddressBookResponse::GetBans`].
+pub struct BanState<A: NetZoneAddress> {
+    pub address: A,
+    pub banned: bool,
+    pub unban_instant: Instant,
+}


Those types seem weird though, wouldn't this make more sense:

Suggested change

//! Data structures related to bans.

use std::time::{Duration, Instant};

use crate::NetZoneAddress;

/// Data within [`crate::services::AddressBookRequest::SetBan`].

pub struct SetBan<A: NetZoneAddress> {

pub address: A,

pub ban: bool,

pub duration: Duration,

}

/// Data within [`crate::services::AddressBookResponse::GetBans`].

pub struct BanState<A: NetZoneAddress> {

pub address: A,

pub banned: bool,

pub unban_instant: Instant,

}

//! Data structures related to bans.

use std::time::{Duration, Instant};

use crate::NetZoneAddress;

/// Data within [`crate::services::AddressBookRequest::SetBan`].

pub struct SetBan<A: NetZoneAddress> {

pub address: A,

pub ban: Option<Duration>,

}

/// Data within [`crate::services::AddressBookResponse::GetBans`].

pub struct BanState<A: NetZoneAddress> {

pub address: A,

pub banned: Option<Instant>,

}

Boog900 · 2024-10-03T22:40:16Z

storage/blockchain/src/service/mod.rs

-//! The `DatabaseReadHandle` can be shared as it is cheaply [`Clone`]able, however,
-//! the `DatabaseWriteHandle` cannot be cloned. There is only 1 place in Cuprate that
-//! writes, so it is passed there and used.
-//!


This does need to be removed for the tx-pool handle, however the database is still only written to in 1 place, the blockchain manager.

You shouldn't need to hold a write handle to the blockchain in the RPC

Boog900 · 2024-10-03T22:44:07Z

types/src/blockchain.rs

+    /// Get a [`Block`] by its height.
+    Block {
+        height: usize,
+    },
+
+    /// Get a [`Block`] by its hash.
+    BlockByHash([u8; 32]),
+
+    /// Get the total amount of non-coinbase transactions in the chain.
+    TotalTxCount,
+
+    /// Get the current size of the database.
+    DatabaseSize,
+
+    // Get the difficulty for the next block in the chain.
+    Difficulty(usize),
+
+    /// Get an output histogram.
+    ///
+    /// TODO: document fields after impl.
+    OutputHistogram(OutputHistogramInput),
+
+    /// Get the coinbase amount and the fees amount for
+    /// `N` last blocks starting at particular height.
+    ///
+    /// TODO: document fields after impl.
+    CoinbaseTxSum {
+        height: usize,
+        count: u64,
+    },
+
+    /// Get the necessary data to create a custom block template.
+    MinerData,


Difficulty is contained in the blockchain context so shouldn't need a new request (my bad for missing this).

MinerData will not be able to be a single request. Looking at its data it would need a request to the blockchain cache and to the tx-pool, both of these requests are already there/in this PR so this could just be removed I think

binaries/cuprated/src/rpc/handler.rs

Boog900

The blockchain manager API should change but for now is fine

p2p/p2p-core/src/ban.rs

Co-authored-by: Boog900 <[email protected]>

add request methods

96099eb

github-actions bot added A-storage Related to storage. A-binaries Related to binaries. labels Sep 23, 2024

add p2p messages

4dc3b2c

github-actions bot added the A-p2p Related to P2P. label Sep 24, 2024

hinto-janai added 2 commits September 24, 2024 18:02

add txpool msgs

e8de295

add blockchain_context msgs

dfbdec3

github-actions bot added the A-consensus Related to consensus. label Sep 24, 2024

add blockchain msgs

e8cb951

github-actions bot added the A-types Related to types. label Sep 25, 2024

hinto-janai added 5 commits September 24, 2024 20:39

fmt

e618bb7

blockchain_manager msgs

a1b3bda

blockchain manager msg types

cf30954

add DB fn signatures

02a99f3

add statics module

6878a25

hinto-janai mentioned this pull request Oct 1, 2024

Tracking Issue for RPC handlers #244

Open

24 tasks

Merge branch 'main' into rpc-msgs

3fb69f2

github-actions bot added the A-docs Related to documentation. label Oct 2, 2024

hinto-janai added 2 commits October 1, 2024 21:03

p2p msg changes, docs

f9ff593

Merge branch 'main' into rpc-msgs

41f4b78

github-actions bot removed the A-docs Related to documentation. label Oct 2, 2024

hinto-janai added 4 commits October 2, 2024 16:06

txpool docs/types

cf33188

blockchain docs/types

64a3e5d

AlternateChains, docs

42d6f97

fixes

e7f052f

hinto-janai commented Oct 3, 2024

View reviewed changes

hinto-janai marked this pull request as ready for review October 3, 2024 00:59

hinto-janai requested a review from Boog900 October 3, 2024 00:59

hinto-janai commented Oct 3, 2024

View reviewed changes

Boog900 requested changes Oct 3, 2024

View reviewed changes

hinto-janai added 6 commits October 4, 2024 16:10

remove blockchain write handle, fix docs

2228d4d

remove BlockchainReadRequest::Difficulty

d0888c8

remove BlockchainReadRequest::MinerData

aaed13a

fix p2p ban types

4f9c5f1

CurrentRxVm -> CurrentRxVms

c886f80

storage: remove Clone off write handle

4fa37de

hinto-janai commented Oct 7, 2024

View reviewed changes

binaries/cuprated/src/rpc/handler.rs Show resolved Hide resolved

hinto-janai requested a review from Boog900 October 8, 2024 00:02

Boog900 approved these changes Oct 8, 2024

View reviewed changes

p2p/p2p-core/src/ban.rs Outdated Show resolved Hide resolved

hinto-janai and others added 4 commits October 8, 2024 16:50

Update p2p/p2p-core/src/ban.rs

337f42f

Co-authored-by: Boog900 <[email protected]>

Merge branch 'main' into rpc-msgs

9aa9ef5

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

0913a2b

fix merge

364ce37

hinto-janai requested a review from Boog900 October 8, 2024 21:32

Boog900 approved these changes Oct 8, 2024

View reviewed changes

Boog900 merged commit 9923d8d into Cuprate:main Oct 8, 2024
6 checks passed

hinto-janai deleted the rpc-msgs branch October 8, 2024 23:28

hinto-janai mentioned this pull request Oct 18, 2024

cuprated: internal signatures required for RPC pt. 2 #320

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cuprated: internal signatures required for RPC #297

cuprated: internal signatures required for RPC #297

hinto-janai commented Sep 23, 2024 •

edited

Loading

hinto-janai left a comment

hinto-janai Oct 3, 2024

Boog900 Oct 3, 2024

hinto-janai Oct 3, 2024

Boog900 Oct 3, 2024

hinto-janai Oct 3, 2024

Boog900 Oct 3, 2024

Boog900 Oct 3, 2024

hinto-janai Oct 3, 2024

hinto-janai Oct 3, 2024

hinto-janai Oct 3, 2024

hinto-janai Oct 3, 2024

Boog900 Oct 3, 2024

hinto-janai Oct 3, 2024 •

edited

Loading

Boog900 Oct 3, 2024 •

edited

Loading

Boog900 Oct 3, 2024

hinto-janai Oct 4, 2024

Boog900 Oct 8, 2024

Boog900 Oct 3, 2024 •

edited

Loading

Boog900 Oct 3, 2024

Boog900 Oct 3, 2024

Boog900 Oct 3, 2024

Boog900 Oct 3, 2024

Boog900 Oct 3, 2024

Boog900 Oct 3, 2024

Boog900 left a comment

cuprated: internal signatures required for RPC #297

cuprated: internal signatures required for RPC #297

Conversation

hinto-janai commented Sep 23, 2024 • edited Loading

What

hinto-janai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Misc

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Misc

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Types (address book)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Types (txpool)

Choose a reason for hiding this comment

Types (everything else)

Choose a reason for hiding this comment

TODO

Choose a reason for hiding this comment

TODO

Choose a reason for hiding this comment

hinto-janai Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Boog900 Oct 3, 2024 • 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

Boog900 Oct 3, 2024 • 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

Boog900 left a comment

Choose a reason for hiding this comment

hinto-janai commented Sep 23, 2024 •

edited

Loading

hinto-janai Oct 3, 2024 •

edited

Loading

Boog900 Oct 3, 2024 •

edited

Loading

Boog900 Oct 3, 2024 •

edited

Loading