feat(byte_array): add slice

[giladchase]

1. When a slice ends before a word boundary (it's last word isn't a full 31 bytes long)
   , it's last word is copied into the remainder word.
   Rationale: this is consistent with ByteArray's pending word, and allows slices of full bytes31
   that include an end_suffix to be shifted-right without allocating a new array.

2. When slices include a start-offset, the offset is applied lazily only upon intoing into a
   ByteArray, otherwise it's only recorded in the first_char_start_offset field.

[reviewable-StarkWare]

This change is 

[giladchase]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• feat(byte_array): add at and index to ByteSpan #8427
• feat(byte_array): add slice #8417 👈 (View in Graphite)
• feat(byte_array): add ByteSpan::to_byte_array #8416
• refactor(byte_array): extract ByteArray::append logic #8486
• feat(byte_array): add ByteArray::span and Span::len #8329
• refactor(test): remove fixed-size bytearray testutils #8343
• refactor(test): remove compare_spans util #8342
• refactor(test): remove compare_byte_array util and rework tests #8336
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[orizi]

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @TomerStarkware)

---

corelib/src/byte_array.cairo line 628 at r2 (raw file):

    /// Returns a slice of the ByteSpan from the given start position with the given length.
    fn slice(self: @ByteSpan, start: usize, len: usize) -> Option<ByteSpan> {

should slice alway be option based?

Code quote:

    /// Returns a slice of the ByteSpan from the given start position with the given length.
    fn slice(self: @ByteSpan, start: usize, len: usize) -> Option<ByteSpan> {

---

corelib/src/byte_array.cairo line 636 at r2 (raw file):

        }

        let abs_start = upcast(*self.first_char_start_offset) + start;

this may be a fallible function now - and i d

Code quote:

        let abs_start = upcast(*self.first_char_start_offset) + start;

---

corelib/src/byte_array.cairo line 653 at r2 (raw file):

                ByteSpan {
                    data: [].span(),
                    first_char_start_offset: downcast(0).unwrap(),

Suggestion:

                    first_char_start_offset: 0,

---

corelib/src/byte_array.cairo line 664 at r2 (raw file):

            true => slice_bytes31((*self.data[end_word]).into(), BYTES_IN_BYTES31, 0, end_offset),
            false => slice_bytes31(*self.remainder_word, remainder_len, 0, end_offset),
        };

Suggestion:

        // Multi-word slice - data words plus optional remainder
        let remainder = if end_word < data_len {
            slice_bytes31((*self.data[end_word]).into(), BYTES_IN_BYTES31, 0, end_offset)
        } else {
            slice_bytes31(*self.remainder_word, remainder_len, 0, end_offset)
        };

---

corelib/src/bytes_31.cairo line 191 at r2 (raw file):

    let (without_prefix_and_suffix, _) = split_bytes31(without_suffix, start + len, len);
    without_prefix_and_suffix
}

no reason for this to be in another file.

Code quote:

/// Extracts a slice of bytes from a word.
/// Returns bytes [start, start+len) where byte 0 is the leftmost (most significant) byte.
/// The input `bytes31` and the output `bytes31`s are represented using `felt252`s to improve
/// performance.
///
/// Note: this function assumes that:
/// 1. `word` is validly convertible to a `bytes31` which has no more than `word_len` bytes of data.
/// 2. `start + len <= word_len`.
/// 3. `word_len <= BYTES_IN_BYTES31`.
/// If these assumptions are not met, it can corrupt the result. Thus, this should be a
/// private function. We could add masking/assertions but it would be more expensive.
pub(crate) fn slice_bytes31(word: felt252, word_len: usize, start: usize, len: usize) -> felt252 {
    if len == 0 {
        return 0;
    }
    // Remove suffix: keep only bytes [0, start+len).
    let (_, without_suffix) = split_bytes31(word, word_len, word_len - (start + len));
    let (without_prefix_and_suffix, _) = split_bytes31(without_suffix, start + len, len);
    without_prefix_and_suffix
}

[giladchase]

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @orizi and @TomerStarkware)

---

corelib/src/byte_array.cairo line 628 at r2 (raw file):

Previously, orizi wrote…

should slice alway be option based?

Wasn't sure, my reasoning was to allow users more choice: they can use slice for checked behavior, and indexing for panic behavior.
Also this allow us to add later on slice_unchecked like rust used to do (they renamed slice->get but point still stands), whereas making this return non-option will require a breaking change later on.

Cons: in array Span::slice we dont return an option, and it sucks to break consistency with it.

WDYT?

---

corelib/src/byte_array.cairo line 636 at r2 (raw file):

Previously, orizi wrote…

this may be a fallible function now - and i d

Missing the end of yr message?

You mean addition overflow or upcast?
upcast says it panics in Sierra stage if the cast isn't legal, but since its boundedint<0,31> it should always fir in start: usize, unless there's something i'm missing about this.

However there were 3 overflow issues (unchecked arithmetic), fixed and added tests for all cases. The remaining unchecked arithmetic ops are guaranteed to succeed (they don't involve user input so are legal by construction).

---

corelib/src/bytes_31.cairo line 191 at r2 (raw file):

Previously, orizi wrote…

no reason for this to be in another file.

Done.

---

corelib/src/byte_array.cairo line 653 at r2 (raw file):

                ByteSpan {
                    data: [].span(),
                    first_char_start_offset: downcast(0).unwrap(),

Done.

---

corelib/src/byte_array.cairo line 664 at r2 (raw file):

            true => slice_bytes31((*self.data[end_word]).into(), BYTES_IN_BYTES31, 0, end_offset),
            false => slice_bytes31(*self.remainder_word, remainder_len, 0, end_offset),
        };

Done + other place.

[orizi]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

---

corelib/src/byte_array.cairo line 628 at r2 (raw file):

Previously, giladchase wrote…

Wasn't sure, my reasoning was to allow users more choice: they can use slice for checked behavior, and indexing for panic behavior.
Also this allow us to add later on slice_unchecked like rust used to do (they renamed slice->get but point still stands), whereas making this return non-option will require a breaking change later on.

Cons: in array Span::slice we dont return an option, and it sucks to break consistency with it.

WDYT?

sounds to leave as is.

---

corelib/src/byte_array.cairo line 636 at r2 (raw file):

Previously, giladchase wrote…

Missing the end of yr message?

You mean addition overflow or upcast?
upcast says it panics in Sierra stage if the cast isn't legal, but since its boundedint<0,31> it should always fir in start: usize, unless there's something i'm missing about this.

However there were 3 overflow issues (unchecked arithmetic), fixed and added tests for all cases. The remaining unchecked arithmetic ops are guaranteed to succeed (they don't involve user input so are legal by construction).

it was mostly about the add - that can't really fail - but better now in any case.

---

corelib/src/byte_array.cairo line 635 at r3 (raw file):

        if start.checked_add(len)? > self.len() {
            return None;
        }

consider - the later accesses would fail as well - so you maybe not need to do all the calculations.

Code quote:

        if start.checked_add(len)? > self.len() {
            return None;
        }

[giladchase]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @orizi and @TomerStarkware)

---

corelib/src/byte_array.cairo line 636 at r2 (raw file):

Previously, orizi wrote…

it was mostly about the add - that can't really fail - but better now in any case.

OK, note that reverted the checked_add here in favor of the one above, due to this thread

---

corelib/src/byte_array.cairo line 635 at r3 (raw file):

Previously, orizi wrote…

consider - the later accesses would fail as well - so you maybe not need to do all the calculations.

Right, only the first one was needed, removed the later two checked_adds.

[orizi]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @TomerStarkware)

---

corelib/src/test/byte_array_test.cairo line 713 at r6 (raw file):

    // Test to demonstrate the optimization of lazy start-offset trimming.
    // Multiple slicing operations on remainder word should be more efficient.
    let ba: ByteArray = "abcdef"; // 10 bytes in remainder

i see 6.

Code quote:

    let ba: ByteArray = "abcdef"; // 10 bytes in remainder

---

corelib/src/byte_array.cairo line 691 at r6 (raw file):

    /// Returns a slice of the ByteSpan from the given start position with the given length.
    fn slice(self: @ByteSpan, start: usize, len: usize) -> Option<ByteSpan> {

and use range style trait.

Suggestion:

 fn slice(self: ByteSpan,