telosnetwork / telos-consensus-client

Consensus client for TelosEVM
Apache License 2.0
1 stars 0 forks source link

Invalid s value in Signature #47

Open poplexity opened 1 month ago

poplexity commented 1 month ago

This issue is to track the need to decide on how best to handle the situation, it's possible that reth will remove this zero check (per speaking with them at the Paradigm frontiers event).

The alloy-primitives crate, when the k256 feature is enabled, will do a check for zero values in the signature. On our invalid transactions we put the sender in the s value of the signature, and this fails the check. Currently, the translator/consensus client do not enable the k256 feature on alloy, but when running all the components together in the e2e test within telos-reth repo, the k256 feature is enabled by reth, and this breaks the translator trying to construct the Signature with a zero s value.

Currently, if we use 0xff to pad the s value, it passes the k256 check but changes the block hashes such that they are different than 1.5 and therefore we cannot match reth vs 1.5 RPC hashes.

If we want to revert this behavior, even just in a build to use for matching against 1.5, the changes are in this commit.

poplexity commented 1 month ago

If we are going to enable the state root in 2.0, which will change the block hash anyway, then it might make sense to use 0xff padding just to be more valid with our invalid signatures. If not, and we want to keep same block hashes between 1.5 and 2.0, then we'll need to ensure we don't have failures due to the 0 s value.

poplexity commented 1 week ago

Currently it seems we have added more reth crate dependencies to the consensus client which now cause the consensus client to panic on zero s value, even when the consensus client is built on it's own (not part of the integration test on reth)

poplexity commented 1 week ago

It seems that alloy version 0.3.x resolves this issue and does not enforce the zero check. We need to update telos-reth to the latest v1.0.6 version and then we can update the alloy dependency in the consensus client to 0.3.x to resolve this.