-
Notifications
You must be signed in to change notification settings - Fork 23
feat: add support for bsc parlia consensus #4
Conversation
crates/interfaces/Cargo.toml
Outdated
@@ -40,6 +40,7 @@ tokio = { workspace = true, features = ["full"] } | |||
secp256k1 = { workspace = true, features = ["alloc", "recovery", "rand"] } | |||
|
|||
[features] | |||
bsc = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. []
is legal. This means this feature requires no additional crate.
crates/interfaces/src/consensus.rs
Outdated
|
||
/// Error type transparently wrapping ParliaConsensusError. | ||
#[error(transparent)] | ||
ParliaConsensusError(#[from] ParliaConsensusError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be more concrete error, but not Parliaxxx
error, for example InvalidDifficulty
error is kind of HeaderValidationError
, let us try to reuse current trait and add more Enum type to current errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. This is a transparent error. I do implement a ParliaConsensusError
enum struct.
crates/revm/src/factory.rs
Outdated
chain_spec: Arc<ChainSpec>, | ||
stack: Option<InspectorStack>, | ||
/// Type that defines how the produced EVM should be configured. | ||
evm_config: EvmConfig, | ||
/// Parlia consensus instance | ||
parlia_consensus: Option<Arc<Parlia<P>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems not a good pattern, I did not see anywhere use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
crates/revm/src/processor.rs
Outdated
@@ -60,9 +72,11 @@ pub struct EVMProcessor<'a, EvmConfig> { | |||
pub(crate) stats: BlockExecutorStats, | |||
/// The type that is able to configure the EVM environment. | |||
_evm_config: EvmConfig, | |||
|
|||
parlia_consensus: Option<Arc<Parlia<P>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems not a good pattern.
If it is used as a flag, we can replace with feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
crates/revm/src/processor.rs
Outdated
} | ||
|
||
#[cfg(feature = "bsc")] | ||
pub fn finalize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to move to consensus.
&mut self, | ||
block: &BlockWithSenders, | ||
executor: &mut EVMProcessor<'_, EvmConfig>, | ||
executor: &mut EVMProcessor<'_, EvmConfig, P>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think EVMProcessor
is a very high level trait, prefer not to inject the Parlia
engine into it.
/// algorithm by | ||
/// DP Mitchell and JA Reeds | ||
|
||
const RNG_LEN: usize = 607; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this file to a more common packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will only be used inside parlia.
@@ -0,0 +1,857 @@ | |||
/// Uniform distribution | |||
/// algorithm by | |||
/// DP Mitchell and JA Reeds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you comment the detailed go implementation lib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does akula handle this random library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied from akula
crates/consensus/parlia/src/lib.rs
Outdated
} | ||
|
||
/// BSC parlia consensus implementation | ||
pub struct Parlia<P> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should't P
have some trait?
crates/consensus/parlia/src/lib.rs
Outdated
)) | ||
} | ||
|
||
pub fn get_snapshot_from_cache(&self, hash: &B256) -> Option<Snapshot> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used in BscEVMProcesser.finalize
crates/consensus/parlia/src/lib.rs
Outdated
} | ||
} | ||
|
||
impl<P> Parlia<P> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see many impl<P> Parlia<P>
, can we structure them better in a more readable way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There're too many method of parlia
, so I try to split them into different topics. Assemble system tx, abi encode/decode, check fork and others
crates/consensus/parlia/src/lib.rs
Outdated
(*STAKE_HUB_CONTRACT, Bytes::from(function.abi_encode_input(&[]).unwrap())) | ||
} | ||
|
||
pub fn unpack_data_into_max_elected_validators(&self, data: &[u8]) -> U256 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these unpack
function still usable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
crates/revm/src/factory.rs
Outdated
chain_spec: Arc<ChainSpec>, | ||
stack: Option<InspectorStack>, | ||
/// Type that defines how the produced EVM should be configured. | ||
evm_config: EvmConfig, | ||
|
||
_phantom: PhantomData<P>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_phantom
seems useless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
crates/revm/src/factory.rs
Outdated
evm_config, | ||
_phantom: PhantomData::default(), | ||
#[cfg(feature = "bsc")] | ||
parlia_cfg: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a factory should not have parlia_cfg
, but it can generate a Processor
that have parlia_cfg
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer to use trait instead of concreat parlia_cfg
?
crates/revm/src/factory.rs
Outdated
self.chain_spec.clone(), | ||
database_state, | ||
self.evm_config.clone(), | ||
); | ||
if let Some(stack) = &self.stack { | ||
evm.set_stack(stack.clone()); | ||
} | ||
#[cfg(feature = "bsc")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not good code pattern, if more chain support, it will be chaos,
crates/revm/src/processor.rs
Outdated
} | ||
|
||
// perf: do not execute empty blocks | ||
if block.body.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even an empty block, may need to consensus finalize (like breath block and so on)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact there should never be empty block on BSC. Anyway, I remove this line.
crates/revm/src/processor.rs
Outdated
total_difficulty: U256, | ||
) -> Result<Vec<Receipt>, BlockExecutionError> { | ||
self.init_env(&block.header, total_difficulty); | ||
let (mut system_tx, mut receipts, mut cumulative_gas_used) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
==> system_txs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixd
crates/revm/src/processor.rs
Outdated
let validator = header.beneficiary; | ||
let parent = self.parlia_consensus.get_header_by_hash(header.number, header.parent_hash)?; | ||
|
||
// The snapshot should be ready after the header stage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we wrap most of the following code into the conseneus parlia crate?
crates/revm/src/processor.rs
Outdated
} | ||
|
||
let nonce = self.db_mut().basic(validator).unwrap().unwrap().nonce; | ||
self.parlia_consensus.distribute_to_validator(nonce, validator, block_reward)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need todo self.transact_system_tx?
crates/stages/src/stages/headers.rs
Outdated
@@ -79,6 +86,24 @@ where | |||
consensus: Arc<dyn Consensus>, | |||
etl_config: EtlConfig, | |||
) -> Self { | |||
#[cfg(feature = "bsc")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it a good pattern to use the feature inside a function?
37d9fc1
to
576673d
Compare
576673d
to
72e843a
Compare
TODO