Fix fork choice by ngotchac · Pull Request #10837 · openethereum/parity-ethereum

ngotchac · 2019-07-04T08:37:14Z

It seems that is_from_route_finalized was computed for the parent's block finalization at each iteration in fn tree_route. Thus if a fork happened, and the common ancestor was finalized, the from branch would be marked as finalized although it wasn't, and the switch to the better branch would never happened.

`is_from_route_finalized` check before switching to parent

tomusdrw

Looks good!

But I really think we need to write a bunch of tests to make sure it's working correctly. Some cases I have in mind - (X) means that X is finalised.

Case 1:
(A) - A1 - A2 - A3[to]
    - B1 - B2[from]

Case 2:
A - A1 - (A2) - A3[from]
  - B1 -  B2[to]

Case 3:
A - (A1)[to] - A2[from]

Case 4:
A - (A1)[from] - A2[to]

I believe Case 4 will incorrectly return is_from_route_finalized: false, but so was the previous code, so we should either carefuly check the consequences and change the behaviour or document it properly and make sure it's will not cause us trouble in the future.

dvdplm

Looks like the correct fix to me. I agree with @tomusdrw that this needs good unit tests (follow up PR?) and I'd really like to hear from @sorpaas too.

dvdplm · 2019-07-04T11:42:35Z

The test failure here is fixable by a rebase.

sorpaas

LGTM!

* master: refactor: whisper: Add type aliases and update rustdocs in message.rs (#10812) Break circular dependency between Client and Engine (part 1) (#10833) tests: Relates to #10655: Test instructions for Readme (#10835) refactor: Related #9459 - evmbin: replace untyped json! macro with fully typed serde serialization using Rust structs (#10657) idiomatic changes to PodState (#10834) Allow --nat extip:your.host.here.org (#10830) When updating the client or when called from RPC, sleep should mean sleep (#10814) Remove excessive warning (#10831) Fix typo in README.md (#10828) ethcore does not use byteorder (#10829) Better logging when backfilling ancient blocks fail (#10796) depends: Update wordlist to v1.3 (#10823) cargo update -p smallvec (#10822) replace memzero with zeroize crate (#10816) Don't repeat the logic from Default impl (#10813) removed additional_params method (#10818) Add Constantinople eips to the dev (instant_seal) config (#10809)

…ereum into ng-fix-fork-choice

ngotchac · 2019-07-05T14:57:20Z

@tomusdrw I added tests for tree_route as you described, but didn't modify the behavior you described on test case 4. I'm actually not quite sure where this could occur, so I don't know if we should modify it or not.

dvdplm · 2019-07-05T17:16:29Z

 	}

+	/// Add a block with randomly generated transactions.
+	#[inline]


This is a test helper yes? Is it performance sensitive?

Yeah I think it's only used for tests. I just followed the structure of the other similar methods, so I'm not sure whether inline is needed here.

dvdplm · 2019-07-05T17:22:28Z

 			let metadata = get_metadata();
 			let block_number = parent_number + 1;
+			let transactions = metadata.transactions;
+			let transactions_root = ordered_trie_root(transactions.iter().map(|tx| {


You sure you need to loop over all txs and encode them like this, and why as a stream and not rlp::encode? Honest question, I really don't know!

yeh, .map(rlp::encode) produces the same result and is shorter

debris

lgtm! there is just one minor idiomatic fix to do in a test :)

debris · 2019-07-06T15:39:08Z

 			let metadata = get_metadata();
 			let block_number = parent_number + 1;
+			let transactions = metadata.transactions;
+			let transactions_root = ordered_trie_root(transactions.iter().map(|tx| {


yeh, .map(rlp::encode) produces the same result and is shorter

tomusdrw

lgtm

tomusdrw · 2019-07-08T10:18:51Z

+			assert_eq!(bc.best_block_hash(), a2_hash);
+
+			mark_finalized(a1_hash, &db, &bc);
+			assert!(!bc.tree_route(a1_hash, a2_hash).unwrap().is_from_route_finalized);


Can we document this counter intuitive behavior in the function docs? A small NOTE would suffice, that is_from_route_finalized works only if from and to are on different forks (i.e. have common ancestor or one is not an ancestor of the other)

Yep, I added a comment to the tree_route method.

Refactor a test.

ordian · 2019-07-08T13:18:36Z

@@ -55,18 +55,26 @@ dependencies = [

 [[package]]
 name = "aho-corasick"
-version = "0.6.8"
+version = "0.6.10"


did you run cargo update? :)
it would make backporting harder, so please revert

Hm, I just deleted the lock file and re-built the project...

instead of deleting, can you checkout Cargo.lock from master and build it with it?

git checkout master Cargo.lock cargo check

Yeah sure, sorry about that. Done.

ordian · 2019-07-08T13:21:06Z

 log = "0.4"
 parity-bytes = "0.1"
 parking_lot = "0.8"
+rand = "0.6"


should be it be a dev dep?

No, actually the generator.rs module is not for tests only

* master: Run cargo fix on a few of the worst offenders (#10854) removed redundant fork choice abstraction (#10849) Extract state-db from ethcore (#10858) Fix fork choice (#10837) Move more code into state-account (#10840) Remove compiler warning (#10865) [ethash] use static_assertions crate (#10860)

phahulin · 2019-08-14T16:29:10Z

Is this included in any of the already released versions (the last one atm is 2.5.6)? If not yet, which version is it scheduled to be included into?

jam10o-new · 2019-08-14T16:35:02Z

@phahulin it was included in 2.6.0 (beta) so it is available on the 2.6 branch releases - I'm not sure if it was backported to stable at any point as well though...

* Fix fork choice: `is_from_route_finalized` check before switching to parent * Add tests for `tree_route` with finalization * Fix Cargo dependencies * Add comment on `tree_route` for finalization. Refactor a test. * Fix compilation error * Checkout Cargo.lock from master

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * Fix fork choice (#10837) * Fix compilation on recent nightlies (#10991)

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * tx-pool: accept local tx with higher gas price when pool full (#10901) * Fix fork choice (#10837) * Cleanup unused vm dependencies (#10787) * Fix compilation on recent nightlies (#10991)

Fix fork choice:

da8c213

`is_from_route_finalized` check before switching to parent

ngotchac added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 4, 2019

ngotchac requested review from dvdplm and sorpaas July 4, 2019 08:37

tomusdrw suggested changes Jul 4, 2019

View reviewed changes

dvdplm approved these changes Jul 4, 2019

View reviewed changes

sorpaas approved these changes Jul 4, 2019

View reviewed changes

ordian modified the milestones: 2.6, 2.7 Jul 5, 2019

ngotchac added 4 commits July 5, 2019 16:52

Add tests for tree_route with finalization

cc49a0d

Merge branch 'ng-fix-fork-choice' of github.com:paritytech/parity-eth…

b831e5a

…ereum into ng-fix-fork-choice

Merge branch 'master' into ng-fix-fork-choice

0d29363

Fix Cargo dependencies

55f3c6f

dvdplm reviewed Jul 5, 2019

View reviewed changes

debris reviewed Jul 6, 2019

View reviewed changes

debris added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 6, 2019

dvdplm requested a review from tomusdrw July 8, 2019 10:01

tomusdrw approved these changes Jul 8, 2019

View reviewed changes

ngotchac added 3 commits July 8, 2019 13:15

Add comment on tree_route for finalization.

d3322cb

Refactor a test.

Merge branch 'master' into ng-fix-fork-choice

c312b2d

Fix compilation error

315893d

ordian reviewed Jul 8, 2019

View reviewed changes

Checkout Cargo.lock from master

de0207b

debris merged commit 073d242 into master Jul 8, 2019

debris deleted the ng-fix-fork-choice branch July 8, 2019 17:21

This was referenced Jul 12, 2019

Node is failing to sync with error "Error: Error(Engine(RequiresClient)" #10085

Closed

Provide a General Test-Setup #10881

Closed

This was referenced Sep 12, 2019

v2.6.3-beta #11040

Merged

v2.5.8-stable #11041

Merged

Conversation

ngotchac commented Jul 4, 2019

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

dvdplm commented Jul 4, 2019

Uh oh!

sorpaas left a comment

Choose a reason for hiding this comment

Uh oh!

ngotchac commented Jul 5, 2019

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!

debris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw 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!

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!

phahulin commented Aug 14, 2019

Uh oh!

jam10o-new commented Aug 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants