Storage chains: indexing, renewals and reference counting #8265

arkpar · 2021-03-04T10:02:31Z

This adds externalities for indexing and renewing transactions from within the runtime.
Runtime may now mark a specific region within the encoded extrinsic data to be indexed and addressable by hash with storage_index_transaction externality. Another function storage_renew_transaction_index renews a previously indexed transaction and prevents it from being pruned.
Reference counting for indexed data is implemented for parity-db natively and for rocksdb by storing a counter in a separate DB value.

Following PR will add a runtime module and a storage proof inherent.

polkadot companion: paritytech/polkadot#2570

…orage

arkpar · 2021-03-04T10:03:34Z

client/light/src/blockchain.rs

@@ -129,10 +129,10 @@ impl<S, Block> BlockchainBackend<Block> for Blockchain<S> where Block: BlockT, S
 		Err(ClientError::NotAvailableOnLightClient)
 	}

-	fn extrinsic(
+	fn transaction(


Renamed this to transaction since it now returns an indexed portion, rather than all of the extrinsic data

primitives/externalities/src/lib.rs

cheme

New externalities feels a lot like offchain indexing here (do trigger some optional behavior from runtime with no feedback).
So I wonder if this could be achievable through offchain worker consuming offchain indexing data containing those indexing operations (it will need to do re-indexing in a second asynchronous step which can be a bit concurrent with pruning but seems manageable).
Thinking twice attaching the block hash with the indexing operation in the offchain db may not be trivial, just mentioning the possibility.
May also be a bit tricky to decode 'extrinsic_headers' then (two different encoding to manage).

client/api/src/client.rs

client/api/src/backend.rs

cheme · 2021-03-09T15:52:17Z

client/db/src/lib.rs

+			DbHash::from_slice(hash.as_ref()),
+			Some(extrinsic[offset..].to_vec())
+		);
+		extrinsic_headers.push((DbHash::from_slice(&hash.as_ref()), extrinsic[..offset].to_vec()));


extrinsic_headers is mixing extrinsic using applied offset with full extrinsic from renewed, is it ok?
Generally I am not sure how 'renewed' is supposed to use its 'size' field.
Also wondering how/if 'extrinsic[..offset]' can be decoded?
The runtime can certainly find the right offset to split transaction between two field, then when decoding one should use the split variant, yes probably work this way (but then 'renewed' also use this variant and should write the split version).

extrinsic_headers is mixing extrinsic using applied offset with full extrinsic from renewed, is it ok?

Yes, all extrinsics that don't index anything, including renewals, are just written as header.

Also wondering how/if 'extrinsic[..offset]' can be decoded?

It is never decoded on its own. Only full extrinsic (header + indexed portion) is decoded. The indexed region is not supposed to be scale-encoded. It's just some bytes that are interpreted by the user.

Generally I am not sure how 'renewed' is supposed to use its 'size' field.

size field is a bit of work in progress. The idea is that the renewal transaction should include fee that is proportional to the existing data size. Here, in commit we would check existing expected indexed data size and don't renew anything if it does not match. However it is not clear yet if actually checking the size in the runtime would be better.

(for whatever reason I was thinking renewed did contain the indexed content, and did need to be strip, but obviously it doesn't).
I do not mark the conversation as resolved because the reply looks useful to me, but it is resolved.

client/db/src/lib.rs

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

primitives/database/src/kvdb.rs

client/db/src/lib.rs

primitives/database/src/lib.rs

…orage

Co-authored-by: cheme <emericchevalier.pro@gmail.com>

…-tx-storage

cheme

LGTM
I was a bit hesitant about the new extrinsics, I think there may be something todo with offchain workers to avoid the new extrinsics, but I am also pretty sure it involves substantial additional work.
But this can always switch or change later (as long as they are not call in a production chain (IIRC those are not exposed in the wasm as long as it is not used by the wasm)).

arkpar · 2021-03-16T17:05:11Z

bot merge force

ghost · 2021-03-16T17:05:16Z

Trying merge.

ghost · 2021-03-16T17:05:17Z

Merge failed: "At least 2 approving reviews are required by reviewers with write access."

client/api/src/backend.rs

client/api/src/client.rs

client/network/src/bitswap.rs

bkchr · 2021-03-17T19:35:17Z

primitives/state-machine/src/ext.rs

@@ -568,6 +568,43 @@ where
 		}
 	}

+	fn storage_index_transaction(&mut self, index: u32, offset: u32) -> Result<(), ()>{


Why do we return a Result here, when this can not fail?

client/api/src/client.rs

bkchr · 2021-03-17T20:00:25Z

client/db/src/lib.rs

+										if let Some(mut t) = self.indexed_transaction(&hash)? {
+											data.append(&mut t);
+										}
+										Block::Extrinsic::decode(&mut data.as_slice())


If we would use a custom Input here, we could prevent calling append above and allocating a third time.
This custom input would just combine data and t

bkchr · 2021-03-17T20:02:13Z

client/db/src/lib.rs

-									|h| self.extrinsic(&h).and_then(|maybe_ex| maybe_ex.ok_or_else(
-										|| sp_blockchain::Error::Backend(
-											format!("Missing transaction: {}", h))))
+						match Vec::<(Block::Hash, Vec<u8>)>::decode(&mut &body[..]) {


This body structure is really implicitly used here. Maybe we could use at least some typedef.

bkchr · 2021-03-17T20:08:09Z

client/db/src/lib.rs

-									}
+		}
+		// Also discard all blocks from displaced branches
+		for h in displaced.leaves() {


This means before we always kept all blocks, even from forks, but now we will prune all fork blocks? Even when block pruning is not enabled.

bkchr · 2021-03-17T20:12:01Z

client/db/src/lib.rs

+		for h in displaced.leaves() {
+			let mut number = finalized;
+			let mut hash = h.clone();
+			// Follow displaced chains up to finality point


Don't we start at the finalized point and move downwards?

Not really. We start with a displaced leaf and follow its parents until we reach a block that is canonical

Yeah I understood it this way.

Shouldn't the comment say Follow displaced chains up to canonical point?

Since they are displaced due to finality, as soon as we meet a canonical parent it is guaranteed to be a part of a finalized chain. I'll update the comment to make it more obvious.

…orage

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

…-tx-storage

…orage

arkpar · 2021-03-18T09:39:50Z

@bkchr All the issues addressed now I believe. Companion build failure seems unrelated.

bkchr · 2021-03-18T10:14:53Z

client/db/src/utils.rs

+				self.0.read(&mut into[..read])?;
+			}
+			None => {
+				return self.0.read(into)


This means we will never read self.1. For our use case here it is okay, because we always should get Some(), but that should really not be used by someone else.

Maybe we should use unreachable here or make JoinInput directly use Vec<u8> and Option<Vec<u8>>

Changed it to operate on slices instead

arkpar · 2021-03-18T11:46:24Z

bot merge

ghost · 2021-03-18T11:46:27Z

Trying merge.

cheme · 2021-03-19T08:15:08Z

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

@@ -103,12 +103,35 @@ pub struct OverlayedChanges {
 	children: Map<StorageKey, (OverlayedChangeSet, ChildInfo)>,
 	/// Offchain related changes.
 	offchain: OffchainOverlayedChanges,
+	/// Transaction index changes,
+	transaction_index_ops: Vec<IndexOperation>,


Just realized while merging master in some PRs, but 'transaction_index_ops' could also support transaction (Same implementation as 'OffchainOverlayedChanges' for offchain indexing).
Without thinking too much about it, I would think it should (would allow conditional indexing).
cc\ @arkpar

I did not bother with that for now. As I understand, transactional semantics are used here to support rollback on failing extrinsics. Indexing is transparent to the runtime anyway. If an extrinsic fails, that fact that it still indexes something does not affect consensus.
Furthermore:

Storage transactions should not be adding to the index until they are pass the failure point. I.e. all fees are applied.

Unlike regular transactions, failing storage transactions should not be included in the block in the first place. Storing them for free would defeat the purpose.

I don't think its a priority.
I agree consensus is not touched and client can even choose to not index anything at all.
That was the same thing with offchain indexing. Can be implemented at any time.
(I was talking about storage transaction, it is used for different purpose, mainly in contracts afaik)

…#8265) * Transaction indexing * Tests and fixes * Fixed a comment * Style * Build * Style * Apply suggestions from code review Co-authored-by: cheme <emericchevalier.pro@gmail.com> * Code review suggestions * Add missing impl * Apply suggestions from code review Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com> * impl JoinInput * Don't store empty slices * JoinInput operates on slices Co-authored-by: cheme <emericchevalier.pro@gmail.com> Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

arkpar added 5 commits March 3, 2021 12:16

Transaction indexing

1429ae4

Tests and fixes

ee7fdc6

Fixed a comment

2a768c1

Merge branch 'master' of github.com:paritytech/substrate into a-tx-st…

0d2ddfa

…orage

Style

f8b94b9

arkpar added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes labels Mar 4, 2021

arkpar requested a review from gavofyork March 4, 2021 10:02

arkpar commented Mar 4, 2021

View reviewed changes

arkpar added the C1-low PR touches the given topic and has a low impact on builders. label Mar 4, 2021

Build

cac8dd1

github-actions bot added the A7-needspolkadotpr label Mar 4, 2021

Style

4caac84

bkchr reviewed Mar 4, 2021

View reviewed changes

primitives/externalities/src/lib.rs Outdated Show resolved Hide resolved

arkpar mentioned this pull request Mar 5, 2021

Update for the new substrate client API paritytech/polkadot#2570

Merged

cheme reviewed Mar 10, 2021

View reviewed changes

arkpar and others added 5 commits March 15, 2021 11:45

Merge branch 'master' of github.com:paritytech/substrate into a-tx-st…

8ace383

…orage

Apply suggestions from code review

f6ebd7b

Co-authored-by: cheme <emericchevalier.pro@gmail.com>

Code review suggestions

0c2eceb

Merge branch 'a-tx-storage' of github.com:paritytech/substrate into a…

4653955

…-tx-storage

Add missing impl

cc07692

github-actions bot removed the A7-needspolkadotpr label Mar 15, 2021

cheme approved these changes Mar 16, 2021

View reviewed changes

paritytech deleted a comment from smisty Mar 16, 2021

arkpar requested a review from bkchr March 16, 2021 17:08

bkchr reviewed Mar 17, 2021

View reviewed changes

Merge branch 'master' of github.com:paritytech/substrate into a-tx-st…

de2956a

…orage

arkpar and others added 2 commits March 17, 2021 21:33

Apply suggestions from code review

f45ea2e

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

Merge branch 'a-tx-storage' of github.com:paritytech/substrate into a…

1f0033d

…-tx-storage

arkpar added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 17, 2021

arkpar added 3 commits March 18, 2021 07:03

impl JoinInput

776bec2

Don't store empty slices

c9f6576

Merge branch 'master' of github.com:paritytech/substrate into a-tx-st…

22bcf57

…orage

paritytech-ci added the D2-breaksapi label Mar 18, 2021

github-actions bot added the A7-needspolkadotpr label Mar 18, 2021

arkpar added D6-newhostfunctions and removed A3-in_progress Pull request is in progress. No review needed at this stage. A7-needspolkadotpr labels Mar 18, 2021

bkchr approved these changes Mar 18, 2021

View reviewed changes

bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Mar 18, 2021

JoinInput operates on slices

8b57d70

paritytech-ci added the D2-breaksapi label Mar 18, 2021

arkpar removed the D2-breaksapi label Mar 18, 2021

ghost merged commit 462384b into master Mar 18, 2021

ghost deleted the a-tx-storage branch March 18, 2021 11:46

cheme reviewed Mar 19, 2021

View reviewed changes

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage chains: indexing, renewals and reference counting #8265

Storage chains: indexing, renewals and reference counting #8265

arkpar commented Mar 4, 2021 •

edited

Loading

arkpar Mar 4, 2021

cheme left a comment

cheme Mar 9, 2021

arkpar Mar 15, 2021

cheme Mar 16, 2021

cheme left a comment

arkpar commented Mar 16, 2021

ghost commented Mar 16, 2021

ghost commented Mar 16, 2021

bkchr Mar 17, 2021

arkpar Mar 18, 2021

bkchr Mar 17, 2021

bkchr Mar 17, 2021

bkchr Mar 17, 2021

arkpar Mar 17, 2021

bkchr Mar 17, 2021

arkpar Mar 17, 2021

bkchr Mar 17, 2021

arkpar Mar 17, 2021

arkpar commented Mar 18, 2021

bkchr Mar 18, 2021

arkpar Mar 18, 2021

arkpar commented Mar 18, 2021

ghost commented Mar 18, 2021

cheme Mar 19, 2021

arkpar Mar 19, 2021

cheme Mar 19, 2021

Storage chains: indexing, renewals and reference counting #8265

Storage chains: indexing, renewals and reference counting #8265

Conversation

arkpar commented Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

cheme 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

cheme left a comment

Choose a reason for hiding this comment

arkpar commented Mar 16, 2021

ghost commented Mar 16, 2021

ghost commented Mar 16, 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

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

arkpar commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Mar 18, 2021

ghost commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Mar 4, 2021 •

edited

Loading