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

Add constants/ crate #280

Merged
merged 21 commits into from
Oct 2, 2024
Merged

Add constants/ crate #280

merged 21 commits into from
Oct 2, 2024

Conversation

hinto-janai
Copy link
Contributor

What

Adds a cuprate_constants crate in the root constants/ folder.

Closes #189.

Why, where

See #189.

How

See below review.

@github-actions github-actions bot added A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-workspace Changes to a root workspace file or general repo file. A-docs Related to documentation. labels Sep 13, 2024
@github-actions github-actions bot added the A-ci Related to CI. label Sep 13, 2024
@Asurar0
Copy link
Contributor

Asurar0 commented Sep 13, 2024

@hinto-janai
Copy link
Contributor Author

Will P2P constants be included in this crate as well?

I haven't checked in depth but if they are pub(crate) and not needed elsewhere, probably not. Constants like CRYPTONOTE_MAX_BLOCK_HEIGHT doesn't really belong anywhere yet is needed by multiple crates - these are the type of constants this crate will contain.

@github-actions github-actions bot added the A-helper Related to cuprate-helper. label Sep 14, 2024
@github-actions github-actions bot added A-p2p Related to P2P. A-consensus Related to consensus. A-storage Related to storage. A-pruning Related to pruning. labels Sep 20, 2024
@github-actions github-actions bot added the A-types Related to types. label Sep 20, 2024
@github-actions github-actions bot removed the A-types Related to types. label Sep 20, 2024
Comment on lines 14 to 21
#[cfg(feature = "block")]
pub mod block;
#[cfg(feature = "build")]
pub mod build;
#[cfg(feature = "genesis")]
pub mod genesis;
#[cfg(feature = "rpc")]
pub mod rpc;
Copy link
Contributor Author

@hinto-janai hinto-janai Sep 20, 2024

Choose a reason for hiding this comment

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

Constants are separated into feature gated modules.

There's more modules that could be added from https://github.com/monero-project/monero/blob/master/src/cryptonote_config.h but it seems lots of them don't need to public and/or belong in a more specific crate.

For now I only added modules that needs to be used in multiple places.

@@ -0,0 +1,22 @@
//! Build related metadata.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cuprate_helper::constants was moved here.

Comment on lines 153 to 177
generate_genesis_consts! {
Mainnet {
nonce: 10000,
block: "010000000000000000000000000000000000000000000000000000000000000000000010270000013c01ff0001ffffffffffff03029b2e4c0281c0b02e7c53291a94d1d0cbff8883f8024f5142ee494ffbbd08807121017767aafcde9be00dcfd098715ebcf7f410daebc582fda69d24a28e9d0bc890d100",
block_hash: "418015bb9ae982a1975da7d79277c2705727a56894ba0fb246adaabb1f4632e3",
tx: "013c01ff0001ffffffffffff03029b2e4c0281c0b02e7c53291a94d1d0cbff8883f8024f5142ee494ffbbd08807121017767aafcde9be00dcfd098715ebcf7f410daebc582fda69d24a28e9d0bc890d1",
tx_hash: "c88ce9783b4f11190d7b9c17a69c1c52200f9faaee8e98dd07e6811175177139",
}

Testnet {
nonce: 10001,
block: "010000000000000000000000000000000000000000000000000000000000000000000011270000013c01ff0001ffffffffffff03029b2e4c0281c0b02e7c53291a94d1d0cbff8883f8024f5142ee494ffbbd08807121017767aafcde9be00dcfd098715ebcf7f410daebc582fda69d24a28e9d0bc890d100",
block_hash: "48ca7cd3c8de5b6a4d53d2861fbdaedca141553559f9be9520068053cda8430b",
tx: "013c01ff0001ffffffffffff03029b2e4c0281c0b02e7c53291a94d1d0cbff8883f8024f5142ee494ffbbd08807121017767aafcde9be00dcfd098715ebcf7f410daebc582fda69d24a28e9d0bc890d1",
tx_hash: "c88ce9783b4f11190d7b9c17a69c1c52200f9faaee8e98dd07e6811175177139",
}

Stagenet {
nonce: 10002,
block: "010000000000000000000000000000000000000000000000000000000000000000000012270000013c01ff0001ffffffffffff0302df5d56da0c7d643ddd1ce61901c7bdc5fb1738bfe39fbe69c28a3a7032729c0f2101168d0c4ca86fb55a4cf6a36d31431be1c53a3bd7411bb24e8832410289fa6f3b00",
block_hash: "76ee3cc98646292206cd3e86f74d88b4dcc1d937088645e9b0cbca84b7ce74eb",
tx: "013c01ff0001ffffffffffff0302df5d56da0c7d643ddd1ce61901c7bdc5fb1738bfe39fbe69c28a3a7032729c0f2101168d0c4ca86fb55a4cf6a36d31431be1c53a3bd7411bb24e8832410289fa6f3b",
tx_hash: "c099809301da6ad2fde11969b0e9cb291fc698f8dc678cef00506e7baf561de4",
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This generates const/statics for genesis block/tx data for all networks.

Copy link
Member

Choose a reason for hiding this comment

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

As much as this is cool, do we actually need this much info?

The consensus-rules crate has a genesis module which is pretty much this just less complex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DB needs access to genesis data and RPC could benefit from access to the genesis hash.

I know cuprate_consensus_rules::genesis exists but it only contains a single non-const function that generates a Block per call.

Copy link
Member

Choose a reason for hiding this comment

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

RPC could benefit from access to the genesis hash.

This request is already impl'd in the DB for the block downloader:

fn find_first_unknown(env: &ConcreteEnv, block_ids: &[BlockHash]) -> ResponseResult {

We don't explicitly check the genesis hash, however if the genesis hash doesn't match BlockchainResponse::FindFirstUnknown(None) is returned.

The DB needs access to genesis data

We could check if the DB has the genesis block in the binary and then add it, instead of requiring the DB to do it it self. This is what I did in the blockchain manager branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This request is already impl'd in the DB for the block downloader

We could check the hash at the RPC level before touching the DB, anyway, removed the module. If I find more usecases I'd like to add this back.

Comment on lines +23 to -25
use cuprate_constants::block::MAX_BLOCK_HEIGHT_USIZE;

use thiserror::Error;

pub const CRYPTONOTE_MAX_BLOCK_HEIGHT: usize = 500000000;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CRYPTONOTE_MAX_BLOCK_HEIGHT is replaced by cuprate_constants::block::MAX_BLOCK_HEIGHT[_USIZE] throughout the codebase.

Also, I think Cuprate is far enough removed from the original CryptoNote that we can start dropping the CRYPTONOTE_ prefix.

@github-actions github-actions bot added A-books Related to Cuprate's books. A-book-architecture Related to the Architecture book. labels Sep 21, 2024
@hinto-janai hinto-janai marked this pull request as ready for review September 21, 2024 00:28
@github-actions github-actions bot added the A-types Related to types. label Sep 28, 2024
constants/src/block.rs Outdated Show resolved Hide resolved
constants/src/output.rs Outdated Show resolved Hide resolved
@Boog900
Copy link
Member

Boog900 commented Oct 1, 2024

I don't think we should include block time/output lock time constants here, unless you need to use them elsewhere?

@github-actions github-actions bot removed the A-types Related to types. label Oct 1, 2024
@hinto-janai
Copy link
Contributor Author

@Boog900 ready.

@hinto-janai
Copy link
Contributor Author

hinto-janai commented Oct 2, 2024

Actually, we need block time targets here: https://github.com/monero-project/monero/blob/9866a0e9021e2422d8055731a586083eb5e2be67/src/rpc/core_rpc_server.cpp#L531

edit: nvm, it's not technically required but it does seem wasteful to have to go through messages and talk to the DB for a constant.

@Boog900
Copy link
Member

Boog900 commented Oct 2, 2024

Actually, we need block time targets here

To get that you will need the current hard-fork even with constants, and if you have the current HF you can just call this:

pub const fn block_time(&self) -> Duration {

I would recommend going through the context cache to get the current HF.

@Boog900 Boog900 merged commit a003e05 into Cuprate:main Oct 2, 2024
7 checks passed
@hinto-janai hinto-janai deleted the constants branch October 2, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-book-architecture Related to the Architecture book. A-books Related to Cuprate's books. A-ci Related to CI. A-consensus Related to consensus. A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-docs Related to documentation. A-helper Related to cuprate-helper. A-p2p Related to P2P. A-pruning Related to pruning. A-storage Related to storage. A-workspace Changes to a root workspace file or general repo file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

constants crate
3 participants