ethcore: use Machine::verify_transaction on parent block #9900

andresilva · 2018-11-13T00:19:44Z

Machine::verify_transaction is meant to be used against the parent header. When verifying transactions in a block we would pass the block header instead of the parent, it would be correct for this case since we'd use header.parent_hash(), but when the miner called it with best_block it wouldn't work properly, e.g. transaction permission contract activation would be off-by-one and the parent_hash passed to TransactionFilter would be incorrect.

also fixes off-by-one activation of transaction permission contract

tomusdrw

lgtm, I'd add a comment to future proof the code.

tomusdrw · 2018-11-13T10:55:36Z

ethcore/src/verification/verification.rs

@@ -147,7 +147,7 @@ pub fn verify_block_family<C: BlockInfo + CallContract>(header: &Header, parent:
 	verify_uncles(params.block, params.block_provider, engine)?;

 	for tx in &params.block.transactions {
-		engine.machine().verify_transaction(tx, header, params.client)?;
+		engine.machine().verify_transaction(tx, parent, params.client)?;


Could you add a comment here why we validate against parent? I guess it might be obvious that current state is not yet available, but I think it's best to mention that explicitly.

andresilva · 2018-11-13T11:02:44Z

Done. I also changed the name of the parameter in verify_transaction to make it clearer that it should be the parent.

ordian · 2018-11-13T11:36:30Z

Why only backport to beta?

andresilva · 2018-11-13T11:51:29Z

@ordian I think it can be backported to stable as well, but the change to transaction_allowed won't apply cleanly I think (should be easy to fix just remove the header.number() + 1 parameter).

* ethcore: use Machine::verify_transaction on parent block also fixes off-by-one activation of transaction permission contract * ethcore: clarify call to verify_transaction

* bump stable 2.1.6 * ethcore: use Machine::verify_transaction on parent block (#9900) * ethcore: use Machine::verify_transaction on parent block also fixes off-by-one activation of transaction permission contract * ethcore: clarify call to verify_transaction * fix: Intermittent failing CI due to addr in use (#9885) Allow OS to set port at runtime * gitlab-ci: make android release build succeed (#9743) * use docker cargo config file for android builds * make android build succeed * Update light-client hardcoded headers for foundation: #6692865, ropsten: #4417537, kovan: #9363457 * Remove rust-toolchain file (#9906) * light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure (#9824) * fix start_gas, handle OOG exceptions & NotEnoughGas * Change START_GAS: 50_000 -> 60_000 * When the `OutOfGas exception` is received then try to double the gas until it succeeds or block gas limit is reached * When `NotEnoughBasGas error` is received then use the required gas provided in the response * fix(light-fetch): ensure block_gas_limit is tried Try the `block_gas_limit` before regard the execution as an error * Update rpc/src/v1/helpers/light_fetch.rs Co-Authored-By: niklasad1 <[email protected]> * fix #9824 merge artifacts * simplify cargo audit * ci: nuke the gitlab caches (#9855)

* Bump beta to version 2.2.1 * fix: Intermittent failing CI due to addr in use (#9885) Allow OS to set port at runtime * Use Weak reference in PubSubClient (#9886) * Fix json tracer overflow (#9873) * Fix json tracer overflow * Replace trace_executed with a direct trace push * Remove unused variable * Add test for 5a51 * Remove duplicate json! * Fix docker script (#9854) * Dockerfile: change source path of the newly added check_sync.sh (#9869) * Allow to seal work on latest block (#9876) * Allow to seal work on latest block. * Test from @todr to check sealing conditions. * gitlab-ci: make android release build succeed (#9743) * use docker cargo config file for android builds * make android build succeed * ethcore: use Machine::verify_transaction on parent block (#9900) * ethcore: use Machine::verify_transaction on parent block also fixes off-by-one activation of transaction permission contract * ethcore: clarify call to verify_transaction * foundation: #6692865, ropsten: #4417537, kovan: #9363457 * Remove rust-toolchain file (#9906) * EIP-712 implementation (#9631) * EIP-712 impl * added more tests * removed size parsing unwrap * corrected TYPE_REGEX to disallow zero sized fixed length arrays, replaced LinkedHashSet with IndexSet, added API spec to docs, fixed Type::Byte encoding branch * use Option<u64> instead of u64 for Type::Array::Length * replace `.iter()` with `.values()` Co-Authored-By: seunlanlege <[email protected]> * tabify eip712.rs * use proper comments for docs * Cargo.lock: revert unrelated changes * tabify encode.rs * EIP 191 (#9701) * added sign_191 rpc method * fixed hash_structured_data return type * added ConfirmationPayload::SignMessage for non-prefixed signatures, added tests for sign191 * renamed WithValidator -> PresignedTransaction * rename applicationData to data in test * adds docs for EIP191Version, renamed SignRequest to EIP191SignRequest * light-fetch: Differentiate between out-of-gas/manual throw and use required gas from response on failure (#9824) * fix start_gas, handle OOG exceptions & NotEnoughGas * Change START_GAS: 50_000 -> 60_000 * When the `OutOfGas exception` is received then try to double the gas until it succeeds or block gas limit is reached * When `NotEnoughBasGas error` is received then use the required gas provided in the response * fix(light-fetch): ensure block_gas_limit is tried Try the `block_gas_limit` before regard the execution as an error * Update rpc/src/v1/helpers/light_fetch.rs Co-Authored-By: niklasad1 <[email protected]> * simplify cargo audit * Use block header for building finality (#9914) * ci: nuke the gitlab caches (#9855)

ethcore: use Machine::verify_transaction on parent block

0281923

also fixes off-by-one activation of transaction permission contract

andresilva added A0-pleasereview 🤓 Pull request needs code review. B1-patch-beta 🕷🕷 M4-core ⛓ Core client code / Rust. labels Nov 13, 2018

tomusdrw approved these changes Nov 13, 2018

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 Nov 13, 2018

ethcore: clarify call to verify_transaction

135c2a5

ordian added this to the 2.3 milestone Nov 13, 2018

andresilva added the B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. label Nov 13, 2018

ordian approved these changes Nov 13, 2018

View reviewed changes

ordian 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 Nov 13, 2018

ordian merged commit 5f3ae4d into master Nov 13, 2018

ordian deleted the andre/fix-verify-transaction branch November 13, 2018 11:58

This was referenced Nov 13, 2018

Backport Stable 2.1.6 #9904

Merged

Backport Beta 2.2.1 #9905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ethcore: use Machine::verify_transaction on parent block #9900

ethcore: use Machine::verify_transaction on parent block #9900

andresilva commented Nov 13, 2018 •

edited

Loading

tomusdrw left a comment

tomusdrw Nov 13, 2018

andresilva commented Nov 13, 2018 •

edited

Loading

ordian commented Nov 13, 2018

andresilva commented Nov 13, 2018

ethcore: use Machine::verify_transaction on parent block #9900

ethcore: use Machine::verify_transaction on parent block #9900

Conversation

andresilva commented Nov 13, 2018 • edited Loading

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Nov 13, 2018

Choose a reason for hiding this comment

andresilva commented Nov 13, 2018 • edited Loading

ordian commented Nov 13, 2018

andresilva commented Nov 13, 2018

andresilva commented Nov 13, 2018 •

edited

Loading

andresilva commented Nov 13, 2018 •

edited

Loading