-
Notifications
You must be signed in to change notification settings - Fork 109
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-0178: SBPF Static Syscalls #178
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! I think we just need to land on which SBPF version this goes into?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Removing relocations is a big win. My only comment is the opcode change - this feels a bit unnecessary. But it is not a deal-breaker.
The opcode `0x9D` must represent the return instruction, which supersedes the | ||
`exit` instruction. The opcode (opcode `0x95`), previously assigned to the | ||
`exit` instruction, must now be interpreted as the new syscall instruction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the motivation behind changing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, changing the name from exit
to return
when it is the same instruction could be confusing. I have already seen this confused in other SIMDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note - we should bundle large sets of proposed ISA changes together into the same SBPF version upgrade, so that clients don't have to support a mis-mash of ISAs based on feature flags. I believe this is the intent of #161, but just re-iterating 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation is that exit
was occupying the slot in the instruction class for controlflow with immediate values and it does not take an immediate value. The new syscall
opcode however does, so it took its place.
proposals/0178-static-syscalls.md
Outdated
## Detailed Design | ||
|
||
The following must go into effect if and only if a program indicates the SBPF | ||
version XX or higher in its ELF header e_flags field, according to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we specify which version XX is?
proposals/0178-static-syscalls.md
Outdated
The resolution of syscalls during ELF loading requires relocating addresses, | ||
which is a performance burden for the validator. Relocations require an entire | ||
copy of the ELF file in memory to either relocate addresses we fetch from the | ||
symbol table or offset addresses to after the start of the virtual machine’s | ||
memory. Moreover, relocations pose security concerns, as they allow the | ||
arbitrary modification of program headers and programs sections. A new | ||
separate opcode for syscalls modifies the behavior of the ELF loader, allowing | ||
us to resolve syscalls without relocations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
proposals/0178-static-syscalls.md
Outdated
phase. `call imm` (opcode `0x85`) instructions must only refer to internal | ||
calls and its immediate field must only be interpreted as a relative address | ||
to jump from the program counter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean there is no longer a need to hash the immediates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does and is the intention.
proposals/0178-static-syscalls.md
Outdated
program reaches the execution stage containing the `0x9D` opcode, an | ||
`EbpfError::UnsupportedInstruction` must be raised. | ||
|
||
### Syscall numbering convention |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer is not a great idea. SVM will continue to diverge more and more as non-mainnet SVM chains and L2s develop. If they wish to push their own syscalls, not only will they need to write their own tooling to handle it, if Solana mainnet adds another syscall that clashes and that same binary is shipped to another chain, or vice-versa, people could lose money. It would make way more sense to use the Murmur3 hash. If they decide to launch a hash collision, at least we tried to stop them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Murmur3 is what the original implementation did, and I was against switching to indexes, but then I got busy on other stuff (I just noticed that master was switched to indexes).
Is the best argument for indexes that lookup is faster? And if that's the argument, isn't it moot considering that we JIT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @deanmlittle,
Thanks for your input. I didn't follow why teams would have to develop their own tooling. On the Solana SDK, changing from consecutive integers to a murmur hash is simply assigning another constant to the function pointers. The compiler toolchain can deal with both cases without changes.
On the other hand, I partially agree that using consecutive numbers may hinder the development of SVM chains. In Agave, we considered that using a contiguous array would add to much complexity to the code just to handle inactive syscalls or deprecated ones, so we are still using a BTree, as there are only 40 syscalls to lookup for.
Agave's implementation does not prevent SVM external users from calculating the murmur32 hash for their own syscalls, as any 32-bit integer can be used for indexing. The numbering convention they use does not need to match ours, provided that the numbers don't coincide.
Using consecutive numbers was a request from Firedancer. I believe either @topointon-jump, @ripatel-fd or @0x0ece can elaborate more on the reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LucasSte The main argument is to optimize for bytecode decode efficiency. Interpreting a syscall instruction with a hash requires at least two memory accesses, having an index requires only one. That's a significant cost saving for an instruction that may be executed up to 1 billion times per second in the future.
@deanmlittle There's no need to lose ABI security protections or break cross-SVM program compatibility. This SIMD makes no mention of removal of the symbol table. Currently, the dynamic symbol table of each program maps syscall names to the syscall hash. It would make sense to redefined st_value
to carry the new syscall ID in this SIMD.
Then, the ELF loader can trivially reject programs that have an unknown syscall name or mismatching ID. And the bytecode verifier should reject syscall invocations that weren't verified via the symbol table. ABIv2 proposed previously by Anza similarly moves checks to bytecode verification from later stages, so this wouldn't be out of line.
The other concern you brought up is compatibility. A public GH repo listing IDs and their users is a common way to solve the enum problem. (Examples of other projects doing this: https://github.com/multiformats/multicodec/blob/master/table.csv https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml)
@LucasSte Could you clarify in the SIMD whether static syscalls are verified during ELF loading or bytecode verification? If we won't have these measures in place to support enums, I agree with Dean that we should keep the hash instead.
Is the best argument for indexes that lookup is faster? And if that's the argument, isn't it moot considering that we JIT?
@alessandrod There is a case for allowing zero-copy execution out of a bytecode buffer (i.e. interpreter). With direct mapping, we've seen that the average per-instruction overhead for mainnet executions is so high that a JIT barely outperforms Firedancer's interpreter even when the compiled program is in program cache. Bytecode translation is more susceptible to DoS due to the high cost of allocating memory and JIT compiling when spam invoking cold programs. FWIW, we are beginning mainnet testing of the full Firedancer client too, which is interpreter-only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alessandrod There is a case for allowing zero-copy execution out of a bytecode buffer (i.e. interpreter).
you don't have to fixup in place right?
I understand your perspective. From the anza POV tho, there would be no overhead in having hashes since we always JIT. From the devs POV, I think we can agree that having SVM compatibility is desirable if it doesn't come at a huge cost?
So is it worth making a protocol compromise to accommodate for fd's current implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a hard preference, so I fine reverting the spec to mumur32. @ripatel-fd and @Lichtso are you OK with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, who are these alternative SVMs, L2s and syscalls?
(and btw, there's no requirement for the indexes to be continuous, an L2 could for example start from 50 to avoid collisions. Or they could also add the syscall to the L1 SVM since this is Solana and not Ethereum...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To name a few: Soon, SonicSVM, Atlas, Eclipse, and redacted. There are many use cases not well-served by mainnet-beta due to protocol development grinding to a halt for the past 2 years for obvious reasons. People are starting to innovate elsewhere, and that's actually a very good thing, as if any of these experiments gain adoption or unlock new use cases, we have the ability to adopt them later after being tested in the wild. Making an absolute mess of the enum by padding the indices may sound like a great idea today, but the 20 different versions of the enum depending upon the chain and the inevitable 50th mainnet syscall breaking everything will eventually prove otherwise. If cross-SVM compatibility is of any importance, the Murmur hash is a much better solution for avoiding collisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protocol development grinding to a halt for the past 2 years for obvious reasons.
@deanmlittle This is a joke, right? 😂 Firedancer implemented 140 feature gates the last 24 months (82 features on Jun 2023, 220 or so today). That's about 6 feature gates per month. Solana has kept up an impressive development pace despite the network-wide focus on security after the outages.
Making an absolute mess of the enum by padding the indices may sound like a great idea today, but the 20 different versions of the enum depending upon the chain and the inevitable 50th mainnet syscall breaking everything will eventually prove otherwise.
This problem is simply not as dramatic as you make it out to be. IPv4 addresses have the same problem space (32 bits), there are about 1 million prefixes serving far more people today than the syscall table would ever have to.
I am also unsure the necessity of the symbol table if we went with this design?
Probably unnecessary. With ~10K syscalls, the probability of at least one hash collision is 1%.
With ~80K it's 50%. I think it's fine to assume that there will never be more than 10K syscalls.
So while much less scalable than a fixed mapping, should still be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the feedback. I'll revert the syscalls code back to murmur32 hash. I'll update the SIMD text shortly.
55b3fb5
to
16c0bed
Compare
No description provided.