SPL errors from hashes

[buffalojoec]

This PR adds unique, custom error codes to every error in SPL libraries - including the two interfaces transfer-hook and token-metadata.

The ability to derive unique u32 error codes from hash inputs is powered by these changes to the spl-program-error crate, which simply hashes the following combination of inputs and takes bytes 13 through 16 as the little-endian u32:

"spl_program_error:" + <enum name> + ":" + <variant name>

This creates unique error codes for each error, as requested in this comment

Closes #5042
Solves discussions in #5168

[joncinque]

The implementation looks perfect! I did just realize though... we'll need a way for someone at the top-level to get more information about the error, since they're essentially random numbers 😓

Ok, hear me out, what if instead of giving each error a random number, we give the first error a random hashed number of namespace:error_name, and everything counts up from there? That way someone just needs to find out that first number, which we force people to add to the source code somewhere to make it easier to discover, similar to solana-frozen-abi, ie:

#[spl_program_error(hash_error_codes = true, error_start = 42)]
pub enum MyError {
    OhNoes,
}

Where 42 is hash('spl_program_error:MyError')[13..17]. If it's incorrect, the macro tells you what number to set as error_start.

We could maybe condense those two into one arg, but I can't come up with any good names. Maybe just have expected_error_start? Or expose a new macro, like spl_program_error_with_unique_error_codes which does the hashing and also takes in the expected start number? These names all stink, so I'll noodle on something better.

[buffalojoec]

Ok, hear me out, what if instead of giving each error a random number, we give the first error a random hashed number of namespace:error_name, and everything counts up from there?

This was a concern of mine on the approach as well. I felt like a unique error per variant was overkill, and posed a higher collision risk.
Also, derive(FromPrimitive) handles this very neatly, as you might have noticed in #5168. You just set the first one and the rest increment. So, I'm game.

On the topic of collisions, I felt like it was probably also overkill, but I have one particular concern about a hash randomly resulting in, say [1, 0, 0, 0] and colliding with (literally any) program errors. For this reason I suggest we lean toward a fixed 9 on the MSB and take only bytes 13, 14, and 15 from the hash. wdyt?

Where 42 is hash('spl_program_error:MyError')[13..17]. If it's incorrect, the macro tells you what number to set as error_start.

Ha, this is cool and would totally work! It smells a bit like a macro anti-pattern to me, but I guess it really doesn't do any harm, and it will help with readability for sure! I'm in for this change as well.

[joncinque]

On the topic of collisions, I felt like it was probably also overkill, but I have one particular concern about a hash randomly resulting in, say [1, 0, 0, 0] and colliding with (literally any) program errors. For this reason I suggest we lean toward a fixed 9 on the MSB and take only bytes 13, 14, and 15 from the hash. wdyt?

Ah right, that's a good point. Rather than remove a whole byte from the randomness, how about we just make sure that the number is >= 256 to be totally safe? and if it happens to be 0 <= x < 256, just shift over 1 byte until you get to a valid set of bytes.

It smells a bit like a macro anti-pattern to me

If you have a better way of surfacing that number easily for people looking at the source, I'm game! But I couldn't come up with anything else

[buffalojoec]

On the topic of collisions, I felt like it was probably also overkill, but I have one particular concern about a hash randomly resulting in, say [1, 0, 0, 0] and colliding with (literally any) program errors. For this reason I suggest we lean toward a fixed 9 on the MSB and take only bytes 13, 14, and 15 from the hash. wdyt?

smh, I meant 256 not 9

Ah right, that's a good point. Rather than remove a whole byte from the randomness, how about we just make sure that the number is >= 256 to be totally safe? and if it happens to be 0 <= x < 256, just shift over 1 byte until you get to a valid set of bytes.

Which number are you referring to? The entire u32 integer itself? In my opinion we want to clear at least 5_000 for Anchor errors, and likely even go a little further just to be safe for people who like to categorize with leading digits. Say, 10_000 or 100_000?

It smells a bit like a macro anti-pattern to me

If you have a better way of surfacing that number easily for people looking at the source, I'm game! But I couldn't come up with anything else

Nah I think it's okay. Unless I find some Rustaceans barking about it online, it's going in

[buffalojoec]

For us VS Code userz 🎉

[joncinque]

Which number are you referring to? The entire u32 integer itself? In my opinion we want to clear at least 5_000 for Anchor errors, and likely even go a little further just to be safe for people who like to categorize with leading digits. Say, 10_000 or 100_000?

Sorry, yeah I mean the whole 4 bytes interpreted as a little-endian u32. Does Anchor start at 5000 for all programs? That seems very odd. If that's the case, we can just do 6000 or something, right? And that's already being plenty generous I'd say. If people use their own start, that's similar to just grinding a bunch of error names until you get something you like.

[buffalojoec]

Which number are you referring to? The entire u32 integer itself? In my opinion we want to clear at least 5_000 for Anchor errors, and likely even go a little further just to be safe for people who like to categorize with leading digits. Say, 10_000 or 100_000?

Sorry, yeah I mean the whole 4 bytes interpreted as a little-endian u32. Does Anchor start at 5000 for all programs? That seems very odd. If that's the case, we can just do 6000 or something, right? And that's already being plenty generous I'd say. If people use their own start, that's similar to just grinding a bunch of error names until you get something you like.

They use the first digit as a category https://docs.rs/anchor-lang/latest/anchor_lang/error/enum.ErrorCode.html
and then users start at 6000 https://docs.rs/anchor-lang/latest/anchor_lang/error/constant.ERROR_CODE_OFFSET.html

I have 10,000 in my latest commit, I guess we could get away with 7,000 ?

Besides our offset, this is ready for another look.

[joncinque]

Just few tiny last little questions, this is extremely close!

[buffalojoec]

Comments resolved!

This is a breaking change for anything using these libraries downstream. I'm thinking:

• spl-program-error: 1.0.0 or 0.3.0 if we want to keep it in development
• Any downstream crates: Bump minor version

wdyt?

[joncinque]

Haha I just realized that this has an extra 0, should be 7_000. Good thing we have multiple rounds of reviews

[buffalojoec]

Ah! my bad

[joncinque]

This is a breaking change for anything using these libraries downstream

Sorry, maybe I'm missing something, but how is this change breaking? It passes CI without any changes to other libraries that are using it currently

[buffalojoec]

This is a breaking change for anything using these libraries downstream

Sorry, maybe I'm missing something, but how is this change breaking? It passes CI without any changes to other libraries that are using it currently

Is it not technically breaking since the underlying error codes are changing?

CI is passing because I included changes to the tests where the expected error changed.

For example, anyone expecting TransferHookError::IncorrectAccount is now getting AccountResolutionError::IncorrectAccount, and furthermore anyone expecting some u32 like 1 is now getting 13498579 or something wild.

[joncinque]

Is it not technically breaking since the underlying error codes are changing?

But that's only if you use hash_error_code_start, right?

[buffalojoec]

Is it not technically breaking since the underlying error codes are changing?

But that's only if you use hash_error_code_start, right?

I'm thinking about if I'm working with spl_token_2022 and I'm expecting certain codes from, say transfer hook helpers. Also spl_token_client for that matter

[joncinque]

I'm thinking about if I'm working with spl_token_2022 and I'm expecting certain codes from, say transfer hook helpers. Also spl_token_client for that matter

Ah yes, that's a breaking change in those libraries for sure. I'm planning on cutting a new minor release for all of these anyway. I'll do it once this lands

[joncinque]

Looks good!

[buffalojoec]

Lmao Lennon would approve