best_containing operations (issue 603) by snd · Pull Request #740 · paritytech/substrate

snd · 2018-09-14T06:23:08Z

This pull request implements the operation for finding the best chain containing a specific block (potentially number-limited to a given block). This is usually the best chain.

This also adds a number of cross-backend tests in the test-client crate used to ensure that behavior is consistent across them.

rphmeier · 2018-09-14T19:45:16Z

+		if maybe_insertion_position.is_none() {
+			return;
+		}
+		let insertion_position = maybe_insertion_position.expect("early return right above. q.e.d.");


refactor as a let x = match { None => return, Some(x) => x }

rphmeier · 2018-09-14T19:46:59Z

+	/// the operation is performed as if there are no blocks greater `max_block_number`
+	/// ignoring all blocks with numbers greater
+	/// TODO [snd] possibly implement this on blockchain::Backend and just redirect here
+	pub fn best_containing(&self, target_hash: Block::Hash, maybe_max_number: Option<<<Block as BlockT>::Header as HeaderT>::Number>) -> error::Result<Option<Block::Hash>> {


I think there's a NumberFor<Block> type definition in the runtime_primitives that might be useful.

rphmeier · 2018-09-14T19:57:09Z

+		}
+
+		// for each chain. longest chain first. shortest last
+		for leaf_hash in self.backend.blockchain().leaves()? {


this loop could be pre-empted by checking if the block with number target_header.number() in the "canonical" chain is target_header. Could return the "best block" in that case, or its ancestor with max_number. Although the import lock probably has to be held for safety with that approach, to ensure that a block import doesn't race against this thread and trigger a reorganization.

i believe i tried this before by checking whether the hash of the block at target_header.number() matches target_hash:

substrate/core/client/src/client.rs

Line 593 in c051949

// let is_target_in_best_chain = self.backend.blockchain().hash(*target_header.number())?.ok_or_else(|| error::Error::from(format!("failed to get hash for block number {}", target_header.number())))? == target_hash;

however self.backend.blockchain().hash(*target_header.number()) returned None
for those numbers that were not uniquely mappable to a hash i.e. there were forks and multiple hashes for that number. i'll investigate again. maybe there's a better method.

The current blockchain backend doesn't handle forks at all, so I've been rewriting it to do so in a local branch.

this loop could be pre-empted by checking if the block with number target_header.number() in the "canonical" chain is target_header

how do i get the header/hash that's in the canonical chain at position number?

i assumed i could do this through ...blockchain().header(BlockId::Number(number)) / ...blockchain().hash(number).

both do not seem to return the header/hash in the canonical chain but rather the header/hash that was last imported at that position.

i've added a test exposing that problem here 57e758b. note that it only tests the in-memory backend.

is this indeed a bug that should be fixed? is there another method to retrieve a block given a number in the canonical chain? should i leave out the pre-emption for now?

both do not seem to return the header/hash in the canonical chain but rather the header/hash that was last imported at that position.

This is exactly what #760 is meant to fix; the underlying blockchain DB literally could not accommodate forks before.

perfect! this should work out then

pre-emption is in. the tests should pass once #760 is merged and this is rebased on it

address grumble #740 (comment)

snd · 2018-09-18T10:38:45Z

when i comment in this code here https://github.com/paritytech/substrate/pull/740/files#diff-5a5b3c7041f582d3b3c912d4bf55aea8R609 which adds block C3 which is here in the block tree https://github.com/paritytech/substrate/pull/740/files#diff-5a5b3c7041f582d3b3c912d4bf55aea8R529 i get

thread 'client::tests::best_containing_with_multiple_forks' panicked at 'called `Result::unwrap()` on an `Err` value: Error(ApplyExtinsicFailed(Stale), State { next_error: None, backtrace: InternalBacktrace { backtrace: None } })', libcore/result.rs:945:5

relevant part: Error(ApplyExtinsicFailed(Stale)

any ideas as to why?

snd · 2018-09-19T14:25:00Z

it's not trivial to Backend.revert the leaves list. should i bother at this time?

it requires removing the reverted block hash, then adding the parent again if the parent has no other children. checking the latter is expensive. we could make it faster by keeping a child counter for each block. that requires additional storage for each block however.

rphmeier · 2018-09-19T23:45:04Z

Correct reversion should definitely prune out forks that branch off of blocks we've discarded. That's also an issue with the current implementation. @arkpar might have some ideas; I'm pretty ambivalent as long as not reverting the leaves list can't break stuff too badly.

snd · 2018-09-20T08:44:05Z

Correct reversion should definitely prune out forks that branch off of blocks we've discarded. That's also an issue with the current implementation. @arkpar might have some ideas; I'm pretty ambivalent as long as not reverting the leaves list can't break stuff too badly.

ok. i'll tackle this last

i found a simpler and faster approach to storing the leaves in the kev-value-db. this shouldn't take much longer

rphmeier · 2018-09-21T15:31:37Z

 use substrate_primitives;
 use substrate_primitives::Blake2Hasher;
-use codec::{Codec, Encode};
+pub use codec::{Codec, Encode, Decode};


i'm not convinced we should re-export these. client already has a parity_codec dependency, perhaps it can import from there?

agreed and fixed

… client tests

addresses #740 (comment)

rphmeier · 2018-09-25T12:29:37Z

+	//	from: Keyring::Alice.to_raw_public().into(),
+	//	to: Keyring::Ferdie.to_raw_public().into(),
+	//	amount: 1,
+	//	nonce: 0,


nonce: 1 should fix it AFAICT

thanks. that fixed the Error(ApplyExtinsicFailed(Stale) issue

snd

looks great! thanks for fixing the backends to make the tests pass and finishing up the leaf list db read/write. i'd say we merge it and do further work on this in other PRs

gavofyork · 2018-09-26T10:17:09Z

 		Ok(BlockchainDb {
 			db,
+			leaves: RwLock::new(leaves),
 			meta: RwLock::new(meta)


we tend to prefer to keep the last , on multiline lists

gavofyork · 2018-09-26T10:17:13Z

 	db: Arc<KeyValueDB>,
-	meta: RwLock<Meta<<Block::Header as HeaderT>::Number, Block::Hash>>,
+	meta: RwLock<Meta<NumberFor<Block>, Block::Hash>>,
+	leaves: RwLock<LeafSet<Block::Hash, NumberFor<Block>>>


we tend to prefer to keep the last , on multiline lists

gavofyork · 2018-09-26T10:18:12Z

+					let route = ::blockchain::tree_route(
+						self,
+						BlockId::Hash(best_hash),
+						BlockId::Hash(*header.parent_hash())


we tend to prefer to keep the last , on multiline lists

gavofyork · 2018-09-26T10:19:08Z

+}
+
+fn genesis_storage() -> StorageMap {
+		let mut storage = genesis_config().genesis_map();


superfluous indent level

gavofyork

Minor style nits. Code quality good and test coverage seems reasonable.

* rough draft of parachains roadmap * flesh out first 2 phases * Fix typo Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * Fix typos and add clarifications Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * remove extra space Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com> * add XCMP work items Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* add log in parse_addr_from_script * refactor for deposit input_addr * refactor for deposit handle

snd added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 14, 2018

rphmeier reviewed Sep 14, 2018

View reviewed changes

rphmeier mentioned this pull request Sep 14, 2018

Non-instant finality and consensus abstraction #746

Closed

rphmeier reviewed Sep 14, 2018

View reviewed changes

snd force-pushed the snd-issue-603 branch from d2b4361 to f2ae190 Compare September 17, 2018 11:44

snd added a commit that referenced this pull request Sep 17, 2018

refactor

f2ae190

address grumble #740 (comment)

snd added a commit that referenced this pull request Sep 17, 2018

refactor using NumberFor

9d23693

address grumble #740 (comment)

rphmeier mentioned this pull request Sep 17, 2018

Introduce notion of finality to substrate #760

Merged

rphmeier reviewed Sep 21, 2018

View reviewed changes

gavofyork added the A1-onice label Sep 24, 2018

snd added 15 commits September 25, 2018 18:07

add stub for Client.best_chain_containing_block_hash

215ea61

add fn blockchain::Backend::leaf_hashes

0b8b5e8

fix typo

f2ad9a5

sketch out Client.best_chain_containing_block_hash

ba36143

fix indent

34076ae

Blockchain.leaf_hashes -> Blockchain.leaves

6577a7b

add unimplemented! stub impls for Blockchain.leaves

4ee90c4

start impl of Blockchain.leaves for in-memory client db

7cb2545

Client.best_chain_containing...: check canonical first and make compile

cf2ed8e

first rough attempt at maintaining leaf list in in-memory db

712e8f0

fix tab indent

d91cf27

add test best_chain_containing_single_block

c1ddba9

add failing test best_chain_containing_with_fork

b019e0e

pub use client::blockchain; in test-client to prevent circular dep in…

48a2323

… client tests

best_chain_containing_with_single_block: improve and test leaves

9936fb9

snd added 3 commits September 25, 2018 18:58

don't reexport codec traits in primitives

12b5b77

addresses #740 (comment)

fix rebase mistake

745b73d

improve LeafSet and use it in state-db

6763e49

rphmeier reviewed Sep 25, 2018

View reviewed changes

correct Transfer nonces to fix ApplyExtinsicFailed(Stale)

7ed0fc6

gavofyork removed A1-onice labels Sep 25, 2018

rphmeier added 8 commits September 25, 2018 16:59

use given backend in canoncal test

3df72d0

kill dead tree-route code in util

cbb0105

fix warnings

dc6ba9e

tests for leafset

2ddf034

reorganizations in in_mem backend

cc163c6

fix reorganization canon block logic

ce07a02

DB commit and safe reversion on write error

16fd8ff

Merge branch 'master' into snd-issue-603

58e0ed5

rphmeier added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 25, 2018

snd commented Sep 26, 2018

View reviewed changes

gavofyork reviewed Sep 26, 2018

View reviewed changes

gavofyork approved these changes Sep 26, 2018

View reviewed changes

gavofyork added A6-seemsok and removed A0-please_review Pull request needs code review. labels Sep 26, 2018

fix style nits

47092d3

rphmeier merged commit 6c7b45e into master Sep 26, 2018

rphmeier deleted the snd-issue-603 branch September 26, 2018 11:34

liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021

Fix/prevtx inputaddr panic (paritytech#740)

621befd

* add log in parse_addr_from_script * refactor for deposit input_addr * refactor for deposit handle

Conversation

snd commented Sep 14, 2018 • edited by rphmeier 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!

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!

rphmeier Sep 17, 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!

snd commented Sep 18, 2018

Uh oh!

snd commented Sep 19, 2018

Uh oh!

rphmeier commented Sep 19, 2018

Uh oh!

snd commented Sep 20, 2018

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!

snd left a comment

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!

gavofyork left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

snd commented Sep 14, 2018 •

edited by rphmeier

Loading

rphmeier Sep 17, 2018 •

edited

Loading