COINBASE opcode unit test asserts wrong value (getLast20Bytes vs actual 21-byte push)

[FinchAlex]

Bug Description

The unit test for the COINBASE (0x41) opcode in OperationsTest.java asserts against getLast20Bytes(), but the actual coinBaseAction implementation pushes the full 21-byte TRON address (including the 0x41 prefix) without calling getLast20Bytes().

This creates a misleading discrepancy: the test says COINBASE returns 20 bytes (no prefix), but production code returns 21 bytes (with prefix). The test only passes by accident because the mock coinbase value has zeros in the prefix position.

The asymmetry is intentional in production code -- coinBaseAction deliberately differs from addressAction/callerAction/originAction which all call getLast20Bytes(). But the test doesn't reflect this.

Production code (OperationActions.java)

// ADDRESS, CALLER, ORIGIN -- strip 0x41 prefix:
public static void addressAction(Program program) {
    DataWord address = program.getContractAddress();
    if (VMConfig.allowMultiSign()) {
        address = new DataWord(address.getLast20Bytes()); // strips prefix
    }
    program.stackPush(address);
}

// COINBASE -- does NOT strip prefix:
public static void coinBaseAction(Program program) {
    DataWord coinbase = program.getCoinbase();
    program.stackPush(coinbase);  // pushes full 21-byte DataWord, 0x41 at byte[11]
}

Test code (OperationsTest.java)

// COINBASE test -- asserts getLast20Bytes() (doesn't match production):
Assert.assertEquals(new DataWord(invoke.getCoinbase().getLast20Bytes()),
    program.getStack().pop());

Environment

Network: N/A (unit test issue)

Software Versions

OS: N/A
JVM: N/A
Git Commit: master (also present in 4.7.6+, likely all versions)
Version: N/A
Code: N/A

Expected Behavior

The COINBASE test should assert the actual value pushed by coinBaseAction:

Assert.assertEquals(invoke.getCoinbase(), program.getStack().pop());

Or at minimum, the test should use a non-zero coinbase address where getLast20Bytes() produces a different result from the full DataWord, so the assertion would catch the discrepancy.

Actual Behavior

The test asserts getLast20Bytes() which strips the 0x41 prefix. This passes only because the mock coinbase is all-zeros (or has zeros at the prefix byte position), making getLast20Bytes() and the full DataWord equivalent.

Frequency

• Always (100%)

Steps to Reproduce

1. Open framework/src/test/java/org/tron/common/runtime/vm/OperationsTest.java
2. Find the COINBASE (0x41) test section
3. Compare the assertion with coinBaseAction in OperationActions.java
4. Observe that the test uses getLast20Bytes() but the production code does not

To demonstrate concretely: change the test's mock invoke to use a coinbase with a non-zero first byte (e.g., 0x41...). The assertion will fail because getLast20Bytes() drops byte[11] (0x41) while the actual opcode preserves it.

Additional Context

Impact

This misleading test caused a bug in a third-party TVM implementation. The implementor followed the test's implication that COINBASE strips the 0x41 prefix (like ADDRESS/CALLER/ORIGIN), when in fact it preserves it. The bug was only caught during mainnet block replay at block 3,270,871 where a contract uses COINBASE as a randomness seed -- the different prefix handling changed the SHA3 hash, causing execution to diverge.

Suggested fix

// Option 1: Fix assertion to match production behavior
Assert.assertEquals(invoke.getCoinbase(), program.getStack().pop());

// Option 2: Use non-zero coinbase in mock so getLast20Bytes() != full DataWord
// This would have caught the discrepancy immediately

A comment explaining the intentional asymmetry between COINBASE and other address opcodes would also help future implementors.

[yanghang8612]

Thanks for the report.

This difference is known and intentional. In java-tron, COINBASE uses the witness address from the block header and pushes program.getCoinbase() as-is. It is not normalized through the same path as ADDRESS/CALLER/ORIGIN, which have their own TRON/EVM address compatibility handling.

This behavior has existed for a long time and is part of the current TVM consensus behavior. Changing COINBASE to strip the prefix would be consensus-affecting and could break historical replay or contracts that already depend on the existing result.

We have evaluated this discrepancy and do not consider it a java-tron runtime bug or security issue. The production implementation and historical mainnet behavior are the source of truth here. The unit test may not document this nuance clearly enough, especially for third-party implementors, so adding a clarifying comment or strengthening the test case could be considered. However, the runtime behavior itself is by design and should not be changed.

[FinchAlex]

Thanks for confirming the behavior is intentional.

Our concern is specifically about the existing unit test in OperationsTest.java (line 453):

// Current test — uses getLast20Bytes(), which strips the leading byte
Assert.assertEquals(new DataWord(invoke.getCoinbase().getLast20Bytes()),
    program.getStack().pop());

This assertion accidentally passes because ProgramInvokeMockImpl.getCoinbase() returns a 20-byte value (E559DE5527492BCB42EC68D07DF0742A98EC3F1E) padded into a 32-byte DataWord — so getLast20Bytes() is a no-op.

In production, getCoinbase() returns a 21-byte TRON address with 0x41 prefix. The coinBaseAction pushes it as-is (unlike addressAction/originAction/callerAction which call getLast20Bytes()). The test doesn't verify this distinction.

A third-party implementor reading this test would reasonably conclude that COINBASE should also go through getLast20Bytes() — which is exactly the bug we hit.

Suggested fix — use a realistic 21-byte TRON address in the mock and assert the full value:

// ProgramInvokeMockImpl.getCoinbase():
@Override
public DataWord getCoinbase() {
    // 21-byte TRON address with 0x41 prefix, matching production behavior.
    byte[] coinBase = Hex.decode("41E559DE5527492BCB42EC68D07DF0742A98EC3F1E");
    return new DataWord(coinBase);
}

// OperationsTest.java COINBASE assertion:
// COINBASE = 0x41
op = new byte[]{0x41};
program = new Program(op, op, invoke, interTrx);
testOperations(program);
Assert.assertEquals(2, program.getResult().getEnergyUsed());
// coinBaseAction pushes getCoinbase() as-is — does NOT call getLast20Bytes()
// unlike ADDRESS/ORIGIN/CALLER. The 0x41 TRON prefix is preserved on stack.
Assert.assertEquals(invoke.getCoinbase(), program.getStack().pop());

This way the test actually documents the intentional difference between COINBASE and ADDRESS/ORIGIN/CALLER.

[yanghang8612]

@FinchAlex Agreed that this is about test coverage/documentation rather than runtime behavior.

The runtime behavior will remain unchanged: COINBASE pushes the witness address as provided by the block header, and it must not be normalized through getLast20Bytes().

That said, the current unit test does not make this distinction explicit because the mock coinbase is a 20-byte value, so getLast20Bytes() does not expose the difference. It is reasonable to improve the test so it documents and protects the intentional behavior.

I would prefer adding a focused COINBASE test, or overriding the mock value locally in this test, instead of changing ProgramInvokeMockImpl.getCoinbase() globally and potentially affecting unrelated tests. The assertion should check invoke.getCoinbase() directly and include a comment that COINBASE intentionally preserves the TRON address prefix.

So this should be treated as a test/documentation improvement, not a java-tron runtime bug or consensus change.

[sin-strokes]

Bug Description

The unit test for the COINBASE (0x41) opcode in OperationsTest.java asserts against getLast20Bytes(), but the actual coinBaseAction implementation pushes the full 21-byte TRON address (including the 0x41 prefix) without calling getLast20Bytes().

This creates a misleading discrepancy: the test says COINBASE returns 20 bytes (no prefix), but production code returns 21 bytes (with prefix). The test only passes by accident because the mock coinbase value has zeros in the prefix position.

The asymmetry is intentional in production code -- coinBaseAction deliberately differs from addressAction/callerAction/originAction which all call getLast20Bytes(). But the test doesn't reflect this.

Production code (OperationActions.java)

// ADDRESS, CALLER, ORIGIN -- strip 0x41 prefix:
public static void addressAction(Program program) {
DataWord address = program.getContractAddress();
if (VMConfig.allowMultiSign()) {
address = new DataWord(address.getLast20Bytes()); // strips prefix
}
program.stackPush(address);
}

// COINBASE -- does NOT strip prefix:
public static void coinBaseAction(Program program) {
DataWord coinbase = program.getCoinbase();
program.stackPush(coinbase); // pushes full 21-byte DataWord, 0x41 at byte[11]
}

Test code (OperationsTest.java)

// COINBASE test -- asserts getLast20Bytes() (doesn't match production):
Assert.assertEquals(new DataWord(invoke.getCoinbase().getLast20Bytes()),
program.getStack().pop());

Environment

Network: N/A (unit test issue)

Software Versions

OS: N/A
JVM: N/A
Git Commit: master (also present in 4.7.6+, likely all versions)
Version: N/A
Code: N/A

Expected Behavior

The COINBASE test should assert the actual value pushed by coinBaseAction:

Assert.assertEquals(invoke.getCoinbase(), program.getStack().pop());
Or at minimum, the test should use a non-zero coinbase address where getLast20Bytes() produces a different result from the full DataWord, so the assertion would catch the discrepancy.

Actual Behavior

The test asserts getLast20Bytes() which strips the 0x41 prefix. This passes only because the mock coinbase is all-zeros (or has zeros at the prefix byte position), making getLast20Bytes() and the full DataWord equivalent.

Frequency

• Always (100%)

Steps to Reproduce

1. Open framework/src/test/java/org/tron/common/runtime/vm/OperationsTest.java
2. Find the COINBASE (0x41) test section
3. Compare the assertion with coinBaseAction in OperationActions.java
4. Observe that the test uses getLast20Bytes() but the production code does not

To demonstrate concretely: change the test's mock invoke to use a coinbase with a non-zero first byte (e.g., 0x41...). The assertion will fail because getLast20Bytes() drops byte[11] (0x41) while the actual opcode preserves it.

Additional Context

Impact

This misleading test caused a bug in a third-party TVM implementation. The implementor followed the test's implication that COINBASE strips the 0x41 prefix (like ADDRESS/CALLER/ORIGIN), when in fact it preserves it. The bug was only caught during mainnet block replay at block 3,270,871 where a contract uses COINBASE as a randomness seed -- the different prefix handling changed the SHA3 hash, causing execution to diverge.

Suggested fix

// Option 1: Fix assertion to match production behavior
Assert.assertEquals(invoke.getCoinbase(), program.getStack().pop());

// Option 2: Use non-zero coinbase in mock so getLast20Bytes() != full DataWord
// This would have caught the discrepancy immediately
A comment explaining the intentional asymmetry between COINBASE and other address opcodes would also help future implementors.

[sin-strokes]

// ADDRESS, CALLER, ORIGIN -- strip 0x41 prefix:
public static void addressAction(Program program) {
DataWord address = program.getContractAddress();
if (VMConfig.allowMultiSign()) {
address = new DataWord(address.getLast20Bytes()); // strips prefix
}
program.stackPush(address);
}

// COINBASE -- does NOT strip prefix:
public static void coinBaseAction(Program program) {
DataWord coinbase = program.getCoinbase();
program.stackPush(coinbase); // pushes full 21-byte DataWord, 0x41 at byte[11]
}