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

fix error:debug_traceCall API returns not valid json format output #10378

Closed
wants to merge 3 commits into from

Conversation

marshalys
Copy link
Contributor

try to fix debug_traceCall API returns not valid json format output when the tx execute is fail
#10376

@taratorio
Copy link
Member

@marshalys can you please share your curl command that reproduces this issue?

@taratorio
Copy link
Member

taratorio commented May 16, 2024

@marshalys can you also explain based on your investigation what exactly was attaching the "null" bit? my expectation was that it would only be "result":{"structLogs":[]},"error":{"code":-32000, ... - not clear what exactly appends the null after {"structLogs":[]}[].

@taratorio
Copy link
Member

taratorio commented May 16, 2024

@marshalys can you please test your use case with PR #10383 instead? I think I found what appends the "null"

@marshalys
Copy link
Contributor Author

@marshalys can you please share your curl command that reproduces this issue?

curl -H "Content-Type: application/json" -X POST --data '{"id":1,"jsonrpc":"2.0","method":"debug_traceCall","params":[{"from":"0xae2fc483527b8ef99eb5d9b44875f005ba1fae13","to":"0x6b75d8af000000e20b7a7ddf000ba900b4009a80","value":"0x16906ca2e71","gas":"0x2eaf9","gasPrice":"0x1175c375f","data":"0x57c7bbec68d12a0d1830360f8ec58fa599ba1b0e9b5fedbf9b6fdac17f958d2ee523a2206206994597c13d831ec70064","accessList":[],"type":"0x2"},"0x27a66641a0f38c81e961deeb142d54dc20ba64632f31ebc3c814057ed502c3e5",{}]}' 127.0.0.1:8545

@marshalys
Copy link
Contributor Author

@marshalys can you please test your use case with PR #10383 instead? I think I found what appends the "null"

to check result has many case depend the api return value, result may be bool or string also.

@marshalys
Copy link
Contributor Author

@marshalys can you also explain based on your investigation what exactly was attaching the "null" bit? my expectation was that it would only be "result":{"structLogs":[]},"error":{"code":-32000, ... - not clear what exactly appends the null after {"structLogs":[]}[].

if the api call failed, the result property is useless, I run the same api at geth, and the output of geth has no result property.

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"tracing failed: max fee per gas less than block base fee ....

@taratorio
Copy link
Member

@marshalys can you please test your use case with PR #10383 instead? I think I found what appends the "null"

to check result has many case depend the api return value, result may be bool or string also.

That's fine. I have other plans to make our json streaming guarantee correctness. Will work on implementing a stack-based Streamer around jsoniter.Stream that closes all pending json objects and fields upon errors. This way in case of errors we will always have generated correct json.

The PR I sent is aimed at simply fixing the issue you are reporting. Would appreciate you trying it out on your machine to verify it fixes the issue you raised please.

That is the direction I prefer to take for now.

@taratorio
Copy link
Member

taratorio commented May 17, 2024

@marshalys can you also explain based on your investigation what exactly was attaching the "null" bit? my expectation was that it would only be "result":{"structLogs":[]},"error":{"code":-32000, ... - not clear what exactly appends the null after {"structLogs":[]}[].

if the api call failed, the result property is useless, I run the same api at geth, and the output of geth has no result property.

{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"tracing failed: max fee per gas less than block base fee ....

That is because as far as I know geth does not use JSON streaming.

Erigon uses json streaming.

If there is an error a client should ignore the result field. Only thing that matters for us is for that json to be valid. As I explained here I have a direction I would like to take for ensuring our json stream always produces valid json in case of errors.

@taratorio
Copy link
Member

Managed to test #10383 which seems to have fixed the reported issue.

Closing in favour of:

@marshalys many thanks for your contribution and for reporting this issue 🙏

taratorio added a commit that referenced this pull request May 17, 2024
)

fixes #10376
fixes #10378

let's wait for user feedback before merging
@marshalys
Copy link
Contributor Author

Yes, I think your conside is good.
@taratorio Thank you for your incredibly quick response! 😊

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.

2 participants