Add tracing output for t8n #616

rodiazet · 2023-04-13T11:42:45Z

Add --trace option for evmone-t8n to output traces to specific files compatible with retesteth.

Modify the tracing format to follow closer the specification at EIP-3155: EVM trace specification.

codecov · 2023-04-14T10:30:34Z

Codecov Report

Merging #616 (69bc40f) into master (35f4141) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #616      +/-   ##
==========================================
- Coverage   97.53%   97.51%   -0.02%     
==========================================
  Files          86       86              
  Lines        8236     8223      -13     
==========================================
- Hits         8033     8019      -14     
- Misses        203      204       +1

Flag	Coverage Δ
blockchaintests	`63.19% <0.00%> (+0.48%)`	⬆️
statetests	`74.26% <0.00%> (+0.32%)`	⬆️
statetests-silkpre	`23.22% <0.00%> (+0.03%)`	⬆️
unittests	`95.11% <100.00%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
lib/evmone/tracing.cpp	`100.00% <100.00%> (ø)`
lib/evmone/vm.cpp	`100.00% <100.00%> (ø)`
test/unittests/tracing_test.cpp	`99.25% <100.00%> (-0.75%)`	⬇️

axic · 2023-04-14T22:58:40Z

lib/evmone/tracing.cpp

@@ -149,6 +150,60 @@ class InstructionTracer : public Tracer
        m_out << std::dec;  // Set number formatting to dec, JSON does not support other forms.
    }
 };
+
+
+class StandardTracer : public InstructionTracer


Standard according to what? Is this matching some other EIP? Please add a description to clarify.

Added a doc comment

axic · 2023-04-14T23:00:29Z

test/t8n/t8n.cpp

                    const auto computed_tx_hash = keccak256(rlp::encode(tx));
+                    const auto computed_tx_hash_str = hex0x(computed_tx_hash);


This is a refactoring, perhaps merge it separately?

How about separated commit within this PR and merge without squashing?

axic · 2023-04-14T23:01:32Z

test/t8n/t8n.cpp

+                            fs::path(output_dir / ("trace-" + std::to_string(i) + "-" +
+                                                      computed_tx_hash_str + ".jsonl"));
+                        // Add 'to file' tracer. Casting required by windows build.
+                        vm.set_option("stdtrace", (const char*)(output_filename.c_str()));


According to this c_str returns const char* so no need to typecast?

Unfortunately this is required by MSVC https://app.circleci.com/pipelines/github/ethereum/evmone/4313/workflows/9e36645f-85db-4644-b7e3-1efdef3e6553/jobs/46959

No, you definitely cannot do it like this. You cast wchar_t* to char* what is totally wrong. std::pathhas a method to convert tostd::string` from "native" string.

axic · 2023-04-14T23:02:05Z

test/t8n/t8n.cpp

+                    {
+                        // Remove old tracers
+                        vm.set_option("stdtrace", "no");
+                        auto output_filename =


Can we use some tmpfile feature so it gets deleted afterwards?

Could use std::tmpfile().

This returns a file and opens it. We need only a file name here. Moreover we cannot delete it because retesteth needs to read it.

chfast

The tracer management seems wrong. Why are you adding multiple standard tracers? You should have one with option to change its output file.

chfast · 2023-04-18T09:04:07Z

test/t8n/t8n.cpp

+                            fs::path(output_dir / ("trace-" + std::to_string(i) + "-" +
+                                                      computed_tx_hash_str + ".jsonl"));
+                        // Add 'to file' tracer. Casting required by windows build.
+                        vm.set_option("stdtrace", (const char*)(output_filename.c_str()));


No, you definitely cannot do it like this. You cast wchar_t* to char* what is totally wrong. std::pathhas a method to convert tostd::string` from "native" string.

rodiazet · 2023-04-18T09:57:51Z

The tracer management seems wrong. Why are you adding multiple standard tracers? You should have one with option to change its output file.

trace flag adds multiple tracers. I wanted to keep the same convention for stdtrace. Do you want to make it differently for stdtrace or add separated flag to handle changing the file? This will require also some additional changes in the Tracer class.

chfast · 2023-08-09T08:09:44Z

test/unittests/tracing_test.cpp

-{"error":null,"gas":0xf4240,"gasUsed":0x0,"output":""}
-)");
-}
+// TEST_F(tracing, trace_create)


update test.

I think better to remove them. Tracing return empty string now.

chfast · 2023-08-09T08:11:27Z

lib/evmone/tracing.cpp

    };

    std::stack<Context> m_contexts;
    std::ostream& m_out;  ///< Output stream.
+    std::ostringstream m_buf;


This improved performance ~2x but I'm not sure if this is the best option. Maybe there is an option to tune clog itself. I'm thinking of omitting this change for now.

@gumb0 do you have any hints about iostream performance?

@chfast What are we going to do with this? Omitting?

We change the default clog buffer to std::ofstream in t8n implementation to redirect the output to file (to satisfy retesteth requirements). No sure then how the suggested change with clog tuning will work here. We probably would have to tune std::ofstream buffer

Yes, omit this change for now.

chfast · 2023-08-09T08:15:42Z

test/t8n/t8n.cpp

@@ -156,6 +176,9 @@ int main(int argc, const char* argv[])
                        transactions.emplace_back(std::move(tx));
                        receipts.emplace_back(std::move(receipt));
                    }
+                    // Set original std::clob buffer


typo.
Also this is not exception safe and probably not needed. I'd go with just leaving a comment when rdbuf is modified that we will not restore the original.

When don't do it the program is crashing randomly.

chfast · 2023-08-09T08:16:25Z

test/t8n/t8n.cpp

                    if (holds_alternative<std::error_code>(res))
                    {
                        const auto ec = std::get<std::error_code>(res);
                        json::json j_rejected_tx;
-                        j_rejected_tx["hash"] = hex0x(computed_tx_hash);
+                        j_rejected_tx["hash"] = computed_tx_hash_str;


This should go to prev commit?

chfast · 2023-08-09T08:21:34Z

test/t8n/t8n.cpp

+                    if (tracing_enabled)
+                    {
+                        auto output_filename =
+                            fs::path(output_dir / ("trace-" + std::to_string(i) + "-" +


You don't need fs::path explicitly as operator / produces a path.

chfast · 2023-08-09T08:21:51Z

test/t8n/t8n.cpp

+                    const auto tmp_clog_buf = std::clog.rdbuf();
+                    if (tracing_enabled)
+                    {
+                        auto output_filename =


const auto.

chfast · 2023-08-09T18:08:14Z

lib/evmone/tracing.cpp

-        const auto& ctx = m_contexts.top();
+        output_stack(stack_top, stack_height);
+        if (!state.return_data.empty())
+            m_buf << R"("returnData":"0x)" << evmc::hex(state.return_data) << '"';


Suggested change

m_buf << R"("returnData":"0x)" << evmc::hex(state.return_data) << '"';

m_buf << R"(,"returnData":"0x)" << evmc::hex(state.return_data) << '"';

This line is not covered by tests and in the same time there is a bug in JSON syntax.

chfast · 2023-08-09T18:15:27Z

lib/evmone/tracing.cpp

+        if (!state.return_data.empty())
+            m_buf << R"("returnData":"0x)" << evmc::hex(state.return_data) << '"';
+        m_buf << R"(,"depth":)" << std::dec << (ctx.depth + 1);
+        m_buf << R"(,"refund":)" << std::dec << state.gas_refund;


goevmlab complains about this because the value can go negative, not sure what they expect to get, but they try to fit it into uint64.

This field is required be retesteth for every test as far as I understand.

Modify the tracing format to follow closer the specification at EIP-3155: EVM trace specification. Co-authored-by: Paweł Bylica <pawel@ethereum.org>

Co-authored-by: Paweł Bylica <pawel@ethereum.org>

Add --trace command line option for evmone-t8n and output the trace to a file named by the transaction hash. Co-authored-by: Paweł Bylica <pawel@ethereum.org>

rodiazet force-pushed the t8n-tracing branch 3 times, most recently from 43a4038 to 1ff1bdc Compare April 13, 2023 13:10

rodiazet requested a review from chfast April 13, 2023 13:19

rodiazet marked this pull request as ready for review April 13, 2023 13:41

rodiazet force-pushed the t8n-tracing branch 6 times, most recently from e252e52 to 4c5e4dd Compare April 14, 2023 10:21

rodiazet force-pushed the t8n-tracing branch 6 times, most recently from b2cb2b5 to 74e9d0d Compare April 14, 2023 14:10

axic reviewed Apr 14, 2023

View reviewed changes

chfast reviewed Apr 18, 2023

View reviewed changes

rodiazet force-pushed the t8n-tracing branch from 74e9d0d to f17f4df Compare April 18, 2023 09:09

rodiazet force-pushed the t8n-tracing branch from f17f4df to c584378 Compare April 18, 2023 09:59

rodiazet force-pushed the t8n-tracing branch from c584378 to be5c65e Compare August 2, 2023 16:07

rodiazet requested review from gumb0 and chfast August 2, 2023 16:09

rodiazet force-pushed the t8n-tracing branch from be5c65e to eacb66d Compare August 7, 2023 11:41

rodiazet force-pushed the t8n-tracing branch 2 times, most recently from c103c66 to 034240f Compare August 7, 2023 15:44

rodiazet linked an issue Aug 7, 2023 that may be closed by this pull request

evm t8n: Support evm trace #566

Closed

rodiazet force-pushed the t8n-tracing branch 9 times, most recently from 2828815 to 6c8fa22 Compare August 8, 2023 15:54

chfast reviewed Aug 9, 2023

View reviewed changes

rodiazet force-pushed the t8n-tracing branch from 6c8fa22 to b17a954 Compare August 9, 2023 12:19

chfast requested changes Aug 9, 2023

View reviewed changes

chfast reviewed Aug 9, 2023

View reviewed changes

rodiazet force-pushed the t8n-tracing branch 2 times, most recently from 26c2bc2 to e3a6c70 Compare August 10, 2023 14:05

rodiazet requested a review from chfast August 10, 2023 14:05

rodiazet force-pushed the t8n-tracing branch 2 times, most recently from 96393d5 to f5eecf9 Compare August 22, 2023 09:06

rodiazet and others added 4 commits August 22, 2023 12:28

tracing: Change format to be closer to EIP-3155

a229167

Modify the tracing format to follow closer the specification at EIP-3155: EVM trace specification. Co-authored-by: Paweł Bylica <pawel@ethereum.org>

tracing: Output to std::clog (instead of std::cerr)

1ef53f3

Co-authored-by: Paweł Bylica <pawel@ethereum.org>

t8n: Refactor tx hash to string conversion

fbd3629

Co-authored-by: Paweł Bylica <pawel@ethereum.org>

t8n: Add --trace option

69bc40f

Add --trace command line option for evmone-t8n and output the trace to a file named by the transaction hash. Co-authored-by: Paweł Bylica <pawel@ethereum.org>

chfast force-pushed the t8n-tracing branch from f5eecf9 to 69bc40f Compare August 22, 2023 10:35

chfast approved these changes Aug 22, 2023

View reviewed changes

chfast merged commit 6608cf3 into master Aug 22, 2023

chfast deleted the t8n-tracing branch August 22, 2023 13:13

danceratopz mentioned this pull request Aug 23, 2023

Tools: Add tracing to evmone t8n ethereum/execution-spec-tests#263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracing output for t8n #616

Add tracing output for t8n #616

rodiazet commented Apr 13, 2023 •

edited by chfast

Loading

codecov bot commented Apr 14, 2023 •

edited

Loading

axic Apr 14, 2023

rodiazet Apr 18, 2023

axic Apr 14, 2023

rodiazet Apr 18, 2023 •

edited

Loading

axic Apr 14, 2023

rodiazet Apr 18, 2023

chfast Apr 18, 2023

axic Apr 14, 2023

axic Apr 14, 2023

rodiazet Apr 18, 2023

chfast left a comment

chfast Apr 18, 2023

rodiazet commented Apr 18, 2023 •

edited

Loading

chfast Aug 9, 2023

rodiazet Aug 9, 2023

chfast Aug 9, 2023

rodiazet Aug 21, 2023

rodiazet Aug 21, 2023

chfast Aug 22, 2023

chfast Aug 9, 2023

rodiazet Aug 9, 2023

chfast Aug 9, 2023

rodiazet Aug 18, 2023

chfast Aug 9, 2023

chfast Aug 9, 2023

chfast Aug 9, 2023

chfast Aug 9, 2023

rodiazet Aug 18, 2023

		const auto computed_tx_hash = keccak256(rlp::encode(tx));
		const auto computed_tx_hash_str = hex0x(computed_tx_hash);

	m_buf << R"("returnData":"0x)" << evmc::hex(state.return_data) << '"';
	m_buf << R"(,"returnData":"0x)" << evmc::hex(state.return_data) << '"';

Add tracing output for t8n #616

Add tracing output for t8n #616

Conversation

rodiazet commented Apr 13, 2023 • edited by chfast Loading

codecov bot commented Apr 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodiazet Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodiazet commented Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodiazet commented Apr 13, 2023 •

edited by chfast

Loading

codecov bot commented Apr 14, 2023 •

edited

Loading

rodiazet Apr 18, 2023 •

edited

Loading

rodiazet commented Apr 18, 2023 •

edited

Loading