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

Extrinsics root is calculated as part of block-building #120

Merged
merged 11 commits into from
Apr 12, 2018

Conversation

gavofyork
Copy link
Member

No description provided.

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Apr 9, 2018
@@ -68,6 +75,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> ];
Copy link
Contributor

@rphmeier rphmeier Apr 9, 2018

Choose a reason for hiding this comment

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

shouldn't this be cleared in finalise? all the values are taken out of it in derive_extrinsics but is it possible it might leave residuals in storage?

Copy link
Member Author

Choose a reason for hiding this comment

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

shouldn't be possible.

Copy link
Contributor

@rphmeier rphmeier Apr 10, 2018

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@rphmeier rphmeier Apr 11, 2018

Choose a reason for hiding this comment

The 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.

@rphmeier rphmeier mentioned this pull request Apr 10, 2018
@@ -174,7 +177,7 @@ impl<B: Backend> PolkadotApi for Client<B, NativeExecutor<LocalDispatch>>
fn check_id(&self, id: BlockId) -> Result<CheckedId> {
// bail if the code is not the same as the natively linked.
if self.code_at(&id)? != LocalDispatch::native_equivalent() {
bail!(ErrorKind::UnknownRuntime);
warn!("This node is out of date. Block authoring may not work correctly.")
Copy link
Contributor

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.

Copy link
Contributor

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!)

Copy link
Member Author

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.

Copy link
Contributor

@rphmeier rphmeier Apr 11, 2018

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

@rphmeier rphmeier added A5-grumble and removed A0-please_review Pull request needs code review. labels Apr 11, 2018
demo/runtime/wasm/target/wasm32-unknown-unknown/release/demo_runtime.compact.wasm
demo/runtime/wasm/target/wasm32-unknown-unknown/release/demo_runtime.wasm
polkadot/runtime/wasm/target/wasm32-unknown-unknown/release/polkadot_runtime.compact.wasm
polkadot/runtime/wasm/target/wasm32-unknown-unknown/release/polkadot_runtime.wasm
substrate/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.compact.wasm
substrate/test-runtime/wasm/target/wasm32-unknown-unknown/release/substrate_test_runtime.wasm
@gavofyork gavofyork added A0-please_review Pull request needs code review. and removed A5-grumble labels Apr 11, 2018
@arkpar
Copy link
Member

arkpar commented Apr 11, 2018

What's the reason for downgrading log?

@gavofyork
Copy link
Member Author

It was actually never upgraded, and the rest of the codebase relied on a symbol not in 0.4.

@rphmeier rphmeier merged commit 5199bc1 into master Apr 12, 2018
@gavofyork gavofyork deleted the gav-runtime-xts branch April 12, 2018 10:19
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
JoshOrndorff pushed a commit to moonbeam-foundation/substrate that referenced this pull request Apr 21, 2021
* Fixes workflow draft release & Adds doc

* Fixes draft conditions
liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
* change pendingorders orderpair {first, second, precision} => {first, second}
* format code and modify test
liuchengxu added a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
* Split out cli crate

* Rearrange the deps alphabetically

* https

* Add crates to workspace memembers

* ChainX runtime should require unwinding too

* .

* Nits

* Reorg assets-runtim-api crate

Should be organized as follows:

assets/
    rpc/
        runtime-api/
            Cargo.toml
       Cargo.toml
    Cargo.toml
liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022
…signature

Derive local challenge from signature instead of public key hash
helin6 pushed a commit to boolnetwork/substrate that referenced this pull request Jul 25, 2023
* Fix optional store items.

* Support querying a block hash.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants