-
Notifications
You must be signed in to change notification settings - Fork 71
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: not fail on signature checks for dry-run txns #1081
Conversation
@@ -559,7 +559,7 @@ namespace eosio { namespace chain { | |||
|
|||
} | |||
|
|||
if( !allow_unused_keys || check_but_dont_fail) { | |||
if( !allow_unused_keys && !check_but_dont_fail) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the same change should be made at line 548 above, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.But EOS_ASSERT
will only throw the error when the first argument is false. Since check_but_dont_fail
should never throw the error when it is true, there is no need to modify at line 548.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, what was I thinking!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the PR description. check_but_dont_fail
is a dry-run
transaction not a read-only
transaction.
@@ -559,7 +559,7 @@ namespace eosio { namespace chain { | |||
|
|||
} | |||
|
|||
if( !allow_unused_keys || check_but_dont_fail) { | |||
if( !allow_unused_keys && !check_but_dont_fail) { | |||
EOS_ASSERT( checker.all_keys_used(), tx_irrelevant_sig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, I think the EOS_ASSERT
should be modified, since we would ideally want to include the time it takes for checker.all_keys_used()
if( !allow_unused_keys ) {
EOS_ASSERT( checker.all_keys_used() || check_but_dont_fail, tx_irrelevant_sig,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have resubmitted the code and updated the PR description
Congratulation @PinelliaC on your first Leap PR. Thanks for the PR! |
Changed the PR title. I was surprised initially that read-only trxs called check_authorization. Thanks. |
[3.2] fix: not fail on signature checks for dry-run txns
[3.2 -> 4.0] fix: not fail on signature checks for dry-run txns
[4.0 -> main] fix: not fail on signature checks for dry-run txns
In this method,
check_but_dont_fail
is equivalent to whether the transaction isdry-run
. However, whendry-run
istrue
, authorization should not be checked.