feat(vm): implement TIP-7939 CLZ opcode#6656
feat(vm): implement TIP-7939 CLZ opcode#6656yanghang8612 wants to merge 2 commits intotronprotocol:developfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| if (clz == 256) { | ||
| program.stackPush(new DataWord(256)); | ||
| } else { | ||
| program.stackPush(DataWord.of((byte) clz)); |
There was a problem hiding this comment.
Nice work overall! One minor suggestion: since DataWord.of(byte num) directly assigns bb[31] = num, casting clz to (byte) when it's in [128, 255] introduces a subtle signed/unsigned ambiguity. Would it be worth simplifying both branches into one using new DataWord(int), which handles the full range cleanly?
There was a problem hiding this comment.
Thanks for the review! The (byte) cast here is safe — DataWord.of(byte) assigns the raw bit pattern to bb[31], and DataWord.value() reads it back as unsigned via new BigInteger(1, data). This is consistent with how byte arrays are used throughout the codebase (e.g., DataWord.of((byte) 0xFF) correctly represents 255). The split avoids the double ByteBuffer.allocate overhead in DataWord(int) for the common non-zero path.
| Assert.assertEquals(new DataWord(255), program.getStack().pop()); | ||
|
|
||
| // Verify energy cost = LOW_TIER(5) + PUSH32 cost(3) = 8 | ||
| Assert.assertEquals(8, program.getResult().getEnergyUsed()); |
There was a problem hiding this comment.
The test coverage is really solid — the chosen vectors (0, 1, 255, 256, boundary bits) cover the most critical scenarios nicely! 🎯
One thing that might be worth adding: a couple of test cases where the CLZ result falls in the 128-255 range (e.g., byte[16]=0x01 → CLZ=128, byte[30]=0x01 → CLZ=247). This would explicitly exercise the (byte) cast path for values above 127, giving extra confidence in the signed/unsigned handling. Would make the already-great test suite even more bulletproof!
There was a problem hiding this comment.
Thanks — good coverage suggestion. Adding two vectors in the [128, 255] band so the (byte) cast path is exercised explicitly: byte[16] = 0x01 → CLZ = 128 and byte[30] = 0x01 → CLZ = 247. Between those and the existing 0 / 1 / 255 / 256 edges, the cast handling on values where the sign bit flips is now directly pinned by an assertion rather than being inferred from DataWord.value()'s new BigInteger(1, data) round-trip.
| // Verify energy cost = LOW_TIER(5) + PUSH32 cost(3) = 8 | ||
| Assert.assertEquals(8, program.getResult().getEnergyUsed()); | ||
|
|
||
| VMConfig.initAllowTvmOsaka(0); |
There was a problem hiding this comment.
Nice work on properly resetting VMConfig.initAllowTvmOsaka(0) in cleanup — shows good testing discipline!
Would it be worth adding a small negative test to verify that CLZ is correctly rejected when allowTvmOsaka is disabled? This would serve as a safety net to ensure the feature gate can never be accidentally bypassed. Just a suggestion to make the gating even more robust 🙂
There was a problem hiding this comment.
Agreed, worth pinning the gate as a hard invariant. Adding a CLZ before Osaka activation case that runs with allowTvmOsaka = 0 and asserts the program halts with the same error TVM raises for any unknown opcode. Keeps the gate from quietly disappearing under a future op-table refactor.
Both this and the [128, 255] vectors above will go in together.
halibobo1205
assigned
CodeNinjaEvan,
xxo1shine and
Federico2014 and unassigned
CodeNinjaEvan,
xxo1shine and
Federico2014
Add test vectors with CLZ in [128, 247] (128/192/247) so the (byte) clz cast in DataWord.of(byte) is exercised against values that would round-trip as negative if read as signed; and a testCLZRejectedWhenOsakaDisabled case that verifies the opcode throws IllegalOperationException when allowTvmOsaka=0, locking in the proposal gate.
7 participants
Summary
Implement TIP-7939 CLZ opcode (
0x1e), gated behind the existingallowTvmOsakaflag.LOW_TIER, matching MUL)TRON_V1_5version withappendOsakaOperationsto support Osaka opcodesRelated: tronprotocol/tips#838