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

fix: rlp decode of SignedTransction with interoperation signature #1533

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

KaoImin
Copy link
Contributor

@KaoImin KaoImin commented Nov 7, 2023

What this PR does / why we need it?

This PR:

  1. Fix the interoperation transaction sender recovery:
    Original
    a. When the transaction is from API, the sender address is recovery by the interoperation logic
    b. When the transaction if from network and insert into mempool directly, the sender address is recovery by rlp decode. However, the rlp decode logic is different from the interoperation.
    Current
    The rlp decode logic is same as interoperation logic.
  2. Remove the useless verify_by_ckb_vm verification code due to the schedule.
  3. Only call CKB-VM mode is supported now

What is the impact of this PR?

No Breaking Change

PR relation:

CI Settings

CI Usage

Tip: Check the CI you want to run below, and then comment /run-ci.

CI Switch

  • Web3 Compatible Tests
  • OCT 1-5 And 12-15
  • OCT 6-10
  • OCT 11
  • OCT 16-19
  • v3 Core Tests

CI Description

CI Name Description
Web3 Compatible Test Test the Web3 compatibility of Axon
v3 Core Test Run the compatibility tests provided by Uniswap V3
OCT 1-5 | 6-10 | 11 | 12-15 | 16-19 Run the compatibility tests provided by OpenZeppelin

@KaoImin KaoImin force-pushed the remove-verify-by-ckb-vm branch from 6163ccd to de3c4ae Compare November 7, 2023 08:35
@yangby-cryptape
Copy link
Collaborator

If there is a bugfix, please split the bugfix and the refactor into two PRs, at least two commits.

As an answer of the #1514, base on this PR, I don't understand whether that issue is confirmed or not.

@KaoImin KaoImin marked this pull request as ready for review November 9, 2023 02:13
@KaoImin KaoImin requested a review from a team as a code owner November 9, 2023 02:13
@KaoImin KaoImin requested review from jjyr and wenyuanhust November 9, 2023 02:13
@KaoImin KaoImin force-pushed the remove-verify-by-ckb-vm branch 2 times, most recently from 9beaf3c to febdc75 Compare November 9, 2023 02:18
@KaoImin
Copy link
Contributor Author

KaoImin commented Nov 9, 2023

If there is a bugfix, please split the bugfix and the refactor into two PRs, at least two commits.

As an answer of the #1514, base on this PR, I don't understand whether that issue is confirmed or not.

They are divided into two commits.

@KaoImin KaoImin changed the title refactor: remove signature verify by CKB VM verification fix: rlp decode of SignedTransction with interoperation signature Nov 9, 2023
@github-actions github-actions bot added the bugfix label Nov 9, 2023
@KaoImin KaoImin requested review from driftluo and yangby-cryptape and removed request for jjyr and wenyuanhust November 9, 2023 02:29
@Flouse
Copy link
Contributor

Flouse commented Nov 14, 2023

/run-ci

Copy link

CI tests run on commit:

CI test list:

  • v3 Core Tests

Please check ci test results later.

Flouse

This comment was marked as resolved.

@KaoImin KaoImin force-pushed the remove-verify-by-ckb-vm branch from febdc75 to befcbb9 Compare November 14, 2023 14:26
@KaoImin KaoImin requested a review from Flouse November 14, 2023 14:27
@KaoImin KaoImin force-pushed the remove-verify-by-ckb-vm branch from befcbb9 to c91a6af Compare November 15, 2023 02:45
Copy link
Collaborator

@yangby-cryptape yangby-cryptape left a comment

Choose a reason for hiding this comment

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

Looks good to me, except the following line:

public: Some(Public::zero()),

  • Is it security?
  • Why it's not just a None?

It's not easy for me to assess it right now.
So I just mark it, let's merge this PR first, then I will spend time to assess it later when I free.

@Flouse
Copy link
Contributor

Flouse commented Nov 15, 2023

/run-ci

Copy link

CI tests run on commit:

CI test list:

  • OCT 1-5 And 12-15
  • OCT 6-10
  • OCT 11
  • OCT 16-19
  • v3 Core Tests
  • Web3 Compatible Tests

Please check ci test results later.

@Flouse
Copy link
Contributor

Flouse commented Nov 15, 2023

If there is a bugfix, please split the bugfix and the refactor into two PRs, at least two commits.
As an answer of the #1514, base on this PR, I don't understand whether that issue is confirmed or not.

They are divided into two commits.

Reminder

In order to preserve the commit history, please do NOT use merge_quere or squash merge for this PR.

@Flouse Flouse merged commit 835cb16 into main Nov 15, 2023
26 checks passed
@KaoImin KaoImin deleted the remove-verify-by-ckb-vm branch November 27, 2023 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The sender for a transaction with CKB-VM mode is incorrect.
4 participants