Drop write lock on sysvars #15497

sakridge · 2021-02-23T20:56:45Z

Problem

Programs can't write to sysvars and taking a write lock on them can make batch sizes smaller than they should be.

Summary of Changes

Lower write lock to read lock for sysvars.

Fixes #

codecov · 2021-02-23T22:14:00Z

Codecov Report

Merging #15497 (1c4ed67) into master (1d145e1) will increase coverage by 0.0%.
The diff coverage is 90.0%.

@@           Coverage Diff            @@
##           master   #15497    +/-   ##
========================================
  Coverage    80.0%    80.0%            
========================================
  Files         410      410            
  Lines      109501   109611   +110     
========================================
+ Hits        87676    87781   +105     
- Misses      21825    21830     +5

behzadnouri · 2021-03-04T17:59:09Z

sdk/program/src/message.rs

@@ -335,7 +335,7 @@ impl Message {
        let mut writable_keys = vec![];
        let mut readonly_keys = vec![];
        for (i, key) in self.account_keys.iter().enumerate() {
-            if self.is_writable(i) {
+            if self.is_writable(i) && !crate::sysvar::is_sysvar_id(key) {


Wouldn't it make sense to reject all together the ones which write-lock sysvar earlier?
e.g. in sanitize or like the ones with duplicate keys here:
https://github.com/solana-labs/solana/blob/34bebb7d0/runtime/src/accounts.rs#L794-L796

That's an option, but it would be a change of behavior which would break some applications on mainnet.

t-nelson · 2021-03-05T20:53:46Z

Do we still just record TX success/failure as a bool? Should be fine if so. Otherwise I'm wondering if this would change the error type in cases where a program attempt to write to a sysvar

sakridge · 2021-03-05T21:11:45Z

Do we still just record TX success/failure as a bool? Should be fine if so. Otherwise I'm wondering if this would change the error type in cases where a program attempt to write to a sysvar

bool? there is an error code. TransactionError.

behzadnouri · 2021-03-05T21:19:11Z

I moved the demote logic to is_writable.
Let me know if I should revert.

sakridge · 2021-03-08T18:50:40Z

I moved the demote logic to is_writable.
Let me know if I should revert.

Ok. Can you plumb the feature gate and also write a test to verify the error behavior is the same before and after this change for the case of referencing and writing to a sysvar? cc @behzadnouri

behzadnouri · 2021-03-09T15:12:36Z

Ok. Can you plumb the feature gate and also write a test to verify the error behavior is the same before and after this change for the case of referencing and writing to a sysvar? cc @behzadnouri

@sakridge which bank should we use to look up the feature-set?

behzadnouri · 2021-03-09T15:55:23Z

@sakridge I added a feature gate for the write-lock demotion.

Moved the logic from Message::is_writable back to Message::get_account_keys_by_lock_type. Because the other one seems to impact serialize_instructions.
Not sure which bank to use for looking up feature-set. Currently this is using the same bank which calls unlock_accounts and prepare_batch. In transaction_status_service.rs it is the bank received through the queue.

sakridge · 2021-03-09T15:59:01Z

@sakridge I added a feature gate for the write-lock demotion.

Moved the logic from Message::is_writable back to Message::get_account_keys_by_lock_type. Because the other one seems to impact serialize_instructions.

Not sure which bank to use for looking up feature-set. Currently this is using the same bank which calls unlock_accounts and prepare_batch. In transaction_status_service.rs it is the bank received through the queue.

Ok. thanks. Yes the same bank which the process_transactions is going through. So it's from blockstore_processor from the replay side, or banking_stage from the TPU side.

sakridge · 2021-03-09T16:02:07Z

core/src/transaction_status_service.rs

-                    transaction.message.get_account_keys_by_lock_type();
+                let (writable_keys, readonly_keys) = transaction
+                    .message
+                    .get_account_keys_by_lock_type(bank.demote_sysvar_write_locks());


@CriesofCarrots @mvines I think this will affect how it shows in the explorer. Should we show sysvar as demoted to read locked even if it is write in the transaction? or as the original raw write lock?

My preference would be to show sysvar accounts as they were processed, so demoted to read-locked.

sakridge · 2021-03-09T16:09:00Z

@sakridge I added a feature gate for the write-lock demotion.

Moved the logic from Message::is_writable back to Message::get_account_keys_by_lock_type. Because the other one seems to impact serialize_instructions.

Not sure which bank to use for looking up feature-set. Currently this is using the same bank which calls unlock_accounts and prepare_batch. In transaction_status_service.rs it is the bank received through the queue.

Ok. thanks. Yes the same bank which the process_transactions is going through. So it's from blockstore_processor from the replay side, or banking_stage from the TPU side.

I looked at the code, I think it looks right.

I think we might want to demote for the builtin keys as well:

solana_sdk::native_loader::check_id(owner)
solana_budget_program::check_id()
solana_vote_program::check_id()
solana_stake_program::check_id()
..
anything under `programs/`

behzadnouri · 2021-03-09T18:06:14Z

I think we might want to demote for the builtin keys as well:

solana_sdk::native_loader::check_id(owner)
solana_budget_program::check_id()
solana_vote_program::check_id()
solana_stake_program::check_id()
..
anything under `programs/`

Added the builtin keys. I had to copy over keys because direct references create cyclical build dependency.

CriesofCarrots · 2021-03-09T18:14:57Z

core/src/transaction_status_service.rs

-                    transaction.message.get_account_keys_by_lock_type();
+                let (writable_keys, readonly_keys) = transaction
+                    .message
+                    .get_account_keys_by_lock_type(bank.demote_sysvar_write_locks());


My preference would be to show sysvar accounts as they were processed, so demoted to read-locked.

CriesofCarrots · 2021-03-09T18:15:16Z

sdk/program/src/message.rs

+        "Vest111111111111111111111111111111111111111",
+        "Vote111111111111111111111111111111111111111",
+        "ownab1e111111111111111111111111111111111111",
+    ]


Should the SystemProgram be included here?

sdk/program/src/message.rs

ryoqun · 2021-03-11T15:07:36Z

@behzadnouri (CC: @sakridge ), I think this pr starts to prohibit this currently-allowed-but-bizarre tx: sending lamports to sysvars.

I think it's ok to prohibit that in the future for the sake of performance. Could you add such a test?

(Ah, also please check that capitalization isn't messed up by that, too.)

behzadnouri · 2021-03-11T15:17:32Z

I think this pr starts to prohibit this currently-allowed-but-bizarre tx: sending lamports to sysvars.

@ryoqun do you see any more serious consequences from that or similar transactions? e.g. breaking already deployed programs, or conflicting block producers vs validators?

sakridge · 2021-03-11T17:29:01Z

@behzadnouri (CC: @sakridge ), I think this pr starts to prohibit this currently-allowed-but-bizarre tx: sending lamports to sysvars.

I think it's ok to prohibit that in the future for the sake of performance. Could you add such a test?

(Ah, also please check that capitalization isn't messed up by that, too.)

Yea. I dont' think there are any use-cases of transferring lamports to sysvar accounts and I haven't seen any on-chain. Although, I haven't done any kind of scan for it.

It would be a good idea to have a test to see what happens in this pathway before and after this change in terms of the capitalization and error behavior though. Both of modifying sysvar data and lamports values.

With the feature cutoff I would think there shouldn't be any difference of behavior with block producer and validator since they would be activated at the same time.

Co-authored-by: Michael Vines <mvines@gmail.com>

…ing methods

…on/encoding methods" This reverts commit ae3e2d2.

Pull request has been modified.

* Drop write lock on sysvars * adds env var for demoting sysvar write lock demotion * moves demote logic to is_writable * feature gates sysvar write lock demotion * adds builtins to write lock demotion * adds system program id to builtins * adds Feature111... * adds an abi-freeze test * mvines set of builtin program keys Co-authored-by: Michael Vines <mvines@gmail.com> * update tests * adds bpf loader keys * Add test sysvar * Plumb demote_sysvar to is_writable * more plumbing of demote_sysvar_write_locks to is_writable * patches test_program_bpf_instruction_introspection * hard codes demote_sysvar_write_locks to false for serialization/encoding methods * Revert "hard codes demote_sysvar_write_locks to false for serialization/encoding methods" This reverts commit ae3e2d2. * change the hardcoded ones to demote_sysvar_write_locks=true * Use data_as_mut_slice Co-authored-by: behzad nouri <behzadnouri@gmail.com> Co-authored-by: Michael Vines <mvines@gmail.com> (cherry picked from commit 54c68ea)

* Drop write lock on sysvars * adds env var for demoting sysvar write lock demotion * moves demote logic to is_writable * feature gates sysvar write lock demotion * adds builtins to write lock demotion * adds system program id to builtins * adds Feature111... * adds an abi-freeze test * mvines set of builtin program keys Co-authored-by: Michael Vines <mvines@gmail.com> * update tests * adds bpf loader keys * Add test sysvar * Plumb demote_sysvar to is_writable * more plumbing of demote_sysvar_write_locks to is_writable * patches test_program_bpf_instruction_introspection * hard codes demote_sysvar_write_locks to false for serialization/encoding methods * Revert "hard codes demote_sysvar_write_locks to false for serialization/encoding methods" This reverts commit ae3e2d2. * change the hardcoded ones to demote_sysvar_write_locks=true * Use data_as_mut_slice Co-authored-by: behzad nouri <behzadnouri@gmail.com> Co-authored-by: Michael Vines <mvines@gmail.com> (cherry picked from commit 54c68ea) Co-authored-by: sakridge <sakridge@gmail.com>

behzadnouri reviewed Mar 4, 2021

View reviewed changes

behzadnouri force-pushed the drop-sysvar-write-lock branch 2 times, most recently from e51c2ea to c5514ae Compare March 4, 2021 22:01

sakridge marked this pull request as ready for review March 5, 2021 17:02

sakridge requested a review from t-nelson March 5, 2021 17:02

mvines mentioned this pull request Mar 7, 2021

Consider mapping sysvars to syscalls #15755

Closed

behzadnouri mentioned this pull request Mar 7, 2021

caches sysvar deserialized from account data #15756

Closed

behzadnouri force-pushed the drop-sysvar-write-lock branch from 752b506 to 415482a Compare March 9, 2021 15:48

sakridge commented Mar 9, 2021

View reviewed changes

CriesofCarrots reviewed Mar 9, 2021

View reviewed changes

mvines reviewed Mar 9, 2021

View reviewed changes

sdk/program/src/message.rs Outdated Show resolved Hide resolved

mvines reviewed Mar 9, 2021

View reviewed changes

sdk/program/src/message.rs Outdated Show resolved Hide resolved

mvines reviewed Mar 9, 2021

View reviewed changes

sdk/program/src/message.rs Show resolved Hide resolved

mvines reviewed Mar 9, 2021

View reviewed changes

sdk/program/src/message.rs Outdated Show resolved Hide resolved

behzadnouri force-pushed the drop-sysvar-write-lock branch 2 times, most recently from 6ed66da to f1f9611 Compare March 9, 2021 21:06

behzadnouri force-pushed the drop-sysvar-write-lock branch from f1f9611 to c826001 Compare March 11, 2021 15:26

sakridge and others added 18 commits March 29, 2021 15:36

Drop write lock on sysvars

8784ad9

adds env var for demoting sysvar write lock demotion

29c34fc

moves demote logic to is_writable

76b147a

feature gates sysvar write lock demotion

cdbd40b

adds builtins to write lock demotion

3b235a4

adds system program id to builtins

d86661b

adds Feature111...

84f0cdc

adds an abi-freeze test

ffde6d1

mvines set of builtin program keys

dd2e3a3

Co-authored-by: Michael Vines <mvines@gmail.com>

update tests

a454f10

adds bpf loader keys

ea35236

Add test sysvar

3403303

Plumb demote_sysvar to is_writable

817da28

more plumbing of demote_sysvar_write_locks to is_writable

611032b

patches test_program_bpf_instruction_introspection

d93801a

hard codes demote_sysvar_write_locks to false for serialization/encod…

30a530e

…ing methods

Revert "hard codes demote_sysvar_write_locks to false for serializati…

be1e550

…on/encoding methods" This reverts commit ae3e2d2.

change the hardcoded ones to demote_sysvar_write_locks=true

e2130ff

sakridge force-pushed the drop-sysvar-write-lock branch from da8c685 to e2130ff Compare March 29, 2021 22:40

Use data_as_mut_slice

1c4ed67

sakridge merged commit 54c68ea into solana-labs:master Mar 30, 2021

sakridge deleted the drop-sysvar-write-lock branch March 30, 2021 17:05

sakridge added the v1.6 label Mar 30, 2021

mergify bot mentioned this pull request Mar 30, 2021

Drop write lock on sysvars (bp #15497) #16233

Merged

brooksprumo mentioned this pull request Aug 23, 2021

backport 19361 v16 #19379

Closed

CriesofCarrots mentioned this pull request Sep 2, 2021

Demote write locks on transaction program ids #19593

Merged

Drop write lock on sysvars #15497

Drop write lock on sysvars #15497

Uh oh!

Conversation

sakridge commented Feb 23, 2021

Problem

Summary of Changes

Uh oh!

codecov bot commented Feb 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

behzadnouri Mar 4, 2021

Choose a reason for hiding this comment

Uh oh!

sakridge Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-nelson commented Mar 5, 2021

Uh oh!

sakridge commented Mar 5, 2021

Uh oh!

behzadnouri commented Mar 5, 2021

Uh oh!

sakridge commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

behzadnouri commented Mar 9, 2021

Uh oh!

behzadnouri commented Mar 9, 2021

Uh oh!

sakridge commented Mar 9, 2021

Uh oh!

sakridge Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

sakridge commented Mar 9, 2021

Uh oh!

behzadnouri commented Mar 9, 2021

Uh oh!

CriesofCarrots Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

behzadnouri Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryoqun commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

behzadnouri commented Mar 11, 2021

Uh oh!

sakridge commented Mar 11, 2021

Uh oh!

Uh oh!

codecov bot commented Feb 23, 2021 •

edited

Loading

sakridge Mar 4, 2021 •

edited

Loading

sakridge commented Mar 8, 2021 •

edited

Loading

ryoqun commented Mar 11, 2021 •

edited

Loading