Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions core/vm/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,17 @@ func NewMemory() *Memory {
}

func (m *Memory) Store() []byte {
return m.store
// Clamp the capacity so callers cannot append into the unallocated tail of
// the backing array, which Resize may later hand back as zeroed memory.
return m.store[:len(m.store):len(m.store)]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The three-index clamping in Store, Data and GetPtr is defensive hardening that upstream ethereum#33056 does not include. It's cheap and correct, and I agree with the reasoning (no current caller appends — I checked GetMemoryCopyPadded and all tracer consumers), but it does add fork-local divergence in a file that will keep receiving cherry-picks. Consider noting in the commit message that these three lines are Sei-local additions, or upstreaming them, so future merges don't silently drop them.

}

// Free returns the memory to the pool.
func (m *Memory) Free() {
// To reduce peak allocation, return only smaller memory instances to the pool.
const maxBufferSize = 16 << 10
if cap(m.store) <= maxBufferSize {
clear(m.store)
m.store = m.store[:0]
m.lastGasCost = 0
memoryPool.Put(m)
Expand Down Expand Up @@ -80,10 +83,14 @@ func (m *Memory) Set32(offset uint64, val *uint256.Int) {
val.PutUint256(m.store[offset:])
}

// Resize resizes the memory to size
// Resize grows the memory to the requested size.
func (m *Memory) Resize(size uint64) {
if uint64(m.Len()) < size {
m.store = append(m.store, make([]byte, size-uint64(m.Len()))...)
if uint64(len(m.store)) < size {
if uint64(cap(m.store)) >= size {
m.store = m.store[:size]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This fast path is correct, but it makes an implicit invariant load-bearing for consensus: bytes in [len(m.store), cap(m.store)) must always be zero. I traced it and it currently holds — Free clears [0, len), the append branch below only runs when cap < size (so growslice always reallocates, and Go zeroes [newlen, cap) for pointer-free element types), and Set/Set32/MSTORE8 never write past len.

The risk is that nothing in the code says so. A future change that shrinks len without clearing — e.g. a Reset() doing m.store = m.store[:0], or a truncating resize — would silently expose stale bytes to the EVM and diverge nodes, with no compiler or test to catch it. Worth a short comment here (and at the clear() in Free) stating the invariant and why clearing only len bytes is sufficient.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This fast path is only sound because m.store[len(m.store):cap(m.store)] is guaranteed to be all zeroes, which in turn depends on the new clear() in Free and on the capacity clamping below. That's the consensus-critical part of this change and it's currently only explained in the PR description. Worth a one-line comment here (and on clear(m.store) in Free) so a future edit doesn't silently break it.

} else {
m.store = append(m.store, make([]byte, size-uint64(len(m.store)))...)
}
}
}

Expand All @@ -105,8 +112,11 @@ func (m *Memory) GetPtr(offset, size uint64) []byte {
return nil
}

// memory is always resized before being accessed, no need to check bounds
return m.store[offset : offset+size]
// memory is always resized before being accessed, no need to check bounds.
// The capacity is clamped so callers cannot append into the unallocated
// tail of the backing array, which Resize may later hand back as zeroed
// memory.
return m.store[offset : offset+size : offset+size]
}

// Len returns the length of the backing slice
Expand All @@ -116,7 +126,8 @@ func (m *Memory) Len() int {

// Data returns the backing slice
func (m *Memory) Data() []byte {
return m.store
// Capacity is clamped, see Store.
return m.store[:len(m.store):len(m.store)]
}

// Copy copies data from the src position slice into the dst position.
Expand Down
42 changes: 42 additions & 0 deletions core/vm/memory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,45 @@ func TestMemoryCopy(t *testing.T) {
}
}
}

func BenchmarkResize(b *testing.B) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] The PR body correctly identifies the clear() in Free as required for correctness ("EVM execution results can diverge across nodes"), but the only test added is a benchmark — nothing asserts the zeroing invariant itself.

Consider adding a test that exercises the pool reuse cycle directly: Resize a Memory to some size, write non-zero bytes into it, Free() it, then NewMemory() + Resize back into the same range and assert every byte is zero. That pins the exact regression this change guards against, and would fail loudly if the clear() were ever dropped or the reslice path extended to shrink.

memory := NewMemory()
for i := range b.N {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This benchmark never frees or resets, so memory grows monotonically to b.N bytes. At default -benchtime=1s with ~ns-per-op, b.N can reach the hundreds of millions, and the doubling growth means roughly 2x that in peak RSS — enough to matter on a constrained CI runner, and worse under -benchtime=10s.

Upstream's version has the same shape, so this is fine to keep as-is for port fidelity. If you'd rather bound it, cycling the size (e.g. memory.Resize(uint64(i % 4096)) after an initial warm Resize) still exercises the cap >= size fast path this change adds, which is the interesting one.

memory.Resize(uint64(i))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Resize(uint64(i)) grows monotonically with b.N, so the backing array ends up b.N bytes — at ~1ns/op and the default -benchtime=1s that's on the order of a gigabyte, and memory.Free() is never called so it's held for the whole run. Consider bounding the size (e.g. memory.Resize(uint64(i%1024)), with a Free()/fresh NewMemory() when it wraps) so a go test -bench=. run can't blow up CI memory; that also exercises the reslice fast path more representatively.

}
}

// TestMemoryViewCapacityClamped verifies that the slices handed out by GetPtr,
// Data and Store cannot be appended into the unallocated tail of the backing
// array. Resize reslices within capacity, so a stray append by a caller (e.g. a
// custom precompile) would otherwise make newly expanded memory read non-zero,
// breaking the EVM invariant that fresh memory is zeroed.
func TestMemoryViewCapacityClamped(t *testing.T) {
m := NewMemory()
m.Resize(128) // grow the backing array...
m.store = m.store[:32] // ...then shrink the view, leaving spare capacity
if cap(m.store) < 128 {
t.Fatalf("test precondition: want spare capacity, have cap %d", cap(m.store))
}

for _, tc := range []struct {
name string
view []byte
}{
{"GetPtr", m.GetPtr(0, 32)},
{"GetPtr offset", m.GetPtr(16, 16)},
{"Data", m.Data()},
{"Store", m.Store()},
} {
if have, want := cap(tc.view), len(tc.view); have != want {
t.Errorf("%s: capacity not clamped: have %d, want %d", tc.name, have, want)
}
// Appending must reallocate rather than write into m.store's tail.
_ = append(tc.view, 0xff)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] _ = append(...) with a discarded result is exactly what staticcheck's SA4010 flags ("this result of append is never used"), and .golangci.yml enables staticcheck with run.tests: true — this may fail lint in CI. It's also redundant: the cap(tc.view) == len(tc.view) assertion above already proves an append must reallocate. Suggest dropping the line, or making it load-bearing, e.g. if v := append(tc.view, 0xff); &v[0] == &tc.view[0] { t.Errorf(...) }.

}

m.Resize(128)
if want := make([]byte, 96); !bytes.Equal(m.store[32:], want) {
t.Errorf("expanded memory not zeroed: %#x", m.store[32:])
}
}
Loading