Skip to content

Conversation

LexLuthr
Copy link
Contributor

This should fix issue with ppInit failing continuously.

@LexLuthr LexLuthr requested a review from a team as a code owner September 26, 2025 09:06
@LexLuthr LexLuthr requested review from rvagg and ZenGround0 and removed request for a team September 26, 2025 09:06
return false, xerrors.Errorf("failed to send transaction: %w", err)
}

txHashLower := strings.ToLower(txHash.Hex())

Choose a reason for hiding this comment

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

I am not a gopher, but some quick snooping through source seems to show that txHash.Hex() should always produce lowercase hex strings?

txHash.Hex() definition seems to be here:

https://github.com/ethereum/go-ethereum/blob/ea28346f91b65c9882e942d2fcad9cdbaa09d706/common/types.go#L86C1-L87C1

which uses go-ethereum hex util:

https://github.com/ethereum/go-ethereum/blob/ea28346f91b65c9882e942d2fcad9cdbaa09d706/common/hexutil/hexutil.go#L83-L88

which uses encoding/hex stdlib's Encode, which should only ever produce lowercase based on the hextable="0123456789abcdef"? https://cs.opensource.google/go/go/+/refs/tags/go1.23.2:src/encoding/hex/hex.go;l=17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same understanding but I remember that DB was complaining about serialization. This lead to basically ppInit never properly working. Curio keept spawning new ppInit tasks. If that is not the case anymore with the pdpv0 branch then feel free to close this. I don't have the error handy.

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 seems we were focusing on different things. Actual fix is .Hex(). strings.ToLower() is there only due to historical context. Perhaps, we can clean that up in other places as well later.

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