Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

bug fix for transaction receipts with multiple transactions in one block #268

Merged
merged 6 commits into from Feb 19, 2019

Conversation

davidbancor
Copy link
Contributor

When calling web3.eth.getTransactionReceipt() for a transaction which was mined with at least one other transaction in the same block, the logs returned include the logs of every other transaction in that block. As you can imagine reproducing the bug isn't trivial, but if you'd like i'll provide a detailed way to reproduce this issue. The fix I added in receiptserializer.js filters the logs in the block the transaction was included in to only return logs that match the original transaction hash.

David Benchimol and others added 2 commits January 1, 2019 18:35
@mikeseese
Copy link
Contributor

@balexander4 verified this issue: #273 (comment)

@davidmurdoch
Copy link
Member

Thanks, @davidbancor and @seesemichaelj! We'll need to write some tests for this before merging, but the fix itself LGTM!

@davidbancor
Copy link
Contributor Author

Could it perhaps be an issue that the fix isn't normalizing the transaction hash to be lowercase, and then when calling web3.eth.getTransactionReceipt you can get different results for passing in a non lowercase transaction hash?

@davidmurdoch
Copy link
Member

@davidbancor It could be. We'll need to add a test for that.

@davidmurdoch
Copy link
Member

@davidbancor Opened an issue for handling mixed-case or uppercase transaction hashes, as Ganache doesn't currently support it (at least not for getting transaction receipts): #308

I don't think it is currently possible to generate a log or transaction that records an upper-mixed-case transaction hash in the context of the filter logging code this PR addresses, however I do think adding the toLowerCase check could prevent possible issues down the line so I'll go ahead and make the change. Thanks again!

Copy link
Contributor

@nicholasjpaterno nicholasjpaterno left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I think we should discuss more how this effects forking.

lib/database/receiptserializer.js Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants