Add proposal for Read-Only accounts that allow credits #2738

aeyakovenko · 2019-02-12T16:28:58Z

Problem

Transactions lock and commit all accounts. If they specified a set of read-only accounts its possible for multiple concurrent transactions to credit those accounts as well as read their state. The runtime cannot check if an Account will be used as RO/RW based on program id because the account is not loaded when it is locked.

With this general purpose solution program ids will be treated as read-only accounts, appropriately cached in the runtime, and remove the need for a dedicated program ids vector in the Transaction structure.

This proposal doesn't change the underlying runtime rules, that non-owned accounts are RO. It is simply a hint to the runtime that accounts referenced in the transaction are RO and not RW.

Summary of Changes

Propose a simple design to specify the read-only keys in a transaction. Programs can be treated as read-only keys.

Fixes #

tag: @garious @mvines

aeyakovenko · 2019-02-12T18:56:40Z

@jackcmay

Don't we have a lot of this already for free? the loader should own the program executable bytes. and the accounts that are not owned by the program can only be "read-only". So we can do the same runtime optimization without an explicit flag.

jackcmay · 2019-02-12T19:04:11Z

@aeyakovenko
Yes, is this a new proposal to exand the concept of read-only to being specified in the transaction. So that even if an account is owned by a program the transaction says it can not be modified?

aeyakovenko · 2019-02-12T19:23:45Z

Yes, is this a new proposal to exand the concept of read-only to being specified in the transaction. So that even if an account is owned by a program the transaction says it can not be modified?

Yea, that is correct. Right now we don't do analysis to check for read-only accounts. I was thinking that if we end up needing full VM support, most of the memory necessary to execute a TX will be read only, like the page tables.

garious · 2019-02-13T01:02:04Z

This chapter describes the existing implementation. If you want to PR this chapter, it can't be merged until the implementation is updated.

aeyakovenko · 2019-02-13T21:03:39Z

@jackcmay so the pipeline can't do the RO/RW check based on program id because the account is not loaded yet.

aeyakovenko · 2019-02-13T23:01:47Z

@jackcmay

The accounts are on disk or some faraway storage. A transaction batch is being processed. Each account takes a lock. For the runtime to decide if right there if its a RO or RW lock, the account needs to be fetched from far away storage and checked for its program id.

jackcmay · 2019-02-13T23:14:43Z

Can't rely completely on the transaction to determine RO vs RW, whats the alternative?

aeyakovenko · 2019-02-13T23:22:40Z

Can't rely completely on the transaction to determine RO vs RW, whats the alternative?

that's what I am proposing. The TX will say which accounts are RO, and the runtime can take the right lock before the account is loaded

jackcmay · 2019-02-13T23:44:06Z

Doing away with the RO for non-owned accounts?

aeyakovenko · 2019-02-13T23:57:14Z

Doing away with the RO for non-owned accounts?

no, just adding metadata to the TX field to speed up processing

jackcmay · 2019-02-14T00:01:27Z

so if locks described in the TX don't jive after the accounts have been loaded bail?

aeyakovenko · 2019-02-14T00:04:17Z

so if locks described in the TX don't jive after the accounts have been loaded bail?

if the TX accounts are marked as RO, and the TX writes to them, then the transaction fails. But if an account is not marked RO and the TX reads them only, it still succeeds.

book/src/SUMMARY.md

jackcmay · 2019-02-14T18:04:22Z

@aeyakovenko These changes to account writability will need further changes to how we pass accounts to programs. We could split the RO and RW accounts into different parameters allowing the VM to abort on a write to RO. If we don't abort on write to RO at execution time we would not write back any RO accounts, but we probably also want a way to detect if there were any writes and fail the TX to avoid bad program states.

jackcmay · 2019-02-14T19:49:41Z

book/src/read-only-read-write-accounts.md

+The first account in the `account_keys` array is always the account that pays
+for the transaction fee, therefore it cannot be read-only.  Since accounts can
+only appear once in the transaction's `account_keys` array, an account can only
+be read-only or read-write in a single transaction, not both.


So transactions that may want to pass an account to one program as RO and another as RW are invalid?

@jackcmay yea, because the runtime treats the TX as one thing, and if any instruction needs RW a copy needs to be made regardless and the write lock needs to be held for the entire pipeline.

jackcmay · 2019-02-14T19:51:08Z

book/src/read-only-read-write-accounts.md

+only appear once in the transaction's `account_keys` array, an account can only
+be read-only or read-write in a single transaction, not both.
+
+* `program_ids: Vec<Pubkey>` - This vector is removed.  Program keys can be


Does this imply that there is a one-to-one ordered relationship between the instructions in the transaction and the number of program keys placed at the end of account_keys?

@jackcmay instructions reference the program id, so many instructions 1 program.

jackcmay · 2019-02-14T19:58:33Z

book/src/read-only-read-write-accounts.md

+that account should fail.  It follows that any other write lock requests will
+fail as well.
+
+## Alternative Design


Not following the alternative design. Says the purpose of the design is to no use a RO account list but then in 1. says there is a RO table. What is the difference between the list and table?

@jackcmay one is a field in a Transcation structure, the other is a data structure in the bank to keep track of what is read-only.

jackcmay · 2019-02-14T19:59:13Z

book/src/read-only-read-write-accounts.md

+
+1. Transaction accounts are locked.
+    a. If the account is present in the `read-only` table the TX does not fail.
+       The pending state for this TX is marked NeedReadLock.


So not failed but marked as pending and cached?

jackcmay · 2019-02-14T19:59:36Z

book/src/read-only-read-write-accounts.md

+    a. If the account is present in the `read-only` table the TX does not fail.
+       The pending state for this TX is marked NeedReadLock.
+2. Transaction accounts are loaded.
+    a. Transaction accounts that are read-only are increase their reference


jackcmay · 2019-02-14T20:00:46Z

book/src/read-only-read-write-accounts.md

+
+This design covers handling of read-only and read-write accounts in the
+[runtime](runtime.md).  Accounts already distinguish themselves as read-only or
+read-write based on the instruction program.  But to identify read-only accounts


What is the instruction program? You means "transaction's instructions"?

jackcmay · 2019-02-14T20:04:17Z

book/src/read-only-read-write-accounts.md

+accounts can be cached in memory and shared between all the threads executing
+transactions.
+
+The current runtime cannot predict if an account is read-only or read-write


Not following this, are you referring to the program's entry point? If so, by the time we call the program's entry point the accounts have been loaded and we know definitely if the account should be treated as RO or RW.

@jackcmay before the fetch from storage.

What entry point are you referring to? The current wording to me implies you are talking about program entry point, at that time we do know what locks we need since we've already pulled the account from storage.

jackcmay · 2019-02-14T20:06:08Z

book/src/read-only-read-write-accounts.md

+placed at the end of the `account_keys` vector with the `read_only_accounts`
+number set to the right number of programs.
+
+## Starvation


Does this imply that no RW operations can be performed while an account is RO locked for other transactions being processed? If an account is continuously referenced for RO operations (multiple concurrent transactions) then all transactions that require RW to that account will fail during that time? Does that also imply that a cached RW request would stall the entire pipeline?

@jackcmay This section addresses starvation. Is that not clear?

Would like to see more detail here, for example, if a RW operation on a currently RO account is requested the pipeline will stall until the RW operation is performed? Does this cache RO requests that occur while the RW operation is being processed?

@jackcmay
"While a pending write transaction exists, any additional read lock requests for
that account should fail. It follows that any other write lock requests will
fail as well."

So then yes stall (serialize)

@jackcmay not stall locally, the client's responsibility to retransmit.

book/src/read-only-read-write-accounts.md

jackcmay · 2019-02-15T00:26:42Z

Approved with the agreed upon changes

book/src/read-only-read-write-accounts.md

mvines · 2019-02-19T03:24:01Z

book/src/read-only-read-write-accounts.md

+
+* Read-only access to userdata.
+
+Instructions that debit or modify the read only account userdata will fail.


nit: read-only account

book/src/read-only-read-write-accounts.md

aeyakovenko · 2019-02-21T00:40:44Z

@mvines @garious any more changes?

garious · 2019-02-21T00:44:20Z

I haven't read it yet. Probably early next week. Did you want it in the book this release?

mvines

good enough for me,

aeyakovenko · 2019-02-21T01:27:53Z

@garious no rush

book/src/read-only-read-write-accounts.md

aeyakovenko · 2019-03-11T23:36:38Z

@garious, bump

garious

You can accelerate the review process by grammar-checking before the engineering review.

book/src/read-only-read-write-accounts.md

garious · 2019-03-12T03:00:16Z

book/src/read-only-read-write-accounts.md

+
+Read-only accounts have the following properties:
+
+* Can be deposited into.  Deposits can be implemented as a simple `atomic_add`.


That's why I've always refer to these accounts as "credit-only", not "read-only". Want to do the same in this doc?

I think how memory is treated is the more significant part. RC, read-credit? RWW, read-withdraw-write?

How about credit-only and credit-debit accounts? How memory is treaded is only the more significant part of this particular proposal. How currency is handled might be more significant to users. In any case, we should always aim to CIWII, and because the runtime allows lamports to increase, it's imprecise to call the account read-only.

Funny how even your title would become short and sweet if you used "credit-only". Instead of "Add proposal for Read-Only accounts that allow credits" it'd reduce to "Add proposal for credit-only accounts".

@garious

ROC, read-only-credit

RWW, read-write-withdraw

That's unnecessarily verbose. These are attributes of things called "accounts". You could say that the userdata of a credit-only account is read-only, and the userdata of a credit-debit account is read-write. Introducing acronyms here is not necessary or helpful.

@garious it helps to identify the accounts referenced in instructions as RO or RW, most of the time the token's are not important because the data is what is observed. Do you want to identify them as C, or W?

For example

ExchangeInstruction::Match

account[0] - R - Trade[0] address
account[1] - RW - Match address from Trade[0]
account[2] - R - Trade[1] address
account[3] - RW - Match address from Trade[1]

being explicit about what is RW or not is really helpful with designing the instruction

R and RW seems fine there. You're referring to the userdata.

@garious yea, so what is the convention we want to use? I'd like to have a standard one that is not to verbose. CO/CD?

Sure, CO/CD. Let's just be consistent in case we want to globally search and replace it later.

book/src/read-only-read-write-accounts.md

garious · 2019-03-12T22:01:29Z

book/src/SUMMARY.md

@@ -39,6 +39,8 @@
  - [Secure Vote Signing](vote-signing-to-implement.md)
  - [Staking Rewards](staking-rewards.md)
  - [Fork Selection](fork-selection.md)
+  - [Data Plane Fanout](data-plane-fanout.md)


rebase fail?

garious · 2019-03-12T23:55:36Z

book/src/credit-only-credit-debit-accounts.md


 * Can be deposited into.  Deposits can be implemented as a simple `atomic_add`.

-* Read-only access to userdata.
+* credit-only access to userdata.


Userdata should be "read-only"

garious · 2019-03-13T02:02:06Z

grammarly?

garious · 2019-03-13T16:23:22Z

@aeyakovenko, if we implement credit-only program accounts, the userdata will be read-write. Therefore there'd be read-only accounts (loaders), credit-only accounts with read-only userdata (system programs), and credit-only accounts with read-write userdata.

aeyakovenko · 2019-03-13T17:50:29Z

@aeyakovenko, if we implement credit-only program accounts, the userdata will be read-write. Therefore there'd be read-only accounts (loaders), credit-only accounts with read-only userdata (system programs), and credit-only accounts with read-write userdata.

@garious there isn't a technical requirement for loaders to be read-only accounts. they can function as credit-only.

garious · 2019-03-14T02:32:51Z

Now can you check your grammarly commit for obvious misinterpretations?

garious

Just the nits to reverse some of the grammarly changes. Put the hyphen back in "credit only" and return "user data" to "userdata".

garious · 2019-03-14T16:36:19Z

@aeyakovenko, I'll merge #3282 if I can get @mvines' approval. After that, can you rebase and replace mentions of "userdata" with "account data"?

garious · 2019-03-14T16:51:27Z

@aeyakovenko, userdata-to-data PR now merged

solana-grimes · 2019-03-15T13:40:43Z

💔 Unable to automerge due to CI failure

* build(deps): bump serde from 1.0.208 to 1.0.209 Bumps [serde](https://github.com/serde-rs/serde) from 1.0.208 to 1.0.209. - [Release notes](https://github.com/serde-rs/serde/releases) - [Commits](serde-rs/serde@v1.0.208...v1.0.209) --- updated-dependencies: - dependency-name: serde dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Update all Cargo files * sync --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: yihau <[email protected]>

aeyakovenko requested a review from jackcmay February 12, 2019 16:28

aeyakovenko force-pushed the rw_accounts branch from 8fdce45 to d2405da Compare February 12, 2019 16:30

garious reviewed Feb 14, 2019

View reviewed changes

book/src/SUMMARY.md Outdated Show resolved Hide resolved

jackcmay reviewed Feb 14, 2019

View reviewed changes

aeyakovenko added the work in progress This isn't quite right yet label Feb 14, 2019

jackcmay approved these changes Feb 15, 2019

View reviewed changes

aeyakovenko removed the work in progress This isn't quite right yet label Feb 15, 2019

aeyakovenko requested a review from mvines February 15, 2019 01:41

mvines reviewed Feb 19, 2019

View reviewed changes

mvines approved these changes Feb 21, 2019

View reviewed changes

garious reviewed Feb 21, 2019

View reviewed changes

book/src/read-only-read-write-accounts.md Outdated Show resolved Hide resolved

book/src/read-only-read-write-accounts.md Outdated Show resolved Hide resolved

aeyakovenko force-pushed the rw_accounts branch from 7e79c0b to d651104 Compare February 21, 2019 19:12

garious reviewed Mar 12, 2019

View reviewed changes

aeyakovenko force-pushed the rw_accounts branch from d651104 to 1fb7ac1 Compare March 12, 2019 10:31

garious reviewed Mar 12, 2019

View reviewed changes

garious approved these changes Mar 14, 2019

View reviewed changes

aeyakovenko force-pushed the rw_accounts branch from bf75380 to 7ac8438 Compare March 14, 2019 17:13

move

46c1b5c

aeyakovenko force-pushed the rw_accounts branch from 7ac8438 to 46c1b5c Compare March 15, 2019 13:35

aeyakovenko added the automerge Merge this Pull Request automatically once CI passes label Mar 15, 2019

solana-grimes removed the automerge Merge this Pull Request automatically once CI passes label Mar 15, 2019

aeyakovenko merged commit 0465abf into solana-labs:master Mar 15, 2019


		* Read-only access to userdata.

		Instructions that debit or modify the read only account userdata will fail.


		Read-only accounts have the following properties:

		* Can be deposited into. Deposits can be implemented as a simple `atomic_add`.

Add proposal for Read-Only accounts that allow credits #2738

Add proposal for Read-Only accounts that allow credits #2738

Conversation

aeyakovenko commented Feb 12, 2019 • edited Loading

Problem

Summary of Changes

aeyakovenko commented Feb 12, 2019

jackcmay commented Feb 12, 2019

aeyakovenko commented Feb 12, 2019

garious commented Feb 13, 2019

aeyakovenko commented Feb 13, 2019

aeyakovenko commented Feb 13, 2019

jackcmay commented Feb 13, 2019

aeyakovenko commented Feb 13, 2019

jackcmay commented Feb 13, 2019

aeyakovenko commented Feb 13, 2019

jackcmay commented Feb 14, 2019

aeyakovenko commented Feb 14, 2019

jackcmay commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackcmay commented Feb 15, 2019

Choose a reason for hiding this comment

aeyakovenko commented Feb 21, 2019

garious commented Feb 21, 2019

mvines left a comment

Choose a reason for hiding this comment

aeyakovenko commented Feb 21, 2019

aeyakovenko commented Mar 11, 2019

garious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeyakovenko Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garious commented Mar 13, 2019

garious commented Mar 13, 2019

aeyakovenko commented Mar 13, 2019

garious commented Mar 14, 2019

garious left a comment

Choose a reason for hiding this comment

garious commented Mar 14, 2019

garious commented Mar 14, 2019

solana-grimes commented Mar 15, 2019

aeyakovenko commented Feb 12, 2019 •

edited

Loading

aeyakovenko Mar 12, 2019 •

edited

Loading