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

feat: Reject new scripts with known bugs #1915

Merged
merged 3 commits into from Feb 1, 2020

Conversation

@xxuejie
Copy link
Member

xxuejie commented Jan 17, 2020

For compatibility reasons, there're certain bugs that we have to leave
to the next hardfork to fix. However those bugs, especially VM bugs
might lead to surprising unexpected behaviors. This change adds a new
checker that checks against newly created cells for scripts with bugs,
and reject those transaction when we can. This way we can alert users
about the bugs as early as we can.

@xxuejie xxuejie requested a review from nervosnetwork/ckb-code-review as a code owner Jan 17, 2020
@xxuejie xxuejie requested review from quake and jjyr Jan 17, 2020
script/src/known_bugs_checker.rs Outdated Show resolved Hide resolved
@doitian doitian requested review from doitian and removed request for quake Jan 17, 2020
@jjyr

This comment has been minimized.

Copy link
Member

jjyr commented Jan 18, 2020

I still think we should not check the relayed txs. Here are the reasons:

  1. The intention of this mechanism is to hint the user to avoid potential VM bugs. If a contract author sends tx via a recent client he already gets the error message hint since we check instructions in RPC. Instead, if the user sends tx via an old client there maybe two results: 1 he observes that the tx gets confirmed without an error message, the potential bugs will still exist in his script. 2 he observes that the tx never gets confirmed since other nodes use a new version client, and fall into self-doubt. both situations do not help the author.
  2. We scan and decode every instruction to check the potential error, its OK in RPC since every user sends tx from the user's own node, it naturally decentralized; but for relayed txs, I doubt there may be a potential weak point to DDOS, and may drawback the relay performance.
@xxuejie

This comment has been minimized.

Copy link
Member Author

xxuejie commented Jan 18, 2020

I still think we should not check the relayed txs. Here are the reasons:

  1. The intention of this mechanism is to hint the user to avoid potential VM bugs. If a contract author sends tx via a recent client he already gets the error message hint since we check instructions in RPC. Instead, if the user sends tx via an old client there maybe two results: 1 he observes that the tx gets confirmed without an error message, the potential bugs will still exist in his script. 2 he observes that the tx never gets confirmed since other nodes use a new version client, and fall into self-doubt. both situations do not help the author.
  2. We scan and decode every instruction to check the potential error, its OK in RPC since every user sends tx from the user's own node, it naturally decentralized; but for relayed txs, I doubt there may be a potential weak point to DDOS, and may drawback the relay performance.

Personally, I'm good either direction. Thoughts? @doitian @janx

tx-pool/src/config.rs Outdated Show resolved Hide resolved
script/src/known_bugs_checker.rs Outdated Show resolved Hide resolved
@doitian

This comment has been minimized.

Copy link
Member

doitian commented Jan 18, 2020

Maybe we need separate switchers for RPC and Relay

@xxuejie xxuejie requested a review from doitian Jan 19, 2020
@doitian doitian self-requested a review Jan 19, 2020
@doitian doitian dismissed their stale review Jan 19, 2020

Comments all resolved

@xxuejie xxuejie force-pushed the xxuejie:reject-scripts-with-known-bugs branch from 87e74d2 to 60e3fb8 Jan 19, 2020
@doitian doitian added this to 👀 Awaiting review in CKB - Pull Requests Jan 20, 2020
xxuejie added 3 commits Jan 17, 2020
For compatibility reasons, there're certain bugs that we have to leave
to the next hardfork to fix. However those bugs, especially VM bugs
might lead to surprising unexpected behaviors. This change adds a new
checker that checks against newly created cells for scripts with bugs,
and reject those transaction when we can. This way we can alert users
about the bugs as early as we can.
@xxuejie xxuejie force-pushed the xxuejie:reject-scripts-with-known-bugs branch from 60e3fb8 to 42c7a5b Jan 31, 2020
@doitian
doitian approved these changes Feb 1, 2020
CKB - Pull Requests automation moved this from 👀 Awaiting review to ✅ Reviewer approved Feb 1, 2020
@doitian

This comment has been minimized.

Copy link
Member

doitian commented Feb 1, 2020

bors r=doitian,zhangsoledad

bors bot added a commit that referenced this pull request Feb 1, 2020
Merge #1915
1915: feat: Reject new scripts with known bugs r=doitian,zhangsoledad a=xxuejie

For compatibility reasons, there're certain bugs that we have to leave
to the next hardfork to fix. However those bugs, especially VM bugs
might lead to surprising unexpected behaviors. This change adds a new
checker that checks against newly created cells for scripts with bugs,
and reject those transaction when we can. This way we can alert users
about the bugs as early as we can.

Co-authored-by: Xuejie Xiao <xxuejie@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Feb 1, 2020

Build succeeded

  • continuous-integration/travis-ci/push
@bors bors bot merged commit 42c7a5b into nervosnetwork:develop Feb 1, 2020
5 checks passed
5 checks passed
Dummy CI CI that does nothing
Details
Travis CI - Pull Request Build Passed
Details
bors Build succeeded
Details
nervosnetwork.ckb Build #20200131.18 succeeded
Details
nervosnetwork.ckb (UnitTest) UnitTest succeeded
Details
CKB - Pull Requests automation moved this from ✅ Reviewer approved to Done Feb 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.