fix: serializing transactions; sort that takes less time and memory

[steveluscher]

Problem

When compiling messages, we sort accounts. The sort involves producing and comparing pubkey strings, which takes time and memory.

Summary of Changes

• Short circuit pub key comparisons at all when you can determine the sort order based on isSigner and isWriteable alone.
• Compare pubkeys without producing strings, copying them, or mutating them. The cmp method scans the 8-bit words in big-endian order and bails as soon as it finds that one is larger than the other.

Test plan

Run the tests in transaction.test.ts. Sort orders still the same.

[steveluscher]

One string created here.

[steveluscher]

Another string created here.

[steveluscher]

The CPU and memory that we consumed to produce base58 strings gets thrown out here if the signedness or writability of the accounts differ.

[codecov]

Codecov Report

Merging #25642 (21bf723) into master (1727ca4) will increase coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   solana-labs/solana#25642   +/-   ##
=======================================
  Coverage    75.4%    75.4%           
=======================================
  Files          40       40           
  Lines        2344     2345    +1     
  Branches      337      338    +1     
=======================================
+ Hits         1768     1769    +1     
  Misses        459      459           
  Partials      117      117           

[jordaaash]

Kind of an aside, but lots of (most?) pubkeys get created from base58 strings. What if we kept the string representation and just return it when asked?

[steveluscher]

In general I think I love that idea. It seems that pubkeys are used for display and comparison frequently, but signing transactions infrequently. We should probably keep them in base58 in userspace. We can validate strings that purport to be base58 then assign them an opaque TypeScript type (see principle 4 where I used Base58EncodedPublicKey as an example).

[steveluscher]

(But I get what you're talking about here, specifically, is memoization of toBase58())

[jordaaash]

Yeah, exactly. I think somewhere downstream of the constructor we could save the base58 representation if provided, and if not, memoize it the first time it's needed. This could speed up a ton of apps (like Metaplex) that perform lots of string comparisons this way.

[steveluscher]

This could speed up a ton of apps (like Metaplex) that perform lots of string comparisons this way.

Yeah, let's do it. Trading memory for CPU is for sure the right thing to do for now.

I wish there was an easy way to observe the ecosystem effect of changes like that. Make it faster. Did it matter? Who could know, without data.

[jordaaash]

It appears that commits 58092f7, 1727ca4, and 20f169c changed the serialization of transactions in a breaking fashion.

v1.43.4 produces a different message for some transactions than v1.43.2, causing signature verification to fail. I think they may need to be reverted while we figure out what changed.

[jordaaash]

Base64 encoded serialized transaction on v1.43.2:

A2j0FLC8qVrDlLgZrhp5RpahkaP3EYcFVrwaA22aNdHptDNAqBzYupUVfFMkKPNoV48BPXkvMW94VLa7nBifKgeC/78RLysnZkuFRI2pjVl7kleg9rJ2VEYEIKS12SKJ9tpVzUeBWJSxfHwKG+qBkvyxHW0ilbJPnjgfBd15lDAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMBBQu73jG3Vn9v4LuQOQXCjl+CUxeYUHkqhFyf2edgP+Sbr1hf1LaKF5gZb5xOxTtFUpIQtlHt7GWxAlpRiF7ptLGH9NzO4Vt5d3dP6AmMurOWFpbmhAsWMPZ5uwxU3y5H4hLWbWY51M5VMUeZpJSAYwNEoG+dmILgY0eTx6DiKnfEWuTFdXcbs2X+A4+ctU9d1CQeXE/Q/nzhe6Mnp3dNkwC85MUmTQggIfMFHojZ4TBa6az0aGYHiVzexasV/3D0JVoAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIyXJY9OJInxuz0QKRSODYMLWhOZ2v8QhASOe9jb6fhZC3BlsePRfEU4nVJ/awTDzVi4bHMaoP21SbbRvAP4KUYGp9UXGSxcUSGMyUw9SvF/WNruCJuh/UTj29mKAAAAAAbd9uHXZaGT2cvhRs7reawctIXtX1s3kTqM9YV+/wCpdTVAg0ViCABjFuBZeitjzp0pm87cwpAkjoFVxxGDF0UGBgIAATQAAAAAYE0WAAAAAABSAAAAAAAAAAbd9uHXZaGT2cvhRs7reawctIXtX1s3kTqM9YV+/wCpCgIBCUMAAPTczuFbeXd3T+gJjLqzlhaW5oQLFjD2ebsMVN8uR+ISAfTczuFbeXd3T+gJjLqzlhaW5oQLFjD2ebsMVN8uR+ISBwcAAwIBBgoJAAoDAQMCCQcBAAAAAAAAAAgHBQECAAIGCdABAA0AAABTb2xhbmEgQXVzdGluBAAAAFNBSEiIAAAAaHR0cHM6Ly9tdG5waG90b2Jvb3RoN2FmYzIyZTA2OTJmNDIwNWI4NDhiOWMwMWUwMjIwMGUyMTE3NTctZGV2LnMzLmFtYXpvbmF3cy5jb20vcHVibGljL0RkY3RwNDVaSzRmNXdrYmpvNnpIZG9laTJERnBHaFJ4V3BrRDQ2bmJwVjEuanNvbvQBAQEAAAD03M7hW3l3d0/oCYy6s5YWluaECxYw9nm7DFTfLkfiEgFkAQgJBAECAgAFCgYJCgoBAAAAAAAAAAA=

Base64 encoded serialized transaction on v1.43.4:

AwRhvLRKor2cu/x4IgOcfjXGcsUN+uSBDTFZEcQzWYvXm5wK+iBMb/F1DbIKJsnF9jG0BXacQFL21ndUDneF0A2rTQVc339u+3JupP3kVBSAeWmJN0NmLAQw+uuaANp4UqTM2URWmTw0mjPU6618oBDTxFl9bZbzm7NilS42WqoOAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMBBQu73jG3Vn9v4LuQOQXCjl+CUxeYUHkqhFyf2edgP+SbrxZuDS+23r+0o5FwsYmPc+Jo11SvFhajk0E5W9Dm00e89NzO4Vt5d3dP6AmMurOWFpbmhAsWMPZ5uwxU3y5H4hJxj3BprGhOInz9PWfW7sKBfj5w1JUuF/RCAMslM2uz8aW8RvTIV1JlrfJkEjRzzbsiT3TAW43gRSNsXVkA5rFO95IvoyQqRs8Ceachn5V8j1oYbVmnMMkRD/zUkoN/odsAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAan1RcZLFxRIYzJTD1K8X9Y2u4Im6H9ROPb2YoAAAAABt324ddloZPZy+FGzut5rBy0he1fWzeROoz1hX7/AKkLcGWx49F8RTidUn9rBMPNWLhscxqg/bVJttG8A/gpRoyXJY9OJInxuz0QKRSODYMLWhOZ2v8QhASOe9jb6fhZFohwF5jwEmFYC5pbObDBDAno+rOTbFJPYv9CsWxbyXkGBgIAATQAAAAAYE0WAAAAAABSAAAAAAAAAAbd9uHXZaGT2cvhRs7reawctIXtX1s3kTqM9YV+/wCpCAIBB0MAAPTczuFbeXd3T+gJjLqzlhaW5oQLFjD2ebsMVN8uR+ISAfTczuFbeXd3T+gJjLqzlhaW5oQLFjD2ebsMVN8uR+ISCgcAAwIBBggHAAgDAQMCCQcBAAAAAAAAAAkHBQECAAIGB9EBAA0AAABTb2xhbmEgQXVzdGluBAAAAFNBSEiJAAAAaHR0cHM6Ly9tdG5waG90b2Jvb3RoN2FmYzIyZTA2OTJmNDIwNWI4NDhiOWMwMWUwMjIwMGUyMTE3NTctZGV2LnMzLmFtYXpvbmF3cy5jb20vcHVibGljLzZGcjJXNkZvTjExb1lGOEVhdUJCS2JRSHZxMlozZjFOQzdrdk1WTUFkMVFqLmpzb270AQEBAAAA9NzO4Vt5d3dP6AmMurOWFpbmhAsWMPZ5uwxU3y5H4hIBZAEJCQQBAgIABQgGBwoKAQAAAAAAAAAA

[steveluscher]

Yep. This sort algorithm changed the sort order of accounts.

it('sorts the way it used to', () => {
  const keysStringSorted = [];
  const keysCmpSorted = [];
  for (let ii = 0; ii < 100; ii++) {
    const key = Keypair.generate().publicKey;
    keysStringSorted.push(key);
    keysCmpSorted.push(key);
  }
  keysStringSorted.sort((a, b) => {
    return a.toBase58().localeCompare(b.toBase58());
  });
  keysCmpSorted.sort((a, b) => {
    return a._bn.cmp(b._bn);
  });
  // Sometimes fails
  expect(keysStringSorted.map(a => a.toBase58())).to.eql(
    keysCmpSorted.map(a => a.toBase58()),
  );
});