-
Notifications
You must be signed in to change notification settings - Fork 36
Add initial PVM test vectors #3
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
base: master
Are you sure you want to change the base?
Conversation
The test writes a zero here, so make sure the register initially contains something *other* than a zero so that we can actually see that it was modified.
@koute Can you kindly add test cases for the host functions of Appendix B.6/B.7/B.8? Here are 4 groups, in priority order:
The first group is DA-centric, the second group is service+VM setup/invocations -- the first 2 groups are valuable to connect to code up connections to state merklization + erasure coding, whereas the latter 2 groups can be done later as they are bookkeeping oriented and are easy to get right once we solve the first 2 groups. Thank you! |
How are these programs supposed to be consumed? The program blob parse expects things like the program to start with BLOB_MAGIC. |
For these initial test vectors the priority is to get basic tests for the instruction set ready. I will also add some host call tests later, but comprehensive test suite for all host calls is probably out-of-scope, at least for pure PVM tests.
Take a look at the schema to see to which parameters of the Ψ equation from the Gray Paper they correspond to, and use them accordingly to test your own PVM implementation.
Yes, my PolkaVM uses its own container format for the program blobs which the GP doesn't use. PolkaVM is not the source of truth for how a PVM should work, the GP is. |
@koute Good point on the GP being the source of truth.
For context I'm building FFI bindings to PolkaVM. So would you say that these tests in particular are for folks who are implementing the PVM from scratch? |
We passed all the test vectors you provided so far. What is the reason for the GP needing to support 32-bit registers while the contracts pallet should definitely aim for 64-bit? After you have 64-bit PVM engineered for contracts pallet shouldn't the GP be adjusted to be 64-bit? |
Yes.
This is only temporary. We will be migrating GP to 64-bit too; we just need to first prototype the design in PolkaVM to make sure it's solid. (Otherwise we might end up with a design that looks good on paper but is bad in practice.) We're working on it right now. (That said, the changes when migrating to 64-bit won't be huge - the registers will be extended and there will be a couple of new instructions, but that's about it as far as major changes go.) As far as the instruction set and the core semantics are concerned, we aim to have both PolkaJAM and pallet-contracts in alignment and we're making effort to make sure they don't diverge. (With PolkaJAM having the priority here, but I believe we can support both with the same VM.) |
In GP(A.1), the program is defined as follows:
Edit: Looks like I misunderstood |
@ec2 Where did you read that these are SCALE compact integers? These are not SCALE compact integers. From the GP: If you look at this equation and crosscheck it with how This is a slightly different varint serialization format which:
|
@koute GP(Appendix I.3) says that I'm not super familiar with SCALE so I assumed that the screenshot just formally describes how SCALE does variable int encoding. |
@koute Sorry to keep hounding you here! I think I found a discrepancy between the testcases and the GP. In the test, the supplied arg to The test case has only TLDR: I think the impl of PVM that made these test cases have the arguments for |
Alright, we don't want to interrupt your deep work but legend has it you implemented PVM in a day =) so if its not too much to ask ... could you give us the simplest "Jam Service" byte code (for a refine+accumulate) for us to implement many of the basic host functions? Given one good example we can probably fill in the rest and provide a few more back. My idea of the simplest "Jam Service" byte code is to compute the sum of squares for a set of integer work items, like If not, do you have a better recommendation for simplest "Jam Service"? Or, a strategy that is better than hand building byte code? This sort of baby JAM test case will help teams get baby JAM implementations blood flowing, and set up a low V (like V=6) cluster complete with QUIC, erasure coding, Patricia Merkle Trie, BMT proofs, and so on. |
I am confused about trap vs halt vs panic in PVM. In GP, the trap instruction will exit with the black square, so does the jump to Lines 53 to 55 in a2b1870
Another question. |
@ec2 Yes, indeed, there is. We will fix it soon. Thanks! We highly appreciate anyone who helps crosscheck these.
@sourabhniyogi The rumors of my exploits seem to be grossly exaggerated; it was actually two days, not one. :P Anyway, we will most likely put up some more tests out in the future, but for now if you quickly want something to test with then your best bet would be to build one yourself. You don't have to build a blob by hand; you could use my work-in-progress PVM assembler. For example:
This will output the program in a PolkaVM-specific container (which is not part of the GP), but you can extract the code blob with a simple Rust program - use
Hm, you're right that the trap instruction in the GP is specified to halt instead of panicking; this should have been a panic instead. I'll see about correcting this; thanks. |
Hello @koute , I recently encountered some issues while using your PVM. Here’s my code:
When I use the following command to compile:
The bytecode content of
My question is, how do I extract the pure program portion as defined in GP_0.36(213), because it seems the first part contains some ASCII-encoded section names. ASCII encoded section name:
GP_0.36(213) should be:
|
With the current Graypaprer spec (0.6.1) the following test vector is incorrect. Test vector
AnalysisThe incorrect current output is explained because some programming languages (such as: RUST en C) provide a negative output for a modulo operation on a negative number.
Source: https://en.wikipedia.org/wiki/Modulo Possible solutions
In summary 'maths' shoud have priority over any implementation ambiguity, therefore as is solution 1 should be the way to go. |
No, it's explained because PVM is based on RISC-V, and that's how the RISC-V's (and also coincidentally how AMD64's) modulo instruction works. It has absolutely nothing to do with how the modulo operator works in any programming language. Two of the main design principles of PVM are:
So the test vector here is correct and is what we want, and changing the semantics here as you suggest is not a good idea as it will provide no practical benefit while having significant practical downsides. Anyway, thank you for bringing this ambiguity to our attention. |
Sure thanks, this is fine of course. But then GP should be adjusted to explicitly state that the modulo operator on a negative number yields a negative number, and not a positive number as expected by 'Maths'. |
Test Case:
|
@clw8998 I can confirm the test vector is correct here and the expected value is To illustrate why let's pick some smaller numbers to make this more obvious. Let's try to divide a positive number first:
Now let's try flipping the sign:
Notice that flipping the sign of one of the inputs doesn't change the numerical value of You're right that mathematically floor does the (in this case) incorrect thing; we will fix the GP. |
…ccessible` tests
These are in sync with which gp version? |
Unless I missed something, should be in sync with the most recent GP 0.6.2. |
@koute Several implementers consider looking at this to interpret polkavm disassembly as form of collusion: ![]() ![]() ![]() Understanding the registry map was one of the first things we did to learn how building JAM Services in Rust with polkatool since this -- Its pretty important to have a mental model of the registry map and I believe a recent comment of yours caused people to believe they should never even think about looking a single line of polkavm code. What do you think of their "collusion" claim? |
No. The claim that this is collusion is nonsense; those are just standard RISC-V register names. But you do need to be careful if you're reading PolkaVM source code as reading some parts (but not all of it, as I already tried to explain in the JAM chat) can be considered against the rules of the JAM prize. |
I'm afraid I don't know what you mean by "the code is filled with ones instead of zeros". By "code" do you mean the blob where the instructions opcodes + arguments are encoded? Or the bitmask? And by "filled with" do you mean what happens when you read out of bounds of the physical dimensions of either of these blobs? Can you explain in more detail or give me an exact equation number? |
Yes. For example, here you have a test which depends on this behavior (panics since the execution goes out of bounds). If you'd fill it with ones instead then that test would run forever (assuming unlimited gas) because |
Ok, thank you for the clarification and for pointing out a proper example. |
aki |
@koute Recently I've revisited PVM test vectors and got several questions: [PageFault cases]
[Post pc value on Panic or Halt]
[TRAP instruction]
Am I missing something? |
This is done to lessen the requirements (it can be tricky to implement grabbing the guest-side program counter in a recompiler) and/or reduce the surface area for consensus bugs - since the program counter value shouldn't really be needed on-chain we've decided to just forcefully set it to zero in the protocol in cases where the execution should not be continue. (Although, to be fair, it might actually be a better idea to set it to But, for the purpose of testing your PVM it's still useful to know where exactly it's trapping, hence the test vectors don't have those set to zero.
Not changing the program counter is correct, otherwise it would skip the instruction after servicing the page fault, which doesn't make any sense. But yes, unless I'm missing something it seems like the A.1 equation should be corrected to not change the instruction pointer when page faulting or running out of gas.
Yes. This behavior is not yet specced in the GP, but for the final gas cost model we will be charging gas not per instruction but per each basic block (otherwise the overhead of the gas metering will be too high); I'm planning to add this to the GP soon-ish. |
Thank you for the clarification, that's really helpful! |
@koute after updating my implementation to 0.6.7 and above, I'm unable to pass your tests. The issue arises after implementing A.5 and A.19; are your tests aligned with this? |
Can you be more specific about what's actually failing? These shouldn't fail after implementing the updated eq. A.5 and A.19, so either I made a whoopsie when updating those equations in the GP, or you implemented them incorrectly. |
I indeed implemented them incorrectly but I was asking if your tests were aligned with the latest version of the GP. I think A.19 is missing a boundary check, I should investigate more anyway and in that case I'll open an issue there. Thank you |
There should be nothing to align because these test programs shouldn't trigger the corner cases which those new equations are designed to handle. |
@koute can we merge this? |
Yes, I don't see why not; I can always made a new PR for any further modifications. |
Hello, i successfully pass trace 1757406516 now but I noticed i had to change how I account pageFaults in instructions. I have no idea why it was like this but apparently my codebase before the fix accounted for the IX gasCost + Trap GasCost when the IX caused a pagefault. By doing so i am able to pass the pvm instructions checks from @koute here in this pr. for example inst_store_imm_indirect_u16_with_offset_nok >= https://github.com/w3f/jamtestvectors/pull/3/files#diff-ca69518f98802d38482848f24f5d3431f0fe2fa744712da6a49e76a9253e4269R57 expects 2 gas to be consumed. only one IX is executed (the one which triggers pagefault). I inspected the graypaper and apparently there is no special handling for when pagefault happens so i guess the current fuzzer implementation is ok... tossing it here so that it happens before the merge |
Initial PVM test vectors/test suite.
This is still incomplete; not every instruction is covered yet and only very simple test cases were added. I will be expanding this aggressively.
Since we will still be making some changes (e.g. 64-bit support) I'll be explicitly versioning this, with a detailed changelog so that anyone who uses these tests can easily keep up.