Allow transaction for offchain indexing #7290

cheme · 2020-10-09T14:03:17Z

This PR enables state-machine transactional layer for the offchain indexing.

Move offchain storage change into the general storage change (first commit), this removes the offchain storage change parameter
from some crates api and is a breaking change.
Users no longer instantiate a offchain layer change with method 'enabled' but use 'enable_storage_change' method on existing OverlayStorageChange if needed.
Make use of OverlayedChangeSet with offchain indexing key and value types.

I wonder if some function could be removed, but I tried to keep changes from touching code logic.

preparing use of change set internally.

indexing.

cheme · 2020-10-09T14:05:55Z

primitives/state-machine/src/overlayed_changes/mod.rs


 		Ok(StorageChanges {
 			main_storage_changes: main_storage_changes.collect(),
 			child_storage_changes: child_storage_changes.map(|(sk, it)| (sk, it.0.collect())).collect(),
-			#[cfg(feature = "std")]
-			offchain_storage_changes: Default::default(),
+			offchain_storage_changes,


I am wondering how the changes were communicated from state-machine to client-db, with this value set to default?
Edit: probably the change delta was not extracted but use with the change struct as is.

cc @bkchr , I am not hundred percent sure things were not passing through the &mut parameter (something I did remove in this PR (see macro change)), but yes client db probably don't read from the right data.

Which macro changes?

Why wasn't it reading from the correct data?

That's the part you also change in #7940 (just terrible way of saying it :(

TLDR, everything is okay here?

Perfect :D Then merge it :D

tomusdrw

Looks good to me. I wonder what are the consequences now though. Would the offchain changes be reverted in case the block is found non-canon? If yes, then we should rather put the indexing storage changes to LOCAL database not PERSISTENT one, cause the latter have different promises.

tomusdrw · 2020-10-12T09:39:22Z

primitives/state-machine/src/overlayed_changes/mod.rs

+	/// Activate offchain indexing.
+	/// Note that this function must only be call before any transactional
+	/// changes.


Suggested change

/// Activate offchain indexing.

/// Note that this function must only be call before any transactional

/// changes.

/// Activate offchain indexing.

///

/// Note that this function must only be called before any transactional changes.

TBH I'd rather prefer OverlayedChanges::with_offchain_indexing() to make sure it can't be abused (i.e. altered in the middle of execution).

yeap agree with this @tomaka

primitives/core/src/offchain/mod.rs

tomusdrw · 2020-10-12T09:41:13Z

primitives/core/src/offchain/mod.rs

@@ -746,6 +746,14 @@ impl TransactionPoolExt {
 	}
 }

+/// Change to be applied to the offchain worker db in regards to a key.
+#[derive(Debug,Clone,Hash,Eq,PartialEq)]


Suggested change

#[derive(Debug,Clone,Hash,Eq,PartialEq)]

#[derive(Debug, Clone, Hash, Eq, PartialEq)]

tomusdrw · 2020-10-12T09:41:43Z

primitives/state-machine/src/overlayed_changes/mod.rs

+	}
+
+	/// Drain all elements of offchain changeset.
+	pub fn drain_offchain(&mut self) -> OffchainOverlayedChangesIntoIter {


I only see this method being used in tests, where do we actually apply these changes?

offchain_drain_commited is in use for the changes (goes through runtime api macro generated fn at the end.
I tried to do a stack trace:
https://github.com/cheme/substrate/blob/a9e1356208c2eb8e50fa9f1ec977ee6672a6cd6a/primitives/state-machine/src/overlayed_changes/mod.rs#L550
https://github.com/cheme/substrate/blob/a9e1356208c2eb8e50fa9f1ec977ee6672a6cd6a/primitives/state-machine/src/overlayed_changes/mod.rs#L514
https://github.com/cheme/substrate/blob/a9e1356208c2eb8e50fa9f1ec977ee6672a6cd6a/primitives/state-machine/src/overlayed_changes/mod.rs#L500
https://github.com/cheme/substrate/blob/a9e1356208c2eb8e50fa9f1ec977ee6672a6cd6a/client/service/src/client/client.rs#L876

But it is true that old function iterator are not all in use. This one is clearly redundant with offchain_drain_committed, I will switch that.

tomusdrw · 2020-10-12T10:23:46Z

primitives/state-machine/src/overlayed_changes/changeset.rs

 	/// Stores the changes that this overlay constitutes.
-	changes: BTreeMap<StorageKey, OverlayedValue>,
+	pub(crate) changes: BTreeMap<K, OverlayedEntry<V>>,


Is the pub(crate) stuff for tests? I think it would be better to get an accessor method that is only compiled when #[cfg(test)]

It is to access the field in primitives/state-machine/src/overlayed_changes/offchain.rs.
I can move the struct declaration in primitives/state-machine/src/overlayed_changes/mod.rs or use 'pub(super)'. I think I prefer the second option.

cheme · 2020-10-12T11:34:36Z

I wonder what are the consequences now though. Would the offchain changes be reverted in case the block is found non-canon?

No, offchain changes are only reverted when transaction are used during block processing (which happens probably only for contract at this point), so we still got every non-canonical changes commited.
I think being able to only write indexing changes on canonicalization would be a good thing.
It is certainly doable (from client-db write changes into journal and only actually write at canonicalization dropping non canonical journals), it will be very similar code to the non-archive node code.
Though I wonder if it should be call 'local' ('canonical' would make more sense or changing 'local' semantic with a new comment could also do the trick).
Or we want to be able to query non canonical block content to and can use similar code to state-db one (just not on trie node but on values (which means that there will be a little bit of new code logic involve)).
That would be probably a more straightforward way to implement 'LOCAL' than #7291 (in which we want to be able to query any chain state which is that much needed).

So, what is the more suitable?

changes only written when canonical (indexing of non canonical is not doable).
changes only written in indexing when canonical, and in memory state-db to be able to query non-canonical.

tomusdrw · 2020-10-12T12:00:41Z

No, offchain changes are only reverted when transaction are used during block processing (which happens probably only for contract at this point), so we still got every non-canonical changes commited.

That's okay then.

The Indexing API was designed for PERSISTENT storage (dont-care-if-canonical one) in mind, and imho for backward compatibility it should stay the same. We can add a parameter to write to (fork-)LOCAL one too if needed, but only as an option.

Or we want to be able to query non canonical block content to and can use similar code to state-db one (just not on trie node but on values (which means that there will be a little bit of new code logic involve)).
That would be probably a more straightforward way to implement 'LOCAL' than #7291 (in which we want to be able to query any chain state which is that much needed).

I'm not sure I fully understand this option here though. My idea of LOCAL was that it always reflects the state of a particular fork - i.e. only stuff that was actually written to the storage by ancestors of the current block (no fork-garbage). If you are running in the context of a block that is on canonical chain, you will indeed see the "canonical" storage, but subsequent blocks imported to the canonical chain should not really affect what the OCW sees (hence "locality").
If that's not feasible to implement I'm okay with some other ideas (which I presume #7291 implements), but we need to carefully document the behavior of such storage.

cheme · 2020-10-12T12:21:27Z

I think #7291 is a bit overcomplicated for the use case. (if we only need to be able to access a single state).

If you are running in the context of a block that is on canonical chain, you will indeed see the "canonical" storage, but subsequent blocks imported to the canonical chain should not really affect what the OCW sees (hence "locality").

IIUC this implies option 1 is enough (put changes in journals and only write them to db on canonicalization, so when you query content it is only canonical content), which is probably the better choice (simpler and OW probably only care with finalized content most of the time).

cheme · 2020-10-14T16:12:04Z

I got the option 1 (journal change and only write them on block finalization) implemented in cheme/substrate@offchain_indexing_tx...cheme:offchain_indexing_tx_2 (did not update most doc and lacks testing).
I think it is fine, it allows writing offchain local storage through the indexing api only, and then querying this will report state of finalized block.
I was also thinking a bit about allowing update through non indexing api, but that would probably need storing pending change differently (having a single encoded journal would be bad, and individual items should probably be use), seems rather doable.
In this case the last modification done to a value would apply on finalization (we cannot use compare_and_set), and may be rather random, not sure it is a good idea to implement this way.

tomusdrw · 2020-10-19T11:43:59Z

I think it is fine, it allows writing offchain local storage through the indexing api only, and then querying this will report state of finalized block.

Wait, that completely changes semantics and is going to be really cumbersome (close to impossible) to use. OCWs don't have any idea about finality and the purpose of Indexing API is to pass data directly to OCWs. If the changes are only applied on finality it means OCWs would need to access the state of the finalized block somehow and it's not currently possible (they only run right after the latest new block is imported and can only access state of that block).

This solution could be acceptable (although imho quite limiting) if we re-triggered OCWs for finalized blocks passing this information somehow, but currently it's not how it works at all.

cheme · 2020-10-19T12:02:38Z

I guess I misinterpreted you previous reply, it sounds like what is targetted by #7291 or option 2 (query does report non canonical change for a given fork).
Option 1 which is rather straight forward implementation is indeed different semantic, I am not sure if it can be useful, the fact that the ocw can trigger with a different code than the one of the finalized bloc is indeed problematic: would require to trigger ocw on finalization explicitely (with the code on chain at the time).
To avoid the massive code of #7291, option 1 could be overloaded with a changeset layer that reapply updates on reorg (a non negligeable cost but should be rather ok for our chain), probably not super hard to implement. I will only allows to query state of the tip of the current fork when #7291 aims to allow queries at any state.

cheme · 2020-10-20T08:43:27Z

Maybe this discussion (local ocw) should move to #7291, since it is a bit out of the scope of this PR (just have offchain indexing conform to state-machine transactional layer).

About idea to use some changeset layer from previous comment, I think it can only lead to concurrency issue, and approach of #7291 (explicitely allow change for a given block hash) is correct.
Still we could have a changeset managed for every blocks, but this is generally the same as #7291 implemented in a different way, I will probably forget this idea.

tomusdrw

lgtm! Couple of typedefs could be introduced to make the iterator types more readable.

tomusdrw · 2020-10-20T10:00:37Z

primitives/state-machine/src/overlayed_changes/offchain.rs

+		btree_map::Iter<'i, (Vec<u8>,Vec<u8>), OverlayedEntry<OffchainOverlayedChange>>,
+		fn((&'i (Vec<u8>, Vec<u8>), &'i OverlayedEntry<OffchainOverlayedChange>))
+			-> (&'i (Vec<u8>, Vec<u8>), &'i OffchainOverlayedChange),


Could we add some typedefs here? This is really hard to read :)

tomusdrw · 2020-10-20T10:00:48Z

primitives/state-machine/src/overlayed_changes/offchain.rs

+
+/// Iterate by reference over the prepared offchain worker storage changes.
+pub struct OffchainOverlayedChangesIter<'i> {
+	inner: Option<sp_std::iter::Map<


Can't you use something like impl Iterator<...>?

I wish I could. In fact if I remove all OffchainIterator struct and just have a function return impl Iterator<...> it will work nice and easy.
I wanted to keep those iterator types to avoid breaking too much the api, but I guess I will try to remove them, I am not sure they are usefull here.

tomusdrw · 2020-10-20T10:01:24Z

primitives/state-machine/src/overlayed_changes/offchain.rs

+		btree_map::IntoIter<(Vec<u8>,Vec<u8>), OverlayedEntry<OffchainOverlayedChange>>,
+		fn(((Vec<u8>, Vec<u8>), OverlayedEntry<OffchainOverlayedChange>))
+			-> ((Vec<u8>, Vec<u8>), OffchainOverlayedChange),


Again, deserves a bunch of typedefs. Especially the (Vec<u8>, Vec<u8>) stuff across the entire file.

tomusdrw · 2020-10-20T10:01:56Z

primitives/state-machine/src/testing.rs

@@ -101,23 +95,21 @@ impl<H: Hasher, N: ChangesTrieBlockNumber> TestExternalities<H, N>
 		let changes_trie_config = storage.top.get(CHANGES_TRIE_CONFIG)
 			.and_then(|v| Decode::decode(&mut &v[..]).ok());
 		overlay.set_collect_extrinsics(changes_trie_config.is_some());
+		overlay.enable_offchain_indexing();


I guess it's not possible to move that to the constructor function?

IIRC that is totally doable (I did consider it), will try.

tomusdrw · 2020-10-20T10:02:53Z

Maybe this discussion (local ocw) should move to #7291, since it is a bit out of the scope of this PR (just have offchain indexing conform to state-machine transactional layer).

💯

primitives/state-machine/src/overlayed_changes/changeset.rs

primitives/state-machine/src/overlayed_changes/mod.rs

primitives/state-machine/src/overlayed_changes/changeset.rs

athei

LGTM

bkchr · 2021-01-20T15:59:52Z

@cheme I think tests are missing?

cheme · 2021-01-20T16:11:03Z

yes will complete that at state-machine level (about the overlay and transaction), do you expect other tests?
(I am thinking about the '/' case and test touching more client but I am not sure if you will handle that in another pr)

bkchr · 2021-01-20T16:15:23Z

I think statemachine level should be enough, as this shouldn't propagate to upper layers ;)

cheme · 2021-01-21T19:27:08Z

I did merge master post #7940 .

I choose as @bkchr to remove the optimization that avoid writing data in state machine (mainly because with test executor it is a bit awkward to use).

Thinking twice I could also have change the default to active and unactivate it in client/service/src/client/call_executor.rs .

Please tell me what you think @todr , @ateissen , @bkchr

Will call 'bœt mërge' tomorrow evening if no disagreement until then.

athei · 2021-01-22T09:21:55Z

No objections. I like the simplification resulting in expressing a None as an empty map. Collections already have a natural None which makes it a good idea to use it when possible.

bkchr · 2021-01-22T09:34:31Z

primitives/state-machine/src/overlayed_changes/mod.rs

@@ -388,6 +390,7 @@ impl OverlayedChanges {
 				.expect("Top and children changesets are started in lockstep; qed");
 			!changeset.is_empty()
 		});
+		self.offchain.overlay_mut().rollback_transaction()?;


Should probably use an expect like for children above?

bkchr · 2021-01-22T09:34:37Z

primitives/state-machine/src/overlayed_changes/mod.rs

@@ -401,6 +404,7 @@ impl OverlayedChanges {
 			changeset.commit_transaction()
 				.expect("Top and children changesets are started in lockstep; qed");
 		}
+		self.offchain.overlay_mut().commit_transaction()?;


bkchr · 2021-01-22T09:34:43Z

primitives/state-machine/src/overlayed_changes/mod.rs

@@ -414,6 +418,7 @@ impl OverlayedChanges {
 			changeset.enter_runtime()
 				.expect("Top and children changesets are entering runtime in lockstep; qed")
 		}
+		self.offchain.overlay_mut().enter_runtime()?;


bkchr · 2021-01-22T09:34:50Z

primitives/state-machine/src/overlayed_changes/mod.rs

@@ -427,6 +432,7 @@ impl OverlayedChanges {
 			changeset.exit_runtime()
 				.expect("Top and children changesets are entering runtime in lockstep; qed");
 		}
+		self.offchain.overlay_mut().exit_runtime()?;


bkchr · 2021-01-22T09:37:20Z

primitives/state-machine/src/overlayed_changes/mod.rs


 		Ok(StorageChanges {
 			main_storage_changes: main_storage_changes.collect(),
 			child_storage_changes: child_storage_changes.map(|(sk, it)| (sk, it.0.collect())).collect(),
-			#[cfg(feature = "std")]
-			offchain_storage_changes: Default::default(),
+			offchain_storage_changes,


Which macro changes?

bkchr · 2021-01-22T09:38:49Z

primitives/state-machine/src/overlayed_changes/mod.rs


 		Ok(StorageChanges {
 			main_storage_changes: main_storage_changes.collect(),
 			child_storage_changes: child_storage_changes.map(|(sk, it)| (sk, it.0.collect())).collect(),
-			#[cfg(feature = "std")]
-			offchain_storage_changes: Default::default(),
+			offchain_storage_changes,


Why wasn't it reading from the correct data?

cheme · 2021-01-22T12:27:37Z

bot merge

ghost · 2021-01-22T12:27:42Z

Trying merge.

cheme added 2 commits October 8, 2020 19:50

Moving offchain change set to state machine overlay change set,

afc95aa

preparing use of change set internally.

Make change set generic over key and value, and use it for offchain

5074e08

indexing.

cheme added A0-please_review Pull request needs code review. D2-breaksapi C1-low PR touches the given topic and has a low impact on builders. labels Oct 9, 2020

cheme requested a review from tomusdrw as a code owner October 9, 2020 14:03

cheme commented Oct 9, 2020

View reviewed changes

cheme added 4 commits October 9, 2020 16:30

Merge branch 'master' into offchain_indexing_tx

dfba4df

test ui change

551c7bf

remaining delta

55d038a

generating with standard method

a9e1356

tomusdrw approved these changes Oct 12, 2020

View reviewed changes

Remove 'drain_committed' function, and documentation.

273b35c

tomusdrw approved these changes Oct 20, 2020

View reviewed changes

tomusdrw added A6-mustntgrumble and removed A0-please_review Pull request needs code review. labels Oct 20, 2020

cheme added 3 commits October 20, 2020 15:03

Default constructor for enabling offchain indexing.

9dfce19

Remove offchain change specific iterators.

35f6877

Merge branch 'master' into offchain_indexing_tx

12d1c9d

athei reviewed Oct 27, 2020

View reviewed changes

primitives/state-machine/src/overlayed_changes/changeset.rs Outdated Show resolved Hide resolved

athei reviewed Oct 27, 2020

View reviewed changes

primitives/state-machine/src/overlayed_changes/mod.rs Outdated Show resolved Hide resolved

cheme added 3 commits November 25, 2020 14:22

fix line break

01f3dbd

Merge branch 'master' into offchain_indexing_tx

33106ce

missing renamings

c862d7b

cheme added the A0-please_review Pull request needs code review. label Dec 8, 2020

gnunicorn added the A3-needsresolving label Jan 7, 2021

cheme added 3 commits January 20, 2021 13:27

Merge branch 'master' into offchain_indexing_tx

4894da4

fix import

e2813af

fix new state-machine tests.

b0fc789

cheme removed A3-needsresolving A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. labels Jan 20, 2021

athei reviewed Jan 20, 2021

View reviewed changes

primitives/state-machine/src/overlayed_changes/changeset.rs Outdated Show resolved Hide resolved

Don't expose InnerValue type.

bd82401

athei approved these changes Jan 20, 2021

View reviewed changes

cheme added 5 commits January 20, 2021 17:44

Add test similar to set_storage.

7e99e2d

Merge branch 'master' into offchain_indexing_tx

b13c279

Merge branch 'master' into offchain_indexing_tx

d10dd3c

Remove conditional offchain storage (hard to instantiate correctly).

443e05f

fix

c796fbc

bkchr approved these changes Jan 22, 2021

View reviewed changes

cheme added 2 commits January 22, 2021 11:03

offchain as children cannot fail if top doesn't

59f617c

Merge branch 'master' into offchain_indexing_tx

0ec6d16

ghost merged commit 947a6bc into paritytech:master Jan 22, 2021

This pull request was closed.

	#[derive(Debug,Clone,Hash,Eq,PartialEq)]
	#[derive(Debug, Clone, Hash, Eq, PartialEq)]

Allow transaction for offchain indexing #7290

Allow transaction for offchain indexing #7290

Conversation

cheme commented Oct 9, 2020

cheme Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheme commented Oct 12, 2020

tomusdrw commented Oct 12, 2020

cheme commented Oct 12, 2020

cheme commented Oct 14, 2020

tomusdrw commented Oct 19, 2020 • edited Loading

cheme commented Oct 19, 2020

cheme commented Oct 20, 2020

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw commented Oct 20, 2020

athei left a comment

Choose a reason for hiding this comment

bkchr commented Jan 20, 2021

cheme commented Jan 20, 2021

bkchr commented Jan 20, 2021

cheme commented Jan 21, 2021

athei commented Jan 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheme commented Jan 22, 2021

ghost commented Jan 22, 2021

cheme Oct 9, 2020 •

edited

Loading

tomusdrw commented Oct 19, 2020 •

edited

Loading