[trace] introduce trace failed to Ext by ordian · Pull Request #11019 · openethereum/parity-ethereum

ordian · 2019-09-03T12:51:18Z

When a subcall fails, we need to clean up the tracing data (last_mem_written, last_store_written) in the ExecutiveVMTracer. Also to properly track subcalls, we maintain a stack of tracing data.

One approach of doing that is to pass mem_written directly to trace_executed. It does introduce a perf regression.

This PR adds a method trace_failed to Ext, which is called when the execution of an instruction has failed. An alternative suggested by @tomusdrw would be to merge trace_failed into trace_executed by passing a Result<(stack_push: &[U256], mem: &[u8]), ()>, but I'm not sure if is any better.

* master: Extract snapshot to own crate (#11010) Edit publish-onchain.sh to use https (#11016)

tomusdrw

I was suggesting to merge the two methods to simplify assumptions for implementation (i.e. trace_executed is ALWAYS called after trace_prepare_execute). It would also deduplicate poping from the stack in the implementation of VMTracer.

But looks good anyway!

tomusdrw · 2019-09-03T20:29:58Z

@@ -166,6 +166,9 @@ pub trait Ext {
 	/// Prepare to trace an operation. Passthrough for the VM trace.
 	fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: U256, _mem_written: Option<(usize, usize)>, _store_written: Option<(U256, U256)>) {}


Can we improve the docs here and underline the fact that this call is ALWAYS followed by either trace_executed or trace_failed. and that the implementation is responsible to clean up in either/both of those methods.

dvdplm

Why did you opt to remove the bounds checks here? For performance reasons? Unless it's a severe regression I think it'd be good to have.

ordian · 2019-09-05T09:35:31Z

Why did you opt to remove the bounds checks here? For performance reasons? Unless it's a severe regression I think it'd be good to have.

If the bounds could still be incorrect, it would indicate another bug and a warning could easily be missed/ignored. But if we reach a consensus on that the checks should be added back, I don't mind.

dvdplm · 2019-09-05T09:37:51Z

But if we reach a consensus on that the checks should be added back, I don't mind.

@tomusdrw thoughts? Better to crash?

tomusdrw · 2019-09-05T10:50:35Z

@dvdplm I'd rather bound the o and o+s with mem.len() to be safe. The bound checking and a warning was introducing a lot of noise.
To be fair I'd rather not crash and have potentially incorrect data in tracing.

dvdplm · 2019-09-05T13:21:33Z

f818b23 lgtm

* master: fix: remove unused error-chain (#11028) fix: remove needless use of itertools (#11029) Convert `std::test` benchmarks to use Criterion (#10999) Fix block detail updating (#11015) [trace] introduce trace failed to Ext (#11019) cli: update usage and version headers (#10924) [private-tx] remove unused rand (#11024)

…he-right-place * master: cleanup json crate (#11027) [spec] add istanbul test spec (#11033) [json-spec] make blake2 pricing spec more readable (#11034) Add blake2_f precompile (#11017) Add new line after writing block to hex file. (#10984) fix: remove unused error-chain (#11028) fix: remove needless use of itertools (#11029) Convert `std::test` benchmarks to use Criterion (#10999) Fix block detail updating (#11015) [trace] introduce trace failed to Ext (#11019) cli: update usage and version headers (#10924) [private-tx] remove unused rand (#11024)

* [trace] add trace_failed to Ext, manage stack of trace data * [evm] call trace_failed only if self.do_trace * [evm] add a comment about do_trace set to true * [vm] improve the doc in trace_prepare_execute * [trace] add the bounds check back

This reverts commit 895dc54.

* [trace] add trace_failed to Ext, manage stack of trace data * [evm] call trace_failed only if self.do_trace * [evm] add a comment about do_trace set to true * [vm] improve the doc in trace_prepare_execute * [trace] add the bounds check back

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * Fix fork choice (#10837) * Fix compilation on recent nightlies (#10991)

* add more tx tests (#11038) * Fix parallel transactions race-condition (#10995) * Add blake2_f precompile (#11017) * [trace] introduce trace failed to Ext (#11019) * Edit publish-onchain.sh to use https (#11016) * Fix deadlock in network-devp2p (#11013) * EIP 1108: Reduce alt_bn128 precompile gas costs (#11008) * xDai chain support and nodes list update (#10989) * EIP 2028: transaction gas lowered from 68 to 16 (#10987) * EIP-1344 Add CHAINID op-code (#10983) * manual publish jobs for releases, no changes for nightlies (#10977) * [blooms-db] Fix benchmarks (#10974) * Verify transaction against its block during import (#10954) * Better error message for rpc gas price errors (#10931) * tx-pool: accept local tx with higher gas price when pool full (#10901) * Fix fork choice (#10837) * Cleanup unused vm dependencies (#10787) * Fix compilation on recent nightlies (#10991)

* master: (70 commits) ethcore: remove `test-helper feat` from build (#11047) Include test-helpers from ethjson (#11045) [ethcore]: cleanup dependencies (#11043) add more tx tests (#11038) Fix parallel transactions race-condition (#10995) [ethcore]: make it compile without `test-helpers` feature (#11036) Benchmarks for block verification (#11035) Move snapshot related traits to their proper place (#11012) cleanup json crate (#11027) [spec] add istanbul test spec (#11033) [json-spec] make blake2 pricing spec more readable (#11034) Add blake2_f precompile (#11017) Add new line after writing block to hex file. (#10984) fix: remove unused error-chain (#11028) fix: remove needless use of itertools (#11029) Convert `std::test` benchmarks to use Criterion (#10999) Fix block detail updating (#11015) [trace] introduce trace failed to Ext (#11019) cli: update usage and version headers (#10924) [private-tx] remove unused rand (#11024) ...

ordian added 3 commits September 2, 2019 16:51

[trace] add trace_failed to Ext, manage stack of trace data

bfc7d1b

[evm] call trace_failed only if self.do_trace

de1efb2

Merge branch 'master' into ao-introduce-trace-failed

17e2156

* master: Extract snapshot to own crate (#11010) Edit publish-onchain.sh to use https (#11016)

ordian added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. B1-patch-beta 🕷🕷 M4-core ⛓ Core client code / Rust. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. labels Sep 3, 2019

ordian added this to the 2.7 milestone Sep 3, 2019

ordian requested review from sorpaas and tomusdrw September 3, 2019 12:53

[evm] add a comment about do_trace set to true

020b324

tomusdrw approved these changes Sep 3, 2019

View reviewed changes

[vm] improve the doc in trace_prepare_execute

8196c62

ordian marked this pull request as ready for review September 4, 2019 13:33

ordian added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Sep 4, 2019

dvdplm approved these changes Sep 5, 2019

View reviewed changes

[trace] add the bounds check back

f818b23

dvdplm merged commit 44c00b1 into master Sep 5, 2019

ordian deleted the ao-introduce-trace-failed branch September 5, 2019 14:18

s3krit added a commit that referenced this pull request Sep 11, 2019

Revert "[trace] introduce trace failed to Ext (#11019)"

5c95e70

This reverts commit 895dc54.

This was referenced Sep 12, 2019

v2.6.3-beta #11040

Merged

v2.5.8-stable #11041

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[trace] introduce trace failed to Ext#11019

[trace] introduce trace failed to Ext#11019
dvdplm merged 6 commits into
masterfrom
ao-introduce-trace-failed

ordian commented Sep 3, 2019 •

edited

Loading

Uh oh!

tomusdrw left a comment

Uh oh!

tomusdrw Sep 3, 2019

Uh oh!

dvdplm left a comment

Uh oh!

ordian commented Sep 5, 2019

Uh oh!

dvdplm commented Sep 5, 2019 •

edited

Loading

Uh oh!

tomusdrw commented Sep 5, 2019

Uh oh!

dvdplm commented Sep 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -166,6 +166,9 @@ pub trait Ext {
		/// Prepare to trace an operation. Passthrough for the VM trace.
		fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: U256, _mem_written: Option<(usize, usize)>, _store_written: Option<(U256, U256)>) {}

Conversation

ordian commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

ordian commented Sep 5, 2019

Uh oh!

dvdplm commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw commented Sep 5, 2019

Uh oh!

dvdplm commented Sep 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ordian commented Sep 3, 2019 •

edited

Loading

dvdplm commented Sep 5, 2019 •

edited

Loading