DB-based blockchain data cache for light nodes by svyatonik · Pull Request #251 · paritytech/substrate

svyatonik · 2018-06-26T11:36:30Z

on top of #250
this PR is for this commit only

This PR is about implementing authorities_at (and in future - code_at) cache as was first mentioned in this comment.

So idea is: when we're importing block, we're checking justification, which includes fetching authorities_at (at the parent' block state) from remote node (on light clients). We're adding this data to the BlockImportOperation and committing it into the list-style db-cache when committing transaction. The cache is organized in the linked list form. Meta column holds the reference to the tail node (most recent entry in our case) + every node keeps the reference to the previous list element. Blocks between two nodes are guaranteed to have the same authorities_at value (the one that we've passed to the BlockImportOperation before). List entries for 'ancient' blocks (> 2048) are got pruned.

In the future (when we'll have 'digest signals' about changes), BlockImportOperation must be altered to mark that authorities_athas/has not changed in the block we're importing.

The same cache will probably be used later for the code_at cache.

pepyakin · 2018-06-29T14:42:29Z

+ // prune authorities from 'ancient' blocks
+ let mut update_best_authorities = best_authorities.is_some();
+ if let Some(ancient_number) = number.checked_sub(AUTHORITIES_ENTRIES_TO_KEEP) {
+ if self.cache.authorities_at_cache().prune_entries(&mut transaction, As::sa(ancient_number))?.1 {


I think it might be a little better if the return value of prune_entries be destructed and named on the separate line

pepyakin · 2018-06-29T15:23:03Z

- self.executor.call(id, "authorities",&[])
- .and_then(|r| Vec::<AuthorityId>::decode(&mut &r.return_data[..])
- .ok_or(error::ErrorKind::AuthLenInvalid.into()))
+ match self.backend.blockchain().cache() .and_then(|cache| cache.authorities_at(*id)) {


unneeded space between .cache() and .and_then

svyatonik · 2018-07-02T05:19:57Z

As @pepyakin suggested (externally), it would be good to to leave at least 1 entry in cache (the newest one) when pruning. Will do (I hope tonight/tomorrow).

gnunicorn

Some house-style remarks, some docs and a few logic questions I'd like to have addressed.

gnunicorn · 2018-07-23T08:20:53Z

+ <<Block as BlockT>::Header as HeaderT>::Number: As<u64>,
+{
+	/// Create new cache.
+	pub fn new(db: Arc<KeyValueDB>, block_index_column: Option<u32>, authorities_column: Option<u32>) -> ClientResult<Self> {


please wrap.

gnunicorn · 2018-07-23T08:37:50Z

+	fn authorities_at(&self, at: BlockId<Block>) -> Option<Vec<AuthorityId>> {
+ let authorities_at = read_id(&*self.db, self.block_index_column, at).and_then(|at| match at {
+ Some(at) => self.authorities_at.value_at_key(at),
+ None => Ok(None),


I am a bit puzzled about this. How is it possible that we'd ever come to None in here?

My guess - when we're calling authorities_at(BlockId::Hash(unknown_hash)). Or am I wrong? :)
Anyway - it is the cache, I'd prefer to write cache code without any extra expects, since this is an optimization anyway.

gnunicorn · 2018-07-23T08:38:35Z

+}
+
+/// Database-backed blockchain cache which holds its entries as a list.
+/// The meta column holds the pointer to the best known cache entry and


what is the definition of best in this context?

gnunicorn · 2018-07-23T08:39:59Z

+ Some(block) => {
+ let valid_from = db_key_to_number(&block)?;
+ read_storage_entry::<Block, T>(&*db, column, valid_from)
+ .map(|entry| Some(Entry {


this is some deep listing ... can we split this up to use less indentations (by using multiple .and_then or something)?

gnunicorn · 2018-07-23T08:41:02Z

+
+	/// Commits the new best pending value to the database. Returns Some if best entry must
+	/// be updated after transaction is committed.
+	pub fn commit_best_entry(&self, transaction: &mut DBTransaction, valid_from: <<Block as BlockT>::Header as HeaderT>::Number, pending_value: Option<T>) -> Option<Entry<<<Block as BlockT>::Header as HeaderT>::Number, T>> {


please wrap.

gnunicorn · 2018-07-23T09:07:21Z

 Block: BlockT,
 {
-	fn import_block(&self, block: Block, justification: ::bft::Justification<Block::Hash>) {
+	fn import_block(&self, block: Block, justification: ::bft::Justification<Block::Hash>, authorities: &[AuthorityId]) {


please wrap.

gnunicorn · 2018-07-23T09:07:44Z

+	#[test]
+	fn client_uses_authorities_from_blockchain_cache() {
+ let client = test_client::new();
+ test_client::client::in_mem::cache_authorities_at(client.backend().blockchain(), Default::default(), Some(vec![[1u8; 32].into()]));


(and the one following: ) please wrap.

gnunicorn · 2018-07-23T09:08:50Z


 impl<Block: BlockT> light::blockchain::Storage<Block> for Blockchain<Block> {
-	fn import_header(&self, is_new_best: bool, header: Block::Header) -> error::Result<()> {
+	fn import_header(&self, is_new_best: bool, header: Block::Header, authorities: Option<Vec<AuthorityId>>) -> error::Result<()> {


please wrap.

gnunicorn · 2018-07-23T09:09:36Z

+}
+
+/// Insert authorities entry into in-memory blockchain cache. Extracted as a separate function to use it in tests.
+pub fn cache_authorities_at<Block: BlockT>(blockchain: &Blockchain<Block>, at: Block::Hash, authorities: Option<Vec<AuthorityId>>) {


please wrap.

gnunicorn · 2018-07-23T09:10:42Z

 pub trait Storage<Block: BlockT>: BlockchainHeaderBackend<Block> {
 	/// Store new header.
-	fn import_header(&self, is_new_best: bool, header: Block::Header) -> ClientResult<()>;
+	fn import_header(&self, is_new_best: bool, header: Block::Header, authorities: Option<Vec<AuthorityId>>) -> ClientResult<()>;


please wrap.

gnunicorn

Thanks, looks good. Just one last clarification - maybe I am reading this wrong?!?

gnunicorn · 2018-07-24T08:19:19Z

+	}
+
+	/// Prune all entries from the beginning up to the block (including entry at the number). Returns
+	/// the number of pruned entries. Pruning never deletes the latest entry in the cache.


Do you have test for that? Because if I understand L218 correctly, we early quit the function without actually pruning if our loop finds the last entry, no?

svyatonik · 2018-07-24T19:25:18Z

The meaning of L218 is about returning from the function if first entry of the list is for block later than last_to_prune. I.e. if there are entries for blocks [10, 20, 30, 40] and we're calling prune_entries(5) this line will fire, because the first list entry (10) is for block 10 > 5.

There are couple of tests for pruning - see best_authorities_are_updated and best_authorities_are_pruned.

Thanks to your comment, I've found an issue with incorrect reporting of number of pruned entries - they have included the last entry even if it was not physically deleted (actually it was deleted and then inserted back into the cache). Fixed it + fixed best_authorities_are_pruned test (now it receives actual number of deleted entries) + added comments to the test. Thanks!

gavofyork · 2018-07-28T08:16:38Z

conficts...

…h/polkadot into light_blockchain_cache

* master: (86 commits) Make contract a separate runtime module (#345) Version bump (#450) DB-based blockchain data cache for light nodes (#251) Update libp2p again (#445) Update version on git head change (#444) Fix the public key of bootnode 3 (#441) Update libp2p (#442) Switch to the master branch of libp2p (#427) Export ws port 9944 and add doc (#440) Iterate over overlay to decide which keys to purge (#436) Exit signal gets its own trait (#433) Add docker image (#375) Reset peers.json if the content is not loadable (#405) Limit number of incoming connections (#391) Fix memory leaks in libp2p (#432) Do not queue empty blocks set for import (#431) 5 random fixes (#1) (#435) Chore: fix typo (#434) Prevent building invalid blocks (#430) Better logging for public key mismatch (#429) ...

* Improve code and dependencies Signed-off-by: koushiro <koushiro.cqx@gmail.com> * remove confused alias Signed-off-by: koushiro <koushiro.cqx@gmail.com>

…armer Subspace networking (farmer)

use db in light clients
28f2c39

svyatonik added A0-please_review Pull request needs code review. A1-onice labels Jun 26, 2018

svyatonik mentioned this pull request Jun 26, 2018
Fetching storage proofs by light client #252
Merged

svyatonik added 4 commits June 28, 2018 16:37

fixed comment
142f1fe

fixed grumbles
68b08e2

Merge branch 'master' into light_separate_db
b61d155

blockchain data cache
702ea9f

svyatonik force-pushed the light_blockchain_cache branch from f5e88b3 to 702ea9fCompare June 29, 2018 10:47

pepyakin reviewed Jun 29, 2018
View reviewed changes

svyatonik added 2 commits June 30, 2018 00:50

Merge branch 'master' into light_blockchain_cache
7366933

fixed grumbles
a2b6ea3

svyatonik removed the A1-onice label Jun 29, 2018

fix compilation
a2b08fd

svyatonik added A5-grumble and removed A0-please_review Pull request needs code review. labels Jul 2, 2018

svyatonik added 2 commits July 17, 2018 09:51

Merge branch 'master' into light_blockchain_cache
2087479

keep the last entry in the cache
ed1cbfd

svyatonik added A0-please_review Pull request needs code review. and removed A5-grumble labels Jul 17, 2018

gnunicorn self-assigned this Jul 23, 2018

gnunicorn suggested changes Jul 23, 2018
View reviewed changes

gnunicorn added A5-grumble and removed A0-please_review Pull request needs code review. labels Jul 23, 2018

fixed grumbles
1f57347

svyatonik added A0-please_review Pull request needs code review. and removed A5-grumble labels Jul 23, 2018

gnunicorn approved these changes Jul 24, 2018
View reviewed changes

gnunicorn added the A6-mustntgrumble label Jul 24, 2018

gnunicorn removed the A0-please_review Pull request needs code review. label Jul 24, 2018

svyatonik added 2 commits July 24, 2018 22:16

fixed reporting of pruned entries
9e04c8f

Merge branch 'master' into light_blockchain_cache
7689c96

svyatonik added A0-please_review Pull request needs code review. and removed A6-mustntgrumble labels Jul 24, 2018

Merge branch 'master' into light_blockchain_cache
edd3169

svyatonik added 2 commits July 29, 2018 00:42

Merge branch 'master' into light_blockchain_cache
da62aae

Merge branch 'light_blockchain_cache' ofhttps://github.com/paritytec…
de48e8d
…h/polkadot into light_blockchain_cache

gavofyork merged commit 5e1cb6b into masterJul 29, 2018

gavofyork deleted the light_blockchain_cache branch July 29, 2018 11:21

liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022
Merge pull requestparitytech#251from subspace/subspace-networking-f…
e83a207
…armer Subspace networking (farmer)

helin6 pushed a commit to boolnetwork/substrate that referenced this pull request Jul 25, 2023
update jsonrpsee (paritytech#251)
3b362fb

Uh oh!

Conversation

svyatonik commented Jun 26, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svyatonik commented Jul 2, 2018

Uh oh!

gnunicorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnunicornJul 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnunicorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svyatonik commented Jul 24, 2018

Uh oh!

gavofyork commented Jul 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

svyatonik commented Jun 26, 2018•
edited
Loading

gnunicornJul 23, 2018•
edited
Loading