-
Notifications
You must be signed in to change notification settings - Fork 525
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
feature: mpl-utils
: Log assert failures for easier debugging
#1163
base: master
Are you sure you want to change the base?
feature: mpl-utils
: Log assert failures for easier debugging
#1163
Conversation
Thanks for this PR! It is indeed awful to debug any of these issues. The lack of logging of specific failures is unfortunately a necessity due to program size and compute constraints. However, ExtendProgram is scheduled to be enabled Monday so that will hopefully solve the size issue. |
Oh does calling |
It's the actual strings. I think we have more space now but we were a handful of bytes from the max a couple months ago. |
Ah I see. |
Hi, just a friendly ping on this. |
b73834d
to
f15e902
Compare
Huzzah, we have space now! Can you just change these to use |
Done! |
core/rust/utils/src/assertions.rs
Outdated
@@ -33,6 +42,12 @@ pub fn assert_owned_by( | |||
error: impl Into<ProgramError>, | |||
) -> ProgramResult { | |||
if account.owner != owner { | |||
msg!( | |||
"owner assertion failed for {key}: expected {expected}, got {actual}", |
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.
"invalid owner for {key} ({actual} != {expected})"
core/rust/utils/src/assertions.rs
Outdated
@@ -47,6 +62,14 @@ pub fn assert_derivation( | |||
) -> Result<u8, ProgramError> { | |||
let (key, bump) = Pubkey::find_program_address(path, program_id); | |||
if key != *account.key { | |||
msg!( | |||
"derivation assertion failed for {actual_key}:\n |
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.
"invalid derivation {actual_key} != {key}"
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.
Looks like \n
is not needed, I'll remove that.
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.
What do you think if we make the log messages a bit shorter so the log does not get cluttered?
Hmm I really think the log messages should provide as much information to debug as possible. |
I agree that log messages should be descriptive, but the observation here is because this is a library not a program. A program might handle an assert failure so it does not end up being a "fatal error". |
OK, I'll shorten the messages for |
At the moment, when an assertion fails, the program only throws an error about the type of assertion that has failed.
This there no indication to which account is the incorrect one, making it a nightmare to debug.
For example:
This PR adds logs that print the account that failed the assertion and some debug information about why the assertion failed.
They only trigger if the assertion fails, so shouldn't effect the compute unit budget during normal operation.