decode block rlp less often#9252
Conversation
debris
added
A0-pleasereview 🤓
| let uncle_bytes = encode_list(&s.block.uncles); | ||
| s.block.header.set_uncles_hash(keccak(&uncle_bytes)); | ||
| } | ||
| if s.block.header.receipts_root().is_zero() || s.block.header.receipts_root() == &KECCAK_NULL_RLP { |
There was a problem hiding this comment.
as mentioned before, this checks could lead to invalid block being locked, so I removed them
| self, | ||
| engine: &EthEngine, | ||
| seal: Vec<Bytes>, | ||
| ) -> Result<SealedBlock, (Error, LockedBlock)> { |
There was a problem hiding this comment.
LockedBlock returned in Err was never used
| block.header, | ||
| block.transactions, | ||
| view.uncles(), | ||
| block.uncles, |
There was a problem hiding this comment.
removed redundant decoding of uncles
| use verification::queue::kind::blocks::Unverified; | ||
|
|
||
| // create unverified block here so the `keccak` calculation can be cached. | ||
| let unverified = Unverified::from_rlp(bytes)?; |
There was a problem hiding this comment.
removed redundant block deserialization
| fn queue_ancient_block(&self, block_bytes: Bytes, receipts_bytes: Bytes) -> Result<H256, BlockImportError> { | ||
| fn queue_ancient_block(&self, unverified: Unverified, receipts_bytes: Bytes) -> Result<H256, BlockImportError> { | ||
| trace_time!("queue_ancient_block"); | ||
| let header: Header = ::rlp::Rlp::new(&block_bytes).val_at(0)?; |
There was a problem hiding this comment.
removed redundant header deserialization
| } | ||
|
|
||
| #[test] | ||
| fn fails_to_import_block_with_invalid_rlp() { |
There was a problem hiding this comment.
this test is not needed, cause import_block takes already deserialized block as a param
| let (h, number, parent) = { | ||
| let header = view!(BlockView, &block).header_view(); | ||
| (header.hash(), header.number(), header.parent_hash()) | ||
| let block = match Unverified::from_rlp(block) { |
There was a problem hiding this comment.
Block::is_good was doing full block deserialization. It's now replaced with Unverified::from_rlp
| } | ||
|
|
||
| let (h, number, parent) = { | ||
| let header = view!(BlockView, &block).header_view(); |
There was a problem hiding this comment.
removed redundant deserialization
| let block_view = Block::new_from_header_and_body(&header, &body); | ||
| drained.push(BlockAndReceipts { | ||
| block: block_view.rlp().as_raw().to_vec(), | ||
| block: block_view.into_inner(), |
There was a problem hiding this comment.
move data instead of copying it
sorpaas
left a comment
sorpaas
left a comment
There was a problem hiding this comment.
LGTM overall. A small grumble on close's additional clones.
| })); | ||
| s.block.header.set_gas_used(s.block.receipts.last().map_or_else(U256::zero, |r| r.gas_used)); | ||
|
|
||
| let closed = self.close()?; |
There was a problem hiding this comment.
Just want to note that this will have one additional clone of unclosed_state (https://github.com/paritytech/parity-ethereum/blob/a19faf01f347e7f485a1e1a82c47e7715e132dba/ethcore/src/block.rs#L397), which is immediately dropped. Not sure whether it's optimized away by compiler.
There was a problem hiding this comment.
Or maybe we can do it the other way around:
pub fn close(self) -> Result<ClosedBlock, Error> {
let unclosed_state = self.block.state.clone();
let locked = self.close_and_lock()?;
Ok(ClosedBlock {
block: locked.block,
unclosed_state,
})
}
debris
added
A8-looksgood 🦄
tomusdrw
left a comment
tomusdrw
left a comment
There was a problem hiding this comment.
Looks good, we should make sure that:
- We can still sync the mainnet and Kovan (I remember some issues when I touched the close/close_and_lock ifs last time)
- We can avoid the early allocation
on_peer_new_block
| // Commit results | ||
| let mut batch = DBTransaction::new(); | ||
| chain.insert_unordered_block(&mut batch, block, receipts, None, false, true); | ||
| chain.insert_unordered_block(&mut batch, encoded::Block::new(unverified.bytes), receipts, None, false, true); |
There was a problem hiding this comment.
So where does the verification actually happen?
There was a problem hiding this comment.
4 lines above, we verify only header for old blocks. No logic has been changed here
| let header: BlockHeader = header_rlp.as_val()?; | ||
| if header.number() > sync.highest_block.unwrap_or(0) { | ||
| sync.highest_block = Some(header.number()); | ||
| let block = Unverified::from_rlp(r.at(0)?.as_raw().to_vec())?; |
There was a problem hiding this comment.
Can we avoid allocating here? We don't even know if the block is valid at this point.
There was a problem hiding this comment.
good catch!
of course we can, I will fix it in the next pr as the allocation is not new (previously it happened in https://github.com/paritytech/parity-ethereum/pull/9252/files#diff-e192cabaad094a396a522f8a2633daceL184)
There was a problem hiding this comment.
Yes, the point is that it was happening after the initial checks - currently we eagerly allocate for every incoming block.
There was a problem hiding this comment.
but these initial checks check only block header, so if someone crafted a malicious block it would cause an allocation anyway
|
just checked syncing Kovan. It looks solid, |
|
|
||
| let unclosed_state = s.block.state.clone(); | ||
|
|
||
| s.engine.on_close_block(&mut s.block)?; |
There was a problem hiding this comment.
Where does this callback happen after this change?
There was a problem hiding this comment.
| sync_client.engine().register_client(Arc::downgrade(&sync_client) as _); | ||
| for i in 1..4 { | ||
| sync_client.import_block(client.block(BlockId::Number(i)).unwrap().into_inner()).unwrap(); | ||
| sync_client.import_block(Unverified::from_rlp(client.block(BlockId::Number(i)).unwrap().into_inner()).unwrap()).unwrap(); |
There was a problem hiding this comment.
This is a very long and noisy line...
| Ok(block) => block, | ||
| Err(_) => { | ||
| debug!(target: "sync", "Bad block rlp"); | ||
| bad = true; |
There was a problem hiding this comment.
This bad/ if bad {…} smells a little, especially in such a big function. How about just returning here and below with Err(BlockDownloaderImportError::Invalid);?
There was a problem hiding this comment.
@dvdplm we still need to update the number of imported blocks before returning.
| Ok(block) => block, | ||
| Err(_) => { | ||
| debug!(target: "sync", "Bad block rlp"); | ||
| bad = true; |
There was a problem hiding this comment.
@dvdplm we still need to update the number of imported blocks before returning.
…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Propagate transactions for next 4 blocks. (openethereum#9265) decode block rlp less often (openethereum#9252) Fix eternalities tests can_create (missing parameter) (openethereum#9270) Update ref to `parity-common` and update `seek` behaviour (openethereum#9257) Comply EIP-86 with the new definition (openethereum#9140) Check if synced when using eth_getWork (openethereum#9193) (openethereum#9210)
6 participants
in total: