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

rlp.EncodeToBytes => tx.MarshalBinary #246

Merged
merged 2 commits into from
Jun 13, 2023
Merged

rlp.EncodeToBytes => tx.MarshalBinary #246

merged 2 commits into from
Jun 13, 2023

Conversation

i-norden
Copy link
Collaborator

@i-norden i-norden commented Jun 12, 2023

For #245

While Thomas was testing geth vs ipld-eth-server output a discrepancy was identified with the "eth_getRawTransaction*" endpoints:

geth:
{
    "jsonrpc": "2.0",
    "id": 1,
    "result": "0x02f86e8204bc42843b9aca00843b9aca0e82520894e22ad83a0de117ba0d03d5e94eb4e0d80a69c62a82117780c080a00fdcce5c89cf7cdece6aafdeb53ddd419708245bfb82c40429b95b903c3af487a00a82d20e108d6e4d9880911ec70bd50f123a091926fc949a9c8ff6189769bdde"
}
ipld-eth-server:
{
    "jsonrpc": "2.0",
    "id": 1,
    "result": "0xb87102f86e8204bc42843b9aca00843b9aca0e82520894e22ad83a0de117ba0d03d5e94eb4e0d80a69c62a82117780c080a00fdcce5c89cf7cdece6aafdeb53ddd419708245bfb82c40429b95b903c3af487a00a82d20e108d6e4d9880911ec70bd50f123a091926fc949a9c8ff6189769bdde"
}

I suspect this is the issue, if it isn't- it is still an issue.

@i-norden i-norden requested a review from telackey June 12, 2023 22:20
@i-norden
Copy link
Collaborator Author

i-norden commented Jun 12, 2023

rlp.EncodeToBytes was also being used for the expected results we compare to in the unit and integration tests, so the results matched and the issue was not detected there.

@AFDudley
Copy link
Contributor

Why did Ian write this patch instead of Thomas?

@telackey
Copy link
Contributor

telackey commented Jun 13, 2023

When I found the bug this afternoon, I discussed it with Ian, and he had an idea on the fix before I had looked in to the cause.

Incidentally, this does look like a fix:

test/test_tx.py::test_get_raw_transaction PASSED
test/test_tx.py::test_get_raw_transaction_by_block PASSED                                                                                                                                                  

From: https://git.vdb.to/cerc-io/system-tests

@i-norden i-norden merged commit 68d6695 into v5 Jun 13, 2023
5 checks passed
@github-cerc-io github-cerc-io deleted the ian_bug_fix branch October 2, 2023 19:04
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.

None yet

3 participants