Skip to content

Conversation

@Sahil-4555
Copy link
Contributor

With Go 1.20, the standard library now provides crypto/subtle.XORBytes, which is more efficient than our existing bitutil.XORBytes. Because the custom implementation is referenced in only a single spot in erigon, deprecating it is straightforward.

// XORBytes xors the bytes in a and b. The destination is assumed to have enough
// space. Returns the number of bytes xor'd.
//
// If dst does not have length at least n,
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't really explain what the n is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It refers to min(len(a), len(b))

xorOff = (binary.LittleEndian.Uint32(cache[dstOff:]) % uint32(rows)) * hashBytes
)
bitutil.XORBytes(temp, cache[srcOff:srcOff+hashBytes], cache[xorOff:xorOff+hashBytes])
subtle.XORBytes(temp, cache[srcOff:srcOff+hashBytes], cache[xorOff:xorOff+hashBytes])
Copy link
Member

Choose a reason for hiding this comment

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

The usage is in the ethash implementation, so you can also consider removing ethash :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Means your suggestion is to remove this entire execution/protocol/rules/ethash directory?

Copy link
Member

Choose a reason for hiding this comment

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

Let's not do that yet :)

@Sahil-4555 Sahil-4555 requested a review from chfast December 9, 2025 11:26
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

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

Already approved. My intention was just to indicate that the single usage of this function is in the semi-dead code, i.e. risk is very low.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants