Enable proper `bytesX` typing for the result of `.split()` and `.slice()`

[mr-zwets]

We should fix the length of bytes types when using split:

bytes2 firstTwoBytes = test.split(2)[0];

The above does not work, when it should always be possible to type the length of the first element of the tuple. It should only be possible to type the length of the second element when the length of the original bytes element is typed.

The typing when getting both elements of the tuple is also wrong:

contract Demo() {
    function example(bytes4 test) {
        bytes3 firstHalf , bytes8 secondHalf = test.split(2);
        require(firstHalf .length == 2);
        require(secondHalf.length == 2);
    }
}

firstHalf can only be bytes2 here, and secondHalf can also only be bytes2, because the original length of the bytes element is typed

[rkalis]

Mathieu and I were talking about this and we think it could make sense to disallow bytesX as function parameters, since these types are not runtime enforced. We could alternatively also add an option to enforce these types by injecting require(x.length === y) statements. This is something we should consider in more detail later.

[rkalis]

Good to note that there would be no way to runtime enforce constructor parameter types, because at "construction" time, no script gets executed because of the way P2SH works.

[mr-zwets]

I like the idea of disallowing bytesX as function parameters better than adding extra compiler checks without the user being informed. However other bytes types also have semantic meaning: pubkey sig and datasig but these don't make any exact claims about their size (I think).

The original issue was mainly focused how bytesX typing works incorrectly with .split()

[mr-zwets]

BytesX type casting

Disallowing bytesX types in function arguments would mean bytesX typecasting will be more common, so we'd need to improve our docs.

I had to reach out to find out how this works internally:

bytes to bytes casting is just semantic, not doing something like padding. That's intended, but might be good to document better.

the main places where specific byte lengths matter is for lockingBytecodes

// these two global function need bytesX inputs
new LockingBytecodeP2PKH(bytes20 pkh): bytes25
new LockingBytecodeP2SH32(bytes32 scriptHash): bytes35

[mr-zwets]

Rational for properly typed .split()

State kept in an nft commitment is often fixed length to be easy parsable (although the last item could be allowed to be variable length). On state transitions, this state is read, chopped up (split) and reconstructed.

Let's say my commitment is always 16 bytes consisting of two bytes8 items, only one of which was mutated.
From a contract security perspective it is nice if newCommitment can explicitly be of type bytes16.

Current workaround

Currently we always need a bytesX typecast when using .split() which seems like something the compiler could easily infer:

bytes oldState = tx.inputs[0].nftCommitment;
// Unnecessary bytesX typecast
bytes8 item1 = bytes8(oldState.split(8)[0]);
bytes8 newItem2 = bytes8(someIntegerValue);
bytes16 newCommitment = item1 + newItem2;

Suggested change

When getting the 1st item of .split() with a numerical index, we can always infer the bytelength:

bytes oldState = tx.inputs[0].nftCommitment;
// Typing the first item of .split()
bytes8 item1 = oldState.split(8)[0];
bytes8 newItem2 = bytes8(someIntegerValue);
bytes16 newCommitment = item1 + newItem2;

When getting the 2nd item of .split() with a numerical index we can infer the bytelength when the length of the original bytestring is known

bytes16 oldState = bytes16(tx.inputs[0].nftCommitment);
bytes8 newItem1 = bytes8(someIntegerValue);
// typing the second item of .split()
bytes8 item2 = oldState.split(8)[1];
bytes16 newCommitment = newItem1 + item2 ;

When using .split() with a variable integer, length inference is not possible, unless maybe the variable is a constant known at compile time. Perfect inference for this edge case is less important

[mr-zwets]

The correctness of the bytes type is important for contract security.
I suggest we do the following:

1. disallow bytesX as function argument
2. restrict the typings in tuple assignment (so only correct inference is allowed)
3. broaden the bytes typings in variable declarations when the .split() index is a numeric

[rkalis]

@mr-zwets and I had a long discussion about how to properly go about this. I'll write down the conclusions below.

For bytesX types in function parameters:

1. We disallow bytesX as function argument.
2. If a user adds e.g. a require(x.length == 20) check, then the compiler will infer that x's type is bytes20 for the rest of the contract.
3. (Optionally) apply the same for if statements.
4. If people want to forgo these checks, they can always manually cast to bytesX(x). We should add a warning to the docs about the security implications of doing that.

For split() typings:

1. We extend type inference to x.split(y) calls when y is a number literal. If the type of x is bytesX, then we can infer both sides of the split, otherwise we can only infer the left side.
2. If the type of x is bytes and the runtime size of x would be too small for the split, the contract just fails, so no security implications there.
3. We make sure that tuple assignment behaves according to the same rules.

We also considered whether function parameter length checks were needed for security (e.g. if a malicious actor is able to do stuff by passing too many or too few parameters). Here are our conclusions about that:

1. If too few arguments are passed, then the script would run into stack index out of bounds error, meaning the contract can't be spent that way.
2. If too many arguments are passed, then certain conditions could be affected, but the CLEANSTACK rule means that the function would still fail at the end because too many values are left on the stack.
3. Because of (1) and (2) we don't see any reason to add automatic function parameter length checks.

[mr-zwets]

the .split() issue should be fixed in the visitTupleAssignment function in TypeCheckTraversal.ts.

the implicitlyCastable check is wrong for BytesType, as splitting changes the semantics of the type assignment, here's an example:

The implicitlyCastable check would allow (bytes4, bytes4) when doing bytes4.split(2), so the semantic meaning of the original type can't be reused for the new assignment.

using implicitlyCastable doesn't really add value here, there's no implicit castings that are really allowed

string.split() -> string, string
bytes.split(variable) -> bytes, bytes
bytes.split(NumberLiteral) -> bytes(NumberLiteral), bytes
bytesX.split(NumberLiteral) -> bytes(NumberLiteral), bytes(X - NumberLiteral)

[mr-zwets]

visitBinaryOp in TypeCheckTraversal.ts says the TupleType could be improved to do type inference

      case BinaryOperator.SPLIT:
        expectAnyOfTypes(node, node.left.type, [new BytesType(), PrimitiveType.STRING]);
        expectInt(node, node.right.type);

        // Result of split are two unbounded bytes types (could be improved to do type inference)
        node.type = new TupleType(
          node.left.type instanceof BytesType ? new BytesType() : PrimitiveType.STRING,
        );
        return node;