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.

Sign up for GitHub

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

Jump to bottom

EVM-732 Tx hash calculation should include tx type and chainID #1706

Merged
merged 10 commits into from
Jul 20, 2023
Merged

EVM-732 Tx hash calculation should include tx type and chainID #1706

merged 10 commits into from
Jul 20, 2023

Conversation

igorcrevar
Copy link
Contributor

@igorcrevar igorcrevar commented Jul 6, 2023 •
edited

Description

Tx hash calculation should include tx type and correct chain id

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

Tx hash calculation and TxRootHash calculation

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

@igorcrevar igorcrevar added the bug fix Functionality that fixes a bug label Jul 6, 2023
@igorcrevar igorcrevar requested a review from a team July 6, 2023 10:16
@igorcrevar igorcrevar self-assigned this Jul 6, 2023
@igorcrevar igorcrevar force-pushed the feature/EVM-732-tx-hash-type branch 4 times, most recently from 75b0577 to e96dc44 Compare July 6, 2023 10:25
@igorcrevar igorcrevar marked this pull request as ready for review July 6, 2023 11:30
@rachit77 rachit77 added the breaking change Functionality that contains breaking changes label Jul 7, 2023
@igorcrevar igorcrevar changed the title EVM-732 Tx hash calculation should include tx type EVM-732 Tx hash calculation should include tx type and chainID Jul 10, 2023
@igorcrevar igorcrevar force-pushed the feature/EVM-732-tx-hash-type branch 3 times, most recently from 9b5016e to 6021c59 Compare July 11, 2023 11:29
Nemanja0x
Nemanja0x previously approved these changes Jul 11, 2023
View reviewed changes
goran-ethernal
goran-ethernal previously approved these changes Jul 11, 2023
View reviewed changes
rachit77
rachit77 previously approved these changes Jul 11, 2023
View reviewed changes
stana-miric
stana-miric previously approved these changes Jul 12, 2023
View reviewed changes
@dusan-maksimovic dusan-maksimovic self-requested a review July 12, 2023 08:02
@Nemanja0x
Copy link
Contributor

There are some issues regarding backward compatibility. It must not be merged until we resolve them

@igorcrevar igorcrevar force-pushed the feature/EVM-732-tx-hash-type branch from 6021c59 to f4f64d4 Compare July 12, 2023 14:37
@igorcrevar igorcrevar added the don't merge Please don't merge this functionality temporarily label Jul 12, 2023
@igorcrevar igorcrevar force-pushed the feature/EVM-732-tx-hash-type branch 6 times, most recently from 9bcf7b4 to 341ca98 Compare July 15, 2023 14:51
@igorcrevar igorcrevar dismissed stale reviews from Nemanja0x and rachit77 July 18, 2023 07:59

a lot more changes since review

vcastellm
vcastellm approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@vcastellm vcastellm temporarily deployed to testnet July 18, 2023 13:48 — with GitHub Actions Inactive
@vcastellm vcastellm temporarily deployed to devnet July 18, 2023 13:54 — with GitHub Actions Inactive
@igorcrevar igorcrevar removed the don't merge Please don't merge this functionality temporarily label Jul 18, 2023
dusan-maksimovic
dusan-maksimovic approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@dusan-maksimovic dusan-maksimovic 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 have that one comment/question.

jsonrpc/eth_endpoint.go Show resolved Hide resolved
Stefan-Ethernal
Stefan-Ethernal reviewed Jul 20, 2023
View reviewed changes
consensus/polybft/blockchain_wrapper.go Outdated Show resolved Hide resolved
consensus/polybft/fsm.go Outdated Show resolved Hide resolved
txpool/txpool.go Show resolved Hide resolved
forkmanager/fork.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
@igorcrevar igorcrevar force-pushed the feature/EVM-732-tx-hash-type branch from 691d50a to 34c8975 Compare July 20, 2023 09:40
@igorcrevar igorcrevar force-pushed the feature/EVM-732-tx-hash-type branch from 00230ba to af892dc Compare July 20, 2023 10:07
@igorcrevar igorcrevar force-pushed the feature/EVM-732-tx-hash-type branch from af892dc to f557e79 Compare July 20, 2023 10:11
@igorcrevar igorcrevar force-pushed the feature/EVM-732-tx-hash-type branch from 5270a02 to 1cdf114 Compare July 20, 2023 11:37
@igorcrevar igorcrevar merged commit 14ea06c into develop Jul 20, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2023
@Stefan-Ethernal Stefan-Ethernal deleted the feature/EVM-732-tx-hash-type branch July 23, 2023 05:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dusan-maksimovic dusan-maksimovic dusan-maksimovic approved these changes

@vcastellm vcastellm vcastellm approved these changes

@Stefan-Ethernal Stefan-Ethernal Stefan-Ethernal approved these changes

@Nemanja0x Nemanja0x Awaiting requested review from Nemanja0x

@rachit77 rachit77 Awaiting requested review from rachit77

@stana-miric stana-miric Awaiting requested review from stana-miric

@goran-ethernal goran-ethernal Awaiting requested review from goran-ethernal

Assignees

@igorcrevar igorcrevar

Labels
breaking change Functionality that contains breaking changes bug fix Functionality that fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@igorcrevar @Nemanja0x @vcastellm @rachit77 @Stefan-Ethernal @dusan-maksimovic @stana-miric @goran-ethernal