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

consensus/parlia: set nonce before evm run #2185

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

buddh0
Copy link
Collaborator

@buddh0 buddh0 commented Jan 26, 2024

Description

consensus/parlia: set nonce before evm run

Rationale

It's strange to set nonce after execute tx, so align with it in TransitionDb
no need to use hardfork to control, there are 3 reasons:

1. The GetNonce function will not be invoked during the execution process of the virtual machine, so placing it before or after the call execution has no impact.

2. Modifications to the state occur on a per-block basis, so placing them after Finalise also does not affect the overall result of block execution.

3. The journal is used for rollback purposes; transactions in Parlia will not revert, so the impact can be ignored.

Example

add an example CLI or API response...

Changes

Notable changes:

  • add each change in a bullet point here
  • ...

@buddh0 buddh0 marked this pull request as draft January 26, 2024 06:33
@buddh0 buddh0 marked this pull request as ready for review January 29, 2024 06:30
@zzzckck zzzckck merged commit 29427c5 into bnb-chain:develop Jan 29, 2024
6 checks passed
@buddh0 buddh0 deleted the Parlia_systemtx_SetNonce branch February 2, 2024 08:45
@maoueh
Copy link
Contributor

maoueh commented Feb 23, 2024

I would like to raise the fact that while indeed that consensus will not break because of such change, it do breaks the determinism of our Firehose tracing system as it expects that the execution happens in the same order whatever version of the node you use.

Now if I reprocess with v1.3.8 to trace the full chain and again with v1.3.9 I will get different results because the SetNonce is not done at the exact same place.

I would like see if the BNB team would be open to revert the change. Seems that the only reason was to better align with Geth ways of doing things, so doesn't feel like it's a strict requirement for a bug fix of some upcoming feature.

minh-bq added a commit to minh-bq/ronin that referenced this pull request Apr 4, 2024
Currently, in system transaction we increase nonce after transaction execution
which is not consistent with normal transaction. This change does not require a
hardfork as:
- Nonce is not used when executing transaction in virtual machine
- Consortium-v2 is after Byzantium so we don't fall through the path to
  calculate root hash after transaction execution

Reference: bnb-chain/bsc#2185
minh-bq added a commit to minh-bq/ronin that referenced this pull request Apr 5, 2024
Currently, in system transaction we increase nonce after transaction execution
which is not consistent with normal transaction. This change does not require a
hardfork as:
- Nonce is not used when executing transaction in virtual machine
- Consortium-v2 is after Byzantium so we don't fall through the path to
  calculate root hash after transaction execution

Reference: bnb-chain/bsc#2185
minh-bq added a commit to axieinfinity/ronin that referenced this pull request Apr 5, 2024
…ion (#434)

Currently, in system transaction we increase nonce after transaction execution
which is not consistent with normal transaction. This change does not require a
hardfork as:
- Nonce is not used when executing transaction in virtual machine
- Consortium-v2 is after Byzantium so we don't fall through the path to
  calculate root hash after transaction execution

Reference: bnb-chain/bsc#2185
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.

4 participants