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/types: support for optional blob sidecar in BlobTx #27841

Merged
merged 22 commits into from
Aug 14, 2023

Conversation

fjl
Copy link
Contributor

@fjl fjl commented Aug 2, 2023

This PR removes the newly added txpool.Transaction wrapper type, and instead adds a way of keeping the blob sidecar within types.Transaction. It's better this way because most code in go-ethereum does not care about blob transactions, and probably never will. This will start mattering especially on the client side of RPC, where all APIs are based on types.Transaction. Users need to be able to use the same signing flows they already have.

However, since blobs are only allowed in some places but not others, we will now need to add checks to avoid
creating invalid blocks. I'm still trying to figure out the best place to do some of these. The way I have it currently is as follows:

  • In block validation (import), txs are verified not to have a blob sidecar.
  • In miner, we strip off the sidecar when committing the transaction into the block.
  • In TxPool validation, txs must have a sidecar to be added into the blobpool.
    • Note there is a special case here: when transactions are re-added because of a chain reorg,
      we cannot use the transactions gathered from the old chain blocks as-is, because they will be missing
      their blobs. This was previously handled by storing the blobs into the 'blobpool limbo'. The code has now
      changed to store the full transaction in the limbo instead, but it might be confusing for code readers why
      we're not simply adding the types.Transaction we already have.

Code changes summary:

  • txpool.Transaction removed and all uses replaced by types.Transaction again
  • blobpool now stores types.Transaction instead of defining its own blobTx format for storage
  • the blobpool limbo now stores types.Transaction instead of storing only the blobs
  • checks to validate the presence/absence of the blob sidecar added in certain critical places

@karalabe
Copy link
Member

karalabe commented Aug 4, 2023

I think the PR is overall good, the missing places for validation is

  • Downloader, where blocks can arrive in pieces and we want to verify them without reconstructing the entire block first. Specifically, we should check after decoding the block bodies that there are no blob txs.
  • When parsing blobs from JSON (i.e. RPC, remote client, etc), we should somehow add this check.
  • When receiving transactions form the network, IMO we should also have a check there. This is a bit debatable because we have one in the txpool.

In general, I think the question is whether we want to validate the presence/absence of blocks at the usage sites or the entry sites. The PR tries to check for them when used, but it might be stricter if we validated them on entry point (rpc, network, etc). Or maybe I would do both, reject them on entry point as bad data and log.Error them later and reject them at usage points as programming error. That way we have a double check to make sure we didn't miss anything, but in theory should only ever hit it on bad user inpu.

@fjl fjl force-pushed the core-types-blobtx-blobs branch from d27eac9 to 1de750f Compare August 7, 2023 15:16
@fjl
Copy link
Contributor Author

fjl commented Aug 7, 2023

I have added a check in InsertReceiptChain to cover fast sync. Regarding checks for transactions from JSON-RPC, it's not necessary because the validation in the txpool catches those. Trying to keep the checks minimal here...

No need to keep track of the txhash in a variable when we are storing
the full tx.
core/block_validator.go Outdated Show resolved Hide resolved
core/types/tx_blob.go Outdated Show resolved Hide resolved
core/types/transaction.go Show resolved Hide resolved
core/block_validator.go Outdated Show resolved Hide resolved
@fjl fjl merged commit 2a6beb6 into ethereum:master Aug 14, 2023
@fjl fjl added this to the 1.13.0 milestone Aug 14, 2023
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
This PR removes the newly added txpool.Transaction wrapper type, and instead adds a way
of keeping the blob sidecar within types.Transaction. It's better this way because most
code in go-ethereum does not care about blob transactions, and probably never will. This
will start mattering especially on the client side of RPC, where all APIs are based on
types.Transaction. Users need to be able to use the same signing flows they already
have.

However, since blobs are only allowed in some places but not others, we will now need to
add checks to avoid creating invalid blocks. I'm still trying to figure out the best place
to do some of these. The way I have it currently is as follows:

- In block validation (import), txs are verified not to have a blob sidecar.
- In miner, we strip off the sidecar when committing the transaction into the block.
- In TxPool validation, txs must have a sidecar to be added into the blobpool.
  - Note there is a special case here: when transactions are re-added because of a chain
    reorg, we cannot use the transactions gathered from the old chain blocks as-is,
    because they will be missing their blobs. This was previously handled by storing the
    blobs into the 'blobpool limbo'. The code has now changed to store the full
    transaction in the limbo instead, but it might be confusing for code readers why we're
    not simply adding the types.Transaction we already have.

Code changes summary:

- txpool.Transaction removed and all uses replaced by types.Transaction again
- blobpool now stores types.Transaction instead of defining its own blobTx format for storage
- the blobpool limbo now stores types.Transaction instead of storing only the blobs
- checks to validate the presence/absence of the blob sidecar added in certain critical places
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Feb 26, 2024
PR 27841 made modifications to TxData - removed blob-specific functions,
and added encode and decode functions.

 Additions:
  core/types/arb_types.go:
Fixed out internal types to follow the new TxData interface.

 Conflicts:
  core/types/transaction.go
Conflicts in decodeTyped because decoding changed. Adapted out types to
the new way.

  core/types/tx_blob.go:
Our changes to TxData conflicted with the PRs. Applied both.

  miner/worker.go
both changed call to ApplyTransaction, applied both.
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.

4 participants