feat: Support eth_fillTransaction#12204
Conversation
hudem1
requested review from
LukaszRozmej,
benaadams,
smartprogrammer93 and
svlachakis
as code owners
|
Claude finished @hudem1's task in 4m 38s —— View job Code Review:
|
|
duplicate of #11552 ? |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude please check the following review Findings M1 — Expensive KZG proof computation runs on unbounded caller input before validation (DoS vector). FillBlobFields (EthRpcModule.cs:496 → 1298-1303) calls ComputeProofsAndCommitments over all caller-supplied blobTx.Blobs before ToTransaction(validateUserInput: true, …) at line 501 validates the blob count against the per-tx consensus limit. Post-PeerDAS that's 128 cell-proofs per blob; a caller can pack the request up to the JSON-RPC max body size (~hundreds of 128 KiB blobs) and force seconds of CPU per unauthenticated call before the tx is rejected as over-limit. eth_fillTransaction has no wallet/config gate (unlike eth_signTransaction's EnableEthSignTransaction), so it's reachable by any RPC client. Fix: validate the blob count (and cheap structural fields) before computing proofs — reject an over-limit blob set up front. M2 — Missing from silently fills a meaningless nonce instead of erroring. Address from = (rpcTx as LegacyTransactionForRpc)?.From ?? Address.Zero; (EthRpcModule.cs:465), then nonceless.Nonce = _txPool.GetLatestPendingNonce(from) (:469). If the caller omits from, the nonce is filled from the zero address's pending nonce — a wrong value returned as if authoritative. geth's setDefaults requires from to fill the nonce. Since the whole point of fill is "node fills what it authoritatively knows," a nonce derived from 0x000…0 is not that. Fix: if from is null and nonce is missing, fail with InvalidInput rather than defaulting the sender. L1 — Spec conformance: gasPrice not populated for typed (0x2/0x3/0x4) outputs. transaction.yaml lists gasPrice as required (DEPRECATED) in Transaction1559Unsigned (and Transaction2930Unsigned). The fill switch sets gasPrice only in the LegacyTransactionForRpc arm (0x0/0x1); for 1559+ it's never set. The output is produced by TransactionForRpc.FromTransaction(tx) without extraData (:516), and EIP1559TransactionForRpc.FromTransaction only sets GasPrice when extraData.BaseFee is not null (EIP1559TransactionForRpc.cs:35-36) — so the 1559 output will omit gasPrice. Also note the PR description claims "gasPrice — node — effective price (0x2)", which the implementation does not do. Verify against the actual FilledTransaction/fill.yaml schema in #803 (it may differ from the signed transaction.yaml); if that schema also marks gasPrice required for 1559, the result is non-conformant. Either way, reconcile the PR description with the code. L2 — sidecarComplete path trusts caller-supplied commitments/proofs without recomputation. When the caller supplies Commitments+Proofs+BlobVersionedHashes, FillBlobFields returns early (:1294-1296) and TryAttachSidecar only checks lengths + (optionally) hash match. Caller-supplied-but-inconsistent proofs pass through into the returned tx. Low severity (unsigned, not broadcast → caller only hurts itself), but worth a note since the method's contract is "node derives the sidecar." L3 — Result shape { tx } is correct for #803, which removed raw from fill results — matches FillTransactionResult. Note (out of scope): SignTransactionResult still carries Raw (:424); if #803 also drops raw from eth_signTransaction, that's a separate follow-up. Tests Good core coverage (1559 fill, legacy gasPrice, field preservation, contract-creation-without-data rejection, blob sidecar derivation). Gaps worth adding: 2930 and 7702 fills, blob tx missing to rejection, blob tx with no blobs rejection, and missing from (M2). The blob test manually calls KzgPolynomialCommitments.InitializeAsync() — confirm the production path guarantees KZG is initialized when eth_fillTransaction receives blobs on a blob-enabled chain, otherwise ComputeProofsAndCommitments can throw. |
|
Claude finished @svlachakis's task in 2m 4s —— View job Review check:
|
| ID | Verdict | Notes |
|---|---|---|
| M1 | ✅ Confirmed | Real ordering issue / DoS vector |
| M2 | ✅ Confirmed | from silently defaults to zero |
| L1 | gasPrice is populated; only the PR description wording is off |
|
| L2 | ✅ Valid (low) | caller sidecar not recomputed |
| L3 | ✅ Correct | { tx } is right per #803 |
| Tests | ✅ Valid gaps | KZG-init concern is addressed in prod |
M1 — KZG computation before blob-count validation — Confirmed
The ordering is exactly as described. FillBlobFields (EthRpcModule.cs:496) runs ComputeProofsAndCommitments over all caller-supplied blobTx.Blobs (:1298-1303), and the per-tx blob-count limit is only enforced later inside ToTransaction at :501 (→ BlobTransactionForRpc.cs:72-77, BlobFieldsTxValidator.ValidateBlobGasLimits against spec.MaxBlobsPerTx). Nothing bounds the blob count before the proof math. eth_fillTransaction has no wallet/config gate (eth_signTransaction requires EnableEthSignTransaction), so it's reachable by any RPC client — a valid unauthenticated-CPU concern, more acute post-PeerDAS (128 cell proofs/blob). Fix as suggested: validate Blobs.Length (and cheap structural fields) up front before computing proofs.
M2 — Missing from fills a meaningless nonce — Confirmed
Address from = (rpcTx as LegacyTransactionForRpc)?.From ?? Address.Zero; (:465) then nonceless.Nonce = _txPool.GetLatestPendingNonce(from) (:469). With from omitted, the returned nonce is Address.Zero's pending nonce, presented as authoritative. Since fill's contract is "node fills what it authoritatively knows," rejecting a missing from (with InvalidInput) when the nonce is absent is the right behavior and matches geth's setDefaults.
L1 — gasPrice on typed outputs — Mostly incorrect
The claim that the 1559 output omits gasPrice doesn't hold. EIP1559TransactionForRpc's constructor (EIP1559TransactionForRpc.cs:35-37) is a ternary with an else branch:
GasPrice = extraData.BaseFee is not null
? transaction.CalculateEffectiveGasPrice(eip1559Enabled: true, extraData.BaseFee.Value)
: transaction.MaxFeePerGas; // <-- fallback still sets GasPriceeth_fillTransaction calls TransactionForRpc.FromTransaction(tx) with no extraData, so BaseFee is null and GasPrice is set to MaxFeePerGas. So the typed output does carry gasPrice — it is not omitted, and the result is not non-conformant on that ground.
The residual valid point is only a doc/wording mismatch: the PR description says gasPrice — node — effective price (0x2), but the emitted value is maxFeePerGas, not the base-fee-adjusted effective price (no base fee is threaded into the output builder). Worth reconciling the PR text with the code, but no code change needed for conformance.
L2 — caller-supplied sidecar trusted — Valid (low)
Confirmed: when Commitments + Proofs + BlobVersionedHashes are all present, FillBlobFields returns early (:1294-1296) and TryAttachSidecar (BlobTransactionForRpc.cs:101-121) only checks lengths and (if hashes present) hash↔commitment consistency — proofs themselves aren't re-verified. Low severity as stated (unsigned, not broadcast), worth a comment given the "node derives the sidecar" contract.
L3 — { tx } shape — Correct
Agreed, and this supersedes the earlier review round's "Medium: missing raw" note: execution-apis#803 removed raw from fill/sign results, so returning only tx is correct and the prior Medium can be dismissed. SignTransactionResult.Raw remaining is a separate, out-of-scope follow-up.
Tests
Suggested gaps are reasonable (2930/7702 fills, blob-missing-to, blob-with-no-blobs, missing-from for M2).
On the KZG-init concern: the production path is safe on blob-enabled chains. InitializePrecompiles (Nethermind.Init/Steps/InitializePrecompiles.cs:21-31) calls KzgPolynomialCommitments.InitializeAsync at startup whenever GetFinalSpec().IsEip4844Enabled. The test's manual InitializeAsync() is only compensating for the lighter test harness; ComputeProofsAndCommitments won't throw for uninitialized KZG in a real blob-enabled node.
Bottom line
- M1 and M2 are real and should be addressed (or explicitly acknowledged) before merge.
- L1 is largely a false positive on the code (gasPrice is populated); only the PR description needs a wording fix.
- L2, L3, and the test gaps are accurate. L3 usefully retires the earlier
raw-field Medium.
Note: @LukaszRozmej flagged this may duplicate #11552 — worth resolving that before investing further in review fixes.
· branch hudem1/feat/eth-fill-tx
|
@claude check the following review 1 —
2 —
3 —
|
|
Claude finished @manusw7's task in 1m 23s —— View job Review check:
|
| ID | Verdict |
|---|---|
1 — SetCode to == null enforcement depends on gas |
✅ Confirmed |
| 2 — Partial sidecar overwrites caller-supplied fields | ✅ Confirmed |
3 — 2× blob base fee headroom |
💡 Valid optional suggestion |
1 — SetCode (0x4) to enforcement is inconsistent — Confirmed
The fill path has no explicit to != null check for SetCode, so validity flips on the unrelated gas field:
SetCodeTransactionForRpc.ToTransaction(SetCodeTransactionForRpc.cs:25-38) only adds the authorization list; it delegates toEIP1559TransactionForRpc.ToTransaction(EIP1559TransactionForRpc.cs:40-64), which validates only fee fields. Neither checksTo.- The only
To-null guard in the chain isLegacyTransactionForRpc.cs:99—To is null && Input is null or empty → ContractCreationWithoutData. A 7702 request withtoomitted but input present slips past it. - When
gasis omitted,eth_estimateGas(EthRpcModule.cs:473-482) exercisesTransactionProcessor.ValidateStatic → SetCodeTxValidation.ValidateNoContractCreation(TransactionProcessor.cs:830), so the fill correctly errors. - When the caller supplies
gas, estimation is skipped and nothing rejectsto == null— the method returns aTransaction7702Unsignedthat can never validate (EIP-7702 forbids contract creation;Transaction7702Unsignedin Remove context 801 #803 requiresto), contradicting the PR description's "caller-mandatorytofor 0x4".
Fix as suggested: an explicit To is null check for SetCode mirroring BlobTransactionForRpc.cs:66-67's MissingToInBlobTx, so behavior is independent of gas. Add a regression test with gas supplied.
2 — Partial sidecar silently overwrites caller fields — Confirmed
FillBlobFields (EthRpcModule.cs:1294-1303):
bool sidecarComplete = blobTx.Commitments is not null && blobTx.Proofs is not null && blobTx.BlobVersionedHashes is { Length: > 0 };
if (blobTx.Blobs is not { Length: > 0 } || sidecarComplete)
return;
// ...recomputes and REPLACES Commitments, Proofs, BlobVersionedHashessidecarComplete is all-or-nothing. If the caller supplies e.g. commitments but not proofs (or omits the hashes), the branch recomputes and overwrites the caller's commitments and blobVersionedHashes — fields that are part of the signed payload. This breaks the invariant that FillTransaction_WhenCallerSuppliesFields_LeavesThemUntouched asserts, and it sits directly next to the complete-sidecar path (:1294-1296 early-return + TryAttachSidecar) which trusts caller values unverified (the earlier L2 note) — two adjacent paths with opposite trust models.
Fix as suggested: either error on a partial sidecar (geth: blob commitments provided while proofs were not), or derive only the missing pieces and error when a supplied value mismatches the derived one. A partial-sidecar test would lock in whichever behavior is chosen.
3 — 2× blob base fee headroom — Valid optional suggestion
FillBlobFields sets MaxFeePerBlobGas = feePerBlobGas (1×, EthRpcModule.cs:1291), while the execution fee uses 2·baseFee + tip (:488). The claim checks out: the cap is a ceiling, not the amount paid (effective blob fee = blob base fee, burned), so a higher cap costs the sender nothing but avoids the filled tx going invalid if the blob base fee rises before inclusion. No spec mandates it and reth uses 1×, so it's a reasonable-but-optional consistency improvement, not a correctness bug. If applied, keep the PR description in sync.
Bottom line
- 1 and 2 are real and should be addressed (or explicitly acknowledged) before merge.
- 3 is a valid optional suggestion; fine as-is.
- Still open from prior rounds: M1 (KZG compute before blob-count validation), M2 (missing
fromfills a meaningless nonce), plus @wurdum's estimate-gas-before-FillBlobFieldsfor blob txs and thechainId/maxFeePerBlobGas-min-1 comments. And @LukaszRozmej's #11552 duplicate question is still worth resolving.
I only checked the review — no code changes were made (not requested).
· branch hudem1/feat/eth-fill-tx
- Require an explicit `from`; reject the silent zero-address default. - Reject a `chainId` that doesn't match the node (geth parity). - Derive blob fields before gas estimation so blob txs estimate correctly. - Default maxFeePerBlobGas to 2x the blob base fee and fail when it can't be computed, instead of emitting "0x0" (below the EIP-4844 minimum of 1). - Add regression tests for the from and chainId guards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@manusw7 thanks! Agreed — Pushed 0b47753 addressing the review feedback:
|
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Polimorphism > cascade of if checks
Replace the per-type if/switch cascade in eth_fillTransaction with virtual FillFeeDefaults / PrepareForGasEstimation methods on the TransactionForRpc hierarchy, driven by a small TxFillContext of node-computed defaults. Each tx type now populates the fee model it uses without the RPC layer branching on the concrete type. Also harden the blob path (moved into BlobTransactionForRpc): bound the blob count before the KZG work and validate caller-supplied blobVersionedHashes against the derived commitments instead of silently overwriting them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The camelCase naming policy already serializes Tx as "tx". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ation Fold the success and error cases into TestCaseSource-driven tests, and add coverage for the blob-count bound and the blob-hash mismatch rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // Every concrete tx type derives from LegacyTransactionForRpc, which carries the common | ||
| // from/nonce/chainId fields. geth/reth require an explicit sender: nonce and gas are | ||
| // meaningless without it, and defaulting to the zero address would fill the wrong account. | ||
| if (rpcTx is not LegacyTransactionForRpc baseTx || baseTx.From is null) | ||
| return ResultWrapper<FillTransactionResult>.Fail("from address not specified", ErrorCodes.InvalidInput); |
There was a problem hiding this comment.
maybe the parameter to this eth_fillTransaction should be of type LegacyTransactionForRpc, so we would eliminate other types just based on type system and eliminate this check? (Maybe worth applying to other methods with input too)
There was a problem hiding this comment.
I looked into it, but it breaks polymorphic deserialization: TransactionJsonConverter is bound to the abstract TransactionForRpc base and intentionally does not apply when the declared target type is a derived class — that is exactly what lets the converter internally call JsonSerializer.Deserialize(ref reader, concreteTxType, ...) without recursing. If the parameter were declared as LegacyTransactionForRpc, the converter would be bypassed and a blob/1559 request would deserialize as plain legacy, silently dropping its fields. It also would not hold for plugin tx types like Optimism's DepositTransactionForRpc, which derives from TransactionForRpc directly — same constraint applies to the other TransactionForRpc-taking methods. (What I did in 6bfd1ac: split the guard into distinct errors (transaction type X is not supported for filling vs from address not specified) so the failure modes are explicit.)
|
Skip on some comments |
…ased method Merge PrepareForGasEstimation and FillFeeDefaults into a single FillDefaults(in TxFillContext) returning Result, called before gas estimation to mirror geth's setDefaults order; estimation now runs with the effective fees, failing like geth when the sender cannot cover them. Split the unsupported-type and missing-from guards into distinct errors, and trim comments to the non-obvious invariants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7 participants
Fixes Closes Resolves #11374
Summary
Implements the
eth_fillTransactionJSON-RPC method (#11374) following ethereum/execution-apis#803.Given a partial transaction, the node fills in the missing fields (nonce, gas, fees, chain id, and the blob sidecar) and returns the unsigned transaction, ready to be signed and submitted. Unlike
eth_sendTransaction, it neither signs nor broadcasts — it is the "fill" half ofeth_sendTransaction(fill + sign + submit). Per #803 the result is{ tx }only; that PR removed therawfield from fill/sign results.Transaction types that can be filled
All five current transaction types are supported. Each maps to its
FilledTransactionoutput schema:0x0TransactionLegacyUnsigned0x1Transaction2930Unsigned0x2Transaction1559Unsigned0x3Transaction4844Unsigned/Transaction4844UnsignedWithSidecar0x4Transaction7702UnsignedWhich fields get filled
Fields fall into three groups: the node defaults/computes what it authoritatively knows, derives the blob sidecar from the caller's blobs, and treats a few fields as caller-mandatory — erroring rather than inventing them.
typenoncefromgaseth_estimateGasvalue0input0xgasPrice0x0,0x1,0x2maxFeePerGas/maxPriorityFeePerGasmaxFee = 2·baseFee + tip0x2,0x3,0x4maxFeePerBlobGas0x3accessList[]0x1,0x2,0x3,0x4chainIdto0x3,0x4blobVersionedHashesblobs0x3blobs/commitments/proofsblobs; node derivescommitments/proofs(proof shape per fork: 1 per blob pre-PeerDAS, 128 cell proofs per blob post-PeerDAS)0x3(with sidecar)authorizationList0x4Error behavior
eth_fillTransactionfails hard (-32000InvalidInput) instead of returning a partially-filled transaction when a required field cannot be filled or derived — e.g. contract creation without data, a blob/setcode tx missingto, or a blob tx with no blobs. This mirrors geth'ssetDefaultsand is required to keep the result conformant with theFilledTransactionschema.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Added
EthRpcModuleTests.FillTransaction.cscovering: EIP-1559 fill (nonce/gas/fees/chainId), legacygasPricefill, preservation of caller-supplied fields, contract-creation-without-data rejection, and blob sidecar derivation (commitments/proofs/versioned hashes) from caller-supplied blobs.Documentation
Requires documentation update
If yes, link the PR to the docs update or the issue with the details labeled
docs. Remove if not applicable.Requires explanation in Release Notes
If yes, fill in the details here. Remove if not applicable.
Remarks
Optional. Remove if not applicable.