Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add transaction hash for debug trace responses #8205

Merged
merged 9 commits into from
Feb 4, 2025

Conversation

matkt
Copy link
Contributor

@matkt matkt commented Jan 30, 2025

PR description

This PR adds the transaction hash to the response of a debug trace API. This aligns the behavior with other clients and makes debugging easier.

for debug_traceBlock* the trx hash will be displayed and for debugTraceTransaction the tx hash will not be displayed

{
      "txHash":"0x49f505ed122c398287e1558275ccfcaa2d22df98df5b55e6c70635e6aa3a6ecc",
      "result":{
        "gas":21164,
        "failed":false,
        "returnValue":"",
        "structLogs":[
          {
            "pc":0,
            "op":"STOP",
            "gas":17592186023252,
            "gasCost":0,
            "depth":1
          }
        ]
      }

Fixed Issue(s)

fixes #8195

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • spotless: ./gradlew spotlessApply
  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

@matkt matkt requested a review from ahamlat January 30, 2025 15:48
Copy link
Contributor

@ahamlat ahamlat left a comment

Choose a reason for hiding this comment

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

LGTM, just a small comment related to formatting json files in the tests.
I think it makes sens to keep existing format, unless you have a strong motivation around it.

"id":1,
"result":{
"txHash":"0x2d8f594c7a98a9cb3d3a21d551f68e916e7ecd767971b061f2994b9a7426fba8",
"result":{
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe review the format to keep is as it is and in the differences only what changes. I see that almost the whole json file changed because of the change in the format, e.g

}, {

is replaced with

},
 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I will not be able to avoid that because it will change with the indentation

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough

@matkt matkt requested a review from ahamlat January 31, 2025 14:41
@matkt matkt enabled auto-merge (squash) February 4, 2025 09:03
@macfarla
Copy link
Contributor

macfarla commented Feb 4, 2025

@matkt think these might be legit failures (not flaky)

DebugTraceJsonRpcHttpBySpecTest > jsonRPCCallWithSpecFile(String, URL) > 5: debug_traceCall_noGasPrice.json FAILED
    org.opentest4j.AssertionFailedError at AbstractJsonRpcHttpBySpecTest.java:250

DebugTraceJsonRpcHttpBySpecTest > jsonRPCCallWithSpecFile(String, URL) > 6: debug_traceCall_withFutureNonce.json FAILED
    org.opentest4j.AssertionFailedError at AbstractJsonRpcHttpBySpecTest.java:250

Signed-off-by: Karim Taam <[email protected]>
@matkt
Copy link
Contributor Author

matkt commented Feb 4, 2025

debug_traceCall_noGasPrice

should be good now

@matkt matkt merged commit 214673c into hyperledger:main Feb 4, 2025
43 checks passed
pullurib pushed a commit to pullurib/besu that referenced this pull request Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debug_traceBlockByX response is missing txHash for trace entries
3 participants