sdk: Limited Borsh 0.9 support (Pubkey and helpers)

[joncinque]

Problem

The upgrade to Solana 1.16 crates has been very painful for a number of teams due to the incompatibility between Borsh 0.9 and 0.10, which should have constituted a major version upgrade for the Solana SDK crates. A lot of these are due to the borsh 0.9 traits not being implemented on the SDK types, not counting the issues like #31960

Summary of Changes

To alleviate one class of issues, also implement the borsh 0.9 traits on Pubkey.

Note that it still won't be possible to build the metaplex crates because they use the solana_program::borsh::get_packed_len helper, which targets borsh 0.10 in the solana crates. The solution here is to version the solana helpers and deprecate the general solana_program::borsh file. This way, it's impossible to break people when we add support for a new version of borsh.

This will also be backported to 1.16.

These manual implementations were generated by running cargo expand on borsh 0.9, copy-pasting, and then adding the proper annotations everywhere to use borsh 0.9 versions.

Fixes #

[mvines]

Note that it still won't be possible to build the metaplex crates because they use the solana_program::borsh::get_packed_len helper, which targets borsh 0.10 in the solana crates.

To build metaplex, would a borsh 0.9 version of get_packed_len need to be reintroduced?

[t-nelson]

can we not derive() the fully qualified macro name instead of copypastaing the expanded version?

that is, something like...

#[derive(BorshSerialized, borsh0_9::BorshSerialize)]
struct Pubkey...

[joncinque]

To build metaplex, would a borsh 0.9 version of get_packed_len need to be reintroduced?

Yes, but only if it's at solana_program::borsh::get_packed_len, which would revert the behavior. Unless there's a way to define a function twice with different trait params. So practically, I don't think so

can we not derive() the fully qualified macro name instead of copypastaing the expanded version?

No, the macro uses borsh:: under the hood, which gets interpreted to the newest version

[t-nelson]

No, the macro uses borsh:: under the hood, which gets interpreted to the newest version

ugh. didn't realize that $crate isn't available in proc-macros 🤢

carry on

[codecov]

Codecov Report

Merging #32511 (346d75d) into master (3dcb382) will decrease coverage by 0.1%.
The diff coverage is 74.6%.

@@            Coverage Diff            @@
##           master   #32511     +/-   ##
=========================================
- Coverage    82.0%    82.0%   -0.1%     
=========================================
  Files         785      785             
  Lines      211100   211073     -27     
=========================================
- Hits       173170   173106     -64     
- Misses      37930    37967     +37     

[mvines]

To build metaplex, would a borsh 0.9 version of get_packed_len need to be reintroduced?

Yes, but only if it's at solana_program::borsh::get_packed_len, which would revert the behavior. Unless there's a way to define a function twice with different trait params. So practically, I don't think so

Restore solana_program::borsh to be borsh 0.9, and add a solana_program::borsh0_10 module for borsh 0.10?

[joncinque]

Restore solana_program::borsh to be borsh 0.9, and add a solana_program::borsh0_10 module for borsh 0.10?

We could, but then we break people who have upgraded 😅

[mvines]

Restore solana_program::borsh to be borsh 0.9, and add a solana_program::borsh0_10 module for borsh 0.10?

We could, but then we break people who have upgraded 😅

Yeah, ⚖️. That's ideally what we would have done from the start probably, but yeah probably too late for that now. Are you thinking of adding a solana_program::borsh0_9::get_packed_len then?

[joncinque]

Are you thinking of adding a solana_program::borsh0_9::get_packed_len then?

I wasn't planning on it, since it wouldn't solve the initial metaplex build issue. But thinking on it more, it could be used if an upgraded program wants to do use the helpers on older types. I'll add it in

[joncinque]

Sorry for the holdup here, I added the borsh0_9.rs helpers. The file is exactly copy-pasted from borsh.rs, but with different imports. I omitted the tests because they will require manually implementing all of the Borsh traits on the test types, and that didn't seem worth the time. Let me know if you disagree!

[ilya-bobyr]

No, the macro uses borsh:: under the hood, which gets interpreted to the newest version

ugh. didn't realize that $crate isn't available in proc-macros nauseated_face

There are solutions like this: https://docs.rs/proc-macro-crate/1.3.1/proc_macro_crate/
Not ideal.

---

My bad. borsh actually uses this crate.
It helps with renames, but it will still choose just one version of the crate.
The one that is the last in Cargo.toml.

[ilya-bobyr]

can we not derive() the fully qualified macro name instead of copypastaing the expanded version?

No, the macro uses borsh:: under the hood, which gets interpreted to the newest version

There is a way to use borsh implementation in both cases, but it requires a proxy crate.
Here is an example: https://github.com/ilya-bobyr/borsh-multi-version/

To build metaplex, would a borsh 0.9 version of get_packed_len need to be reintroduced?

Yes, but only if it's at solana_program::borsh::get_packed_len, which would revert the behavior. Unless there's a way to define a function twice with different trait params. So practically, I don't think so

I do not think this is possible.
Exactly because it is possible to define both 0.9 and 0.10 versions of the BorshSchema for a type.
At which point, the get_packed_len call becomes ambiguous.

[joncinque]

There is a way to use borsh implementation in both cases, but it requires a proxy crate.
Here is an example: https://github.com/ilya-bobyr/borsh-multi-version/

Very cool, thanks for showing how to do it!

Actually, while I have you here @ilya-bobyr ... I was thinking of moving the borsh dependency entirely out of the sdk, and do it through some sort of new crate like solana-borsh, probably through some new proxy traits defined in that crate. but I think that might just shift the problem elsewhere and not actually help. What do you think of that approach?

And what do you think of this PR? I'd like to land it soon to help adoption of 1.16

[joncinque]

@ilya-bobyr as discussed offline, I've also added borsh0_10.rs and re-export everything from that file in borsh.rs