Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Verify transaction against its block during import #10954

Merged
merged 4 commits into from
Aug 15, 2019
Merged

Verify transaction against its block during import #10954

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af33394
Verify transaction against its block during import
grbIzl Aug 7, 2019
64aad44
Client method for transaction verification added
grbIzl Aug 8, 2019
8f6b8cc
Verification methods united
grbIzl Aug 13, 2019
537d725
Verification sequence for transaction verifier returned
grbIzl Aug 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ethcore/src/miner/miner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,11 @@ impl Miner {
let sender = transaction.sender();

// Re-verify transaction again vs current state.
let result = client.verify_signed(&transaction)
let result = client.verify_for_pending_block(&transaction, &open_block.header)
.map_err(|e| e.into())
.and_then(|_| {
client.verify_signed(&transaction)
})
.map_err(|e| e.into())
.and_then(|_| {
open_block.push_transaction(transaction, None)
Expand Down
5 changes: 5 additions & 0 deletions ethcore/src/miner/pool_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ impl<'a, C: 'a> PoolClient<'a, C> where
pub fn verify_signed(&self, tx: &SignedTransaction) -> Result<(), transaction::Error> {
self.engine.machine().verify_transaction(&tx, &self.best_block_header, self.chain)
}

/// Verifies transaction against its block (before its import into this block)
pub fn verify_for_pending_block(&self, transaction: &UnverifiedTransaction, header: &Header) -> Result<(), transaction::Error> {
self.engine.machine().verify_transaction_basic(transaction, header)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Got it.

}
}

impl<'a, C: 'a> fmt::Debug for PoolClient<'a, C> {
Expand Down