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

Store contract class data in client #1631

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tdelabro
Copy link
Collaborator

No description provided.

@tdelabro tdelabro force-pushed the store-contract-class-data-in-client branch 2 times, most recently from 58d8c7a to b40b67c Compare June 17, 2024 15:01
@tdelabro tdelabro marked this pull request as ready for review June 17, 2024 15:01
@tdelabro
Copy link
Collaborator Author

ci linter failed because counter10 doesn't exist on main, but it is built during the starknet-rpc-test and those passes. So not an issue

crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
starknet-rpc-test/add_declare_transaction.rs Outdated Show resolved Hide resolved
starknet-rpc-test/get_class.rs Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
starknet-rpc-test/add_declare_transaction.rs Outdated Show resolved Hide resolved
starknet-rpc-test/get_class.rs Show resolved Hide resolved
@tdelabro tdelabro requested a review from EvolveArt June 19, 2024 10:58
@@ -369,6 +371,16 @@ pub fn new_full(
}
}

task_manager.spawn_essential_handle().spawn(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is currently spawned inside role.is_authority(). do we want this worker to be there for full nodes as well? although, this entire setup doesn't work with multiple full nodes anyways right? as far as I understand, only the full node that receives the txn will store the class data

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as far as I understand, only the full node that receives the txn will store the class data

Yup. Him and the sequencer that produces the block.

I think it is better to relly on p2p to share this data. It should be there soon

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I asked internally about the future p2p protocol

Copy link
Collaborator

Choose a reason for hiding this comment

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

WIll the sequencer store it though? The storing only happens at the RPC layer. If a txn reaches the full node, it gets validated and is then gossiped in the mempool. It never enters the RPC of the sequencer, just goes inside the pallet directly no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think Starknet full nodes, relay the RPC calls directly, not the TXs.
So they will send the full data to the sequencer, it needs it to compile sierra into casm and check the hashes match

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh ok, I thought we were talking about running Madara as a full node. That won't work anymore with this then, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Madara full node will also just relay the write RPC. not store anything

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I thought only extrinsics are gossiped from the full node to the validator in Substrate

crates/client/rpc/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Show resolved Hide resolved
crates/client/contract-class-data/src/lib.rs Outdated Show resolved Hide resolved
unsafe { std::str::from_utf8_unchecked(crate::static_keys::LAST_PROVED_BLOCK) }.to_string(),
)),
}
pub fn last_proved_block(&self) -> Result<Option<BlockHash>, DbError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can delete this actually, not used anymore. or we can create an issue if you want to do it in another PR :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will start using it in the future, when we read messages from l1 again, won't we?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I don't think we need the last proved block to read a message from the L1

crates/client/rpc/src/contract_class_v0.rs Outdated Show resolved Hide resolved
@tdelabro tdelabro force-pushed the store-contract-class-data-in-client branch from 8ad406a to 5f20367 Compare June 20, 2024 10:27
@tdelabro tdelabro force-pushed the store-contract-class-data-in-client branch from d66f38a to 3b07dc0 Compare June 24, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants