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

Don't ban remote peer when ckb_vm received Ctrl-C signal when processing chunk_tx #4702

Conversation

eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Oct 29, 2024

What problem does this PR solve?

Issue Number: close #4701

Related changes

  • let ckb_vm::Error::External("stopped") return ErrorKind::Internal instead of ErrorKind::Script

return ErrorKind::Internal then ckb will not think the tx is malformled:

fn is_malformed_from_verification(error: &Error) -> bool {
match error.kind() {
ErrorKind::Transaction => error
.downcast_ref::<TransactionError>()
.expect("error kind checked")
.is_malformed_tx(),
ErrorKind::Script => true,
ErrorKind::Internal => {
error
.downcast_ref::<InternalError>()
.expect("error kind checked")
.kind()
== InternalErrorKind::CapacityOverflow
}
_ => false,
}
}

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • None

Release note

Title Only: Include only the PR title in the release note.

@eval-exec eval-exec force-pushed the exec/ignore-ctrl-c-signal-cause-chunk-stop branch 2 times, most recently from 753b86c to 76b0438 Compare October 29, 2024 08:06
@chenyukang
Copy link
Collaborator

seems not a proper PR title.

@eval-exec eval-exec changed the title let ckb_vm::Error::External("stopped") return ErrorKind::Internal instead of ErrorKind::Script let ckb_vm::Error::External("stopped") return ErrorKind::Internal instead of ErrorKind::Script Oct 29, 2024
@eval-exec eval-exec marked this pull request as ready for review October 29, 2024 08:07
@eval-exec eval-exec requested a review from a team as a code owner October 29, 2024 08:07
@eval-exec eval-exec requested review from quake and removed request for a team October 29, 2024 08:07
@eval-exec eval-exec changed the title let ckb_vm::Error::External("stopped") return ErrorKind::Internal instead of ErrorKind::Script Don't ban remote peer when ckb_vm received Ctrl-C signal when processing chunk_tx Oct 29, 2024
@eval-exec
Copy link
Collaborator Author

seems not a proper PR title.

Got it, I'll update the title. Any suggestions?

@eval-exec
Copy link
Collaborator Author

Need to create a new Error to handle this case:InternalErrorKind::Interrupts

@eval-exec eval-exec closed this Oct 30, 2024
@eval-exec eval-exec reopened this Oct 30, 2024
@eval-exec eval-exec marked this pull request as draft October 30, 2024 02:42
@eval-exec eval-exec marked this pull request as ready for review November 25, 2024 02:52
@eval-exec eval-exec force-pushed the exec/ignore-ctrl-c-signal-cause-chunk-stop branch from 3779db3 to cf10772 Compare November 25, 2024 02:53
@eval-exec eval-exec marked this pull request as draft November 25, 2024 02:59
@eval-exec eval-exec force-pushed the exec/ignore-ctrl-c-signal-cause-chunk-stop branch from cf10772 to a724ef7 Compare November 25, 2024 03:04
@@ -1180,6 +1180,7 @@ where
let mut scheduler = Scheduler::new(tx_data, version, self.syscalls_generator.clone());
let map_vm_internal_error = |error: VMInternalError| match error {
VMInternalError::CyclesExceeded => ScriptError::ExceededMaximumCycles(max_cycles),
VMInternalError::External(reason) if reason.eq("stopped") => ScriptError::Interrupts,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's other reason may make VM scheduler return VMInternalError::External?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

script/src/syscalls/debugger.rs
48:            .map_err(|e| VMError::External(format!("String from buffer {e:?}")))?;
script/src/syscalls/mod.rs
126:            _ => Err(Error::External(format!("CellField parse_from_u64 {i}"))),
148:            _ => Err(Error::External(format!("HeaderField parse_from_u64 {i}"))),
164:            _ => Err(Error::External(format!("InputField parse_from_u64 {i}"))),
197:            _ => Err(Error::External(format!("SourceEntry parse_from_u64 {i}"))),
243:            _ => Err(Error::External(format!("Place parse_from_u64 {i}"))),

@eval-exec eval-exec marked this pull request as ready for review November 25, 2024 03:10
@chenyukang
Copy link
Collaborator

how can we add a unit test for this, it seems hard but worth having a try.

chenyukang
chenyukang previously approved these changes Nov 25, 2024
@eval-exec
Copy link
Collaborator Author

how can we add a unit test for this, it seems hard but worth having a try.

Sure, I will add unit test.

@eval-exec eval-exec marked this pull request as draft November 25, 2024 05:16
@eval-exec eval-exec force-pushed the exec/ignore-ctrl-c-signal-cause-chunk-stop branch 2 times, most recently from 3918966 to 86b3c08 Compare November 25, 2024 06:06
@eval-exec eval-exec marked this pull request as ready for review November 25, 2024 06:08
@eval-exec eval-exec force-pushed the exec/ignore-ctrl-c-signal-cause-chunk-stop branch from 86b3c08 to 64ef41a Compare November 25, 2024 08:17
@eval-exec eval-exec force-pushed the exec/ignore-ctrl-c-signal-cause-chunk-stop branch from 64ef41a to 5f70ba5 Compare November 26, 2024 07:43
@zhangsoledad zhangsoledad added this pull request to the merge queue Nov 29, 2024
Merged via the queue into nervosnetwork:develop with commit 188d0a6 Nov 29, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid Banning Peer on ckb_vm::Error::External Triggered by Ctrl-C Signal
3 participants