perf: skip molecule checking in release build

[quake]

we should view the the slice as infallible, trusted entity in release build, skip length and bounds check.

[quake]

benchmark, sync-test

[nervos-bot-user]

Benchmark Result

• TPS: 471
• CKB Version: 621579f
• Instance Type: c5.xlarge
• Instances Count: 3
• Bench Type: In2Out2
• CKB Logger Filter: info,ckb=debug

[nervos-bot-user]

Sync Test Result

• CKB Version: 621579f
• Instance Region: eu-west-2
• Sync Start: 2021-02-21 02:34:07
• Sync End: 2021-02-21 04:45:18
• Sync Height: 3845008
• Sync Speed: 488 blocks/s

[quake]

benchmark, sync-test

[nervos-bot-user]

Benchmark Result

• TPS: 484
• CKB Version: 621579f
• Instance Type: c5.xlarge
• Instances Count: 3
• Bench Type: In2Out2
• CKB Logger Filter: info,ckb=debug

[ybian19]

sync-test

[nervos-bot-user]

Sync Test Result

• CKB Version: 621579f
• Instance Region: ap-southeast-1
• Sync Start: 2021-02-22 01:32:55
• Sync End: 2021-02-22 03:21:06
• Sync Height: 3849505
• Sync Speed: 593 blocks/s

[doitian]

I'm against this PR. The function name suggests that it will panic on invalid data, it is surprising that it does not. It is very easy to be abused.

If we really want to skip the verification only in release mode, we should add a new function to do that.

[quake]

I'm against this PR. The function name suggests that it will panic on invalid data, it is surprising that it does not. It is very easy to be abused.

If we really want to skip the verification only in release mode, we should add a new function to do that.

how about change the name to from_verified_slice_should_be_ok and move it to store crate as a private trait?

[quake]

sync-test

[nervos-bot-user]

Sync Test Result

• CKB Version: 621579f
• Instance Region: eu-west-2
• Sync Start: 2021-02-26 07:32:03
• Sync End: 2021-02-26 09:47:53
• Sync Height: 3872051
• Sync Speed: 475 blocks/s

[doitian]

how about change the name to from_verified_slice_should_be_ok and move it to store crate as a private trait?

What's the reason that we cannot use new_unchecked in the places that we are sure the slice is valid but need to change the behavior of this function?

[yangby-cryptape]

First, just an additional explanation:
Before the mainnet launched and the first month after mainnet launched, there are several panics in our sentry were caused because the format of data from storage were malformed, some of those panics were sent by this function from_slice_should_be_ok(..).

Do no checks means we can make sure that:

• We would never make mistakes again when update data structures.
• The RocksDB will be always work well.

The worst situation I imagined is: no panics caused at beginning, but only malformed data, then we found the bug several days later.

Speaking of this, I remembered that in Matt Damon's film "The Martian", their rocket exploded because the NASA skip few checks which only have very very very small probability to find bugs.

Second, there no strong evidences could approve that those checks cost too many resource, so that we have to remove them.

p.s. I'm not opposed to this optimization, but also I don't want to approve it. And, the changes are right, no issue for codes.

[nervos-bot-user]

Can one of the admins verify this patch?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[doitian]

Close since it does not gains enough approvals.