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 zero from field in Tx #719

Merged
merged 10 commits into from
Sep 21, 2022
Merged

Fix zero from field in Tx #719

merged 10 commits into from
Sep 21, 2022

Conversation

Kourin1996
Copy link
Contributor

@Kourin1996 Kourin1996 commented Sep 7, 2022

Fix EDGE-787

Description

This PR fixes the issue that from field in the transaction data JSON-RPC API returns is zero address in the latest code. The issue is caused by that block data is unmarshalled before writing block in latest IBFT and this encoding doesn't add from field in the marshaled data.

This PR adds 2 fixes:
(1) Recover from field in transaction in blockchain module before writing the block into storage.
(2) Recover from field if the transaction in the block doesn't have from field when fetching block from storage and update the data in storage as well.

Relevant issue:
#684

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

Please complete this section if any breaking changes have been made, otherwise delete it

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

Please complete this section if you ran manual tests for this functionality, otherwise delete it

Documentation update

Please link the documentation update PR in this section if it's present, otherwise delete it

Additional comments

Please post additional comments in this section if you have them, otherwise delete it

@Kourin1996 Kourin1996 added the bug fix Functionality that fixes a bug label Sep 7, 2022
@Kourin1996 Kourin1996 self-assigned this Sep 7, 2022
Copy link
Contributor

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks great 💯

I would just like to note that this issue was present even before we upgraded our consensus layer - the previous IBFT implementation (incorrectly) had a side effect of instantiating the From field of a transaction when verifying a block (even though it was not set), something that no normal consensus implementation should do. The new IBFT implementation doesn't do any such shenanigans, so this error propagated out (the transaction From not being marshaled in RLP) to other parts of the system.

Just make sure to resolve the linting errors.

Thank you for fixing this 🙏

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

❗ No coverage uploaded for pull request base (release/0.5.1@68df30d). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             release/0.5.1     #719   +/-   ##
================================================
  Coverage                 ?   52.56%           
================================================
  Files                    ?      130           
  Lines                    ?    17108           
  Branches                 ?        0           
================================================
  Hits                     ?     8993           
  Misses                   ?     7470           
  Partials                 ?      645           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Aleksao998 Aleksao998 left a comment

Choose a reason for hiding this comment

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

LGTM

blockchain/blockchain.go Outdated Show resolved Hide resolved
blockchain/blockchain.go Outdated Show resolved Hide resolved
@Kourin1996 Kourin1996 changed the base branch from develop to release/0.5.1 September 20, 2022 14:23
@Kourin1996 Kourin1996 changed the base branch from release/0.5.1 to develop September 20, 2022 14:23
@Kourin1996 Kourin1996 changed the base branch from develop to release/0.5.1 September 20, 2022 14:34
Copy link
Contributor

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good again 💯

@Kourin1996 Kourin1996 merged commit 1d6ba17 into release/0.5.1 Sep 21, 2022
@Kourin1996 Kourin1996 deleted the fix/set-tx-from branch September 21, 2022 09:25
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fix Functionality that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants