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(rpc): Improve error messages from send transaction RPC #2049

Merged
merged 11 commits into from
Jul 16, 2020
Merged

feat(rpc): Improve error messages from send transaction RPC #2049

merged 11 commits into from
Jul 16, 2020

Conversation

doitian
Copy link
Member

@doitian doitian commented Apr 30, 2020

This PR adds extra information in transaction verification errors, such as:

  • Which input, cell_dep, header_dep or output causes the error
  • If there's a limit, what's the value of the limit.

There're also ambiguous error messages. For example, the pool has its own cycles limit on a single transaction, which is configurable in the config file. The block also as a limit on all the transactions in the block. Apparently, for a single transaction, the cycles should not exceed the block limit. This PR also avoids using the same error for different reasons.

@doitian doitian requested review from a team and quake April 30, 2020 02:17
@doitian doitian marked this pull request as draft April 30, 2020 02:17
@doitian doitian added this to 🚧 In progress in CKB - Pull Requests May 7, 2020
@doitian doitian linked an issue May 12, 2020 that may be closed by this pull request
@doitian doitian changed the title feat(rpc): Improve messages for some RPC errors feat(rpc): Improve error messages from send transaction RPC May 14, 2020
@k
Copy link

k commented May 25, 2020

Looking forward to this. It was hard to decipher the error message:

2020-05-25 11:58:49.902 -07:00 GlobalRuntime-0 INFO ckb-script  Error validating script group Byte32(0x2b0f12e62a438b5cccc51a0680bb7c142436e1bdf4c4b9932e47dbed9b366419) of transaction Byte32(0x19a88b96de6bc874a2748e7192c99aabff0a3ac4b9423d6b70edc92a7c4f57bd): Script(ValidationFailure(-31))

I was looking for the error code -31 in this repository but didn't realize it was from a lock script. Would be good to know what stage the validation is on, whether its a lock script, type script, or other validation failure. Especially since technically type scripts could also use the error code -31. I suppose one could look up the script with the Byte32(0x2b0f12e62a438b5cccc51a0680bb7c142436e1bdf4c4b9932e47dbed9b366419) but the lock vs type script is a helpful heuristic.

@doitian
Copy link
Member Author

doitian commented Jun 5, 2020

Looking forward to this. It was hard to decipher the error message:

2020-05-25 11:58:49.902 -07:00 GlobalRuntime-0 INFO ckb-script  Error validating script group Byte32(0x2b0f12e62a438b5cccc51a0680bb7c142436e1bdf4c4b9932e47dbed9b366419) of transaction Byte32(0x19a88b96de6bc874a2748e7192c99aabff0a3ac4b9423d6b70edc92a7c4f57bd): Script(ValidationFailure(-31))

I was looking for the error code -31 in this repository but didn't realize it was from a lock script. Would be good to know what stage the validation is on, whether its a lock script, type script, or other validation failure. Especially since technically type scripts could also use the error code -31. I suppose one could look up the script with the Byte32(0x2b0f12e62a438b5cccc51a0680bb7c142436e1bdf4c4b9932e47dbed9b366419) but the lock vs type script is a helpful heuristic.

Roger that

@doitian doitian marked this pull request as ready for review July 8, 2020 03:14
@doitian
Copy link
Member Author

doitian commented Jul 13, 2020

Looking forward to this. It was hard to decipher the error message:

2020-05-25 11:58:49.902 -07:00 GlobalRuntime-0 INFO ckb-script  Error validating script group Byte32(0x2b0f12e62a438b5cccc51a0680bb7c142436e1bdf4c4b9932e47dbed9b366419) of transaction Byte32(0x19a88b96de6bc874a2748e7192c99aabff0a3ac4b9423d6b70edc92a7c4f57bd): Script(ValidationFailure(-31))

I was looking for the error code -31 in this repository but didn't realize it was from a lock script. Would be good to know what stage the validation is on, whether its a lock script, type script, or other validation failure. Especially since technically type scripts could also use the error code -31. I suppose one could look up the script with the Byte32(0x2b0f12e62a438b5cccc51a0680bb7c142436e1bdf4c4b9932e47dbed9b366419) but the lock vs type script is a helpful heuristic.

Fixed in #2164

@doitian
Copy link
Member Author

doitian commented Jul 13, 2020

Rebased since it does not depend on #2038

@doitian
Copy link
Member Author

doitian commented Jul 13, 2020

Fixed up 883dcf9 because of the clippy error

error: redundant field names in struct initialization
   --> verification/src/transaction_verifier.rs:398:21
    |
398 |                     index: index,
    |                     ^^^^^^^^^^^^ help: replace it with: `index`
    |
    = note: `-D clippy::redundant-field-names` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names

CKB - Pull Requests automation moved this from 🚧 In progress to ✅ Reviewer approved Jul 16, 2020
@doitian
Copy link
Member Author

doitian commented Jul 16, 2020

bors r=quake,zhangsoledad,yangby-cryptape

bors bot added a commit that referenced this pull request Jul 16, 2020
2049: feat(rpc): Improve error messages from send transaction RPC r=quake,zhangsoledad,yangby-cryptape a=doitian

This PR adds extra information in transaction verification errors, such as:

* Which `input`, `cell_dep`, `header_dep` or `output` causes the error
* If there's a limit, what's the value of the limit.

There're also ambiguous error messages. For example, the pool has its own cycles limit on a single transaction, which is configurable in the config file. The block also as a limit on all the transactions in the block. Apparently, for a single transaction, the cycles should not exceed the block limit. This PR also avoids using the same error for different reasons.

Co-authored-by: ian <ian@nervos.org>
@zhangsoledad
Copy link
Member

bors r=quake,zhangsoledad,yangby-cryptape

@bors
Copy link
Contributor

bors bot commented Jul 16, 2020

Build succeeded:

  • continuous-integration/travis-ci/push

@bors bors bot merged commit b8c29de into nervosnetwork:develop Jul 16, 2020
CKB - Pull Requests automation moved this from ✅ Reviewer approved to Done Jul 16, 2020
@doitian doitian deleted the better-rpc-error-2 branch September 11, 2020 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

can ckb show more error messages
5 participants