fn queue_transactions accepts UnverifiedTransactions #10887

debris · 2019-07-15T11:42:45Z

importing transactions on a client layer is a mess, this pr tries too simplify it a bit and reduce the number of redundant transaction encoding and decoding. example scenario before this pr and after this pr

before

light client, relay:

ethcore/light/src/net/mod.rs decodes transaction
ethcore/sync/src/api.rs encodes transaction
ethcore/src/client/client.rs decodes transaction
miner/src/pool/verifier.rs encodes transaction
ethcore/src/machine.rs decodes transaction

now

light client, relay

ethcore/light/src/net/mod.rs decodes transaction
miner/src/pool/verifier.rs encodes transaction

it's still not perfect, but definitely better than it was before

…ransaction bytes

debris · 2019-07-15T11:51:15Z

ethcore/src/client/client.rs

-
-			let txs: Vec<UnverifiedTransaction> = transactions
-				.iter()
-				.filter_map(|bytes| client.engine.decode_transaction(bytes).ok())


this verification is performed by a miner in import_external_transactions

So we previous ignored invalid rlps and now we return an err in on_peer_transactions? This seems like an improvement to me. But maybe it was done on purpose?

So we previous ignored invalid rlps and now we return an err in on_peer_transactions?

Yes

This seems like an improvement to me. But maybe it was done on purpose?

No, it was a regression introduced very recently

This closure is invoked in a IoWorker thread pool and the whole thing was done to prevent blocking networking task for useless stuff like decoding transactions.

debris · 2019-07-15T11:52:40Z

ethcore/src/client/client.rs

 			client.notify(|notify| {
-				notify.transactions_received(&txs, peer_id);
+				notify.transactions_received(&hashes, peer_id);


maybe we should move this notification after miner imports the transactions? cause those transactions may be invalid

AFAIR this notification only updates last_sent_transactions of a peer, so that we don't propagate the same transaction that the peer sent us

debris · 2019-07-15T11:54:03Z

ethcore/src/engines/mod.rs

-		self.machine().decode_transaction(transaction)
+	/// Verifies len of transaction RLP
+	fn verify_transaction_len(&self, transaction: &[u8]) -> Result<(), transaction::Error> {
+		if transaction.len() > self.params().max_transaction_size {


max_transaction_size is configurable since #8417 but I don't know if this is really needed... maybe we could just remove it?

No we can't just remove it. We need a way to tell users that their transaction is not being accepted to the pool and will never be propagated. We can obviously hardcode that into a miner, but it feels like a regression.

ordian

LGTM

ordian · 2019-07-16T10:06:22Z

ethcore/src/client/client.rs

 			});

-			client.importer.miner.import_external_transactions(client, txs);
+			// TODO: queue_transactions.queue, should accept FnOnce, so it can capture outer variables


could you open an issue?

I wanted to do it initially, but it requires reworking IoWorkers. Currently every IoMessage is passed to every handler. We just have one handler for ClientIoMessage, I guess we could maybe hack around it with some internal mutability though.

ordian · 2019-07-16T10:18:24Z

ethcore/src/client/client.rs

-
-			let txs: Vec<UnverifiedTransaction> = transactions
-				.iter()
-				.filter_map(|bytes| client.engine.decode_transaction(bytes).ok())


So we previous ignored invalid rlps and now we return an err in on_peer_transactions? This seems like an improvement to me. But maybe it was done on purpose?

tomusdrw

I can't accept this PR in it's current form it basically reverts a lot of work that has been done to optimize networking and transaction importing pipeline.

tomusdrw · 2019-07-16T11:00:06Z

ethcore/light/src/net/mod.rs

@@ -246,7 +246,7 @@ pub trait Handler: Send + Sync {
 	/// Called when a peer makes an announcement.
 	fn on_announcement(&self, _ctx: &dyn EventContext, _announcement: &Announcement) { }
 	/// Called when a peer requests relay of some transactions.
-	fn on_transactions(&self, _ctx: &dyn EventContext, _relay: &[UnverifiedTransaction]) { }
+	fn on_transactions(&self, _ctx: &dyn EventContext, _relay: Vec<UnverifiedTransaction>) { }


This looks like a regression. Luckily it's only a light client, so we shouldn't have many transactions but you may end up cloning a lot of data.

tomusdrw · 2019-07-16T11:07:11Z

ethcore/src/client/client.rs

 			});

-			client.importer.miner.import_external_transactions(client, txs);
+			// TODO: queue_transactions.queue, should accept FnOnce, so it can capture outer variables
+			for res in client.importer.miner.import_external_transactions(client, transactions.clone()) {


This is unacceptable, it means you clone ~4MB a second.

tomusdrw · 2019-07-16T11:08:22Z

ethcore/src/client/client.rs

 			});

-			client.importer.miner.import_external_transactions(client, txs);
+			// TODO: queue_transactions.queue, should accept FnOnce, so it can capture outer variables


I wanted to do it initially, but it requires reworking IoWorkers. Currently every IoMessage is passed to every handler. We just have one handler for ClientIoMessage, I guess we could maybe hack around it with some internal mutability though.

tomusdrw · 2019-07-16T11:10:15Z

ethcore/src/engines/mod.rs

-		self.machine().decode_transaction(transaction)
+	/// Verifies len of transaction RLP
+	fn verify_transaction_len(&self, transaction: &[u8]) -> Result<(), transaction::Error> {
+		if transaction.len() > self.params().max_transaction_size {


No we can't just remove it. We need a way to tell users that their transaction is not being accepted to the pool and will never be propagated. We can obviously hardcode that into a miner, but it feels like a regression.

tomusdrw · 2019-07-16T11:12:01Z

ethcore/sync/src/chain/handler.rs

@@ -673,8 +673,7 @@ impl SyncHandler {
 		trace!(target: "sync", "{:02} -> Transactions ({} entries)", peer_id, item_count);
 		let mut transactions = Vec::with_capacity(item_count);
 		for i in 0 .. item_count {
-			let rlp = r.at(i)?;
-			let tx = rlp.as_raw().to_vec();
+			let tx = r.val_at(i)?;


The whole point of passing Bytes was to avoid doing any intensive work (like hashing transactions) synchronously in the network IoHandlers.
The whole refactoring was done after a careful profiling of issues that happens on the mainnet where network lock could be acquired for a very long time.

tomusdrw · 2019-07-16T11:13:27Z

ethcore/src/client/client.rs

-
-			let txs: Vec<UnverifiedTransaction> = transactions
-				.iter()
-				.filter_map(|bytes| client.engine.decode_transaction(bytes).ok())


This closure is invoked in a IoWorker thread pool and the whole thing was done to prevent blocking networking task for useless stuff like decoding transactions.

tomusdrw · 2019-07-16T11:16:43Z

Regarding light client: note that all transactions always come from the RPC and we expect there is only few of them. Additional decoding-encoding is way lower priority than full node pipeline on the mainnet where you can have 16MBs packets of transactions received many times a second.

debris · 2019-07-30T10:50:24Z

I'll rework this code and open a new pr

fn queue_transactions accepts UnverifiedTransactions instead of raw t…

d810072

…ransaction bytes

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

debris requested a review from dvdplm July 15, 2019 11:42

debris commented Jul 15, 2019

View reviewed changes

debris requested review from tomusdrw and ordian July 16, 2019 09:45

ordian approved these changes Jul 16, 2019

View reviewed changes

ordian added this to the 2.7 milestone Jul 16, 2019

ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 16, 2019

tomusdrw suggested changes Jul 16, 2019

View reviewed changes

tomusdrw added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Jul 16, 2019

debris closed this Jul 30, 2019

denisgranha deleted the queue_transactions branch March 17, 2020 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fn queue_transactions accepts UnverifiedTransactions #10887

fn queue_transactions accepts UnverifiedTransactions #10887

debris commented Jul 15, 2019 •

edited

Loading

debris Jul 15, 2019

ordian Jul 16, 2019 •

edited

Loading

debris Jul 16, 2019

tomusdrw Jul 16, 2019

debris Jul 15, 2019

ordian Jul 16, 2019 •

edited

Loading

debris Jul 15, 2019

tomusdrw Jul 16, 2019

ordian left a comment

ordian Jul 16, 2019

tomusdrw Jul 16, 2019

ordian Jul 16, 2019 •

edited

Loading

tomusdrw left a comment

tomusdrw Jul 16, 2019

tomusdrw Jul 16, 2019

tomusdrw Jul 16, 2019

tomusdrw Jul 16, 2019

tomusdrw Jul 16, 2019

tomusdrw Jul 16, 2019

tomusdrw commented Jul 16, 2019 •

edited

Loading

debris commented Jul 30, 2019

fn queue_transactions accepts UnverifiedTransactions #10887

fn queue_transactions accepts UnverifiedTransactions #10887

Conversation

debris commented Jul 15, 2019 • edited Loading

before

now

Choose a reason for hiding this comment

ordian Jul 16, 2019 • 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

ordian Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian Jul 16, 2019 • edited Loading

Choose a reason for hiding this comment

tomusdrw 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

tomusdrw commented Jul 16, 2019 • edited Loading

debris commented Jul 30, 2019

debris commented Jul 15, 2019 •

edited

Loading

ordian Jul 16, 2019 •

edited

Loading

ordian Jul 16, 2019 •

edited

Loading

ordian Jul 16, 2019 •

edited

Loading

tomusdrw commented Jul 16, 2019 •

edited

Loading