Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SIMD-0099: Feature activation status syscall #99

Closed
wants to merge 1 commit into from

Conversation

2501babe
Copy link
Member

@2501babe 2501babe commented Jan 4, 2024

get whether a feature is active while in a bpf program

@2501babe
Copy link
Member Author

2501babe commented Jan 4, 2024

please feel free to add whoever else may be interested, including a firedancer team member please!

also note the fate of this simd is intertwined with #76

@CantelopePeel
Copy link

The native version of a BPF program should just be upgraded when you want to update its functionality. Feature gates are accounts, and they will need to be in the txns account list, which seems like a bad compromise.

These are my initial thoughts, will review more later.

Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I like the implementation. I also don't believe this will impact any of the efforts outlined by #76.

I do have a few thoughts on motivation, though, with the larger Core BPF and Feature Gate process initiatives in mind (#76).

The motivation herein is stated to be focused on providing the ability for the Stake Program to be upgraded via feature gates.

The Stake Program is regularly updated using feature gates, so some mechanism for it to be aware of their status is necessary.

Also, the following alternative is mentioned.

eliminate the use of feature gates in what are now native programs, and instead upgrade them via a mechanism to swap the bytecode implementation of the program at the appropriate time

This alternative shadows the method by which #88 proposes migrating native programs to BPF. However, as #88 outlines, each native program which migrates to BPF will have its own specification for how upgrades are handled once it has become BPF.

I think this proposal presents a nice additional safeguard for upgrading Core BPF programs, should contributors choose to use it in their specification. For certain programs - such as Stake - engineers may be more comfortable gating code changes behind feature gates, and performing program upgrades via the upgradeable loader.

The alternative, as you mentioned, would be to feature-gate a program upgrade using a runtime account swap, which is potentially more dangerous. You could also keyholder-upgrade the program with no feature gate at all (!!).

In other words, I feel like this is a neat way to cascade the runtime feature activation process - which has many benefits - to decoupled Core BPF programs that are implementation-agnostic.

If you feel like Stake (and maybe others) should still be upgraded using feature gates, I'm in favor of this proposal.

I'm also a bit curious on the potential linear increase in compute as a product of feature upgrades over time. Any thoughts on that?

@buffalojoec
Copy link
Contributor

The native version of a BPF program should just be upgraded when you want to update its functionality. Feature gates are accounts, and they will need to be in the txns account list, which seems like a bad compromise.

From what I can tell in the proposal and the linked proof of concept, it seems as though these accounts do not need to be included in the transaction. The proof of concept is using the invoke context's feature set to query whether or not a feature ID is active. Is that accurate @2501babe ?

@lheeger-jump
Copy link
Contributor

I think this proposal presents a nice additional safeguard for upgrading Core BPF programs, should contributors choose to use it in their specification. For certain programs - such as Stake - engineers may be more comfortable gating code changes behind feature gates, and performing program upgrades via the upgradeable loader.

This seems to imply we want two methods for doing the same thing? I do not think we need more than one method for updating these new native programs. We can just get rid of the existing feature flags and use the program update mechanism.

@CriesofCarrots
Copy link
Contributor

One valuable thing the process described in this SIMD provides -- but maybe doesn't make explicit -- is a mechanism to coordinate upgrades to Core BPF programs with any necessary runtime changes, so they go into effect on the same slot. While the minimum-delegation feature may be one that could be handled by removing the existing feature-gate flag and using the program update mechanism (I haven't examined the code to say for sure), there are plenty of other program changes we will consider that will definitely need this kind of coordination (this one for the stake program, for instance).

In these cases, we would need to either: (a) update the runtime to handle both pre- and post-upgrade versions of a program/program accounts and make sure the program upgrade only happens after the runtime feature is activated; (b) wrap the program upgrade in the same feature gate as the runtime changes; or (c) upgrade the program to handle both pre- and post-activation logic. I'm not sure how often (a) would be feasible; it's probably worth considering. Of (b) and (c), assuming it is practical within the size constraints of a BPF program, I tend to agree with this SIMD that approach (c) is simplest.

@Lichtso
Copy link
Contributor

Lichtso commented Jan 7, 2024

In general, try to avoid syscalls as they will be removed in PRv2 anyway. Also, this is very similar to sysvars where we have this ugly situation that they are available as accounts and as syscalls. The syscalls for sysvars were only introduced as an optimization so that you don't have to specify them in the transaction and more importantly in the call chain of instructions all the way up to the place where they are used. This is a very old restriction much like that programs must be passed the entire call chain through CPI. All of this is not necessary even in PRv1 and sysvars could become normal accounts again.

Also, core programs will be modular and being able to update their entire ELF is a blessing. In the general runtime and built-in programs we don't have that luxury and thus need to feature gate individual parts. That is often a lot harder, and more restrictive as one has to make both paths (feature off and on) compatible code wise. And in situations where it is truely preferable we could just use the feature gate account instead of a syscall.

Anyway, I think it would make more sense to have an SIMD for a set of "core accounts" similar to core programs, which are always implicitly available to all transactions. That would solve many problems which currently require syscalls.

@2501babe
Copy link
Member Author

2501babe commented Jan 8, 2024

I'm also a bit curious on the potential linear increase in compute as a product of feature upgrades over time. Any thoughts on that?

its standard practice to remove the feature check and any now-unreachable code after a feature is activated on all networks

From what I can tell in the proposal and the linked proof of concept, it seems as though these accounts do not need to be included in the transaction. The proof of concept is using the invoke context's feature set to query whether or not a feature ID is active.

yes this is accurate

One valuable thing the process described in this SIMD provides -- but maybe doesn't make explicit -- is a mechanism to coordinate upgrades to Core BPF programs with any necessary runtime changes, so they go into effect on the same slot.

this is true and i could edit the pr to state it more explicitly... another thing is that (iirc) one of the motives behind the upcoming feature program is so features can be disabled in the case of anything, so that would work for this also. im not 100% committed to retaining feature flags in core bpf programs, but i do think theres value in it because even after the native programs are separated out, there will be some interdependency between them and the runtime

Anyway, I think it would make more sense to have an SIMD for a set of "core accounts" similar to core programs, which are always implicitly available to all transactions.

oh i like the idea of this actually. and i wasnt aware syscalls were going away in runtime v2

@2501babe
Copy link
Member Author

going to withdraw this for now. as it stands, feature_set::reduce_stake_warmup_cooldown is the only feature in the stake program that must be coordinated with the runtime, but as it is already active on devnet and testnet, we likely dont need to worry about it by time the stake bpf port is ready. may revisist this idea in the future if necessary for other programs

@2501babe 2501babe closed this Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants