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

EIP-4844: Shard Blob Transactions #4844

Merged
merged 8 commits into from Mar 2, 2022

Conversation

protolambda
Copy link
Contributor

@protolambda protolambda commented Feb 26, 2022

This EIP introduces shard blob transactions. See EIP abstract.

TODO: update discussions-to link (Done)

@eth-bot
Copy link
Collaborator

eth-bot commented Feb 26, 2022

All tests passed; auto-merging...

(pass) eip-4844.md

classification
updateEIP
  • passed!

@protolambda protolambda force-pushed the eip-shard-blob-txs branch 2 times, most recently from aeb7d5e to 8a360a8 Compare February 26, 2022 01:40
@protolambda
Copy link
Contributor Author

Force pushes log: Got number 4844 unexpectedly, 4843 doesn't exist, had to change the name. Updated discussions link. Fixed a remaining TODO (tx.header -> tx.message to follow naming pattern established in CL).

@protolambda protolambda changed the title Shard Blob Transactions EIP-4844: Shard Blob Transactions Feb 26, 2022
@MicahZoltu
Copy link
Contributor

I'm very suspicious of an EIP numbered 4844 with @lightclient as an author and issue/PR number 4843 completely missing from GitHub. As soon as I figure out how you managed to skip 4843 I will decide your fate.

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

External links and switching EIP links to relative are the only blockers.

EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
@protolambda
Copy link
Contributor Author

@MicahZoltu thank you for your review. I implemented your suggestions and moved the background links to the eth magicians discussion thread.

Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

CI failures:

  • ./_site/EIPS/eip-4844.html
    • internally linking to ./eip-4490.md, which does not exist (line 941)
      EIP-4490
    • linking to internal hash #Networking that does not exist (line 430)
      Networking

For the anchor link, maybe try ###Networking? I have seen people use anchor links in the past in EIPs, but I never looked into how it works. The rendering system uses Jekyll, so that would be where I would start if you want to dig deeper.

eip-4844.md: description exceeds max length of 140 characters

EIPS/eip-4844.md Outdated
This is equal to the theoretical maximum size of a block today (30M gas / 16 gas per calldata byte = 1.875M bytes), and so it will not greatly increase worst-case bandwidth.
Post-merge, block times are expected to be static rather than an unpredictable Poisson distribution, giving a guaranteed period of time for large blocks to propagate.

The _sustained_ load of this EIP is much lower than alternatives (eg. [EIP-4488](./eip-4488.md) and [EIP-4490](./eip-4490.md)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Any EIPs you link to are considered "dependencies", and thus require those EIPs be as far along in the EIP process as this one. EIP-4490 isn't an EIP at all, and EIP-4488 is a draft I think (and may not reach final before this). I recommend just describing (very briefly) the alternatives and then including additional links out in the discussions-to thread.

As mentioned previously, EIPs should stand on their own aside from their dependencies, and the EIP document isn't an appropriate place to make comparisons against other proposals aside from a high level, "we could do X, but we decided this is superior because of Y".

@protolambda
Copy link
Contributor Author

@MicahZoltu please have a look again:

  • 9e7ecd9 removes the (pre)draft dependencies and describes the differences instead
  • 9b7adeb makes the tx type dependencies (all final EIPs) explicit requirements

EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
and `access_list` as in [`EIP-2930`](./eip-2930.md).

[`EIP-2718`](./eip-2718.md) is extended with a "wrapper data", the typed transaction can be encoded in two forms, dependent on the context:
- Network (default): `TransactionType || TransactionNetworkPayload`, or `LegacyTransaction`
Copy link
Member

Choose a reason for hiding this comment

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

I believe that technically, EIP-2718 txs are encoded as rlp(rlp(tx type || payload)). This essentially means they are encoded as 0xbX byte strings over the wire. Not sure how relevant that is to this conversation here.

Copy link
Contributor

@MicahZoltu MicahZoltu Mar 1, 2022

Choose a reason for hiding this comment

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

Haven't we had this debate before? There is no nested RLP encoding, it is just RLP, and that is only with regards to how they are encoded by the gossip layer, which has nothing to do with this I don't think.

```python
def tx_hash(tx: SignedBlobTransaction) -> Bytes32:
# The pre-image is prefixed with the transaction-type to avoid hash collisions with other tx hashers and types
return keccak256(BLOB_TX_TYPE + ssz.hash_tree_root(tx.message))
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, why this vs. BLOB_TX_TYPE + hash(tx)[1:]?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC ssz.hash_tree_root lets you do fancy proofs of specific items within an object, rather than needing to provide the entire object.

EIPS/eip-4844.md Outdated Show resolved Hide resolved
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

nice!

EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
The `priority_fee_per_gas` and `max_basefee_per_gas` fields follow [EIP-1559](./eip-1559.md) semantics,
and `access_list` as in [`EIP-2930`](./eip-2930.md).

[`EIP-2718`](./eip-2718.md) is extended with a "wrapper data", the typed transaction can be encoded in two forms, dependent on the context:
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is to be a general purpose extension to 2718, we might want to strip it out into a separate EIP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before creating a whole new EIP I want to get feedback in ACD first (there may be different ways we split this EIP).

EIPS/eip-4844.md Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
return ecrecover(tx_hash(tx), sig.v, sig.r, sig.s)
```

### Beacon chain validation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest moving CL code changes out into consensus-specs repo and use "events" and other types of mechanisms to just reference the outer CL functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will work on a draft for the CL side this week. More clarification about the consensus specs and EIP relation would be appreciated.

EIPS/eip-4844.md Outdated Show resolved Hide resolved
EIPS/eip-4844.md Outdated Show resolved Hide resolved
- Along other consensus-layer upgrade modifications to `SignedBeaconBlock`, the `beacon_block` gossip topic is updated
and the `beacon_blocks_by_range/2/` and `beacon_blocks_by_root/2/` type tables are updated.
- Blobs sidecars are relayed through a new global GossipSub topic: `blobs_sidecar`
- Blobs sidecars are synced through ReqResp protocols:
Copy link
Contributor

Choose a reason for hiding this comment

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

Important to define how long these are expected to be served (e.g. on the order of a month?) to put the bound on when this new data can be pruned


Instead of putting rollup block data in transaction calldata, rollups would expect rollup block submitters
to put the data into blobs. This guarantees availability (which is what rollups need) but would be much cheaper than calldata.
Rollups need data to be available once, long enough to ensure honest actors can construct the rollup state, but not forever.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thus why we can have different p2p/node pruning rules than the rest of block data

protolambda and others added 2 commits March 1, 2022 23:09
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
@eth-bot eth-bot enabled auto-merge (squash) March 2, 2022 09:52
EIPS/eip-4844.md Outdated Show resolved Hide resolved
@eth-bot eth-bot merged commit 5868c8c into ethereum:master Mar 2, 2022
@Pandapip1
Copy link
Member

Pandapip1 commented Mar 20, 2022

As soon as I figure out how you managed to skip 4843 I will decide your fate.

@MicahZoltu if someone opens an issue or PR and then gets banned from GitHub, the issue then 404's (see, or rather fail to see #4928). Just so you know for the future :)

@MicahZoltu
Copy link
Contributor

Why didn't I even receive an email if that was the case? For 4928 I at least got an email, since the person got banned after the email went out (which is immediate).

@Pandapip1
Copy link
Member

Okay, I have no idea then.

PowerStream3604 pushed a commit to PowerStream3604/EIPs that referenced this pull request May 19, 2022
* Shard Blob Transactions

* EIP-4844: implement review suggestions from Micah

* EIP-4844: update (pre-)draft EIP comparisons to not introduce dependencies, describe instead

* EIP-4844: list required tx type EIP requirements

* EIP-4844: formatting review fixes by @lightclient

Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>

* EIP-4844: clarifications by @djrtwo

Co-authored-by: Danny Ryan <dannyjryan@gmail.com>

* EIP-4844: clarify blobs downloading requirement, define is_data_available

* Update EIPS/eip-4844.md

Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
Co-authored-by: Micah Zoltu <micah@zoltu.net>
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.

None yet

6 participants