Light Client transaction queue, initial LightDispatcher #4501

rphmeier · 2017-02-09T20:16:52Z

Goes after #4485

The TXQ only manages local transactions, but still distinguishes between current and future.
Doesn't do intensive verification because someone only hurts their own node by triggering pathological behavior. Nonces are initially guessed at based on what's provided in the transactions for an account (which will usually be accurate/fetched from network), but ancient TXs can still be culled.

The dispatcher uses the Dispatcher interface from #4485, but it's sort of incomplete: needs stuff like gas price corpus and gas amount estimation to be fully complete (this needs the request cache, on_demand request batching, and the very scary Remote Transaction Execution)

NikVolf · 2017-02-13T14:49:22Z

rpc/src/v1/helpers/dispatch.rs

-						})
-				);
-				WithToken::No(signed)
+				hardware_signature(&*accounts, address, t, network_id).map(WithToken::No)


how is this related to light client?

Just factoring out common code shared between the FullDispatcher and the LightDispatcher. The dispatcher's role is to fill optional fields of transaction requests, fulfill signer requests, and dispatch transactions onto the network.

(the reason for the split is that the light node must fetch next nonce, expected gas price, estimated gas from the network, while a full node has all this information available locally)

rphmeier · 2017-02-13T15:04:03Z

rpc/src/v1/helpers/dispatch.rs

+			};
+
+			if accounts.is_hardware_address(address) {
+				return hardware_signature(&*accounts, address, t, network_id).map(WithToken::No)


@NikVolf here you can see the invocation of the hardware_signature bit from the light dispatcher.

NikVolf · 2017-02-13T15:10:40Z

rpc/src/v1/helpers/dispatch.rs

+	{
+		let network_id = None; // TODO: fetch from client.
+		let address = filled.from;
+		let best_header = self.client.block_header(BlockId::Latest)


seing a lot of the same expectations over and over again
worse drying it in the common function?

NikVolf · 2017-02-13T15:26:38Z

ethcore/light/src/client/mod.rs

 use ethcore::blockchain_info::BlockChainInfo;
 use ethcore::spec::Spec;
 use ethcore::service::ClientIoMessage;
 use ethcore::encoded;
 use io::IoChannel;

-use util::hash::{H256, H256FastMap};
+use util::hash::H256;


can be just util::H256 with the other guys next lines i believe

rphmeier · 2017-02-13T16:00:39Z

@NikVolf grumbles addressed

NikVolf · 2017-02-13T16:36:03Z

lgtm
but other guys might have too look also

tomusdrw

Couple of grumbles regarding transaction queue.

tomusdrw · 2017-02-14T08:45:20Z

ethcore/light/src/transaction_queue.rs

+impl TransactionQueue {
+	/// Import a pending transaction to be queued.
+	pub fn import(&mut self, tx: PendingTransaction) -> Result<TransactionImportResult, TransactionError>  {
+		let sender = tx.sender();


Indentation?

Seems right to me. Weirdness in online viewer is just line wrapping.

tomusdrw · 2017-02-14T08:46:41Z

ethcore/light/src/transaction_queue.rs

+	pub fn import(&mut self, tx: PendingTransaction) -> Result<TransactionImportResult, TransactionError>  {
+		let sender = tx.sender();
+		let hash = tx.hash();
+		let nonce = tx.nonce;


Don't you need to check if transaction is already in queue (by hash?)

tomusdrw · 2017-02-14T08:47:12Z

ethcore/light/src/transaction_queue.rs

+
+				match acct_txs.current.binary_search_by(|x| x.nonce.cmp(&nonce)) {
+					Ok(idx) => {
+						trace!(target: "txqueue", "Replacing existing transaction from {} with nonce {}",


Probably replacement should happen only if gas_price is higher than the previous one, no?

With the intended use-case (storing transactions only of local origin), it makes sense to replace whenever the same nonce is encountered.

Right, but if you ever propagate transaction with lower gas price it will never be replaced in full nodes transaction queue (at least the ones using default implementations) giving the user false feeling that the transaction was replaced, although this could be left to be displayed in UI.

If the user wants to try with a lower gas price then they're welcome to; the least we can do is allow our implementation to propagate it to the network. What the peers do with the transaction is beyond our control. The fact that it would probably not work should be left to documentation instead of behavior.

tomusdrw · 2017-02-14T08:49:03Z

ethcore/light/src/transaction_queue.rs

+						// current is sorted with one tx per nonce,
+						// so if a tx with given nonce wasn't found that means it is either
+						// earlier in nonce than all other "current" transactions or later.
+						debug_assert!(idx == 0 || idx == cur_len);


I would suggest standard assert here at the beginning, we can switch back to debug_assert before release.

tomusdrw · 2017-02-14T08:52:53Z

ethcore/light/src/transaction_queue.rs

+#[derive(Debug, Default, Clone, PartialEq, Eq)]
+pub struct TransactionQueue {
+	by_account: HashMap<Address, AccountTransactions>,
+	by_hash: H256FastMap<PendingTransaction>,


It should be optimized to avoid transaction cloning (currently we store each transaction at least twice) - with large transaction data it may result in high memory usage. (esp. important for future transactions, which might not be included in blockchain at all).

That's fair. I suppose the transactions can be shared with an internal Rc?

You're putting TransactionQueue into RwLock, so I'm afraid it needs to be Arc.

tomusdrw · 2017-02-14T08:58:39Z

ethcore/light/src/transaction_queue.rs

+						trace!(target: "txqueue", "Replacing existing transaction from {} with nonce {}",
+							sender, nonce);
+
+						acct_txs.current[idx] = tx.clone();


Old transaction should be removed from by hash

tomusdrw · 2017-02-14T09:00:54Z

ethcore/light/src/transaction_queue.rs

+
+	    let res = match self.by_account.entry(sender) {
+			Entry::Vacant(entry) => {
+				entry.insert(AccountTransactions {


It seems that we will store a lot of old transactions, cause if all transactions from particular sender are imported we will remove the entry here.
But then if some node propagates it again we will insert it to the queue with Assumed nonce, and will never cull it unless the sender actually sends another transaction.

Light nodes don't have the capability to validate transactions, so this queue isn't intended to store anything propagated from the network, only local stuff from RPC. The intended usage is to fetch actual nonces at the best block for all queued_senders and call cull using those.

tomusdrw · 2017-02-14T09:03:24Z

ethcore/light/src/transaction_queue.rs

+	// believed current nonce (gotten from initial given TX or `cull` calls).
+	cur_nonce: CurrentNonce,
+	current: Vec<PendingTransaction>, // ordered "current" transactions (cur_nonce onwards)
+	future: BTreeMap<U256, PendingTransaction>, // "future" transactions.


Could be just HashMap, no? Doesn't seem that we use ordering anywhere

Ordering relied on here: https://github.com/ethcore/parity/pull/4501/files#diff-e1ba00eb97ba4534d6c62db683387edbR229

tomusdrw · 2017-02-14T09:04:55Z

ethcore/light/src/transaction_queue.rs

+	/// Get all transactions ready to be propagated.
+	/// `best_block_number` and `best_block_timestamp` are used to filter out conditionally
+	/// propagated transactions.
+	pub fn ready_transactions(&self, best_block_number: u64, best_block_timestamp: u64) -> Vec<PendingTransaction> {


Should indicate that the transactions are not sorted by any priority (actually it will batch transactions from single sender togethe).
Since we propagate transactions in chunks some senders will always be a bit more priviledged by some node.

Haven't implemented the transaction propagation code for the light node just yet. I'll add docs that it's in "arbitrary" order.

tomusdrw · 2017-02-14T09:08:32Z

ethcore/light/src/transaction_queue.rs

+						} else if idx == cur_len && acct_txs.current.last().map_or(false, |f| f.nonce + 1.into() != nonce) {
+							trace!(target: "txqueue", "Queued future transaction for {}, nonce={}", sender, nonce);
+							let future_nonce = nonce;
+							acct_txs.future.insert(future_nonce, tx.clone());


Future queue should have a hard limit (probably current queue also), otherwise you can use arbitrary amount of nodes memory just by propagating future transactions that will never be included in the blockchain.

rphmeier · 2017-02-14T11:14:47Z

@tomusdrw Grumbles should be addressed. Duplication of transaction data is avoided by having the AccountTransactions store simply a TransactionInfo which is the hash, nonce, and condition of the transaction. When fetching pending transactions we just look up the hash in the by_hash map afterwards.

tomusdrw · 2017-02-14T12:28:25Z

ethcore/light/src/transaction_queue.rs

+					Some(Condition::Timestamp(time)) => time <= best_block_timestamp,
+				}).map(|info| info.hash)
+			})
+			.filter_map(|hash| self.by_hash.get(&hash))


Might be good to put .expect here instead to track potential issues (had issues like this in standard transaction pool)

I chose to use filter_map specifically to avoid panics in this case. I think it's better for something not to be propagated than for the whole node to go down. Our style is to have panickers be provable: I don't think the invariant that by_hash be fully consistent with by_account is strong enough to assert (although it should really be the case). Adding a warning here "Inconsistency detected between by_account and by_hash" makes sense to me, though.

tomusdrw

Looks good, minor suggestion left.

rphmeier added 10 commits February 8, 2017 19:21

light txq skeleton

b3440ba

Merge branch 'lightrpc' into light-txq

869d193

initial transaction queue implementation

33266f7

add several tests + minor bugfixes

6a92477

LightProvider struct using light transaction queue

baf0dbc

implement send_raw_transaction

a559dfe

verify raw transactions against Engine

325c6aa

LightDispatcher definition

7be2c14

Merge branch 'lightrpc' into light-txq

53774d4

implement light dispatcher

e53d023

rphmeier added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M4-core ⛓ Core client code / Rust. labels Feb 9, 2017

rphmeier added this to Scrapyard in Light Client Feb 9, 2017

rphmeier moved this from Scrapyard to Icebox in Light Client Feb 9, 2017

rphmeier added 2 commits February 9, 2017 22:37

remove accidental gap in next nonce

5e65081

Merge branch 'lightrpc' into light-txq

2c43b02

rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Feb 10, 2017

rphmeier moved this from Icebox to In Review in Light Client Feb 10, 2017

Merge branch 'master' into light-txq

c7d83bd

NikVolf reviewed Feb 13, 2017

View reviewed changes

rphmeier commented Feb 13, 2017

View reviewed changes

NikVolf reviewed Feb 13, 2017

View reviewed changes

best_block_header function

4f1afcc

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 13, 2017

tomusdrw added the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Feb 14, 2017

tomusdrw removed the A8-looksgood 🦄 Pull request is reviewed well. label Feb 14, 2017

tomusdrw suggested changes Feb 14, 2017

View reviewed changes

tomusdrw reviewed Feb 14, 2017

View reviewed changes

rphmeier added 3 commits February 14, 2017 12:05

address grumbles

e911fc2

store pending transactions only once

63ad8cb

ready transactions order documentation

1d9db57

rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 14, 2017

tomusdrw reviewed Feb 14, 2017

View reviewed changes

tomusdrw approved these changes Feb 14, 2017

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 14, 2017

warning on detected inconsistency

e591b44

rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Feb 14, 2017

rphmeier merged commit 36ea555 into master Feb 15, 2017

rphmeier deleted the light-txq branch February 15, 2017 13:06

rphmeier moved this from In Review to Done in Light Client Feb 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Light Client transaction queue, initial LightDispatcher #4501

Light Client transaction queue, initial LightDispatcher #4501

rphmeier commented Feb 9, 2017 •

edited

NikVolf Feb 13, 2017

rphmeier Feb 13, 2017

rphmeier Feb 13, 2017

rphmeier Feb 13, 2017

NikVolf Feb 13, 2017 •

edited

NikVolf Feb 13, 2017 •

edited

rphmeier commented Feb 13, 2017

NikVolf commented Feb 13, 2017

tomusdrw left a comment

tomusdrw Feb 14, 2017

rphmeier Feb 14, 2017

tomusdrw Feb 14, 2017

tomusdrw Feb 14, 2017

rphmeier Feb 14, 2017

tomusdrw Feb 14, 2017

rphmeier Feb 14, 2017 •

edited

tomusdrw Feb 14, 2017

tomusdrw Feb 14, 2017 •

edited

rphmeier Feb 14, 2017

tomusdrw Feb 14, 2017

tomusdrw Feb 14, 2017

tomusdrw Feb 14, 2017

rphmeier Feb 14, 2017

tomusdrw Feb 14, 2017

rphmeier Feb 14, 2017

tomusdrw Feb 14, 2017

rphmeier Feb 14, 2017

tomusdrw Feb 14, 2017

rphmeier commented Feb 14, 2017 •

edited

tomusdrw Feb 14, 2017

rphmeier Feb 14, 2017 •

edited

tomusdrw left a comment

Light Client transaction queue, initial LightDispatcher #4501

Light Client transaction queue, initial LightDispatcher #4501

Conversation

rphmeier commented Feb 9, 2017 • edited

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

NikVolf Feb 13, 2017 • edited

Choose a reason for hiding this comment

NikVolf Feb 13, 2017 • edited

Choose a reason for hiding this comment

rphmeier commented Feb 13, 2017

NikVolf commented Feb 13, 2017

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

rphmeier Feb 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw Feb 14, 2017 • edited

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 commented Feb 14, 2017 • edited

Choose a reason for hiding this comment

rphmeier Feb 14, 2017 • edited

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

rphmeier commented Feb 9, 2017 •

edited

NikVolf Feb 13, 2017 •

edited

NikVolf Feb 13, 2017 •

edited

rphmeier Feb 14, 2017 •

edited

tomusdrw Feb 14, 2017 •

edited

rphmeier commented Feb 14, 2017 •

edited

rphmeier Feb 14, 2017 •

edited