From 95df8f0e35c09b3cb8b3def1d62b17e9751141ee Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 21 May 2024 18:50:18 +0200 Subject: [PATCH 1/3] conversion: fix append instead of overwrite in MarshalSSZTo. OBS! This is a breaking change --- conversion.go | 32 +++++++++++++++++--------------- conversion_test.go | 7 ++----- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/conversion.go b/conversion.go index 5cafac2..7efc7f7 100644 --- a/conversion.go +++ b/conversion.go @@ -540,25 +540,27 @@ func bigEndianUint56(b []byte) uint64 { uint64(b[2])<<32 | uint64(b[1])<<40 | uint64(b[0])<<48 } -// MarshalSSZTo implements the fastssz.Marshaler interface and serializes the -// integer into an already pre-allocated buffer. +// MarshalSSZTo implements the fastssz.Marshaler interface, serializes and appends +// the integer to the dst buffer, and returns the (potentially reallocated) buffer. +// +// https://github.com/ferranbt/fastssz/blob/main/interface.go#L4 +// type Marshaler interface { +// MarshalSSZTo(dst []byte) ([]byte, error) +// MarshalSSZ() ([]byte, error) +// SizeSSZ() int +// } func (z *Int) MarshalSSZTo(dst []byte) ([]byte, error) { - if len(dst) < 32 { - return nil, fmt.Errorf("%w: have %d, want %d bytes", ErrBadBufferLength, len(dst), 32) - } - binary.LittleEndian.PutUint64(dst[0:8], z[0]) - binary.LittleEndian.PutUint64(dst[8:16], z[1]) - binary.LittleEndian.PutUint64(dst[16:24], z[2]) - binary.LittleEndian.PutUint64(dst[24:32], z[3]) - - return dst[32:], nil + dst = binary.LittleEndian.AppendUint64(dst, z[0]) + dst = binary.LittleEndian.AppendUint64(dst, z[1]) + dst = binary.LittleEndian.AppendUint64(dst, z[2]) + dst = binary.LittleEndian.AppendUint64(dst, z[3]) + return dst, nil } // MarshalSSZ implements the fastssz.Marshaler interface and returns the integer // marshalled into a newly allocated byte slice. func (z *Int) MarshalSSZ() ([]byte, error) { - blob := make([]byte, 32) - _, _ = z.MarshalSSZTo(blob) // ignore error, cannot fail, surely have 32 byte space in blob + blob, _ := z.MarshalSSZTo(make([]byte, 0, 32)) // ignore error, cannot fail, surely have 32 byte space in blob return blob, nil } @@ -584,8 +586,9 @@ func (z *Int) UnmarshalSSZ(buf []byte) error { // HashTreeRoot implements the fastssz.HashRoot interface's non-dependent part. func (z *Int) HashTreeRoot() ([32]byte, error) { + b, _ := z.MarshalSSZTo(make([]byte, 0, 32)) // ignore error, cannot fail var hash [32]byte - _, _ = z.MarshalSSZTo(hash[:]) // ignore error, cannot fail + copy(hash[:], b) return hash, nil } @@ -752,7 +755,6 @@ var ( ErrEmptyNumber = errors.New("hex string \"0x\"") ErrLeadingZero = errors.New("hex number with leading zero digits") ErrBig256Range = errors.New("hex number > 256 bits") - ErrBadBufferLength = errors.New("bad ssz buffer length") ErrBadEncodedLength = errors.New("bad ssz encoded length") ) diff --git a/conversion_test.go b/conversion_test.go index 3a1cc71..90a3a23 100644 --- a/conversion_test.go +++ b/conversion_test.go @@ -719,7 +719,7 @@ func TestSSZEncodeDecodeHash(t *testing.T) { t.Fatal(err) } if exp := hex2Bytes(tt.exp); !bytes.Equal(b, exp) { - t.Errorf("testcase %d, encoding got:\n%x\nexp:%x\n", i, b, exp) + t.Errorf("testcase %d, encoding\nhave: %x\nwant: %x\n", i, b, exp) } z2 := new(Int) if err := z2.UnmarshalSSZ(b); err != nil { @@ -733,16 +733,13 @@ func TestSSZEncodeDecodeHash(t *testing.T) { t.Fatal(err) } if exp := hex2Bytes(tt.exp); !bytes.Equal(r[:], exp) { - t.Errorf("testcase %d, hashing got:\n%x\nexp:%x\n", i, r, exp) + t.Errorf("testcase %d, hashing\nhave: %x\nwant: %x\n", i, r, exp) } } } func TestSSZEncodeDecodeErrors(t *testing.T) { small := make([]byte, 31) - if _, err := new(Int).MarshalSSZTo(small); !errors.Is(err, ErrBadBufferLength) { - t.Fatalf("overflow marshal error mismatch: have %v, want %v", err, ErrBadBufferLength) - } if err := new(Int).UnmarshalSSZ(small); !errors.Is(err, ErrBadEncodedLength) { t.Fatalf("underflow unmarshal error mismatch: have %v, want %v", err, ErrBadEncodedLength) } From d111c46cb0b9bd95b5aa75b37f5618f9af9e4f53 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 11 Jun 2024 09:59:20 +0200 Subject: [PATCH 2/3] conversion: remove MarshalSSZTo --- conversion.go | 40 +++++++++++++++++++++++++++++++++++----- conversion_test.go | 5 ++++- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/conversion.go b/conversion.go index 7efc7f7..b1742fb 100644 --- a/conversion.go +++ b/conversion.go @@ -540,8 +540,20 @@ func bigEndianUint56(b []byte) uint64 { uint64(b[2])<<32 | uint64(b[1])<<40 | uint64(b[0])<<48 } -// MarshalSSZTo implements the fastssz.Marshaler interface, serializes and appends -// the integer to the dst buffer, and returns the (potentially reallocated) buffer. +// MarshalSSZAppend _almost_ implements the fastssz.Marshaler (see below) interface. +// Originally, this method was named `MarshalSSZTo`, and ostensibly implemented the interface. +// However, it was noted (https://github.com/holiman/uint256/issues/170) that the +// actual implementation did not match the intended semantics of the interface: it +// inserted the data instead of appending. +// +// Therefore, the `MarshalSSZTo` has been removed: to force users into making a choice: +// - Use `MarshalSSZAppend`: this is the method that appends to the destination buffer, +// and returns a potentially newly allocated buffer. This method will become `MarshalSSZTo` +// in some future release. +// - Or use `MarshalSSZInto`: this is the original method which places the data into +// the destination buffer, without ever reallocating. +// +// fastssz.Marshaler interface: // // https://github.com/ferranbt/fastssz/blob/main/interface.go#L4 // type Marshaler interface { @@ -549,7 +561,7 @@ func bigEndianUint56(b []byte) uint64 { // MarshalSSZ() ([]byte, error) // SizeSSZ() int // } -func (z *Int) MarshalSSZTo(dst []byte) ([]byte, error) { +func (z *Int) MarshalSSZAppend(dst []byte) ([]byte, error) { dst = binary.LittleEndian.AppendUint64(dst, z[0]) dst = binary.LittleEndian.AppendUint64(dst, z[1]) dst = binary.LittleEndian.AppendUint64(dst, z[2]) @@ -557,10 +569,27 @@ func (z *Int) MarshalSSZTo(dst []byte) ([]byte, error) { return dst, nil } +// MarshalSSZInto is the first attempt to implement the fastssz.Marshaler interface, +// but which does not obey the intended semantics. See MarshalSSZAppend and +// - https://github.com/holiman/uint256/pull/171 +// - https://github.com/holiman/uint256/issues/170 +// @deprecated +func (z *Int) MarshalSSZInto(dst []byte) ([]byte, error) { + if len(dst) < 32 { + return nil, fmt.Errorf("%w: have %d, want %d bytes", ErrBadBufferLength, len(dst), 32) + } + binary.LittleEndian.PutUint64(dst[0:8], z[0]) + binary.LittleEndian.PutUint64(dst[8:16], z[1]) + binary.LittleEndian.PutUint64(dst[16:24], z[2]) + binary.LittleEndian.PutUint64(dst[24:32], z[3]) + + return dst[32:], nil +} + // MarshalSSZ implements the fastssz.Marshaler interface and returns the integer // marshalled into a newly allocated byte slice. func (z *Int) MarshalSSZ() ([]byte, error) { - blob, _ := z.MarshalSSZTo(make([]byte, 0, 32)) // ignore error, cannot fail, surely have 32 byte space in blob + blob, _ := z.MarshalSSZAppend(make([]byte, 0, 32)) // ignore error, cannot fail, surely have 32 byte space in blob return blob, nil } @@ -586,7 +615,7 @@ func (z *Int) UnmarshalSSZ(buf []byte) error { // HashTreeRoot implements the fastssz.HashRoot interface's non-dependent part. func (z *Int) HashTreeRoot() ([32]byte, error) { - b, _ := z.MarshalSSZTo(make([]byte, 0, 32)) // ignore error, cannot fail + b, _ := z.MarshalSSZAppend(make([]byte, 0, 32)) // ignore error, cannot fail var hash [32]byte copy(hash[:], b) return hash, nil @@ -755,6 +784,7 @@ var ( ErrEmptyNumber = errors.New("hex string \"0x\"") ErrLeadingZero = errors.New("hex number with leading zero digits") ErrBig256Range = errors.New("hex number > 256 bits") + ErrBadBufferLength = errors.New("bad ssz buffer length") ErrBadEncodedLength = errors.New("bad ssz encoded length") ) diff --git a/conversion_test.go b/conversion_test.go index 90a3a23..8d581b0 100644 --- a/conversion_test.go +++ b/conversion_test.go @@ -740,11 +740,14 @@ func TestSSZEncodeDecodeHash(t *testing.T) { func TestSSZEncodeDecodeErrors(t *testing.T) { small := make([]byte, 31) + if _, err := new(Int).MarshalSSZInto(small); !errors.Is(err, ErrBadBufferLength) { + t.Fatalf("overflow marshal error mismatch: have %v, want %v", err, ErrBadBufferLength) + } if err := new(Int).UnmarshalSSZ(small); !errors.Is(err, ErrBadEncodedLength) { t.Fatalf("underflow unmarshal error mismatch: have %v, want %v", err, ErrBadEncodedLength) } large := make([]byte, 33) - if _, err := new(Int).MarshalSSZTo(large); err != nil { + if _, err := new(Int).MarshalSSZAppend(large); err != nil { t.Fatalf("underflow marshal error mismatch: have %v, want %v", err, nil) } if err := new(Int).UnmarshalSSZ(large); !errors.Is(err, ErrBadEncodedLength) { From e68536926728abacbe0d4b1596ade91ed21b72a2 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 11 Jun 2024 10:07:29 +0200 Subject: [PATCH 3/3] fix test coverage --- conversion_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/conversion_test.go b/conversion_test.go index 8d581b0..5e7b66a 100644 --- a/conversion_test.go +++ b/conversion_test.go @@ -753,6 +753,9 @@ func TestSSZEncodeDecodeErrors(t *testing.T) { if err := new(Int).UnmarshalSSZ(large); !errors.Is(err, ErrBadEncodedLength) { t.Fatalf("overflow unmarshal error mismatch: have %v, want %v", err, ErrBadEncodedLength) } + if _, err := new(Int).MarshalSSZInto(large); err != nil { + t.Fatalf("unexpected error: %v", err) + } } func TestRLPEncode(t *testing.T) {