Prevent the invoke and upgrade of programs in the same tx batch#14653
Conversation
jackcmay
added
work in progress
jackcmay
force-pushed
the
upgrade-locks
branch
from
8d057e9 to
1803b9d
Compare
jackcmay
added
CI
jackcmay
force-pushed
the
upgrade-locks
branch
2 times, most recently
from
0667b1d to
13897ea
Compare
Codecov Report
@@ Coverage Diff @@
## master #14653 +/- ##
=======================================
Coverage 80.3% 80.3%
=======================================
Files 403 403
Lines 101132 101172 +40
=======================================
+ Hits 81286 81323 +37
- Misses 19846 19849 +3 |
|
@jackcmay - could we just require the program account be writable here, and enforce that in Upgrade instruction handling? |
|
Yeah, that would probably work too, more efficient but weird 😜 |
Why weird? Touching the core runtime to do this is pretty scary. Requiring a write lock the way I propose avoids special cases |
|
It's weird in the first place that we are pulling an account not specified in the tx so there's that. But requiring an executable to be loaded as writeable is non-intuitive and being enforced by the loader moves some of the knowledge away from accounts load/lock code. Don't get me wrong, I think it's an acceptable solution and comes with less overhead. |
Yea. I like this idea to just require it as writable. |
bennofs
left a comment
bennofs
left a comment
There was a problem hiding this comment.
Looks fine to me. I agree that loading an account not specified in the TX sounds scary though, I'll think a little bit more about that.
mvines
left a comment
mvines
left a comment
There was a problem hiding this comment.
I love there's no runtime/ changes anymore. Looks good except for the need for a feature gate :(
jackcmay
force-pushed
the
upgrade-locks
branch
from
7a8c47f to
6780605
Compare
| (abort_on_all_cpi_failures::id(), "Abort on all CPI failures"), | ||
| (use_loaded_executables::id(), "Use loaded executable accounts"), | ||
| (turbine_retransmit_peers_patch::id(), "turbine retransmit peers patch #14631"), | ||
| (prevent_upgrade_and_invoke::id(), "Prevent upgrade and invoke in same tx batch"), |
There was a problem hiding this comment.
maybe this? nbd though
| (prevent_upgrade_and_invoke::id(), "Prevent upgrade and invoke in same tx batch"), | |
| (prevent_upgrade_and_invoke::id(), "require writeable program account during upgrade #14653"), |
jackcmay
added
the
automerge
mergify
Bot
removed
the
automerge
|
automerge label removed due to a CI failure |
* Prevent the invoke and upgrade of programs in the same tx batch * Pass program address as writable in the upgrade instruction (cherry picked from commit e3bd9e5) # Conflicts: # programs/bpf/Cargo.lock # programs/bpf/Cargo.toml # programs/bpf/tests/programs.rs
* Prevent the invoke and upgrade of programs in the same tx batch * Pass program address as writable in the upgrade instruction (cherry picked from commit e3bd9e5) # Conflicts: # programs/bpf/Cargo.lock # programs/bpf/Cargo.toml
…14653) (#14679) * Prevent the invoke and upgrade of programs in the same tx batch (#14653) * Prevent the invoke and upgrade of programs in the same tx batch * Pass program address as writable in the upgrade instruction (cherry picked from commit e3bd9e5) # Conflicts: # programs/bpf/Cargo.lock # programs/bpf/Cargo.toml # programs/bpf/tests/programs.rs * fix conflicts Co-authored-by: Jack May <jack@solana.com>
5 participants
Problem
Upgradeable programs cannot be invoked and upgraded in the same transaction batch but the programs are not locked as writeable
Summary of Changes
Identify upgrade instructions in the transaction batch and reject any single transaction that attempts to both upgrade and invoke and also lock any programs in the batch that are being upgraded.
Fixes #