feat(starfish): send full block #6803
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 4 Skipped Deployments
|
VorobyevIlya
force-pushed
the
consensus/feat/starfish/send-full-block
branch
from
7a9fb27 to
c1e421a
Compare
VorobyevIlya
force-pushed
the
consensus/feat/starfish/send-full-block
branch
from
6ecd338 to
6c73112
Compare
piotrm50
force-pushed
the
consensus/feat/starfish-consensus
branch
from
6b470f7 to
3d6f76d
Compare
VorobyevIlya
force-pushed
the
consensus/feat/starfish/send-full-block
branch
2 times, most recently
from
be705ab to
7c46133
Compare
| suspended_block_headers: BTreeMap<BlockRef, SuspendedBlockHeader>, | ||
| /// Keeps full blocks for suspended block headers | ||
| suspended_blocks: BTreeMap<BlockRef, VerifiedBlock>, | ||
| // TODO: should it be here? need to discuss |
There was a problem hiding this comment.
add a brief explanation. Is it ever ever-growing set? If so, it might be too big at the end.
There was a problem hiding this comment.
yes, there should be some eviction mechanism there. I will add to do.
There was a problem hiding this comment.
I don't think this is needed here as we only will fetch missing transactions that become committed at some point in the future.
There was a problem hiding this comment.
ok, removing it
| /// Handles the request to fetch blocks by references from the peer. | ||
| async fn handle_fetch_blocks( | ||
| &self, | ||
| peer: AuthorityIndex, | ||
| block_refs: Vec<BlockRef>, | ||
| highest_accepted_rounds: Vec<Round>, | ||
| ) -> ConsensusResult<Vec<Bytes>>; | ||
| ) -> ConsensusResult<(Vec<Bytes>, Vec<Bytes>)>; |
There was a problem hiding this comment.
What are those two vectors of bytes?
There was a problem hiding this comment.
add the comment above
There was a problem hiding this comment.
The function returns pair of Vec. The first vector contains serialization of the headers of blocks from block_refs
and the second contains serializations of transactions of these blocks.
| async fn fetch_blocks( | ||
| &self, | ||
| peer: AuthorityIndex, | ||
| block_refs: Vec<BlockRef>, | ||
| highest_accepted_rounds: Vec<Round>, | ||
| timeout: Duration, | ||
| ) -> ConsensusResult<Vec<Bytes>> { | ||
| ) -> ConsensusResult<(Vec<Bytes>, Vec<Bytes>)> { |
There was a problem hiding this comment.
I think it would be easier to handle if we introduce a new type called Block that wraps BlockHeader and Transactions - we already have SerializedBlock which contains those two serialized things anyway, but I think it's a bit weird to handle those things separately afterwards.
| @@ -1007,7 +1123,25 @@ pub(crate) struct SubscribeBlocksRequest { | |||
| #[derive(Clone, prost::Message)] | |||
| pub(crate) struct SubscribeBlocksResponse { | |||
| #[prost(bytes = "bytes", tag = "1")] | |||
| block: Bytes, | |||
| serialized_block_header: Bytes, | |||
There was a problem hiding this comment.
Why do we send headers and transactions separately, instead of having sending a serialized Block type that wraps those two things? Would not that be easier to handle?
There was a problem hiding this comment.
It might be easier to handle at the network level, but not on the consensus level. It requires Serialization of two serialized vectors, which I am not sure that efficient. The same for deserialization. First, we deserialize Bytes into Block, then we need to split this and serialize into Bytes block_header and transaction_data. This does not sound good to me.
| @@ -601,46 +602,50 @@ impl<C: NetworkClient> CommitSyncer<C> { | |||
| ) | |||
| .await?; | |||
| // 5. Verify the same number of blocks are returned as requested. | |||
| if request_block_refs.len() != serialized_blocks.len() { | |||
| if request_block_refs.len() != serialized_blocks.0.len() | |||
There was a problem hiding this comment.
Working with a tuple here looks weird imo - let's try to create a wrapper type.
| @@ -443,20 +532,35 @@ impl DagState { | |||
|
|
|||
| /// Gets the last proposed block from this authority. | |||
| /// If no block is proposed yet, returns the genesis block. | |||
| pub(crate) fn get_last_proposed_block(&self) -> VerifiedBlockHeader { | |||
| self.get_last_block_for_authority(self.context.own_index) | |||
| pub(crate) fn get_last_proposed_block(&self) -> Option<VerifiedBlock> { | |||
There was a problem hiding this comment.
Why does this return an option now?
There was a problem hiding this comment.
previously Genesis block was instead of None. I think it is better like this, but there is no big difference. The comment above was outdated.
There was a problem hiding this comment.
now it's inconsistent with the following methods get_last_proposed_block_header and get_last_block_header_for_authority.
There was a problem hiding this comment.
changed it back
| } | ||
|
|
||
| impl BlockStoreAPI | ||
| for parking_lot::lock_api::RwLockWriteGuard<'_, parking_lot::RawRwLock, DagState> | ||
| { | ||
| fn get_blocks(&self, refs: &[BlockRef]) -> Vec<Option<VerifiedBlockHeader>> { | ||
| fn get_blocks(&self, refs: &[BlockRef]) -> Vec<Option<VerifiedBlock>> { |
There was a problem hiding this comment.
Why is this still needed? If not needed then I'd remove it
There was a problem hiding this comment.
ok, removed it
| @@ -468,7 +572,7 @@ impl DagState { | |||
| .iter() | |||
| .find(|(block_ref, _)| block_ref.author == authority) | |||
| .expect("Genesis should be found for authority {authority_index}"); | |||
| genesis_block.clone() | |||
| (**genesis_block).clone() | |||
There was a problem hiding this comment.
it is just transformation to header, we changed function to return header
There was a problem hiding this comment.
I think explicitly writing genesis_block.verified_block_header is more readable.
VorobyevIlya
force-pushed
the
consensus/feat/starfish/send-full-block
branch
from
75c965a to
7802696
Compare
fix clippy warnings fix transaction commitment for test header clippy some comments fixed serialized block verification comments and cleaning up fix fetched bytes computation add some comments renamed handle send block to handle subscribe block remove send block from network client fix assert implement fetch blocks for tonic network fix writing to disc only headers and only blocks fix test checking cashed block to work with headers changes synchronizerHandle to work with headers instead of blocks add logic to store full block data for suspended headers in block manager create separate recent_refs_by_authority structures for headers and blocks create accept_block function which accepts header into DagState and also saves the block into recent blocks change test for getting blocks from cash to work with headers implemented reading block headers from rocksdb implemented check for headers in store and fixed corresponding test read headers instead of blocks in dagState creation. Implemented read_headers in mem_store fix verified block creation merge all fake and mock core dispatchers add addBlockHeaders to coreThreadCommand fix tests compilation fix imports add a new struct VerifiedBlock fix warnings fmt create add block headers command it compiles! fixing wrapper continues fix decoupling header from transactions decouple serialized bytes fix typo implement a wrapper around serialized header and transactions -- SerializedBlock (not finished) implement write and read functions for blocks for RocksDB fix test compilation fix compilation errors add a new struct VerifiedBlock
VorobyevIlya
force-pushed
the
consensus/feat/starfish/send-full-block
branch
from
7802696 to
7793322
Compare
| fn try_from(verified_block: VerifiedBlock) -> ConsensusResult<Self> { | ||
| let (serialized_block_header, serialized_transactions) = verified_block.serialized(); | ||
| let serialized_header_and_transactions = SerializedHeaderAndTransactions { | ||
| serialized_block_header: serialized_block_header.clone(), |
There was a problem hiding this comment.
I think yes, I have only references to Bytes here, I can't move them unless I change serialized function or get access to this data in some other way. But cloning Bytes should be cheap, it doesn't copy bytes but works with references
VorobyevIlya
merged commit 8cbbe97
into
consensus/feat/starfish-consensus
Description of change
This PR introduces a full block structure, which contains a header and transactions.
Changes in network communications:
In streaming we now use SerialiedBlocks structure which contain serialization of a header and serializetion of transactions.
In synchronizer Functions fetching blocks are removed or changed to fetch headers. The only place where we still fetch blocks is in the commitSyncer.
Commit:
Certified commit contains blocks, CommittedSubDag contains only headers for now, blocks should be added later. In stake verification we use only headers, since it is enough.
CoreThreadDispatcher: a new command AddBlockHeaders was added. Several Fake/Mock dispatchers were merged into one.
Storage: we store now both headers and full blocks. If blocks are written to disk then their headers are written there too. There is a possibility to read blocks or only headers. Changes implemented both for mem_store and RocksDB.
Linearizer now operates with headers.
DagState and BlockManager:
DagState works with headers now since for building DAG only headers are important. However, we still store recent_blocks since we need to request blocks for fetching.
Functions such as accept block work with headers now. However, there can be situations when we already have data but the corresponding block header is not accepted yet. We can also have accepted header without data. Structures for storing such blocks and headers were added to BlockManager. They should be cleaned at some point, it is something to be done in future PRs.
Links to any relevant issues
closes #6707.
Type of change
Choose a type of change, and delete any options that are not relevant.
How the change has been tested
Change checklist
Tick the boxes that are relevant to your changes, and delete any items that are not.
Release Notes