cuprated: Blockchain Manager #274

Boog900 · 2024-09-09T16:07:39Z

What

This PR contains the blockchain manager, there are still quite a few TODOs however I think it is in a state to be reviewed and merged.

More details in comments

Boog900

A lot of this code still needs a bit of work, however I am starting to need to build other sections so to prevent a massive PR for the whole binary this will have to do.

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

binaries/cuprated/src/blockchain/interface.rs

+//! The blockchain manger interface.
+//!
+//! This module contains all the functions to mutate the blockchain's state in any way, through the
+//! blockchain manger.


This file contains the interface for the blockchain manager.

I decided to not go with a tower::service interface, as I just don't think it is needed here. The benefits of a Service interface here would just be uniformity with every other service, the drawbacks are the extra over head of holding/cloning another handle (admittedly low) and just the extra hassle of holding another service handle.

I think these free functions as a way to mutate the blockchain makes more sense.

Couldn't you say this about every Service though? Breaking the pattern and using global statics seems lazy? This is yet another interface that must be learned and it doesn't come with docs on how to use it.

If it's a 1-off in cuprated I think it's fine but there should be documentation on what the statics are, how to use generally use the interface, etc.

That's fair, the API here should be pretty easy to use - all you need to do is call the functions, I'll add some more docs.

Couldn't you say this about every Service though

Other service are exposed this one is not. There are other benefits to the Service interface, like the wide range of middleware but since we are not going to make use of them for the blockchain manager that isn't a benefit here.

binaries/cuprated/src/blockchain.rs

binaries/cuprated/src/blockchain/interface.rs

binaries/cuprated/src/blockchain/manager.rs

binaries/cuprated/src/blockchain/manager/handler.rs

binaries/cuprated/src/blockchain/syncer.rs

hinto-janai

Only nits, left another review.

binaries/cuprated/src/blockchain/interface.rs

binaries/cuprated/src/blockchain/manager.rs

binaries/cuprated/src/blockchain/manager/commands.rs

binaries/cuprated/src/blockchain/manager/handler.rs

binaries/cuprated/src/blockchain/syncer.rs

binaries/cuprated/src/blockchain/types.rs

hinto-janai · 2024-10-05T00:13:18Z

binaries/cuprated/src/blockchain.rs

+    blockchain_write_handle
+        .ready()
+        .await
+        .expect(PANIC_CRITICAL_SERVICE_ERROR)
+        .call(BlockchainWriteRequest::WriteBlock(
+            VerifiedBlockInformation {
+                block_blob: genesis.serialize(),
+                txs: vec![],
+                block_hash: genesis.hash(),
+                pow_hash: cryptonight_hash_v0(&genesis.serialize_pow_hash()),
+                height: 0,
+                generated_coins: genesis.miner_transaction.prefix().outputs[0]
+                    .amount
+                    .unwrap(),
+                weight: genesis.miner_transaction.weight(),
+                long_term_weight: genesis.miner_transaction.weight(),
+                cumulative_difficulty: 1,
+                block: genesis,
+            },
+        ))
+        .await
+        .expect(PANIC_CRITICAL_SERVICE_ERROR);


This doesn't need to change for now but I still think cuprate_constants::genesis should exist - having the genesis block inlined in some async function in cuprated feels wrong. It's not about de-duplicatation but more about separating concerns and being clean, these constants deserve a name instead of being inlined.

Also, this is only mainnet right?

having the genesis block inlined in some async function in cuprated feels wrong

This is just mapping the genesis block got from generate_genesis_block to VerifiedBlockInformation.

Also, this is only mainnet right?

No this should work on all 3 current networks, although it does (like monerod) make some assumptions about the genesis blocks format

mapping

I realize this, I'm saying this mapping should exist in cuprate_constants::genesis as well, next to the Block.

hinto-janai · 2024-10-05T00:22:15Z

binaries/cuprated/src/blockchain/interface.rs

+//! The blockchain manger interface.
+//!
+//! This module contains all the functions to mutate the blockchain's state in any way, through the
+//! blockchain manger.


Couldn't you say this about every Service though? Breaking the pattern and using global statics seems lazy? This is yet another interface that must be learned and it doesn't come with docs on how to use it.

If it's a 1-off in cuprated I think it's fine but there should be documentation on what the statics are, how to use generally use the interface, etc.

hinto-janai · 2024-10-05T00:32:16Z

binaries/cuprated/src/blockchain/interface.rs

+};
+
+/// The channel used to send [`BlockchainManagerCommand`]s to the blockchain manger.
+pub static COMMAND_TX: OnceLock<mpsc::Sender<BlockchainManagerCommand>> = OnceLock::new();


Having a pub static OnceLock read/write available to the entire binary doesn't seem necessary, there should also be docs on who/when/where this OnceLock should be initialized.

Can this be privately defined where it is initiated then accompanied with:

/// Access to the blockchain manager's [...] /// /// # Invariant /// Must be initialized with [...] /// /// # Panics /// Panics if not initialized in [...] pub fn command_tx() -> &mpsc::Sender<BlockchainManagerCommand> { &*COMMAND_TX.get().expect("wasn't initialized") }

Oops yeah wasn't meant to be fully public

I added some more docs to this

hinto-janai · 2024-10-05T00:39:48Z

binaries/cuprated/src/blockchain/interface.rs

+/// This is used over something like a dashmap as we expect a lot of collisions in a short amount of
+/// time for new blocks so we would lose the benefit of sharded locks. A dashmap is made up of `RwLocks`
+/// which are also more expensive than `Mutex`s.
+pub static BLOCKS_BEING_HANDLED: OnceLock<Mutex<HashSet<[u8; 32]>>> = OnceLock::new();


Does this need to be pub? It's only currently used in this file.

To be honest pub static Mutex is pretty bad code smell - global resources like this really should be documented with who/when/how it's used.

If this only needs to be mutated within this file, what about doing the same as COMMAND_TX? Make this private and expose a fn() -> &HashSet<_, _> while documented that it takes a lock, and the usage patterns of that lock.

I've moved it into the only function it is needed in + made it a LazyLock

hinto-janai · 2024-10-05T00:42:03Z

binaries/cuprated/src/blockchain/interface.rs

+    // TODO: remove this when we have a working tx-pool.
+    if given_txs.len() != block.transactions.len() {
+        return Err(IncomingBlockError::UnknownTransactions(
+            block_hash,
+            (0..usize_to_u64(block.transactions.len())).collect(),
+        ));
+    }


Why usize_to_u64? Shouldn't it be usize if it's an index?

These values are sent over the P2P network, which requires a u64. I could change this and probably will when the tx-pool is made and do the conversion in the P2P hanlder.

However I would rather wait to make the tx-pool rather than preemptively change this.

hinto-janai · 2024-10-05T01:12:32Z

binaries/cuprated/src/blockchain/syncer.rs

+        + 'static,
+    CN::Future: Send + 'static,
+{
+    tracing::info!("Starting blockchain syncer");


Maybe use tracing::{info,debug,trace} here.

I think this is fine - this file will be completely changed at some point as well

hinto-janai · 2024-10-05T01:15:16Z

binaries/cuprated/src/blockchain/types.rs

+
+impl Service<ChainSvcRequest> for ChainService {
+    type Response = ChainSvcResponse;
+    type Error = tower::BoxError;


anyhow::Error?

It needs to match the bounds in the p2p crate, I am planning to change those in a separate PR though

binaries/cuprated/src/blockchain/types.rs

binaries/cuprated/src/constants.rs

hinto-janai · 2024-10-05T01:22:29Z

binaries/cuprated/src/signals.rs

+///
+/// A [`RwLock`] where a write lock is taken during a reorg and a read lock can be taken
+/// for any operation which must complete without a reorg happening.
+pub static REORG_LOCK: RwLock<()> = RwLock::const_new(());


I have mixed feelings using locks in a C-style like this... The solution is to design things such that you can scope and pass resources properly, I'm not going to ask this to be changed since that will involve a lot of work but I hope you realize this pattern is bad as well.

related issue: #305

hinto-janai · 2024-10-07T21:16:31Z

binaries/cuprated/src/blockchain/interface.rs

-pub static BLOCKS_BEING_HANDLED: OnceLock<Mutex<HashSet<[u8; 32]>>> = OnceLock::new();
+/// This channel is initialized in [`init_blockchain_manager`](super::manager::init_blockchain_manager), the functions
+/// in this file document what happens if this is not initialized when they are called.
+pub(super) static COMMAND_TX: OnceLock<mpsc::Sender<BlockchainManagerCommand>> = OnceLock::new();


This (super) is fine but I would have preferred if it were moved to manager.rs and kept private - it is still writeable from interface.rs since it's defined there, even though it only needs a &mpsc::Sender.

Doesn't matter too much I guess but then again things being scoped is nice.

hinto-janai · 2024-10-07T21:23:32Z

binaries/cuprated/src/blockchain/interface.rs

+    static BLOCKS_BEING_HANDLED: LazyLock<Mutex<HashSet<[u8; 32]>>> =
+        LazyLock::new(|| Mutex::new(HashSet::new()));


Could/should this be a tokio::sync::Mutex?

As it is now, if handle_incoming_block is asynchronously called from many many places, the std::sync::Mutex may block the executor more than tokio::sync::Mutex, right?

Although IDK really, the lock isn't held for long so this probably doesn't matter.

Although std Mutex is blocking we hold the lock for a tiny amount of time + an async Mutex is a lot more expensive: https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use

hinto-janai · 2024-10-07T21:27:57Z

binaries/cuprated/src/blockchain/manager/handler.rs

+    /// Broadcast a valid block to the network.
+    async fn broadcast_block(&mut self, block_bytes: Bytes, blockchain_height: usize) {
+        self.broadcast_svc
+            .ready()
+            .await
+            .expect("Broadcast service cannot error.")
+            .call(BroadcastRequest::Block {
+                block_bytes,
+                current_blockchain_height: usize_to_u64(blockchain_height),
+            })
+            .await
+            .expect("Broadcast service cannot error.");
+    }


Oh I see. Maybe Broadcast service is Infallible would work then, current msg is fine too.

Boog900 added 27 commits August 16, 2024 02:30

add cuprated skeleton

b4b73ec

fmt and add deny exception

39d48fe

add main chain batch handler

a018469

add blockchain init

f909c26

very rough block manager

f25588d

misc changes

1c93ea1

move more config values

05d0cf2

add new tables & types

d648871

add function to fully add an alt block

e1ae848

resolve current todo!s

ed887a7

add new requests

bc619b6

WIP: starting re-orgs

029f439

add last service request

6927b05

commit Cargo.lock

21e4b3a

Merge branch 'main' into storage-alt-blocks

a9d8eee

add test

123aedd

more docs + cleanup + alt blocks request

ba5c5ac

clippy + fmt

f92375f

document types

a864f93

move tx_fee to helper

6119972

more doc updates

b211210

fmt

68807e7

fix imports

c03065b

remove config files

1831fa6

Merge branch 'main' into cuprated-blockchain

20033ee

fix merge errors

da78cbd

Merge branch 'storage-alt-blocks' into cuprated-blockchain

b5f8475

github-actions bot added A-p2p Related to P2P. A-test-utils Related to test-utils. A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. labels Sep 9, 2024

Boog900 added 2 commits October 1, 2024 22:58

Merge branch 'main' into cuprated-blockchain

a7553c2

fix merge

69f9d84

github-actions bot removed A-test-utils Related to test-utils. A-storage Related to storage. A-types Related to types. labels Oct 3, 2024

Boog900 added 5 commits October 3, 2024 03:36

clean up the blockchain manger

caaeced

add more docs + cleanup imports

8cff481

fix typo

4af0f89

fix doc

6702dbe

remove unrelated changes

403964b

github-actions bot removed the A-helper Related to cuprate-helper. label Oct 3, 2024

Boog900 commented Oct 3, 2024

View reviewed changes

Boog900 marked this pull request as ready for review October 3, 2024 22:16

Boog900 requested a review from hinto-janai October 3, 2024 22:16

Boog900 mentioned this pull request Oct 3, 2024

cuprated: internal signatures required for RPC #297

Merged

hinto-janai reviewed Oct 4, 2024

View reviewed changes

hinto-janai requested changes Oct 5, 2024

View reviewed changes

Boog900 added 5 commits October 5, 2024 16:39

improve interface globals

783f426

manger -> manager

375a1e1

enums instead of bools

f50d921

move chain service to separate file

27a8acd

more review fixes

a21e489

Boog900 requested a review from hinto-janai October 5, 2024 20:32

add link to issue

c87edf2

hinto-janai reviewed Oct 7, 2024

View reviewed changes

hinto-janai approved these changes Oct 7, 2024

View reviewed changes

Boog900 added 2 commits October 8, 2024 18:13

fix syncer + update comment

e7aaf3c

fmt

173f4f7

Boog900 merged commit 8be3698 into main Oct 8, 2024
6 checks passed

Boog900 deleted the cuprated-blockchain branch October 8, 2024 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cuprated: Blockchain Manager #274

cuprated: Blockchain Manager #274

Boog900 commented Sep 9, 2024 •

edited

Loading

Boog900 left a comment

Boog900 Oct 3, 2024 •

edited

Loading

hinto-janai Oct 5, 2024

Boog900 Oct 5, 2024

hinto-janai left a comment •

edited

Loading

hinto-janai Oct 5, 2024

Boog900 Oct 5, 2024 •

edited

Loading

hinto-janai Oct 7, 2024

hinto-janai Oct 5, 2024

hinto-janai Oct 5, 2024

Boog900 Oct 5, 2024

Boog900 Oct 5, 2024

hinto-janai Oct 5, 2024

Boog900 Oct 5, 2024

hinto-janai Oct 5, 2024

Boog900 Oct 5, 2024

hinto-janai Oct 5, 2024

Boog900 Oct 5, 2024

hinto-janai Oct 5, 2024

Boog900 Oct 5, 2024

hinto-janai Oct 5, 2024

hinto-janai Oct 7, 2024

hinto-janai Oct 7, 2024

hinto-janai Oct 7, 2024

Boog900 Oct 8, 2024

hinto-janai Oct 7, 2024

		static BLOCKS_BEING_HANDLED: LazyLock<Mutex<HashSet<[u8; 32]>>> =
		LazyLock::new(\|\| Mutex::new(HashSet::new()));

cuprated: Blockchain Manager #274

cuprated: Blockchain Manager #274

Conversation

Boog900 commented Sep 9, 2024 • edited Loading

What

Boog900 left a 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

hinto-janai left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Boog900 Oct 5, 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

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

Boog900 commented Sep 9, 2024 •

edited

Loading

Boog900 Oct 3, 2024 •

edited

Loading

hinto-janai left a comment •

edited

Loading

Boog900 Oct 5, 2024 •

edited

Loading