Use reserved account keys list to restrict tx write locks

[jstarry]

Problem

The transaction scheduler and the runtime both demote transaction write locks for builtin programs and sysvars using a static list of reserved IDs. This change replaces the currently used static lists of builtin program and sysvar IDs with a dynamic set of reserved account keys that can be updated at epoch boundaries with feature gates (added here: #84)

Summary of Changes

• Many Transaction and Message constructors and functions now require a reserved_account_keys argument to properly assess if a transaction account can be write locked.

The change is split into two commits so that the functional changes aren't drowned in test updates to plumb the new reserved account keys argument through.

Breaking Changes

• solana_sdk::transaction::SanitizedTransaction::try_from_legacy_transaction has a new reserved_account_keys argument
• solana_sdk::transaction::SanitizedTransaction::try_create has a new reserved_account_keys argument
• solana_sdk::transaction::SanitizedTransaction::try_new has a new reserved_account_keys argument
• solana_program::message::LegacyMessage::new has a new reserved_account_keys argument
• solana_program::message::SanitizedMessage::try_new has a new reserved_account_keys argument
• solana_program::message::LoadedMessage::new has a new reserved_account_keys argument
• solana_program::message::LoadedMessage::new_borrowed has a new reserved_account_keys argument

Feature Gate Issue: #540

[apfitzge]

Seems i'm coming in late in the process here; what's the reason we are taking this path of making these consensus breaking? It seems the Anza's client could simply choose to filter such transactions out early without adding complexity to the protocol.

[jstarry]

@apfitzge you're right that filtering could happen instead and that's what firedancer is planning to do

Here's my main reasoning:

1. Demoting write locks is an existing mechanism that we are kind of stuck with for now but aim to deprecate in the future. Since this mechanism is already presumed to be depended on by downstream clients and since there are already a handful of active builtin programs and upcoming sysvars that we want to prevent write locks for, this approach solves the issue in a relatively simple way without breaking any downstream clients.

2. The ideal way to handle this is to fail offending transactions but have them land on chain with an error explaining which reserved account was write locked unsuccessfully so that clients can see and fix the error. That change is arguably much more complex than extending the write lock demotion mechanism and will also require comms to migrate users.

3. Filtering in the scheduler is simple but similarly requires users to migrate. But the key point is that it doesn't allow us to communicate actionable errors back to users because the transactions are just dropped. We could duplicate the filtering logic in RPC transaction APIs but even that's not really enough because anyone not using those APIs would still be confusingly broken.

[CriesofCarrots]

The CI failure looks legit.
In the meantime, talk to me more about the breaking changes in solana_program wrt the backward compatibility policy. I have some thoughts, but want to hear what you're thinking.

[jstarry]

In the meantime, talk to me more about the breaking changes in solana_program wrt the backward compatibility policy. I have some thoughts, but want to hear what you're thinking.

Well, first of all I think it's unfortunate that the most convenient place for the sanitized types has always been solana-program which is subject to the backwards compatibility policy. I have viewed these as primarily internal types that were exposed in client sdk's incidentally. I think we could mirror the types in something like solana-runtime-transaction and then avoid needing to break the api in the client sdks.

On the other hand, since master has moved to 2.0, it's ok to introduce breaking changes here. I think the most unfortunate API change is the message is_writable API since it's the only change to a non-sanitized type and probably most likely to be used by end users. I could revert that breaking change since the only time we call that API in consensus sensitive path is inside sanitized::LegacyMessage which could handle the reserved keys at the call-site instead.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 90.26549% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (499d36e) to head (fd03ab9).

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #541     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         851      851             
  Lines      231209   231373    +164     
=========================================
+ Hits       189495   189611    +116     
- Misses      41714    41762     +48     

[CriesofCarrots]

Sorry about the delay; I was ooo for a few days. Thanks for your api/backward compatibility thoughts; echoes my thinking exactly.

I think the most unfortunate API change is the message is_writable API since it's the only change to a non-sanitized type and probably most likely to be used by end users. I could revert that breaking change since the only time we call that API in consensus sensitive path is inside sanitized::LegacyMessage which could handle the reserved keys at the call-site instead.

Yes, I agree. I think it would be best to preserve Message::is_writable for now. I haven't looked carefully at the approach you proposed to tell which is better, but an alternative would be to add a new method that takes reserved_account_keys and deprecate the existing method.

[CriesofCarrots]

I think this looks good, aside from the Message::is_writable breakage. However, the first commit is a bear; I would have more confidence in my review if it were broken up a bit. For instance, the bank.rs changes that add and update the Bank::reserved_account_keys field should probably be a separate commit. And if any of the api interface changes can be separated from each other (and built on top of each other), that would be a natural progression.

[CriesofCarrots]

Sidenote: not a thing to be dealt with in this PR, but it seems really clunky that this method takes 4 different bank-related things, including the Bank itself.

[jstarry]

Yes, hurts my eyes every time too.

[jstarry]

I think this looks good, aside from the Message::is_writable breakage.

Reverted the breakage here: a4771a9

However, the first commit is a bear; I would have more confidence in my review if it were broken up a bit. For instance, the bank.rs changes that add and update the Bank::reserved_account_keys field should probably be a separate commit.

Broken up here: #769

And if any of the api interface changes can be separated from each other (and built on top of each other), that would be a natural progression.

Open to ideas but that first commit (51e49c6) now basically only has two functional changes (applying the reserved account keys set in LoadedMessage and LegacyMessage) and the rest of the changes is plumbing the keys to every place we construct one of those messages. I understand that's a lot of code but I think functionally it's relatively straightforward now that the banking changes are pulled out.

[CriesofCarrots]

Open to ideas

That commit actually touches 7 different methods, which is a lot of plumbing to follow. If it were me, I would have started with a commit for each top-level method (ie. each of the SanitizedTransaction methods) to show the scope of those changes. And then a commit for each of the LegacyMessage/LoadedMessage/SanitizedMessage methods, since those are dependent on the SanitizedTransaction changes.
Looks like the is_writable change could be done at any time in that progression, but could easily be a separate commit.

To be explicit, I am talking about splitting into separate commits, not separate PRs. I understand that merging the method changes into the code-base piecemeal is a bad idea.

Well, anyway, I'm approving this as-is (or will re-approve, if you merge #769 first and then rebase this)

[CriesofCarrots]

Is this just to avoid importing ReservedAccountKeys? You use ReservedAccountKeys::empty_key_set() everywhere else.

[jstarry]

Yeah can't import it from solana-program