Update sealing just once when externally importing many blocks #1541
Conversation
| }; | ||
| self.miner.import_transactions(self, transactions, fetch_account) | ||
| fn import_external_transactions(&self, transactions: Vec<SignedTransaction>) -> Vec<Result<TransactionImportResult, Error>> { | ||
| self.miner.import_external_transactions(self, transactions) |
There was a problem hiding this comment.
I'm not sure this function provides any value any more. It may be used for testing, but I don't think it's particularly useful for even that since we could just call "import_queued_transactions"
There was a problem hiding this comment.
Let's get rid of it if it's not used.
| // lets assume that all txs are valid | ||
| self.imported_transactions.lock().unwrap().extend_from_slice(&transactions); | ||
|
|
||
| for sender in transactions.iter().filter_map(|t| t.sender().ok()) { | ||
| let nonce = self.last_nonce(&sender).unwrap_or(fetch_account(&sender).nonce); | ||
| let nonce = self.last_nonce(&sender).unwrap(); |
There was a problem hiding this comment.
Why it's removed? If there are no other transactions from this sender in queue self.last_nonce/1 may return None
There was a problem hiding this comment.
Ah, ok - it's only used for tests (and there doesn't seem to be any tests using that.
I would change it to expect("..") with some nice description that last_nonce should be populated for tests.
tomusdrw
added
A4-gotissues 💥
|
Besides the comments that I left it looks good 👍 - it minimizes calls to |
tomusdrw
added
A0-pleasereview 🤓
| Vec<Result<TransactionImportResult, Error>> { | ||
|
|
||
| let mut transaction_queue = self.transaction_queue.lock().unwrap(); | ||
| let results = self.add_transactions_to_queue(chain, transactions, TransactionOrigin::External, |
There was a problem hiding this comment.
We use a bit different indentation style. I think I would write something like this here:
let results = self.add_transactions_to_queue(
chain, transactions, TransactionOrigin::External, &mut transaction_queue
);Or would just keep it single line.
(Same in L693)
tomusdrw
added
A5-grumble 🔥
|
LGTM except for indentation grumble. 👍 |
gavofyork
added
A0-pleasereview 🤓
gavofyork
added
A8-looksgood 🦄
|
will fix this on merge myself. |
Fixes Issue #1372
The change factors out an "add_transactions_to_queue" private function on the miner which queues transactions without updating sealing. The externally visible "import_transactions" function will add transactions and seal (with the throttling from @5d3ff3d). The externally visible "chain_new_blocks" function chains all the new transactions and updates sealing once.