Networking and backend fixes #155

arkpar · 2018-05-11T12:04:01Z

Added filtering for BFT messages. Message order is now preserved.
~~Fixed an issue with the DB backend implementation~~
Fixed a minor issue with sync service.

rphmeier · 2018-05-11T12:36:35Z

substrate/client/db/src/lib.rs

-		Ok(self.db.get(columns::BLOCK_INDEX, &number_to_db_key(number))
-		   .map_err(db_err)?
-		   .map(|hash| block::HeaderHash::from_slice(&hash)))
+		Ok(self.header(BlockId::Number(number))?.map(|hdr| hdr.blake2_256().into()))


this is fixed and tested in #152

rphmeier · 2018-05-11T13:10:23Z

substrate/network/src/consensus.rs

+			},
+			Ok(status) => {
+				if status != BlockStatus::InChain {
+					trace!(target:"sync", "Ignored unknown parent BFT message from {}, hash={}", peer_id, message.parent_hash);


this doesn't seem right.

we are building on parent A

our peers produce B and begin building on B. Due to latency we still think the chain head is A.

we shouldn't ignore messages about that consensus just because there was latency in delivering B to us.

Or imagine if we're syncing and well behind the chain head. our peers might think we have those messages but we just ignored them! then things will stall again.

note that the property for correctness of the BFT implementation is that messages eventually reach and are processed by the other validators. In practice, "eventually" isn't possible, but we can at least make a best-effort of 10 minutes or so.

what we want to keep:

unrecognized parent_hash messages (for some time)

parent_hash == chain head messages

what we don't want to keep

parent_hash in chain but has children already

rphmeier · 2018-05-11T13:11:24Z

substrate/network/src/protocol.rs

 			}
 		}
+
+		self.consensus.lock().collect_garbage(Some(header.parent_hash));


if this doesn't fire every block then we will have a lot of garbage piling up.

seems better to keep track of the header hash of the last notification and pass all the parent headers since then.

That list would be huge if this is called at block 0 before connecting to any peers and then at the end of the full sync. For now I'll just clear all messages if there is a gap.

fairly dangerous but fine if we can assume there are always enough validators at the head of the chain to complete consensus.

rphmeier · 2018-05-11T13:12:26Z

substrate/network/src/consensus.rs

-		self.messages.retain(|_, &mut (timestamp, _)| timestamp < now + expiration);
+		let hashes = &mut self.message_hashes;
+		self.messages.retain(|&(ref hash, ref message)| {
+				best_block_parent.map_or(true, |parent_hash| {


is this not a no-op when None is passed?

rphmeier · 2018-05-11T13:34:52Z

substrate/client/db/src/lib.rs

 				transaction.put(columns::JUSTIFICATION, &key, &justification.encode());
 			}
-			transaction.put(columns::BLOCK_INDEX, &hash, &key);
+			transaction.put(columns::BLOCK_INDEX, &key, &hash);


I think this breaks #152 fix.

and almost definitely breaks fn id which looks up by hash.

(...this is why unit tests are useful)

screwed up when reverting my fix

…-fix

* Add fn to validate egress routes * Add blank tests * Reject routing to non existent parachains * Reject if routing to self * Reject if egress route parachain ids out of order * Extract method for checking egress routes * Reject empty egress routes * Extract test method * Generate empty trie root constant in build script * Remove unwraps * Hardcode EMPTY_TRIE_ROOT and add test to verify * Const not pu

* Adds script to test gas from a loop contract * Better load gas test

* refactor for multisig remove doublemap for multisig * Feature/linked node (paritytech#79) provide linked node data struct for runtime module storage this linked node support option data or no option data * provide linkednode struct in cxsupport * refactor linked node * add option for LinkedNode * refactor linkednode remove template mode, use associate type to replace it * Fix static net address (paritytech#80) * Feature/linked_node provide multinodexindex (paritytech#82) provide multinodexindex * fix bug for linked_node when add same index node, do nothing for this node * refactor financialrecords to support withdraw cache refactor financialrecords to support withdraw cache and remove deposit fee * btc bridge * rename num/hash relationship data (NumberForHash/HashsForNumber) * let HashsForNumber map to a vec to get all forked block * add blocknumber in BlockHeaderFor * tokenbalances refactor tokenbalances to support issue token in genesis * reject issue chainx token and provide u32, usize as for tokenbalance * Perfect deposit (paritytech#83) * Add deposit cache * Perfect deposit * Perfect withdraw (paritytech#84) * Perfect withdraw * add network check in btc bridge * when meet testnet, jump header bit check * check the bit change block in genesis * Fix test build * Feature/refactor match (paritytech#86) * matchorder and pendingorders * Fix op_return account id parse * tokenbalances: provide reserved type for reservedtoken * Fix merge error * update genesis_config * Update genesis_config * x-btc * provide codec for btreemap due to orphan for mod, use local struct named `CodecBTreeMap` * Update latest bitcoin-rust dependeces * Implement initial vote weight via coin age (paritytech#87) * Use struct instead of map as much as possible * Unchecked initial version * All intentions except these validators are candidates * Add harsh test * Put candidates into stats * Rename unstake to deactive * Revert StakeWeight * Remove useless code * Remove MAX_INTENTIONS * Refactor with btreemap (paritytech#88) * Refactor NominationRecordsOf to NominationRecords using BTreeMap * Remove candidate_count * Rename deactive to deactivate * optimization match (paritytech#89) * remove ensureaccountliquid in tokenbalances and support for ensureaccountliquied has changed in staking module * Hotfix/fix stakeweight type (paritytech#90) * Revert StakeWeight type * Change Balance to u64 * Fix total_minted and remove valid_cands_weight * Fix insert registration information failure (paritytech#91) * Change receive_address type * Update exterbn * update secp256k1 dependency (paritytech#94) * Fix receive_address bug * Support new substrate in cxrml * update rust-secp256k1 dependeces * Runtime build ok * Build ok * New runtime interface * Update all runtime module * Runtime build ok * All build ok * Add node runtime to support chainx runtime * Update new runtime

* Integrate pallet-authorship So that we can know how many blocks a validator authored in some session. * Support nested structs * Sort manual json * . * Fix types_gen.py the disorder of fields * . * . * Add some missing types in chainx_types_manual.json

…-fork-tree Work around stack overflow by faking block finalization in pruning code

* Update to rc6. * Fix clippy warnings.

Networking and backend fixes

c5cc70a

arkpar added the A0-please_review Pull request needs code review. label May 11, 2018

rphmeier reviewed May 11, 2018

View reviewed changes

arkpar added 2 commits May 11, 2018 14:42

Fixed test

6111c62

Reverted DB fix

eeaf645

rphmeier reviewed May 11, 2018

View reviewed changes

rphmeier added A5-grumble and removed A0-please_review Pull request needs code review. labels May 11, 2018

rphmeier reviewed May 11, 2018

View reviewed changes

arkpar added 3 commits May 11, 2018 15:58

Reverted DB fix properly

168ff05

Merge branch 'master' of github.com:paritytech/polkadot into ark-sync…

8fb71bc

…-fix

Preserve messages with unknown parent_hash

1c2311b

arkpar added A0-please_review Pull request needs code review. and removed A5-grumble labels May 11, 2018

rphmeier approved these changes May 14, 2018

View reviewed changes

rphmeier added A8-looksgood and removed A0-please_review Pull request needs code review. labels May 14, 2018

rphmeier merged commit 1de2727 into master May 14, 2018

rphmeier deleted the ark-sync-fix branch May 14, 2018 10:48

JoshOrndorff pushed a commit to moonbeam-foundation/substrate that referenced this pull request Apr 21, 2021

Adds script to test gas from a loop contract (paritytech#155)

db1be29

* Adds script to test gas from a loop contract * Better load gas test

liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022

Merge pull request paritytech#155 from subspace/fix-stack-overflow-in…

31a2fd1

…-fork-tree Work around stack overflow by faking block finalization in pruning code

helin6 pushed a commit to boolnetwork/substrate that referenced this pull request Jul 25, 2023

Update to rc6. (paritytech#155)

3ea9d3b

* Update to rc6. * Fix clippy warnings.

Networking and backend fixes #155

Networking and backend fixes #155

Uh oh!

Conversation

arkpar commented May 11, 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!

rphmeier May 11, 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!

rphmeier May 11, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arkpar commented May 11, 2018 •

edited

Loading

rphmeier May 11, 2018 •

edited

Loading

rphmeier May 11, 2018 •

edited

Loading