Added checking tx-type using transactions permission contract for miners #7359
Conversation
|
It looks like @VladLupashevskyi signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
tomusdrw
added
A0-pleasereview 🤓
|
I think the check should actually be done before importing transactions to the queue as well. Otherwise we might be storing and propagating a lot of invalid transactions. Should be done here: Other than that: great work, thank you @VladLupashevskyi |
ethcore/src/miner/miner.rs
Outdated
| // Check whether transaction type is allowed for sender | ||
| let result = match self.engine.machine().verify_transaction(&tx, open_block.header(), chain.as_block_chain_client()) { | ||
| Err(Error::Transaction(TransactionError::NotAllowed)) => { | ||
| Err(From::from(TransactionError::NotAllowed)) |
There was a problem hiding this comment.
favor .into() instead of From::from
| @@ -419,7 +420,15 @@ impl Miner { | |||
| for tx in transactions { | |||
| let hash = tx.hash(); | |||
| let start = Instant::now(); | |||
| let result = open_block.push_transaction(tx, None); | |||
| // Check whether transaction type is allowed for sender | |||
| let result = match self.engine.machine().verify_transaction(&tx, open_block.header(), chain.as_block_chain_client()) { | |||
There was a problem hiding this comment.
I guess open_block's parent state always has to be available in the client, so this is fine.
|
Agree with @tomusdrw. It is assumed that transaction queue contains valid transactions only. |
|
@arkpar although the state of the transaction might change in the meantime, with current queue we have no way of invalidating such transactions, so I'm ok with check in both places. |
|
Makes sense |
ethcore/src/miner/miner.rs
Outdated
| @@ -680,6 +698,12 @@ impl Miner { | |||
| Err(e) | |||
| }, | |||
| Ok(transaction) => { | |||
| // This check goes here because verify_transaction takes SignedTransaction parameter | |||
| if self.engine.machine().verify_transaction(&transaction, &best_block_header, client.as_block_chain_client()).is_err() { | |||
There was a problem hiding this comment.
The error here should not be interpreted as permission failure. Other checks might be added in the future. Best would be just to pass it to the caller:
self.engine.machine().verify_transaction(&transaction, &best_block_header, client.as_block_chain_client())?;
|
@arkpar Agreed. And thank you, my Rust knowledge is much better now :) I didn't know about this amazing "?" operator before. |
arkpar
added
A8-looksgood 🦄
…ons-permission-contract-fix
In this commit you can find solution to the issue #7350 which I opened recently.
Now block miners are checking whether the tx type is allowed for sender using transaction permission contract.