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

Fix some weirdness in offchain_worker #7541

Merged
merged 1 commit into from
Nov 16, 2020
Merged
Changes from all commits
Commits
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
44 changes: 36 additions & 8 deletions frame/executive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@

use sp_std::{prelude::*, marker::PhantomData};
use frame_support::{
storage::StorageValue, weights::{GetDispatchInfo, DispatchInfo, DispatchClass},
StorageValue, StorageMap, weights::{GetDispatchInfo, DispatchInfo, DispatchClass},
traits::{OnInitialize, OnFinalize, OnRuntimeUpgrade, OffchainWorker},
dispatch::PostDispatchInfo,
};
Expand Down Expand Up @@ -453,7 +453,7 @@ where
// We need to keep events available for offchain workers,
// hence we initialize the block manually.
// OffchainWorker RuntimeApi should skip initialization.
let digests = Self::extract_pre_digest(header);
let digests = header.digest().clone();

<frame_system::Module<System>>::initialize(
header.number(),
Expand All @@ -463,15 +463,16 @@ where
frame_system::InitKind::Inspection,
);

// Frame system only inserts the parent hash into the block hashes as normally we don't know
// the hash for the header before. However, here we are aware of the hash and we can add it
// as well.
frame_system::BlockHash::<System>::insert(header.number(), header.hash());

// Initialize logger, so the log messages are visible
// also when running WASM.
frame_support::debug::RuntimeLogger::init();

<AllModules as OffchainWorker<System::BlockNumber>>::offchain_worker(
// to maintain backward compatibility we call module offchain workers
// with parent block number.
Copy link
Member Author

Choose a reason for hiding this comment

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

This was changed here: 8974349#diff-71647e596916530d84fd02546feb833ff703dbf19397d076c499ab7dbf265da2R349

While I don't understand the reason behind this.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR it was because before that change we didn't initialize frame_system with new header data (the call was marked skip_initialize), so it was executed in the context of parent block.

I'm fine with the change, but this is definitely breaking, so we should make sure to include this in release notes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh! Makes sense, but with the change to passing the header we already started executing on the "correct" block.

header.number().saturating_sub(1u32.into())
)
<AllModules as OffchainWorker<System::BlockNumber>>::offchain_worker(*header.number())
}
}

Expand All @@ -481,7 +482,7 @@ mod tests {
use super::*;
use sp_core::H256;
use sp_runtime::{
generic::Era, Perbill, DispatchError, testing::{Digest, Header, Block},
generic::{Era, DigestItem}, Perbill, DispatchError, testing::{Digest, Header, Block},
traits::{Header as HeaderT, BlakeTwo256, IdentityLookup},
transaction_validity::{
InvalidTransaction, ValidTransaction, TransactionValidityError, UnknownTransaction
Expand Down Expand Up @@ -547,6 +548,10 @@ mod tests {
sp_io::storage::set(super::TEST_KEY, "module".as_bytes());
200
}

fn offchain_worker(n: T::BlockNumber) {
assert_eq!(T::BlockNumber::from(1u32), n);
}
}
}

Expand Down Expand Up @@ -1115,4 +1120,27 @@ mod tests {
);
});
}

#[test]
fn offchain_worker_works_as_expected() {
new_test_ext(1).execute_with(|| {
let parent_hash = sp_core::H256::from([69u8; 32]);
let mut digest = Digest::default();
digest.push(DigestItem::Seal([1, 2, 3, 4], vec![5, 6, 7, 8]));

let header = Header::new(
1,
H256::default(),
H256::default(),
parent_hash,
digest.clone(),
);

Executive::offchain_worker(&header);

assert_eq!(digest, System::digest());
assert_eq!(parent_hash, System::block_hash(0));
assert_eq!(header.hash(), System::block_hash(1));
});
}
}