Implement eth/65 (EIP-2464) #11626

vorot93 · 2020-04-11T15:15:17Z

This PR adds support for Ethereum protocol version 65 (eth/65) described in EIP-2464. This is a pretty rough draft and can definitely be improved, but I hope the overall direction is good.

Fixes #11500

ethcore/sync/src/chain/propagator.rs

ethcore/client-traits/src/lib.rs

tomusdrw · 2020-04-14T09:19:09Z

ethcore/src/client/client.rs

@@ -1889,6 +1889,10 @@ impl BlockChainClient for Client {
 		self.transaction_address(id).and_then(|address| self.chain.read().transaction(&address))
 	}

+	fn pooled_transaction(&self, hash: H256) -> Option<Arc<VerifiedTransaction>> {
+		self.importer.miner.transaction(&hash)


The importer separation was initially there to try to separate import pipeline from the client (which would become responsible only for queries). However I'm not sure how much it's still relevant, so I don't believe any change is required here, just giving some context.

tomusdrw · 2020-04-14T09:20:57Z

ethcore/sync/Cargo.toml

@@ -34,6 +34,7 @@ rlp = "0.4.5"
 snapshot = { path = "../snapshot" }
 trace-time = "0.1"
 triehash-ethereum = { version = "0.2", path = "../../util/triehash-ethereum" }
+transaction-pool = "2"


I think it might be worth to introduce some abstraction to avoid introducing this dependency. For instance client-traits could have trait QueuedTransactionthat would include all the things thatsync/network` needs.

tomusdrw · 2020-04-14T09:24:06Z

ethcore/sync/src/chain/handler.rs

@@ -590,9 +591,11 @@ impl SyncHandler {
 			difficulty,
 			latest_hash,
 			genesis,
+			unsent_pooled_hashes: if eth_protocol_version >= ETH_PROTOCOL_VERSION_65.0 { Some(io.chain().transactions_to_propagate().into_iter().map(|tx| *tx.hash()).collect()) } else { None },


Using transactions_to_propagate here is going to be extremely inefficient. This method creates an ordered iterator over all tranasctions in the pool, and it's quite costly - especially if the pool is large.

We support a more efficient way to query all hashes from the transaction pool (see Miner::pending_transaction_hashes) and I think it's worth to use it here as well.

I think there is also some recommended limit in the spec, isn't there?

Should I somehow expose dyn MinerService here then to take advantage of pending_transaction_hashes?

tomusdrw · 2020-04-14T16:34:37Z

ethcore/sync/src/chain/propagator.rs

+			let mut affected = false;
+			let mut packet = RlpStream::new();
+			packet.begin_unbounded_list();
+			if let Some(s) = &mut peer.unsent_pooled_hashes {


As I said earlier we should be using last_sent_hashes here as well, we already have similar logic in transaction propagation.

Actually this whole method might not be needed at all. We should use current transaction propagation logic and this new ETH65 consolidation protocol should only be used to LIMIT the transactions we are sending to hashes that were actually requested (or not seen) by the other peer.

ethcore/sync/src/chain/propagator.rs

tomusdrw · 2020-04-14T16:36:16Z

ethcore/sync/src/chain/supplier.rs

@@ -232,6 +219,24 @@ impl SyncSupplier {
 		Ok(Some((BlockHeadersPacket.id(), rlp)))
 	}

+	/// Respond to GetPooledTransactions request
+	fn return_pooled_transactions(io: &dyn SyncIo, r: &Rlp, peer_id: PeerId) -> RlpResponseResult {
+		const LIMIT: usize = 256;


We already have a bunch of constants like this, we should keep them nearby with proper documentation.

tomusdrw · 2020-04-14T16:37:36Z

ethcore/sync/src/chain/supplier.rs

+		const LIMIT: usize = 256;
+
+		let transactions = r.iter().take(LIMIT).filter_map(|v| {
+			v.as_val::<H256>().ok().and_then(|hash| io.chain().pooled_transaction(hash))


As mentioned earlier I think the method should support requesting a batch of transactions from the pool instead of going one-by-one.

tomusdrw · 2020-04-14T16:38:26Z

ethcore/sync/src/chain/supplier.rs

+
+		let added = transactions.len();
+		let mut rlp = RlpStream::new_list(added);
+		for tx in transactions {


We have methods to make sure the package does not end up oversized, please take a look how we construct transaction packages during propagation I think the same logic should apply here (instead of having new arbitrary limit of 256 transactions).

The 256 comes from the EIP and is a suggestion. Defntly should be combined with io.payload_soft_limit() (4Mb I think) like we do in other places.

dvdplm

Left a few comments, but I think it'd be best to address Tomeks concerns before digging in further; looks like there might be some significant changes coming.

dvdplm · 2020-04-20T12:55:28Z

ethcore/sync/src/chain/supplier.rs

@@ -232,6 +219,24 @@ impl SyncSupplier {
 		Ok(Some((BlockHeadersPacket.id(), rlp)))
 	}

+	/// Respond to GetPooledTransactions request


Can be improved.

dvdplm · 2020-04-20T13:13:50Z

ethcore/sync/src/chain/supplier.rs

+
+		let added = transactions.len();
+		let mut rlp = RlpStream::new_list(added);
+		for tx in transactions {


The 256 comes from the EIP and is a suggestion. Defntly should be combined with io.payload_soft_limit() (4Mb I think) like we do in other places.

dvdplm · 2020-04-20T13:22:04Z

ethcore/sync/src/chain/mod.rs

@@ -343,6 +349,8 @@ pub struct PeerInfo {
 	asking_blocks: Vec<H256>,
 	/// Holds requested header hash if currently requesting block header by hash
 	asking_hash: Option<H256>,
+	/// Holds requested transaction IDs


Suggested change

/// Holds requested transaction IDs

/// Hash of the transaction we're requesting from the peer

?

dvdplm · 2020-04-20T13:22:26Z

ethcore/sync/src/chain/mod.rs

@@ -343,6 +349,8 @@ pub struct PeerInfo {
 	asking_blocks: Vec<H256>,
 	/// Holds requested header hash if currently requesting block header by hash
 	asking_hash: Option<H256>,
+	/// Holds requested transaction IDs
+	asking_pooled_transactions: Option<Vec<H256>>,


Suggested change

asking_pooled_transactions: Option<Vec<H256>>,

asking_pooled_transaction: Option<Vec<H256>>,

dvdplm · 2020-04-20T13:24:01Z

ethcore/sync/src/chain/mod.rs

@@ -676,6 +691,8 @@ pub struct ChainSync {
 	sync_start_time: Option<Instant>,
 	/// Transactions propagation statistics
 	transactions_stats: TransactionsStats,


Do we need changes here too to account for eth/65 traffic? Would be good to have good data on protocol usage.

dvdplm · 2020-04-20T13:26:01Z

ethcore/sync/src/chain/mod.rs

@@ -676,6 +691,8 @@ pub struct ChainSync {
 	sync_start_time: Option<Instant>,
 	/// Transactions propagation statistics
 	transactions_stats: TransactionsStats,
+	/// Unfetched transactions


Are these txs that we have seen announced but not fetched yet? Are they txs that we know we do not have or?

dvdplm · 2020-04-20T13:26:42Z

ethcore/sync/src/chain/mod.rs

@@ -717,6 +734,7 @@ impl ChainSync {
 			snapshot: Snapshot::new(),
 			sync_start_time: None,
 			transactions_stats: TransactionsStats::default(),
+			unfetched_pooled_transactions: Default::default(),


Suggested change

unfetched_pooled_transactions: Default::default(),

unfetched_pooled_transactions: H256FastMap::default(),

(to me it's more readable to see the actual type, but maybe it's just me)

dvdplm · 2020-04-20T13:32:38Z

ethcore/sync/src/chain/mod.rs

@@ -335,6 +339,8 @@ pub struct PeerInfo {
 	network_id: u64,
 	/// Peer best block hash
 	latest_hash: H256,
+	/// Unpropagated tx pool hashes
+	unsent_pooled_hashes: Option<H256FastSet>,


What is the point of Option here? Is None different from the hash set being empty, semantically? I mean to say: isn't it unlikely we'll be running with this set None during normal operation, and if so there's not much of a space saving?

vorot93 · 2020-05-28T12:19:42Z

@tomusdrw Is it correct that unsent_pooled_hashes is an unnecessary addition in general and the better approach would be to compute the difference between last_sent_hashes and transaction queue?

vorot93 commented Apr 11, 2020

View reviewed changes

ethcore/sync/src/chain/propagator.rs Show resolved Hide resolved

vorot93 requested review from tomusdrw, dvdplm, niklasad1 and ordian April 11, 2020 21:27

tomusdrw suggested changes Apr 14, 2020

View reviewed changes

vorot93 added the A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. label Apr 15, 2020

dvdplm reviewed Apr 20, 2020

View reviewed changes

vorot93 force-pushed the vorot93/eth-65 branch from b2ea4b7 to f20fd98 Compare May 15, 2020 06:02

vorot93 force-pushed the vorot93/eth-65 branch from edb713a to 0d85ff6 Compare May 24, 2020 00:16

vorot93 force-pushed the vorot93/eth-65 branch from 0d85ff6 to 3374ed3 Compare July 4, 2020 01:57

Implement eth/65 (EIP-2464)

b4af110

vorot93 force-pushed the vorot93/eth-65 branch from 3374ed3 to b4af110 Compare September 10, 2020 21:23

aoaham approved these changes Sep 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement eth/65 (EIP-2464) #11626

Implement eth/65 (EIP-2464) #11626

vorot93 commented Apr 11, 2020 •

edited

Loading

tomusdrw Apr 14, 2020

tomusdrw Apr 14, 2020

tomusdrw Apr 14, 2020

tomusdrw Apr 14, 2020

vorot93 May 23, 2020 •

edited

Loading

tomusdrw Apr 14, 2020

tomusdrw Apr 14, 2020

tomusdrw Apr 14, 2020

tomusdrw Apr 14, 2020

dvdplm Apr 20, 2020

dvdplm left a comment

dvdplm Apr 20, 2020

dvdplm Apr 20, 2020

dvdplm Apr 20, 2020

dvdplm Apr 20, 2020

dvdplm Apr 20, 2020

dvdplm Apr 20, 2020

dvdplm Apr 20, 2020

dvdplm Apr 20, 2020

vorot93 commented May 28, 2020

	/// Holds requested transaction IDs
	/// Hash of the transaction we're requesting from the peer

	asking_pooled_transactions: Option<Vec<H256>>,
	asking_pooled_transaction: Option<Vec<H256>>,

	unfetched_pooled_transactions: Default::default(),
	unfetched_pooled_transactions: H256FastMap::default(),

Implement eth/65 (EIP-2464) #11626

Are you sure you want to change the base?

Implement eth/65 (EIP-2464) #11626

Conversation

vorot93 commented Apr 11, 2020 • 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

vorot93 May 23, 2020 • 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

dvdplm left a 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

vorot93 commented May 28, 2020

vorot93 commented Apr 11, 2020 •

edited

Loading

vorot93 May 23, 2020 •

edited

Loading