8000 [SC] Sync of KIP71-related values by hyunsooda · Pull Request #1427 · klaytn/klaytn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

[SC] Sync of KIP71-related values #1427

Merged
merged 25 commits into from
Jul 5, 2022

Conversation

hyunsooda
Copy link
Contributor
@hyunsooda hyunsooda commented Jun 10, 2022

Proposed changes

This change syncs parent chain's important values (gasPrice and KIP71-related values) when a bridge tx is failed by the ErrGasPriceBelowBaseFee error. Also, that tx is removed from bridgeTxPool and delegates its reconstruction of valid tx with synced gasPrice to Value Transfer Recovery feature. The UpperBoundBaseFee becomes gasPrice in construction of value transfer transaction.

  • A new protocol message is added by awareness of failed transactions that do not have receipts (i.e., failed txs by precheck)
  • The corresponding sender and receiver of the new protocol message are added and bumped up in the protocol version.
  • Three APIs (one setter and two getters) for sync of parent chain value are added
  • Unused gen_config.go and its generate code were removed.## Types of changes

Need for New Test Environment

cc @2dvorak, @henry-will
Of the lack of a simulation environment such as the simulated backend, testing only one chain with one contract, etc, I faced the need for a real testing environment (two or more chains, two or more contracts, and a real chain pipeline). Consideration of reliable testing seems required.

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • 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

  • Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...


logger.Error("Bridge tx is removed which has lower gasPrice than UpperBoundBaseFee")
// Remove the tx and delegate re-execution of the tx by Value Transfer Recovery feature
if err := sbh.subbridge.GetBridgeTxPool().RemoveTx(tx); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to perform vtrecovery logic with just removing? I'm just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The value transfer recovery (VTR) finds unhandled events from the counterpart chain. As the event was not handled by the parent chain, the VTR finds unhandled events and regenerates the corresponding txs.
  2. The received invalid txs must be removed because they live in bridgeTxPool forever and consumes unnecessary resending overhead. (i.e., txPending is counted)

@hqjang-pepper
Copy link
Contributor

By the way, any reason of removing gen_config.go?
I am curious why you only removed node/sc/gen_config.go.
The gen_config.go file also exists in node/cn and dbsyncer

@hyunsooda
Copy link
Contributor Author

The node/sc/gen_config.go had marshal and unmarshal functions for TOML format. The functions had not been used at all in ServiceChain package.

@henry-will henry-will added this to the v1.9.0 milestone Jun 15, 2022
Copy link
Contributor
@hqjang-pepper hqjang-pepper left a comment

Choose a reason for hiding this comment

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

LGTM, fix the linter problem

@hyunsooda
Copy link
Contributor Author

@hqjang-pepper, As the target branch to be merged is klaytn:dynamicGasPrice, I think the implementation of KIP71 should be confirmed and merged first to dev branch before merging this PR. So, how about delaying the linter fix until that implementation is merged to dev?

@henry-will henry-will added the need to merge Need to merge for the next time label Jun 30, 2022
@hyunsooda hyunsooda mentioned this pull request Jun 30, 2022
10 tasks
@aidan-kwon
Copy link
Member

@kjeom PTAL

@hyunsooda
Copy link
Contributor Author

@2dvorak, Please take a look again.

Copy link
Contributor
@jimni1222 jimni1222 left a comment

Choose a reason for hiding this comment

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

LGTM Thank you for changings!

@2dvorak 2dvorak self-requested a review July 5, 2022 06:35
@kjeom
Copy link
kjeom commented F438 Jul 5, 2022

LGTM.

  • You need to check the checklist.
  • Please share when the new test env. and cases is built.

@hyunsooda hyunsooda merged commit 8735242 into klaytn:dynamicGasPrice Jul 5, 2022
@hyunsooda
Copy link
Contributor Author

@kjeom Added.

@blukat29 blukat29 removed the need to merge Need to merge for the next time label Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
46ED
Development

Successfully merging this pull request may close these issues.

8 participants
0