core/types: cleanup tx signer logic #31434
Conversation
This removes the signer type train in favor of defining a single object that can handle all tx types. Supported types are enabled via a map. Notably, the Pnew signer also supports disabling legacy transactions.
fjl
requested review from
holiman,
karalabe,
gballet,
rjl493456442 and
zsfelfoldi
as code owners
fjl
changed the title
|
PTAL |
There was a problem hiding this comment.
This is nice as it removes a bunch of duplicated code for all post eip-155 signers. It also makes sense for signature recovery legacy type transactions to fall through directly to the legacy signer instead of being evaluated by each previous signer in reverse fork order. And ofc, now that computing the hash is coupled with the transaction and not the signer, that prevents the need to do similar chained evaluation to find matching hashing logic for a tx from a previous fork.
Hard for me to give a firm thumbs-up on anything in this area as this is very sensitive core stuff, but from what I've seen lgtm.
|
So one note here, this could probably be even faster by using a bitmap but I decided not to go there. We can do it if it's wanted, but I have a hunch it will be easier for downstream projects/forks to work with this type and add their own entries when it's a map. |
| return nil, nil, nil, ErrTxTypeNotSupported | ||
| } | ||
| if tt == LegacyTxType { | ||
| return s.legacy.SignatureValues(tx, sig) | ||
| } | ||
| // Check that chain ID of tx matches the signer. We also accept ID zero here, |
There was a problem hiding this comment.
This is kinda outdated now, right? We are not accepting 0 anymore
There was a problem hiding this comment.
Ah now I get the logic
This removes the signer type-train in favor of defining a single object that can handle all tx types. Supported types are enabled via a map. Notably, the new signer also supports disabling legacy transactions.
This removes the signer type-train in favor of defining a single object that can handle all tx types. Supported types are enabled via a map. Notably, the new signer also supports disabling legacy transactions.