-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
Conversation
I think the PR is overall good, the missing places for validation is
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. |
Only blob transactions can have blobs, so there is no need to have these methods on every single tx type.
Seems better to have a variable for the iteration item.
d27eac9
to
1de750f
Compare
I have added a check in |
No need to keep track of the txhash in a variable when we are storing the full tx.
Also fixes a pretty serious bug where the WithoutBlobSidecar method didn't actually remove the sidecar...
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
…reum#27841)" This reverts commit 278a4af.
…reum#27841)" This reverts commit 278a4af.
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.
This PR removes the newly added
txpool.Transaction
wrapper type, and instead adds a way of keeping the blob sidecar withintypes.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 ontypes.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:
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 bytypes.Transaction
againtypes.Transaction
instead of defining its ownblobTx
format for storagetypes.Transaction
instead of storing only the blobs