core/types: rlp decode unsigned transactions

[nik-suri]

This PR enhances RLP decoding for transactions to enable decoding unsigned transactions.

This is necessary for compatibility with other ethereum libraries and better communication between different systems. For example, ethereumjs does not include signature bytes at all when serializing an unsigned transaction -- https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/tx/src/eip1559Transaction.ts#L294-L305. Prior to this change, attempting to decode unsigned transactions serialized by ethereumjs would result in the following errors (depending on transaction type):

rlp: too few elements for types.LegacyTx
rlp: too few elements for types.AccessListTx
rlp: too few elements for types.DynamicFeeTx

This PR also adds tests to ensure that the decoding works on hardcoded example payloads.

[nik-suri]

The tests appear to have failed due to a flaky issue:

--- FAIL: TestSkeletonSyncRetrievals (1.86s)
    skeleton_test.go:832: test 7, mid state: dropped peers mismatch: have 0, want 1

I have verified that the tests run successfully locally.

Is there a way to re-run the CI tests?

[karalabe]

I'm unsure if this is a good idea. RLP decoding currently protects our internals that transactions always have all the required fields set. If we make certain fields optional, we'd need to go through every single place in our code that might end up decoding transactions and would need to check to make sure the needed fields are set.

I'm also unsure why it's a good idea to transfer unsigned transactions via RLP, since RLP is mostly used for consensus / wire format, where you cannot have an unsigned tx.

[holiman]

The 'legacy' way to RLP-interpret unsigned transactions is to use all zeroes for r,s,v. I don't see why we need to modify the rlp decoder at this point, in 2022, when it's been like this for a long++ time.

[karalabe]

I'm going to close as "too dangerous to allow nil values unmashalled from the network".

[nik-suri]

Transferring unsigned transactions via RLP is a valid use-case that is in use today for airgapped transactions. Metamask produces a QR code that contains the CBOR of an rlp-encoded unsigned transaction, which is meant to be decoded, checked, and then signed on the backend. This flow is performed today by hardware wallets such as keystone (please see https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/tx/src/eip1559Transaction.ts#L281-L305)

Our backend is written in go and uses go-ethereum, which is why we need compatibility with the transaction serialization that is done in Metamask and ethereumjs/tx. We do not want to blind-sign payloads sent to us from Metamask. We need to

1. ensure that the payload deserializes into a valid ethereum transaction.
2. run security checks against the transaction to ensure we are not signing something we don't want to.

I don't understand why we would need to go through every place in the geth code to check the needed fields are set? This is a backwards-compatible change that will not break any existing clients. If clients begin passing unsigned transactions to methods which require signature fields r,s,v to be set, it seems to be the proper design that geth should panic and the client should fix their request. Passing in unsigned vs signed txns is a client-side issue, not a geth issue. Geth should simply be able to decode unsigned/signed transactions for maximum compatibility with other libraries, and clients should be responsible for making sure their RLP-encoded transactions are compliant.

I would appreciate if you could re-open this PR and reconsider merging this change.

[0x0ece]

+1 to this, this would be great.

We run into this exact issue with metamask/eth js too. I guess from "their" point of view, they're building the payload that you have to sign via private key. That payload does NOT contain r,s,v, so that's what they encode in RLP.

It'd be nice to be able to parse this payload directly via geth. In our case we had to copy&paste geth objects, essentially doing what this PR is doing by making some fields optional, deserialize into our own objects and then copy over the content into geth objects. Pretty nasty code.

[conorpp]

@karalabe would you consider reopening if we created a new type to use for unsigned transactions?

type UnsignedDynamicFeeTx struct {
	ChainID    *big.Int
	Nonce      uint64
	GasTipCap  *big.Int // a.k.a. maxPriorityFeePerGas
	GasFeeCap  *big.Int // a.k.a. maxFeePerGas
	Gas        uint64
	To         *common.Address `rlp:"nil"` // nil means contract creation
	Value      *big.Int
	Data       []byte
	AccessList AccessList
}

type DynamicFeeTx struct {
	UnsignedDynamicFeeTx
	// Signature values
	V *big.Int `json:"v" gencodec:"required"`
	R *big.Int `json:"r" gencodec:"required"`
	S *big.Int `json:"s" gencodec:"required"`
}

Then we can opt in to just using UnsignedDynamicFeeTx and the rest of the codebase doesn't need to add error checking.

Being able to parse unsigned transactions is very important to know what you are signing before you sign it. It's common to need to do this in any architecture where you don't want to store your keys in the browser with metamask. It's not trivial to just add some zeros since the underlying RLP structure needs to be parsed first to update the length fields.

Appreciate your feedback!

[nik-suri]

@karalabe @holiman could you please respond with feedback? I think the comments I and others have given above are very reasonable and we would appreciate hearing from you!