Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core, params: prototype withdrawal transactions #24468

Closed

Conversation

ralexstokes
Copy link
Member

@ralexstokes ralexstokes commented Feb 24, 2022

PR prototyping "push"-style withdrawal transaction processing from the beacon chain to the EVM.

TODO:

  • add block validations for tx collation
  • to Wei
  • clean up tx hash

Open questions:

  • Geth uses a transaction's hash to identify it -- in keeping w/ the use of SSZ elsewhere for the serialization of this transaction type, I'd suggest the hash identifier is the SSZ hash tree root of the withdrawal receipt container. Open to other options though.
  • @fjl had the idea to emit logs for each withdrawal transaction processed. I'm open to the idea and left a note where it would make sense to write them to the state, but I'll leave out the implementation for now.

Notes:

This PR will accompany a draft EIP that suggests EL upgrades to go into Shanghai to facilitate validator withdrawals. This stye of withdrawal method is accompanied by the CL upgrade in this PR: ethereum/consensus-specs#2836

This PR assumes that this transaction type 0x3 is placed alongside regular user transactions in the ExecutionPayload -- the CL PR needs to be updated to reflect this change as noted there.

Implementation rationale:

This PR changes the "outer" transaction processing logic during the application of state transitions. Another option would be to leverage more of the EVM machinery (detecting the transaction type during EVM execution) but that leads to a number of special cases to support this new transaction type (e.g. msg.from could be nil but that fails a number of invariants in the EVM that from is always some account address). The overall design of the "push"-style transactions also explicitly forgoes EVM processing (to avoid dealing with issues of gas accounting and dealing with failed executions on withdrawal consumption) so the current implementation seems like the best spot to add support for this new transaction type.

@ralexstokes ralexstokes force-pushed the prototype-withdrawal-txns branch 2 times, most recently from 01f343b to a59b7cc Compare February 25, 2022 00:53
@protolambda
Copy link
Contributor

At Optimism we're exploring a "deposit tx" (same as "system tx", but rollup terms) type that only the consensus layer can put in a block. It is put in a block based on consensus processing, like a "deposit" in a rollup, a system change, or a "withdrawal" in the case of beacon chain.

Here's the diff of the prototype: ethereum-optimism/reference-optimistic-geth@base...optimism-prototype

The fee model is not entirely determined yet, there are many options. We've some discussion here:

The most minimal version of a fee model that seems viable to us enables the consensus layer to put a non-refundable "stipend gas" in (useful for system txs, and enables system to take different forms of payment for the gas in advance), and a "dynamic fee" to buy gas based on a fee paid from the user on L2 and the current L2 base-fee (any unconsumed gas can be refunded here).

The nice thing about this is that it does not change precompiles abilities (no storage, no minting), and allows for future updates by the consensus layer (new contracts can be deployed, new calls can be made, etc.) without hardforking the execution layer. And any EVM tooling only has to be updated once to support this tx type, and can then follow mainnet transaction processing without implementing these new precompiles each hardfork.

@ralexstokes
Copy link
Member Author

That general design seems to make a lot of sense for the rollup use case.

For withdrawals from the beacon chain, I'd like to just ignore gas costs by bounding the number of possible transactions in a given execution block to a small number and calling it a day -- this approach was already spec'd out by @djrtwo in the PR linked above: ethereum/consensus-specs#2836

@mcdee
Copy link
Contributor

mcdee commented Feb 25, 2022

Agree that if the number of transactions are bounded, and their operation simple and does not touch the EVM at all, then adding gas costs to this is unnecessary. The requirement will then be on the consensus layer to ensure that it keeps the number of transactions down to a reasonable value in each block.

Regarding the hash, it's one of those friction points where the two different encoding systems rub up against each other. From what I can see the consensus layer wouldn't need to provide the hash of the transaction, in which case it could continue to be the hash of the RLP encoding rather than the SSZ hash. If that's the case it seems to make sense to keep it the RLP hash rather than the SSZ hash, for simplicity's sake on the EL side (and all of the downstream code that hashes transactions for one reason or another, e.g. explorers).

core/types/transaction.go Outdated Show resolved Hide resolved
@ralexstokes ralexstokes force-pushed the prototype-withdrawal-txns branch 3 times, most recently from 43b43f0 to 914f8a7 Compare February 25, 2022 18:19
@ralexstokes ralexstokes force-pushed the prototype-withdrawal-txns branch from 914f8a7 to 9a194e8 Compare February 25, 2022 19:07
@ralexstokes ralexstokes changed the title wip: core: prototype withdrawal transactions core, params: prototype withdrawal transactions Feb 25, 2022
@rjl493456442
Copy link
Member

Any specific reason for using the ssz encoding?

@ralexstokes
Copy link
Member Author

@rjl493456442

Any specific reason for using the ssz encoding?

the withdrawal record begins life on the beacon chain where we use SSZ. at some point, it crosses over into the EL where we use RLP. we can put the translation boundary where ever we want and this PR assumes the layout given in the CL PR here: ethereum/consensus-specs#2836 but i'm open to a different configuration.

one thing i do think would help is converting from Gwei to Wei on the beacon chain before making the withdrawal receipt -- this is essentially just encoding the amount as 32 bytes which doesn't require any substantially new logic on the beacon chain.

@rjl493456442
Copy link
Member

rjl493456442 commented Mar 1, 2022

@ralexstokes @mcdee Personally I also think the hash derived by RLP encoding is more suitable.

From CL's perspective, it needs to pass the withdrawal information to EL which accounts need to have their balances updated. And this information doesn't need to be formal transaction format.

For example, it's current payload attributes definition in Geth. We can simply add one more fields
for indicating these information.

// PayloadAttributesV1 structure described at https://github.com/ethereum/execution-apis/pull/74
type PayloadAttributesV1 struct {
	Timestamp                       uint64                        `json:"timestamp"     gencodec:"required"`
	Random                            common.Hash          `json:"random"        gencodec:"required"`
	SuggestedFeeRecipient common.Address     `json:"suggestedFeeRecipient"  gencodec:"required"`
        Withdrawer                      []common.Address  `json:"withdrawer"  gencodec:"required"`
        WithdrawalAmounts.      []*big.Int                     `json:"withdrawalAmounts"  gencodec:"required"`
}

And in EL, it can build the transaction structure by using these information and also pass back the relevant transaction hash to CL(if it's needed).

cc @MariusVanDerWijden if I miss something?

Comment on lines 417 to 419
} else if tx.Type() == WithdrawalTxType {
// TODO: use SSZ root here instead?
h = prefixedRlpHash(tx.Type(), tx.inner)
Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be a duplicate of the else block. Why not just leave this else if condition out entirely?

Regarding the comment, you SHOULD include the transaction type in any signing (to avoid transaction malleability, see rationale for EIP-2718), and for future consistency I recommend applying that same rule for hashing in general (even when signatures aren't used) just so you can have consistency across the codebase.

Partially out of scope for this PR, and I suspect we have discussed this before, but why are typed transactions "RlpHashed"? Why not just hash the bytes directly (no need for RLP, you already have a byte array so it seems you should just hash that byte array).

Copy link
Member Author

Choose a reason for hiding this comment

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

i pulled it out to call out where we would use the SSZ hash tree root if we went down that route but thinking now to just use the typical prefixedRlpHash so i'll go remove now

yes agree, and that is what prefixedRlpHash does here to mix in the type to any hash (you could then turn around and sign)

Copy link
Contributor

Choose a reason for hiding this comment

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

In the blob txs EIP SSZ type we define prefixedSSZHash that does keccak256(type ++ hash_tree_root(tx.message)).
This outer keccak256 leaves compatibility with the proof structure of other transaction type pre-images, is collision resistant with other compatible SSZ types due to the type prefix, and exposes the hash_tree_root to do those fancy merkle proofs over. In this case I would do the same, but tx.message just becomes tx, since the transaction is not signed.

@ralexstokes
Copy link
Member Author

@rjl493456442 @mcdee yeah, having thought about it some more, i'm thinking to not bother with the hash tree root here and use the usual algorithm for determining the transaction hash

@ralexstokes ralexstokes force-pushed the prototype-withdrawal-txns branch 2 times, most recently from 2ab42bf to 1e5968b Compare March 1, 2022 23:15
@ralexstokes ralexstokes force-pushed the prototype-withdrawal-txns branch from 1e5968b to 96aae3d Compare March 1, 2022 23:48
@holiman
Copy link
Contributor

holiman commented Nov 16, 2022

I get the sense that this PR is a prototype which by now is abandoned -- can we close it in favour of #25838 ?

@lightclient
Copy link
Member

I think yes.

@ralexstokes ralexstokes deleted the prototype-withdrawal-txns branch November 16, 2022 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants