Polkadot service #82

arkpar · 2018-02-25T13:44:50Z

This PR integrates the client, consensus, networking and transaction pool into a Service struct.
Still TODO:

Integrate keystore
CLI options for network configuration (at least bootnodes)
Gossip protocol for BFT
BFT message accumulation and sorting

gavofyork · 2018-03-09T17:57:41Z

substrate/network/src/consensus.rs

+				trace!(target:"sync", "Error broadcasting statement notification: {:?}", e);
+			}
+		} else {
+				trace!(target:"sync", "Ignored statement from unregistered peer {}", peer_id);


…ensus-network

rphmeier · 2018-03-20T03:38:31Z

polkadot/consensus/src/service.rs

+		}
+	}
+
+	fn run_bft<C, P>(bft_service: &BftService<P, C>, network: Arc<net::ConsensusService>, client: &C, handle: reactor::Handle, header: &Header) -> Result<(), <P as bft::ProposerFactory>::Error> where


nit: more idiomatic as a free function

rphmeier · 2018-03-20T03:38:35Z

polkadot/consensus/src/service.rs

+		bft_service.build_upon(&header, input, output, handle.clone())
+	}
+
+	fn process_message(msg: net::BftMessage, authorities: &[AuthorityId], parent_hash: HeaderHash) -> Option<bft::Communication> {


rphmeier · 2018-03-20T03:39:14Z

polkadot/consensus/src/service.rs

+impl Drop for Service {
+	fn drop(&mut self) {
+		if let Some(thread) = self.thread.take() {
+			thread.join().expect("The service thread has panicked");


i worry that this could cause shutdown to deadlock

The service now explicitly shuts down the network and client event stream, that should cancel all futures and stop the event loop.

rphmeier · 2018-03-20T03:42:18Z

polkadot/consensus/src/service.rs

+
+impl<E> Sink for BftSink<E> {
+	type SinkItem = bft::Communication;
+	type SinkError = E;


could we leave a TODO to replace this with the ! type when that's stabilized?

rphmeier · 2018-03-20T03:54:09Z

polkadot/consensus/src/service.rs

+						debug!("Error starting BFT agreement: {:?}", e);
+					}
+				}).map_err(|e| debug!("BFT agreement error: {:?}", e));
+				if let Err(_e) = core.run(start_bft.into_future()) {


instead of doing this in a loop, couldn't we do

let bft_starter = client.import_notification_stream().for_each(|notification| ...) and core.run(bft_starter)?

rphmeier · 2018-03-20T05:23:38Z

polkadot/service/src/lib.rs

+		let mut storage = Default::default();
+		let key: AccountId = Keyring::One.into();
+
+		let genesis_config = GenesisConfig {


isn't this duplicated from the CLI?

moved, not duplicated

rphmeier · 2018-03-20T05:24:25Z

polkadot/service/src/lib.rs

+				let events = thread_client.import_notification_stream().map(|notification| {
+					thread_network.on_block_imported(&notification.header);
+				});
+				if let Err(_) = core.run(events.into_future()) {


same, i think that this is a misuse of into_future() and the loop isn't strictly required

rphmeier · 2018-03-20T05:25:45Z

substrate/bft/src/lib.rs

 	/// Signal that a valid block with the given header has been imported.
 	///
 	/// If the local signing key is an authority, this will begin the consensus process to build a
 	/// block on top of it. If the executor fails to run the future, an error will be returned.
-	pub fn build_upon(&self, header: &Header) -> Result<(), P::Error> {
+	pub fn build_upon<InStream, OutSink, E>(&self, header: &Header, input: InStream, output: OutSink, executor: E) -> Result<(), P::Error> where


may as well skip the executor as a parameter if it isn't owned by the service anymore.

Skip the executor as a parameter of what? This function?

yeah. this function takes an executor as a parameter just so it can call executor.execute at the end. might as well just do that at the call sites and save some complexity

rphmeier · 2018-03-20T05:31:17Z

substrate/client/src/client.rs

+	/// Block was broadcasted on the network.
+	NetworkBroadcast,
+	/// Block that was collated by this node.
+	Own,


what does this mean in the context of the 2-phase commit?

This means that the block body was generated on this machine and was not received from the network.

the block body is almost always generated on another machine, but any block bodies coming directly from the consensus process have already been evaluated

Added a variant for that.

rphmeier · 2018-03-20T05:31:41Z

substrate/client/src/client.rs

+	/// Block is part of the initial sync with the network.
+	NetworkInitialSync,
+	/// Block was broadcasted on the network.
+	NetworkBroadcast,


aren't all blocks broadcast over the network in some sense? doesn't it make more sense to provide hints about whether a block is the head of the chain or not?

The head hint is provided as a separate parameter to import function. This is for something else. Mainly for logging and possibly skipping certain checks for local blocks.

rphmeier · 2018-03-20T05:34:37Z

substrate/client/src/client.rs

+				header: header,
+				is_new_best: is_new_best,
+			};
+			if let Err(e) = self.import_notification_sink.lock().try_send(notification) {


this fails if the buffer is full; seems dangerous to discard the notification in this case. maybe use the Sink::send api and wait for the future to resolve?

Import is more important and should not block on the notification handler. Normally this should never happen but if it does, skipping a notification is fine since there will be a new block eventually anyway. The handlers are not supposed to expect to receive notifications for every block anyway. It is quite possible that the node goes to sleep or loses connectivity for a while and when it goes back online there won't be any notifications for all the blocks that arrived in the meantime.

skipping a notification is fine since there will be a new block eventually anyway

this is reasoning about "eventually" a little too long term for me to be comfortable using this in practice.
there won't be a new block if not enough authorities start the agreement process (e.g. if there are 1/3 offline and one of the "honest" nodes skips this notification. then the agreement would only complete when this guy restarted his node)

Increased the buffer to 65536 for now. Unfortunately it is preallocated.

still dangerous but probably workable for now. we should write a fix after this PR -- maybe a 50ms timer where the same chain head being observed twice in a row starts consensus?

I'll address that in the following PR. Probably ny replacing multiqueue with a vector of mpsc channels.

rphmeier · 2018-03-20T05:36:45Z

substrate/network/src/consensus.rs

+		if self.peers.contains_key(&peer_id) {
+			// TODO: validate signature?
+			if let Err(e) = self.bft_message_sink.try_send(message) {
+				trace!(target:"sync", "Error broadcasting BFT message notification: {:?}", e);


dropping messages when the buffer is full will almost certainly cause consensus to fail

The network handler should not block. If the buffer is full this means that the system just lacks performance to handle the messages and consensus will fail anyway. Blocking here will just move congestion to the lower level.

I disagree -- we could get 65 messages all at once and then one will be dropped. But if those are the last messages we were going to receive then it's fine to let the agreement logic process them slowly. It isn't fine to drop the last commit message, for example.

Increased the buffer to 65536 for now. Unfortunately it is preallocated.

worrying but workable for PoC-1. This is something that should fail on OOM and not before

I'll address that in the following PR. Probably ny replacing multiqueue with a vector of mpsc channels.

rphmeier · 2018-03-20T05:39:16Z

substrate/network/src/error.rs

@@ -23,7 +25,7 @@ error_chain! {
 	}

 	links {
-		Client(client::error::Error, client::error::ErrorKind);
+		Client(client::error::Error, client::error::ErrorKind) #[doc="Blockchain error"];


is it always a blockchain error?

gavofyork · 2018-03-21T07:49:55Z

looks reasonable to me

…ensus-network

rphmeier · 2018-03-26T08:53:20Z

polkadot/api/src/lib.rs

@@ -140,7 +140,7 @@ pub trait PolkadotApi {

 /// A checked block ID used for the substrate-client implementation of CheckedBlockId;
 #[derive(Debug, Clone, Copy)]
-pub struct CheckedId(BlockId);
+pub struct CheckedId(pub BlockId);


making this pub kind of defeats the purpose of the interface

rphmeier · 2018-03-26T08:58:07Z

substrate/network/src/consensus.rs

+
+	pub fn on_statement(&mut self, peer_id: PeerId, statement: message::Statement) {
+		if let Some(ref mut peer) = self.peers.get_mut(&peer_id) {
+			// TODO: validate signature?


also TODO (but beyond this PR) is handle only current parent-hash statements

This will be addressed in an upcoming PR

rphmeier · 2018-03-26T09:00:13Z

substrate/network/src/consensus.rs

+
+	pub fn on_bft_message(&mut self, peer_id: PeerId, message: message::BftMessage) {
+		if self.peers.contains_key(&peer_id) {
+			// TODO: validate signature?


aren't the signatures being checked? I thought you mentioned that to me

(it's done in process_message on the other side of this stream)

Yeah, but eventually there has to be a check here as well to avoid propagating invalid messages. Or it should be moved here.

* Fix and re-enable ts-block tests * Update frontier checkout * Runtime impl_version bump * cargo update * Remove comments Co-authored-by: Joshy Orndorff <[email protected]>

nonce removed

provide multinodexindex

* 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

Farmer bin to lib - Phase 2

arkpar added 3 commits February 25, 2018 14:42

Block import notifications

f22cbda

Build fix

a7e3f9d

Consensus messages supported in the networking

197aaf2

arkpar added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 25, 2018

Started consensus service

9cb9410

gavofyork reviewed Mar 9, 2018

View reviewed changes

arkpar added 7 commits March 12, 2018 18:53

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

6147304

…ensus-network

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

e96486e

…ensus-network

BFT service

0d5fe7c

Transaction propagation

c808a91

Polkadot service

b0f85b0

CLI integration

158a000

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

1bbee71

…ensus-network

rphmeier reviewed Mar 20, 2018

View reviewed changes

Build fix

dd30b9c

arkpar changed the title ~~Consensus network messages~~ Polkadot service Mar 20, 2018

rphmeier reviewed Mar 20, 2018

View reviewed changes

arkpar added 2 commits March 20, 2018 17:35

Added signatures validation

41e28f6

Removed executor argument

7c0ea3d

arkpar added 3 commits March 21, 2018 04:03

Refactored steam loops; Queue size increased

da31b51

Limit queue size

9e38d2d

Fixed doc comment

884d3a1

arkpar 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 Mar 21, 2018

arkpar added 3 commits March 21, 2018 09:16

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

153b93b

…ensus-network

Fixed wasm build

a82a2e3

Fixed wasm build

1572f14

rphmeier reviewed Mar 26, 2018

View reviewed changes

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

Check id properly

a4f656f

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

rphmeier approved these changes Mar 30, 2018

View reviewed changes

rphmeier merged commit 8561d59 into master Mar 30, 2018

rphmeier deleted the ark-consensus-network branch March 30, 2018 11:54

iStrike7 referenced this pull request in gasp-xyz/substrate Aug 3, 2021

Merge pull request #82 from mangata-finance/feature/nonce-in-xyk-removal

81566e0

nonce removed

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

Feature/linked_node provide multinodexindex (paritytech#82)

f7e13cc

provide multinodexindex

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

Merge pull request paritytech#82 from subspace/farmer_lib_structs

56df131

Farmer bin to lib - Phase 2

Polkadot service #82

Polkadot service #82

Conversation

arkpar commented Feb 25, 2018 • edited Loading

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

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

Choose a reason for hiding this comment

rphmeier Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar Mar 20, 2018 • edited Loading

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

rphmeier Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

gavofyork commented Mar 21, 2018

Choose a reason for hiding this comment

rphmeier Mar 26, 2018 • edited Loading

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 Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

arkpar commented Feb 25, 2018 •

edited

Loading

rphmeier Mar 20, 2018 •

edited

Loading

rphmeier Mar 26, 2018 •

edited

Loading

arkpar Mar 20, 2018 •

edited

Loading

rphmeier Mar 20, 2018 •

edited

Loading

rphmeier Mar 26, 2018 •

edited

Loading

arkpar Mar 26, 2018 •

edited

Loading