-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Extrinsics root is calculated as part of block-building #120
Changes from 5 commits
12a3235
2df8297
a5d5d46
0ebfccf
3624417
dd37fec
01a55d4
f43b69f
bed4492
39d0ddf
0b929de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ authors = ["Parity Technologies <[email protected]>"] | |
|
||
[dependencies] | ||
error-chain = "0.11" | ||
log = "0.3" | ||
polkadot-executor = { path = "../executor" } | ||
polkadot-runtime = { path = "../runtime" } | ||
polkadot-primitives = { path = "../primitives" } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ version = "0.1.0" | |
authors = ["Parity Technologies <[email protected]>"] | ||
|
||
[dependencies] | ||
log = "0.4.0" | ||
transaction-pool = "1.9.0" | ||
error-chain = "0.11" | ||
polkadot-api = { path = "../api" } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ authors = ["Parity Technologies <[email protected]>"] | |
|
||
[dependencies] | ||
parking_lot = "0.4" | ||
log = "0.4" | ||
error-chain = "0.11" | ||
jsonrpc-core = { git="https://github.com/paritytech/jsonrpc.git" } | ||
jsonrpc-macros = { git="https://github.com/paritytech/jsonrpc.git" } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,17 @@ use codec::Slicable; | |
#[cfg(any(feature = "std", test))] | ||
use runtime_io::{twox_128, TestExternalities}; | ||
|
||
/// Compute the extrinsics root of a list of extrinsics. | ||
pub fn extrinsics_root<H: Hashing, E: codec::Slicable>(extrinsics: &[E]) -> H::Output { | ||
extrinsics_data_root::<H>(extrinsics.iter().map(codec::Slicable::encode).collect()) | ||
} | ||
|
||
/// Compute the extrinsics root of a list of extrinsics. | ||
pub fn extrinsics_data_root<H: Hashing>(xts: Vec<Vec<u8>>) -> H::Output { | ||
let xts = xts.iter().map(Vec::as_slice).collect::<Vec<_>>(); | ||
H::enumerated_trie_root(&xts) | ||
} | ||
|
||
pub trait Trait { | ||
type Index: Parameter + Default + SimpleArithmetic + Copy; | ||
type BlockNumber: Parameter + SimpleArithmetic + Default + Bounded + Copy; | ||
|
@@ -68,6 +79,7 @@ decl_storage! { | |
pub BlockHash get(block_hash): b"sys:old" => required map [ T::BlockNumber => T::Hash ]; | ||
|
||
pub ExtrinsicIndex get(extrinsic_index): b"sys:xti" => required u32; | ||
pub ExtrinsicData get(extrinsic_data): b"sys:xtd" => required map [ u32 => Vec<u8> ]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be cleared in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't be possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (with the current map implementation which may be subject to change). It's a bug waiting to happen IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's both notionally and logically impossible. furthermore there's no way of definitively clearing a mapping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a later PR that (re-)instates an "list" type might be reasonable though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's logically impossible as soon as mappings get any kind of metadata keys (for example, to enable iteration). This is something we will have to watch out for, but not a blocker for this PR. |
||
RandomSeed get(random_seed): b"sys:rnd" => required T::Hash; | ||
// The current block number being processed. Set by `execute_block`. | ||
Number get(block_number): b"sys:num" => required T::BlockNumber; | ||
|
@@ -170,6 +182,19 @@ impl<T: Trait> Module<T> { | |
pub fn inc_account_index(who: &T::AccountId) { | ||
<AccountIndex<T>>::insert(who, Self::account_index(who) + T::Index::one()); | ||
} | ||
|
||
/// Note what the extrinsic data of the current extrinsic index is. If this is called, then | ||
/// ensure `derive_extrinsics` is also called before block-building is completed. | ||
pub fn note_extrinsic(encoded_xt: Vec<u8>) { | ||
<ExtrinsicData<T>>::insert(Self::extrinsic_index(), encoded_xt); | ||
} | ||
|
||
/// Remove all extrinsics data and save the extrinsics trie root. | ||
pub fn derive_extrinsics() { | ||
let extrinsics = (0..Self::extrinsic_index()).map(<ExtrinsicData<T>>::take).collect(); | ||
let xts_root = extrinsics_data_root::<T::Hashing>(extrinsics); | ||
<ExtrinsicsRoot<T>>::put(xts_root); | ||
} | ||
} | ||
|
||
#[cfg(any(feature = "std", test))] | ||
|
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.
dangerous proposition. I think authorities would prefer to get slashed for being temporarily offline than risk accidentally losing their whole bond.
also means that when we update the runtime, if >2/3 of the authorities have this behavior, they will continue growing the chain with the wrong rules.
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 get why this change is in this PR -- the
genesis.wasm
is a nuisance to work around, but we should just stop checking this stuff in and have an environment variable for when we want to use a specific runtime wasm!)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.
to be honest we're going to have to introduce semantic versioning here anyway. it's impractical to ensure that a node is updated, restarted and where necessary migrated at exactly the block boundary when the runtime changes, so there will need to be changeover periods. bailing here is unreasonable.
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.
right, but there are different approaches to that, and given that the API can change between runtimes, it's likely we might do something based on linking multiple versions of the
polkadot-api
crate. Network, consensus, and other client-level code might change between runtimes as well.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'd rather just have semantic versioning of the API and be done with it.
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 agree to classify that as beyond the scope of this PR and introduce that later?
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