Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

block cleanup #9117

Merged
merged 14 commits into from
Jul 30, 2018
Merged

block cleanup #9117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a0610a7
blockchain insert expects owned block instead of block reference
debris Jul 13, 2018
b09d311
reduce a number of times a block is deserialized
debris Jul 14, 2018
21d429b
Merge branch 'master' into block_insert
debris Jul 15, 2018
306bb01
removed cached uncle_bytes from block
debris Jul 15, 2018
e60d907
removed is_finalized from OpenBlock
debris Jul 15, 2018
35c63ee
removed unused parity_machine::WithMetadata trait
debris Jul 15, 2018
09d4961
removed commented out code
debris Jul 15, 2018
21d3de2
Merge branch 'master' into block_insert
debris Jul 16, 2018
461a428
Merge branch 'master' into block_insert
debris Jul 19, 2018
e64a006
remove unused metadata from block
debris Jul 19, 2018
a598be7
Merge branch 'master' into block_insert
debris Jul 19, 2018
10d05d7
remove unused metadata from block
debris Jul 19, 2018
8409d11
Merge branch 'master' into block_insert
debris Jul 27, 2018
9e894e2
BlockDetails extras may have at most 5 elements
debris Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 9 additions & 46 deletions ethcore/src/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,6 @@ pub struct ExecutedBlock {
pub traces: Tracing,
/// Hashes of last 256 blocks.
pub last_hashes: Arc<LastHashes>,
/// Finalization flag.
pub is_finalized: bool,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this flag is completely redundant and is never set to true

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're going to use it #9113? cc @andresilva

I think we won't implement Casper again in the short terms, so some of the #8401 refactorings might use a better interface. But I do think allowing finalized block is a feature that we may use in other engines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if I see correctly, it's not used by #9113

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right. Aura only finalize ancestry blocks so it only needs BlockChain::mark_finalized. However, do we want to support instant finalization for some of our engines in the future? That's where we will need this flag.

I'm not sure whether POC-1 engine (which will replace the current Tendermint engine) will need this. The spec do mention instant finality. But I'm not really familiar with the algorithm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

However, do we want to support instant finalization for some of our engines in the future? That's where we will need this flag.

Than we should add this flag in the future. Imo, we shouldn't keep in our codebase anything that is not used right now

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't say I fully agree with this, but it sounds okay to me. 👍

Based on the above rationale, however, another thing we need to remove is metadata (https://github.com/paritytech/parity/blob/21d3de236005732253dee442cfbcb52ebcbbc1e7/ethcore/src/block.rs#L119 and https://github.com/paritytech/parity/blob/21d3de236005732253dee442cfbcb52ebcbbc1e7/ethcore/src/blockchain/extras.rs#L155). Also introduced by #8401, but not used by current engines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

metadata removed! :)

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 remove this field as well:

https://github.com/paritytech/parity-ethereum/blob/a598be7b6e3b0d35006d6c25d99e966b002749c7/ethcore/src/blockchain/extras.rs#L156

Metadata or finalization has not been set by anyone, so we can just change the !is_short_version to be 5 RLP fields. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done ;)

/// Block metadata.
pub metadata: Option<Vec<u8>>,
}

impl ExecutedBlock {
Expand All @@ -138,8 +134,6 @@ impl ExecutedBlock {
Tracing::Disabled
},
last_hashes: last_hashes,
is_finalized: false,
metadata: None,
}
}

Expand Down Expand Up @@ -228,26 +222,6 @@ impl ::parity_machine::Transactions for ExecutedBlock {
}
}

impl ::parity_machine::Finalizable for ExecutedBlock {
fn is_finalized(&self) -> bool {
self.is_finalized
}

fn mark_finalized(&mut self) {
self.is_finalized = true;
}
}

impl ::parity_machine::WithMetadata for ExecutedBlock {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this interface is never used

fn metadata(&self) -> Option<&[u8]> {
self.metadata.as_ref().map(|v| v.as_ref())
}

fn set_metadata(&mut self, value: Option<Vec<u8>>) {
self.metadata = value;
}
}

/// Block that is ready for transactions to be added.
///
/// It's a bit like a Vec<Transaction>, except that whenever a transaction is pushed, we execute it and
Expand All @@ -264,10 +238,7 @@ pub struct OpenBlock<'x> {
#[derive(Clone)]
pub struct ClosedBlock {
block: ExecutedBlock,
uncle_bytes: Bytes,
unclosed_state: State<StateDB>,
unclosed_finalization_state: bool,
unclosed_metadata: Option<Vec<u8>>,
}

/// Just like `ClosedBlock` except that we can't reopen it and it's faster.
Expand All @@ -276,15 +247,13 @@ pub struct ClosedBlock {
#[derive(Clone)]
pub struct LockedBlock {
block: ExecutedBlock,
uncle_bytes: Bytes,
}

/// A block that has a valid seal.
///
/// The block's header has valid seal arguments. The block cannot be reversed into a `ClosedBlock` or `OpenBlock`.
pub struct SealedBlock {
block: ExecutedBlock,
uncle_bytes: Bytes,
}

impl<'x> OpenBlock<'x> {
Expand Down Expand Up @@ -432,14 +401,12 @@ impl<'x> OpenBlock<'x> {
let mut s = self;

let unclosed_state = s.block.state.clone();
let unclosed_metadata = s.block.metadata.clone();
let unclosed_finalization_state = s.block.is_finalized;

s.engine.on_close_block(&mut s.block)?;
s.block.state.commit()?;

s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes())));
let uncle_bytes = encode_list(&s.block.uncles).into_vec();
let uncle_bytes = encode_list(&s.block.uncles);
s.block.header.set_uncles_hash(keccak(&uncle_bytes));
s.block.header.set_state_root(s.block.state.root().clone());
s.block.header.set_receipts_root(ordered_trie_root(s.block.receipts.iter().map(|r| r.rlp_bytes())));
Expand All @@ -451,10 +418,7 @@ impl<'x> OpenBlock<'x> {

Ok(ClosedBlock {
block: s.block,
uncle_bytes,
unclosed_state,
unclosed_metadata,
unclosed_finalization_state,
})
}

Expand All @@ -468,8 +432,8 @@ impl<'x> OpenBlock<'x> {
if s.block.header.transactions_root().is_zero() || s.block.header.transactions_root() == &KECCAK_NULL_RLP {
Copy link
Collaborator Author

@debris debris Jul 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should also get rid of these checks, cause they may lead to invalid blocks being produced. e.g

let closed_block = open_block.close();
let reopened_block = closed_block.reopen(engine);
reopened_block.push_transactions(transactions);
// close_and_lock is not recalculating `transactions_root`
let locked_block = reopened_block.close_and_lock();

s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes())));
}
let uncle_bytes = encode_list(&s.block.uncles).into_vec();
if s.block.header.uncles_hash().is_zero() || s.block.header.uncles_hash() == &KECCAK_EMPTY_LIST_RLP {
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 {
Expand All @@ -485,7 +449,6 @@ impl<'x> OpenBlock<'x> {

Ok(LockedBlock {
block: s.block,
uncle_bytes,
})
}

Expand Down Expand Up @@ -514,7 +477,6 @@ impl ClosedBlock {
pub fn lock(self) -> LockedBlock {
LockedBlock {
block: self.block,
uncle_bytes: self.uncle_bytes,
}
}

Expand All @@ -523,8 +485,6 @@ impl ClosedBlock {
// revert rewards (i.e. set state back at last transaction's state).
let mut block = self.block;
block.state = self.unclosed_state;
block.metadata = self.unclosed_metadata;
block.is_finalized = self.unclosed_finalization_state;
OpenBlock {
block: block,
engine: engine,
Expand All @@ -533,7 +493,6 @@ impl ClosedBlock {
}

impl LockedBlock {

/// Removes outcomes from receipts and updates the receipt root.
///
/// This is done after the block is enacted for historical reasons.
Expand Down Expand Up @@ -566,7 +525,9 @@ impl LockedBlock {
}
s.block.header.set_seal(seal);
s.block.header.compute_hash();
Ok(SealedBlock { block: s.block, uncle_bytes: s.uncle_bytes })
Ok(SealedBlock {
block: s.block
})
}

/// Provide a valid seal in order to turn this into a `SealedBlock`.
Expand All @@ -584,7 +545,9 @@ impl LockedBlock {
// TODO: passing state context to avoid engines owning it?
match engine.verify_local_seal(&s.block.header) {
Err(e) => Err((e, s)),
_ => Ok(SealedBlock { block: s.block, uncle_bytes: s.uncle_bytes }),
_ => Ok(SealedBlock {
block: s.block
}),
}
}
}
Expand All @@ -601,7 +564,7 @@ impl SealedBlock {
let mut block_rlp = RlpStream::new_list(3);
block_rlp.append(&self.block.header);
block_rlp.append_list(&self.block.transactions);
block_rlp.append_raw(&self.uncle_bytes, 1);
block_rlp.append_list(&self.block.uncles);
block_rlp.out()
}
}
Expand Down
Loading