support for golang tracers + add golang callTracer by wgr523 · Pull Request #558 · XinFinOrg/XDPoSChain

wgr523 · 2024-06-17T02:33:12Z

Proposed changes

support for golang tracers + add golang callTracer

Types of changes

What types of changes does your code introduce to XDC network?
Put an ✅ in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices apply)
Regular KTLO or any of the maintaince work. e.g code style
CICD Improvement

Impacted Components

Which part of the codebase this PR will touch base on,

Put an ✅ in the boxes that apply

Checklist

Put an ✅ in the boxes once you have confirmed below actions (or provide reasons on not doing so) that

This PR has sufficient test coverage (unit/integration test) OR I have provided reason in the PR description for not having test coverage
Provide an end-to-end test plan in the PR description on how to manually test it on the devnet/testnet.
Tested the backwards compatibility.
Tested with XDC nodes running this version co-exist with those running the previous version.
Relevant documentation has been updated as part of this PR
N/A

minor changes in API refine api_tracer.go refine Tracer interface

cf. ethereum#23708

wgr523 · 2024-06-17T14:44:34Z

This PR's XXX_legacy.js tracers are not completely equal to XXX.js tracers before this PR. Shall we make XXX_legacy.js equal to prior XXX.js?

s1na · 2024-06-17T15:37:29Z

Why did you have to change these parts?

This change is for preparation of EIP2028. The reason we need EIP2028 is as follows:
If EIP2028 is not enabled, the test revert_reason.json will not pass unless we change the test itself. Check this commit that change the test json itself: 0a496c3
So actually this code change does not necessary belong to this PR. Do you want me to undo it?

The consensus will be changed if we enable eip-2028. I can include it in the PR of EIP-1559 if you need it.

Yes please let's remove that from this PR. It would be a separate discussion if EIP-2028 is useful for XDC.

Yes please let's remove that from this PR. It would be a separate discussion if EIP-2028 is useful for XDC.

I removed it and force pushed commits.

The consensus will be changed if we enable eip-2028. I can include it in the PR of EIP-1559 if you need it.

No hurry. At the time of eip2028, we need to remember to undo this commit: e1b7ba6

s1na · 2024-06-17T15:55:02Z

I think we can directly make this default callTracer

Sure. Then this tracer will override js tracer with the same name. Shall we name the js tracer?

Right callTracer should be the native tracer. IMO the old one can be kept as callTracerLegacy and the "new" JS call tracer I don't think it's really necessary.

Changed native tracer to the default one, for call tracer and noop tracer.

s1na · 2024-06-17T16:22:00Z

This PR's XXX_legacy.js tracers are not completely equal to XXX.js tracers before this PR. Shall we make XXX_legacy.js equal to prior XXX.js?

Can you post the diff?

wgr523 · 2024-06-19T15:17:39Z

This PR's XXX_legacy.js tracers are not completely equal to XXX.js tracers before this PR. Shall we make XXX_legacy.js equal to prior XXX.js?

Can you post the diff?

Sure. The diff by running diff call_tracer_legacy.js call_tracer_dev-upgrade.js -u where call_tracer_dev-upgrade.js is the one before this PR:

diff call_tracer_legacy.js call_tracer_dev-upgrade.js -u
--- call_tracer_legacy.js	2024-06-19 22:56:44.000000000 +0800
+++ call_tracer_dev-upgrade.js	2024-06-19 22:57:58.000000000 +0800
@@ -61,14 +61,7 @@
 			if (this.callstack[left-1].calls === undefined) {
 				this.callstack[left-1].calls = [];
 			}
-			this.callstack[left-1].calls.push({
-				type:    op,
-				from:    toHex(log.contract.getAddress()),
-				to:      toHex(toAddress(log.stack.peek(0).toString(16))),
-				gasIn:   log.getGas(),
-				gasCost: log.getCost(),
-				value:   '0x' + db.getBalance(log.contract.getAddress()).toString(16)
-			});
+			this.callstack[left-1].calls.push({type: op});
 			return
 		}
 		// If a new method invocation is being done, add to the call stack
@@ -139,12 +132,13 @@
 				// If the call was a contract call, retrieve the gas usage and output
 				if (call.gas !== undefined) {
 					call.gasUsed = '0x' + bigInt(call.gasIn - call.gasCost + call.gas - log.getGas()).toString(16);
-				}
-				var ret = log.stack.peek(0);
-				if (!ret.equals(0)) {
-					call.output = toHex(log.memory.slice(call.outOff, call.outOff + call.outLen));
-				} else if (call.error === undefined) {
-					call.error = "internal failure"; // TODO(karalabe): surface these faults somehow
+
+					var ret = log.stack.peek(0);
+					if (!ret.equals(0)) {
+						call.output = toHex(log.memory.slice(call.outOff, call.outOff + call.outLen));
+					} else if (call.error === undefined) {
+						call.error = "internal failure"; // TODO(karalabe): surface these faults somehow
+					}
 				}
 				delete call.gasIn; delete call.gasCost;
 				delete call.outOff; delete call.outLen;
@@ -214,7 +208,7 @@
 		} else if (ctx.error !== undefined) {
 			result.error = ctx.error;
 		}
-		if (result.error !== undefined && (result.error !== "execution reverted" || result.output ==="0x")) {
+		if (result.error !== undefined) {
 			delete result.output;
 		}
 		return this.finalize(result);

For other tracers, legacy tracer and dev-upgrade tracer are the same.

s1na · 2024-06-20T10:04:40Z

The diff by running diff call_tracer_legacy.js call_tracer_dev-upgrade.js -u where call_tracer_dev-upgrade.js is the one before this PR

between those two files call_tracer_legacy.js seems more correct.

wgr523 · 2024-06-21T02:46:24Z

The diff by running diff call_tracer_legacy.js call_tracer_dev-upgrade.js -u where call_tracer_dev-upgrade.js is the one before this PR

between those two files call_tracer_legacy.js seems more correct.

So I guess call_tracer_legacy.js is already ready in this PR. No further change is needed.

gzliudan · 2024-07-09T02:17:31Z

Request:

curl -s -X POST -H "Content-Type: application/json" ${RPC} -d '{
  "jsonrpc": "2.0",
  "id": 1001,
  "method": "debug_traceBlockByNumber",
  "params": [
    "latest", 
    {"tracer":"callTracer"}
  ]
}' | jq

Response:

{
  "jsonrpc": "2.0",
  "id": 1001,
  "result": [
    {
      "result": {
        "type": "CALL",
        "from": "xdc4398241671b3dd484fe3213a4fb7511f30e7d7c0",
        "to": "xdc0000000000000000000000000000000000000089",
        "value": "0x0",
        "gas": "0x2b068",
        "gasUsed": "0x147ce",
        "input": "0xe341eaa400000000000000000000000000000000000000000000000000000000002b85b848d26996721c2750b15a0a5d37de015365ae688b0bb031e52723d6e4d73d63ae",
        "output": "0x"
      }
    }
  ]
}

Why from and to are xdc prefix ?

gzliudan · 2024-07-11T01:40:58Z

Why call strings.ToLower ? I think return string(s) is enough:

We should keep the address is checksumed

reduce CPU cost and network response time

Ok. Ethereum uses lower case. If we don't need lower case we can just return the string(s)

gzliudan · 2024-07-11T08:35:39Z

The callTracer handles address prefix properly now. But the callTracerLegacy returns 0x prefix for address when XDC is started with the flag --enable-xdc-prefix which should returns xdc prefix for address.

s1na · 2024-07-15T13:07:19Z

 		// balance transfer may never fail.
 		if vmerr == vm.ErrInsufficientBalance {
-			return nil, 0, false, vmerr, nil
+			return nil, 0, false, nil, vmerr


Can you explain this change? It is a sensitive part of the code.

I am convinced this will introduce a bug and should be resolved.

I agree. I checked history geth commit: ethereum@b9df7ec
Although the variable name is vmerr, this error means no balance to pay for the tx. It should be consensus err and should be returned in 4-th err place.
I'll revert this

s1na · 2024-07-15T13:19:38Z

@@ -1,4 +1,4 @@
-// Copyright 2017 The go-ethereum Authors
+// Copyright 2021 The go-ethereum Authors


I believe there is no way to access this particular tracer. It has the same name as the native callTracer. IMHO we can simply drop this (or replace the callTracerLegacy with it). Depends if we want users to still access the "old tracer".

I think we can drop it.

I believe there is no way to access this particular tracer. It has the same name as the native callTracer. IMHO we can simply drop this (or replace the callTracerLegacy with it). Depends if we want users to still access the "old tracer".

fixed and code pushed

… vmerr

s1na

Looks good to me

* feat: rename Tracer interface to EVMLogger; minor changes in API refine api_tracer.go refine Tracer interface * fix: broken tracer tests * feat: add BenchmarkTransactionTrace * feat: tracer CaptureEnter CaptureExit in evm * feat: upgrade js tracers with geth upstream * chore: clean test * feat: eth/tracers: support for golang tracers + add golang callTracer cf. ethereum#23708 * chore: clean testdata json * fix: change test due to IntrinsicGas is not upgraded * feat: make native Tracer the default Tracer * fix: update tracers.New in api * fix: addr prefix in callTracer * fix: remove `native` in BenchmarkTracers * fix: return consensus error of InsufficientBalance for tx, instead of vmerr * chore: drop js tracers: call and noop

wgr523 added 7 commits June 17, 2024 21:45

feat: rename Tracer interface to EVMLogger;

96c6b9c

minor changes in API refine api_tracer.go refine Tracer interface

fix: broken tracer tests

95eb5d0

feat: add BenchmarkTransactionTrace

a2dba52

feat: tracer CaptureEnter CaptureExit in evm

89afd2e

feat: upgrade js tracers with geth upstream

e88bdbd

chore: clean test

fec009d

feat: eth/tracers: support for golang tracers + add golang callTracer

afa19f8

cf. ethereum#23708

wgr523 force-pushed the golang-tracer branch from 54955b5 to 0a496c3 Compare June 17, 2024 13:46

wgr523 requested a review from liam-lai June 17, 2024 14:07

s1na reviewed Jun 17, 2024

View reviewed changes

wgr523 added 3 commits June 21, 2024 00:35

chore: clean testdata json

8d176f7

fix: change test due to IntrinsicGas is not upgraded

e1b7ba6

feat: make native Tracer the default Tracer

6bdee24

wgr523 force-pushed the golang-tracer branch from 405a275 to 6bdee24 Compare June 20, 2024 16:44

wgr523 requested review from gzliudan and s1na June 21, 2024 02:42

fix: update tracers.New in api

0c7c95d

gzliudan reviewed Jul 11, 2024

View reviewed changes

fix: addr prefix in callTracer

de2be8f

wgr523 force-pushed the golang-tracer branch from 5e8fd65 to de2be8f Compare July 13, 2024 02:06

s1na reviewed Jul 15, 2024

View reviewed changes

fix: remove native in BenchmarkTracers

89fb632

wgr523 added 2 commits July 23, 2024 23:02

fix: return consensus error of InsufficientBalance for tx, instead of…

056e16d

… vmerr

chore: drop js tracers: call and noop

a3d48d0

s1na approved these changes Jul 25, 2024

View reviewed changes

gzliudan approved these changes Jul 29, 2024

View reviewed changes

liam-lai merged commit 52077f1 into dev-upgrade Aug 6, 2024

liam-lai deleted the golang-tracer branch August 6, 2024 07:14

gzliudan mentioned this pull request Aug 6, 2024

Backport golang native tracers from go-ethereum #220

Closed

gzliudan mentioned this pull request Aug 19, 2024

debug_traceBlockByNumber way too slow #378

Closed

gzliudan mentioned this pull request Sep 20, 2024

split vm.Context into BlockContext and TxContext #638

Merged

19 tasks

		@@ -1,4 +1,4 @@
		// Copyright 2017 The go-ethereum Authors
		// Copyright 2021 The go-ethereum Authors

Conversation

wgr523 commented Jun 17, 2024

Proposed changes

Types of changes

Impacted Components

Checklist

Uh oh!

wgr523 commented Jun 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzliudan Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1na commented Jun 17, 2024

Uh oh!

wgr523 commented Jun 19, 2024

Uh oh!

s1na commented Jun 20, 2024

Uh oh!

wgr523 commented Jun 21, 2024

Uh oh!

gzliudan commented Jul 9, 2024

Uh oh!

gzliudan Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzliudan commented Jul 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s1na left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gzliudan Jun 19, 2024 •

edited

Loading

gzliudan Jul 11, 2024 •

edited

Loading