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

bug: failing MXP Tests #1538

Closed
DavePearce opened this issue Nov 22, 2024 · 4 comments
Closed

bug: failing MXP Tests #1538

DavePearce opened this issue Nov 22, 2024 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@DavePearce
Copy link
Collaborator

Three tests in MxpTest have been @Disabled since they are all failing with this error:

java.lang.IllegalArgumentException: hub.context/RETURN_DATA_OFFSET has invalid value (9223372036854775807)

The value 9223372036854775807 is 0x7FFFFFFFFFFFFFFF and equals Long.MAX_VALUE. For the record, the tests are:

  • testMxpRandomTriggerMxpx()
  • testRandomMxpInstructionsFollowedByTriggeringRoob()
  • testMxpRandomAdvanced()
@DavePearce DavePearce added the bug Something isn't working label Nov 22, 2024
DavePearce added a commit that referenced this issue Nov 22, 2024
This disables three tests which lead to an overflow of the
`hub/RETURN_DATA_OFFSET` column during trace generation.  See #1538.
@DavePearce
Copy link
Collaborator Author

DavePearce commented Nov 22, 2024

And we begin tracing this back. Problematic value comes from here (OperationAncillaries):

public static MemorySpan outputDataSpan(final MessageFrame frame) {
   
   ...
    
   switch (opCode) {
      case RETURN, REVERT -> {
        long offset = Words.clampedToLong(frame.getStackItem(0));
        long length = Words.clampedToLong(frame.getStackItem(1));
        return MemorySpan.fromStartLength(offset, length);
      }

Which looks to me rather like it was generated from the stack?. Yes, and we can see where our magic Long.MAX_VALUE appears from:

  static long clampedToLong(final Bytes uint) {
    if (uint.size() <= 8) {
      final long result = uint.toLong();
      return result < 0 ? Long.MAX_VALUE : result;
    }

    final Bytes trimmed = uint.trimLeadingZeros();
    if (trimmed.size() <= 8) {
      final long result = trimmed.toLong();
      return result < 0 ? Long.MAX_VALUE : result;
    } else {
      // clamp to the largest int.
      return Long.MAX_VALUE;
    }
  }

This is in besu/evm/internal/Words.java.

@DavePearce
Copy link
Collaborator Author

DavePearce commented Nov 22, 2024

So, the sequence is:

  1. Random big integer created in MxpTest here:
      size1 = EWord.of(util.getRandomBigIntegerByBytesSize(0, MAX_BYTE_SIZE));
      offset1 = EWord.of(util.getRandomBigIntegerByBytesSize(0, MAX_BYTE_SIZE));
  1. This is passed in as one of the arguments for a REVERT or RETURN, after being clamped to Long.MAX_VALUE (as above).
  2. MemorySpan is created to represent the RETURN_DATA which is very large, and this then fails assigning the field RETURN_DATA_OFFSET in the relevant Trace.java (because this field is marked as an i32).

@letypequividelespoubelles How would you want to proceed from here? Seems like we could use clampedToInt() in this case. Or perhaps its better to restrict the values for size1 and/or offset1 above? Otherwise it seems a bit iffy to me.

DavePearce added a commit that referenced this issue Nov 22, 2024
* Remove non-determinism from tests

This removes non-determinism from the tests caused by a shared RNG
across tests.

* Disable tests with `RETURN_DATA_OFFSET` overflow

This disables three tests which lead to an overflow of the
`hub/RETURN_DATA_OFFSET` column during trace generation.  See #1538.
@DavePearce
Copy link
Collaborator Author

@letypequividelespoubelles I think we can close this now right?

@letypequividelespoubelles
Copy link
Collaborator

yes I think @OlivierBBB 's #1545 fixed it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants