Verify transaction against its block during import

[grbIzl]

Currently transaction's basic verification is not performed during its import from the pool (transaction's verify_basic method is not called).
As a result, incorrect transaction (for example, with incorrect chain id) can be added into the block and mined. Such blocks won't be accepted by other nodes (this transaction's verification will be called for the new block on other nodes). But incorrect block will stay on originator's node.

Solution: perform engine's verify_transaction_basic before adding transaction to the block during import (this will call transaction's verify_basic).

Drawback: this additional verification will increase import's time.

Closes #10411

[dvdplm]

Have you measured the increased import time with this change?

[tomusdrw]

I think basic verification should rather be performed before verify_signed.
Also I would like to avoid using engine directly and rather have client.verify_for_pending_block(&transaction, &open_block.header) to keep the logic contained.

[grbIzl]

Have you measured the increased import time with this change?

With the fix:
Total pushed 1713 transactions in 354 ms
Avg transaction's import time: 0.206 ms

Without fix:
Total pushed 1636 transactions in 320 ms
Avg transaction's import time: 0.196 ms

Only the time of the transactions' import to the block was measured.

[dvdplm]

With the fix:
Total pushed 1713 transactions in 354 ms
Avg transaction's import time: 0.206 ms

Without fix:
Total pushed 1636 transactions in 320 ms
Avg transaction's import time: 0.196 ms

Are they different transactions? If yes, can you measure with the same txs?

[grbIzl]

Are they different transactions? If yes, can you measure with the same txs?

No, it was actually the same tx sent > 1000 times in the loop.

curl --data '{"jsonrpc":"2.0","method":"personal_sendTransaction","params":[{"from":"0x004ec07d2329997267Ec62b4166639513386F32E","to":"0x00Bd138aBD70e2F00903268F3Db08f2D25677C9e","value":"0xde0b6b3a7640000"}, "user"],"id":0}' -H "Content-Type: application/json" -X POST localhost:8540

[tomusdrw]

Looks good, but I think we can simplify this even further.

[tomusdrw]

Why not call verify_transaction here as well (and verify against header not best_block_header)? So that we can actually simplify the miners code and perform only one check.

[grbIzl]

Sorry, I don't understand your suggestion. Which verify_transaction method do you mean? verify_transaction from machine doesn't call required verification methods. verify_transaction from pool client doesn't check against custom header, only for best block's

[tomusdrw]

Yeah, sorry. I meant:

	/// Verifies transaction against provided pending block header.
	/// 
	/// The check is performed before the transaction is imported into this block.
	/// We also check any conditions that rely on chain status.
	/// If you want to verify against chain's best block use `verify_signed`.
	pub fn verify_for_pending_block(&self, transaction: &SignedTransaction, header: &Header) -> Result<(), transaction::Error> {
		self.engine.machine().verify_transaction_basic(transaction, header)?;
		self.engine.machine().verify_transaction(transaction, header, self.chain)?;
        Ok(())
}

So that client.verify_for_pending_block is the only method we call in the miner code:

let result = client.verify_for_pending_block(&transaction, &open_block.header)
				.map_err(|e| e.into())

If verify_signed is then not used anywhere after this change we should remove it.

My point is to have one single "verify" for the mining code exposed by PoolClient abstraction, so that you don't need to know details about basic/signed verification and the order of checks performed (they can stay encapsulated in PoolClient).

[grbIzl]

Thanks! Got it.

[tomusdrw]

Ah, unfortunately we can't change the order here. basic needs to be performed before verify_unordered, otherwise we are risking a DoS attack vector.

Maybe just use engine.machine() methods here directly.

[grbIzl]

Ok