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

Revert "Replace Kip103ContractCaller by BlockchainContractCaller" #1923

Merged
merged 1 commit into from Aug 8, 2023

Conversation

blukat29
Copy link
Contributor

@blukat29 blukat29 commented Aug 7, 2023

Proposed changes

This reverts commit 15fb12b.

BlockchainContractCaller as-is was not appropriate to substitute Kip103ContractCaller.

  • Kip103ContractCaller, in its implementation, increments the caller.From nonce in CallContract
  • BlockchainContractCaller does not
  • This difference caused the Baobab sync test failure.

This PR revives Kip103ContractCaller as a quick fix.

Types of changes

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

Further comments

Copy link
Contributor

@hyunsooda hyunsooda left a comment

Choose a reason for hiding this comment

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

I have two questions:

  1. I'm struggling to understand the discrepancy between this and old versions' code.
    I got that there's an inconsistent state between Finalize() call's one and the contract caller's one. The last block's state (contract caller's state) was used for the KIP-103 contract read. Here, even the current transactions created different state compared to the last block's state, the read operation of KIP-103 with the state at last block seems not correlated. So, I'm now confusing how two different states are created here. The RebalanceTreasury() function modifies the currently being modified state and read the contract storage with the last block's state. The final state should not be different if the reading the KIP-103 contract does not have effect by the current state and I think there's no any correlation. If no, the memo logging must be different.

  2. If we consider a scenario where no transactions occur at the KIP-103 activation block number, could this error potentially go unnoticed or be overlooked to some extent?

@ian0371 ian0371 added the need to merge Need to merge for the next time label Aug 7, 2023
@blukat29
Copy link
Contributor Author

blukat29 commented Aug 7, 2023

@hyunsooda Good question.

There was a bug, a side effect, in Kip103ContractCaller where the nonce of 0x0 account was incremented during contract calls. BlockchainContractCaller does not have the bug. That's why the memo and account balances were all correct yet sync test failed.

@blukat29 blukat29 merged commit 251b032 into klaytn:dev Aug 8, 2023
11 checks passed
@blukat29 blukat29 deleted the revert-kip103-common-caller branch September 11, 2023 07:03
@blukat29 blukat29 removed the need to merge Need to merge for the next time label Nov 14, 2023
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

6 participants