Skip to content
This repository has been archived by the owner on Aug 19, 2024. It is now read-only.

[SC] Added response handler for invalid txs #1295

Closed
wants to merge 6 commits into from

Conversation

hyunsooda
Copy link
Contributor

@hyunsooda hyunsooda commented Mar 31, 2022

Proposed changes

An observation that some bridge txs continuously stay in the bridge tx pool was reported. In order to remove bridge tx from the pool, the corresponding receipt from the counterpart chain must arrive as an answer of the successful process. That txs are resent until that corresponding receipt arrives. It makes sense. However, the problem is the sync is not matched between the sender side and receiver side.

As a pipeline of the current implementation, the sender sends two RPC requests:

  • sending bridge tx to be mined
  • receiving back the corresponding receipt at every newly generated block (i.e., every 1s)

The receiver receives two requests and handles, but that tx is not mined in the new block at the time of the request, which causes no receipt response at this point. After that, the sender sends two requests again. The number of tx is two since he didn't get a receipt from the previous request. The receiver's new block now contains the previous tx and sends a receipt but doesn't include the fresh tx that comes in this request. The same scenario is looped and pending tx never becomes zero.

This PR adds:

  • A request and response handler for invalid bridge txs sends invalid bridge tx hashes back to the sender so that the sender can remove those txs. An example of an invalid tx is "already known tx". (There's no error code for the type of invalid txs)
  • Do not send anchor tx if the anchor period does not arrive.

Refer to optimization issue at #1294.

Types of changes

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...

Comment on lines +41 to +45
ServiceChainInvalidTxResponseMsg = 0x06

ServiceChainCall = 0x06
ServiceChainResponse = 0x07
ServiceChainNotify = 0x08
ServiceChainCall = 0x07
ServiceChainResponse = 0x08
ServiceChainNotify = 0x09
Copy link
Member

Choose a reason for hiding this comment

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

This kind of protocol update can make a mismatch in the message protocol.
Why don't you increase the protocol version? We can consider a better protocol version management policy also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What a great catch! Thank you so much. The SC protocol version scope is currently going on another PR(#1248). Once that PR is finalized, let me update it.

@hyunsooda hyunsooda requested a review from ehnuje as a code owner April 13, 2022 09:40
@aidan-kwon aidan-kwon added the do not merge don't merge this PR yet label Apr 14, 2022
@henry-will henry-will modified the milestones: v1.8.3, v1.9.0 Apr 14, 2022
@henry-will henry-will removed the do not merge don't merge this PR yet label Apr 14, 2022
@hyunsooda hyunsooda added the do not merge don't merge this PR yet label Apr 16, 2022
@hyunsooda
Copy link
Contributor Author

This PR should be delayed until #1248 is finalized.

@henry-will henry-will removed the do not merge don't merge this PR yet label Apr 18, 2022
@henry-will henry-will modified the milestones: v1.9.0, v1.9.1 Jun 30, 2022
@hyunsooda
Copy link
Contributor Author

A handler of invalid tx removal is now moved to #1427. The remainder of TODO is to avoid sending duplicated anchoring tx https://github.com/klaytn/klaytn/pull/1295/files#diff-1623298e4e46f32d0a4b35ea8ba2fd0a9de08103caf3820b7ce69293b6e765e5R352-R364.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants