RPC: add eth_signTransaction#11517
Conversation
|
Claude finished @svlachakis's task in 6m 26s —— View job PR Review:
|
| Check | Result |
|---|---|
Raw encoding flags (SkipTypedWrapping | InMempoolForm) match eth_sendRawTransaction decode flags |
✅ Consistent |
| Fee cap applies before signing | ✅ |
Sidecar attached before _wallet.Sign (signature preserved over NetworkWrapper) |
✅ |
SecurityException from wallet caught and surfaced as auth error |
✅ |
Type promotion Legacy → EIP1559 copies From correctly |
✅ |
tx.Hash = tx.CalculateHash() called after signing |
✅ |
ByteArrayConverter (global) handles Raw byte[] → hex serialization |
✅ |
FromTransaction(tx) echo returns runtime subtype (LegacyTransactionForRpc or EIP1559TransactionForRpc) |
✅ Verified by test |
svlachakis
changed the title
eth_signTransaction implementationeth_signTransaction
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude review again |
|
Claude finished @svlachakis's task in 6m 7s —— View job PR Review:
|
| Finding | Resolution |
|---|---|
| H1 — Proofs count not length-validated | ✅ Replaced per-field length checks with manager.ValidateLengths(wrapper) which validates all three arrays in one call (BlobProofsManagerV0:38–42) |
| M1 — Missing EIP-1559 nonce test | ✅ [TestCase(TxType.EIP1559, "nonce", ...)] added at EthRpcModuleTests.SignTransaction.cs:32 |
| M2 — No AccessList signing test | ✅ AccessList covered in both error-path and happy-path round-trip tests |
| M3 — Silent inheritance dependency on cast | ✅ Comment added at EthRpcModule.cs:367–368 |
L1 — IsTypeDefaulted public getter without documentation |
✅ Doc comment added in TransactionForRpc.cs:52–59 explaining getter/setter visibility |
| L2 — Blob count vs BlobVersionedHashes not cross-validated | ✅ manager.ValidateHashes(wrapper, tx.BlobVersionedHashes) now cross-checks |
New findings
Medium
M1 — PromoteToEip1559IfDefaultLegacy promotion branch is dead code
EthRpcModule.cs:430–448
The new EIP1559TransactionForRpc { ... } block at the end of PromoteToEip1559IfDefaultLegacy is unreachable:
IsTypeDefaulted = trueis set only whenTransactionJsonConverter.GetDefaultType()is called (notype, nogasPrice, no fee discriminators in JSON)GetDefaultType()is hardcoded to returntypeof(EIP1559TransactionForRpc)- Therefore,
IsTypeDefaulted = truealways means the runtime type is alreadyEIP1559TransactionForRpc - The guard
rpcTx is AccessListTransactionForRpc or EIP1559TransactionForRpcalways fires and returns early before the promotion block
Additionally, any isDefaulted = true request would already be rejected by HasFeeFields() (no fee fields present), so the function never even runs in that case. If the intent was to auto-upgrade typeless Legacy txs to EIP-1559, the converter already handles this. The dead block should either be removed or documented with a comment explaining the triggering scenario.
Low
L1 — Blob gas excluded from fee cap
EthRpcModule.cs:455
CheckTxFeeCap evaluates maxFeePerGas * gasLimit only. For blob transactions, the user also pays maxFeePerBlobGas * blobCount * bytesPerBlobGas. A blob tx with low execution gas but very high blob gas could bypass the cap check. This matches geth's reference implementation, so it's acceptable — but worth a comment noting the intentional scope.
L2 — Combined null check gives imprecise error message
EthRpcModule.cs:478–479
"blobs, commitments and proofs must all be provided" fires when only one of commitments or proofs is null. Splitting into two separate checks ("commitments must be provided alongside blobs" / "proofs must be provided alongside blobs") would give callers better diagnostics without any logic change — the length parity is validated below by manager.ValidateLengths(wrapper).
Correctness cross-checks (re-verified)
| Check | Result |
|---|---|
ValidateLengths checks blobs/commitments/proofs count parity |
✅ BlobProofsManagerV0:38–42 |
ValidateHashes called with null guard (always non-null after ToTransaction()) |
✅ |
Encoding flags match eth_sendRawTransaction decode flags |
✅ |
| Fee cap applied before signing | ✅ |
SecurityException caught and surfaced as auth error |
✅ |
Type promotion guards cover Blob and SetCode via AccessListTransactionForRpc inheritance |
✅ |
IsTypeDefaulted doc comment correctly explains cross-assembly visibility |
✅ |
|
@claude review again, added one more feature for geth partity |
|
Claude finished @svlachakis's task in 3m 36s —— View job PR Review:
|
| Finding | Resolution |
|---|---|
| H1 (r1) — Proofs count not length-validated | ✅ manager.ValidateLengths(wrapper) |
| M1 (r1) — Missing EIP-1559 nonce test | ✅ [TestCase(TxType.EIP1559, "nonce", ...)] added |
| M2 (r1) — No AccessList signing test | ✅ AccessList covered in error-path and round-trip tests |
| M3 (r1) — Silent inheritance dependency on cast | ✅ Comment added |
L1 (r1) — IsTypeDefaulted public getter |
✅ Replaced by HasExplicitType with doc comment |
| L2 (r1) — Blob count vs BlobVersionedHashes not cross-validated | ✅ manager.ValidateHashes |
M1 (r2) — PromoteToEip1559IfDefaultLegacy dead code |
✅ Method replaced by PromoteLegacyToEip1559IfTypeImplicit using HasExplicitType — the promotion path is now reachable and correct |
| L1 (r2) — Blob gas excluded from fee cap undocumented | ✅ Explanatory comment added |
| L2 (r2) — Combined null check gave imprecise error message | ✅ Split into separate Commitments is null / Proofs is null checks |
New feature: HasExplicitType geth-parity promotion
The HasExplicitType flag is set by DeriveTxType when the JSON contains an explicit "type" field (hasExplicitType = true). PromoteLegacyToEip1559IfTypeImplicit uses it to auto-promote typeless gasPrice-only requests to EIP-1559 — matching geth's eth_signTransaction behavior. The logic is clean and the test SignTransaction_LegacyShapeJson_RespectsExplicitTypePinning exercises both paths (pinned legacy stays legacy; typeless legacy promotes to EIP-1559).
Remaining findings
Low
L1 — or EIP1559TransactionForRpc in the promotion guard is a dead branch
EthRpcModule.cs:436
if (rpcTx is AccessListTransactionForRpc or EIP1559TransactionForRpc) return rpcTx;Since EIP1559TransactionForRpc : AccessListTransactionForRpc, the second pattern is never evaluated — every EIP1559TransactionForRpc already matches AccessListTransactionForRpc. As written, a reader unfamiliar with the hierarchy might infer these are sibling classes. Either drop the redundant arm or add a comment:
// EIP1559, Blob, and SetCode all inherit from AccessListTransactionForRpc — no promotion needed.
if (rpcTx is AccessListTransactionForRpc) return rpcTx;L2 — HasExplicitType doc comment doesn't cover the programmatic construction case
TransactionForRpc.cs:60–68
The doc says "True when the JSON request contained an explicit type field." Objects constructed in C# code (not deserialized from JSON) will always have HasExplicitType = false, which means any LegacyTransactionForRpc built in code passed through eth_signTransaction would be promoted. The flag is only meaningful on deserialized instances. A one-line caveat in the doc comment (/// <remarks>Always <c>false</c> for programmatically constructed instances; only set during JSON deserialization.</remarks>) would prevent misuse by future consumers of this public property.
Correctness cross-checks
| Check | Result |
|---|---|
HasExplicitType = true when JSON has "type": "0x0" |
✅ DeriveTxType:176 |
HasExplicitType = false when JSON omits type but has gasPrice → promotion fires |
✅ |
All prior tx types (AccessList, EIP1559, Blob, SetCode) inherit AccessListTransactionForRpc — guard covers them |
✅ |
BlobTransactionForRpc sidecar (Commitments/Proofs) are WhenWritingNull — not echoed in tx field (correct for geth parity) |
✅ |
RpcTxFeeCap default 1 ETH documented with units in config XML |
✅ |
ulong cap type matches 1e18 default without overflow |
✅ |
Status: mergeable. No Critical, High, or Medium findings remain. The two Low items above are polish suggestions and don't block merge.
|
@claude review again |
|
Claude finished @svlachakis's task in 3m 48s —— View job PR Review:
|
| Finding | Resolution |
|---|---|
L1 (r3) — or EIP1559TransactionForRpc redundant arm in promotion guard |
✅ Removed; guard is now just rpcTx is AccessListTransactionForRpc |
L2 (r3) — HasExplicitType doc missing programmatic-construction caveat |
✅ <remarks> block now covers this explicitly |
New findings
Low
L1 — SetCode (EIP-7702) transaction not covered in round-trip or missing-field tests
src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.SignTransaction.cs
The happy-path round-trip (SignTransaction_WhenValid_RawRoundTripsAndTxEcho) and the missing-field test both cover Legacy, AccessList, and EIP1559. SetCodeTransactionForRpc : EIP1559TransactionForRpc is registered in TransactionJsonConverter and would reach eth_signTransaction; it requires AuthorizationList to be non-null in ToTransaction(). A minimal round-trip case or at least a missing-AuthorizationList error test would confirm the signing path works end-to-end for type 4 and guard against future regressions.
Correctness cross-checks (re-verified)
| Check | Result |
|---|---|
or EIP1559TransactionForRpc dead arm removed from promotion guard |
✅ |
HasExplicitType doc covers programmatic construction (false by default) |
✅ |
SetCode is EIP1559TransactionForRpc subclass → caught by AccessListTransactionForRpc guard |
✅ |
tx.SenderAddress set by ToTransaction() before _wallet.Sign(tx, chainId) |
✅ |
_wallet.Sign is void; SecurityException is correctly caught |
✅ |
InternalsVisibleTo placed in source file — consistent with project pattern (e.g. LogIndexFilterVisitor.cs) |
✅ |
FormatWeiAsEther centi cast (ulong)centi safe since centi = x % 100 ∈ [0, 99] |
✅ |
HasExplicitType / IsTypeDefaulted serve distinct purposes — no redundancy |
✅ |
Status: mergeable. No Critical, High, or Medium findings. The one Low item above is a polish suggestion and does not block merge.
|
@claude review my refactoring, I think it's better approach and cleaner |
|
Claude finished @svlachakis's task in 5m 20s —— View job PR Review:
|
| Finding | Resolution |
|---|---|
| r4 L1 — SetCode not covered | ✅ [TestCase(TxType.SetCode, ...)] added to both round-trip and missing-field tests |
| All r1–r3 findings | ✅ Carried forward from previous reviews |
Refactoring assessment
The approach is cleaner and architecturally correct on all three axes:
1. PromoteToEip1559IfTypeDefaulted moved to domain model (TransactionForRpc)
Moving the promotion logic from EthRpcModule to TransactionForRpc is the right call — the promotion is intrinsic to how a transaction understands its own type. EthRpcModule now just calls rpcTx.PromoteToEip1559IfTypeDefaulted() at line 374 without knowing the policy details. ✅
2. IsTypeDefaulted stays internal — better than prior public getter
In r1/r2, IsTypeDefaulted had temporarily been widened to public bool ... { get; internal set; } to allow cross-assembly reads from EthRpcModule. Moving the method to the same assembly as IsTypeDefaulted (both now in Nethermind.Facade) eliminates that need entirely — the property stays internal and no public API surface was permanently expanded. ✅
3. GetDefaultType static local inlined
The GetDefaultType(out bool isDefaulted) helper existed solely to set a flag that could have been set inline. Removing it and replacing with a direct isDefaulted = true + inlined fallback is clearly simpler. The new comment "No explicit type field — every branch below is defaulted" makes the logic self-documenting. ✅
4. isDefaulted = true for gasPrice-discriminated paths (intentional behavior change)
Previously, only the "no discriminators at all" fallback set isDefaulted = true. Now, ANY path without an explicit type field sets isDefaulted = true, including the gasPrice discriminator path. This is the geth-parity change that makes typeless gasPrice-only requests auto-promote to EIP-1559 in eth_signTransaction — and it's tested explicitly in SignTransaction_LegacyShapeJson_RespectsExplicitTypePinning. ✅
New findings
Low
L1 — Third guard in PromoteToEip1559IfTypeDefaulted is dead given current hierarchy
TransactionForRpc.cs:101
if (this is AccessListTransactionForRpc) return this; // catches AccessList + all descendants
if (this is not LegacyTransactionForRpc legacy) return this; // ← never firesSince all registered subclasses are either LegacyTransactionForRpc or a descendant of AccessListTransactionForRpc : LegacyTransactionForRpc, any instance that passes line 100 must already be LegacyTransactionForRpc. The return this branch on line 101 is unreachable today. It's harmless defense-in-depth, but the comment on line 98 ("this single check excludes everything that's already a typed tx") could be expanded to note line 101 is a guard against hypothetical future subtypes:
// EIP-1559, Blob, and SetCode all derive from AccessListTransactionForRpc, so this single
// check excludes everything already typed. The LegacyTransactionForRpc guard below handles
// any future direct subtype that might be added without inheriting from AccessList.L2 — isDefaulted = true also fires when "type": null is present
TransactionForRpc.cs:204
// No explicit "type" field — every branch below is "defaulted".
isDefaulted = true;The comment says "no explicit type field", but this path is also reached when "type": null appears in the JSON (the property exists, Deserialize<TxType?> returns null, the setType is not null guard is skipped). In the previous code isDefaulted = false was set unconditionally at the top, so "type": null would produce isDefaulted = false. Now it produces isDefaulted = true, enabling promotion.
This is a minor edge-case behaviour change for malformed input ("type": null is not a valid JSON-RPC transaction); in practice it makes the handler more permissive, which is fine. But the comment could acknowledge this: "No valid explicit type field — either absent or null".
Correctness cross-checks
| Check | Result |
|---|---|
PromoteToEip1559IfTypeDefaulted called before ToTransaction — fee-field check evaluates original type |
✅ HasFeeFields at line 360 runs on pre-promotion tx |
gasPrice-only → EIP1559 promotion: MaxFeePerGas = MaxPriorityFeePerGas = gasPrice |
✅ lines 112–113 |
IsTypeDefaulted = true for gasPrice path enables promotion, explicit "type":"0x0" pins Legacy |
✅ tested by SignTransaction_LegacyShapeJson_RespectsExplicitTypePinning |
Discriminator-typed txs (Blob, SetCode) still guarded by AccessListTransactionForRpc check — no promotion |
✅ |
IsTypeDefaulted stays internal — no new public API surface |
✅ |
ResolveType pre-Berlin downgrade: gasPrice tx now has isDefaulted = true, but Type ?? default returns TxType.Legacy anyway — same result |
✅ |
| SetCode round-trip test added | ✅ |
| All prior findings from r1–r4 retained | ✅ |
Status: mergeable. No Critical, High, or Medium findings. Both Low items are comment/documentation polish and don't affect correctness or safety.
|
@claude review again, refactored it again it was wrong before - broke the existing use case |
|
Claude finished @svlachakis's task in 3m 34s —— View job PR Review:
|
| JSON shape | isDefaulted | Concrete type |
|---|---|---|
Explicit "type" field |
false |
type-mapped |
Discriminator field (accessList, maxFeePerGas, …) |
false |
discriminator-matched |
gasPrice only (no discriminators) |
true |
LegacyTransactionForRpc |
| Nothing at all | true |
EIP1559TransactionForRpc |
The "gasPrice → isDefaulted = true" change is a no-op for ToTransaction() (Legacy type is still Legacy regardless), and enables the geth-parity auto-promotion to EIP-1559 in eth_signTransaction. The new Istanbul test case in SpecAwareResolutionCases explicitly verifies the downgrade-is-a-no-op invariant. ✅
Correctness cross-checks
| Check | Result |
|---|---|
Discriminator-matched txs (accessList, maxFeePerGas, …) are NOT promoted in eth_signTransaction |
✅ isDefaulted = false for these paths |
gasPrice-only tx → promoted to EIP-1559 (geth parity) |
✅ isDefaulted = true → PromoteToEip1559IfTypeDefaulted fires |
gasPrice-only tx on pre-Berlin spec in eth_call/eth_estimateGas → still Legacy |
✅ ResolveType returns TxType.Legacy regardless |
{"accessList":[]} on Istanbul spec → AccessList, not downgraded to Legacy |
✅ isDefaulted = false now |
PromoteToEip1559IfTypeDefaulted guards cover all current subclasses via AccessListTransactionForRpc |
✅ |
IsTypeDefaulted stays internal (no new public API surface) |
✅ |
| All r1–r5 findings carried forward | ✅ |
Remaining findings
Low
L1 — Third guard in PromoteToEip1559IfTypeDefaulted still dead (carried from r5)
TransactionForRpc.cs:101
if (this is AccessListTransactionForRpc) return this;
if (this is not LegacyTransactionForRpc legacy) return this; // ← never firesEvery registered subclass is either LegacyTransactionForRpc or a descendant of AccessListTransactionForRpc : LegacyTransactionForRpc. Any instance that clears line 100 is a plain LegacyTransactionForRpc, so line 101's return this is unreachable today. The existing comment on lines 98–99 explains the first guard but doesn't state that line 101 is intentional defence-in-depth. A brief addition makes the intent clear to the next reader:
// The second guard below is unreachable today but guards against a future direct LegacyTransactionForRpc
// subtype that doesn't inherit from AccessListTransactionForRpc.
if (this is not LegacyTransactionForRpc legacy) return this;Status: mergeable. The r5 regression is correctly fixed, no Critical/High/Medium findings remain. The one Low item above is comment polish only.
|
@claude review again, made a minor fix with the cast based on your finding. is it functionally equilevant? |
|
Claude finished @svlachakis's task in 1m 34s —— View job PR Review:
|
| Path | Concrete type returned | Reaches cast? |
|---|---|---|
gasPrice-only, no discriminators |
LegacyTransactionForRpc |
✅ Yes — cast succeeds trivially |
| Absolute default (no type, no gasPrice, no discriminators) | EIP1559TransactionForRpc |
❌ No — caught by AccessListTransactionForRpc guard |
The is not LegacyTransactionForRpc check was dead code (flagged as r6 L1); the (LegacyTransactionForRpc) cast is the cleaner expression of the same invariant.
Behavioural difference (hypothetical only): If a future type were ever registered with IsTypeDefaulted = true and it didn't descend from either AccessListTransactionForRpc or LegacyTransactionForRpc, the old code would silently return this (no promotion), while the new code throws InvalidCastException. The hard cast is actually preferable — it fails loudly rather than silently doing the wrong thing.
Status
No Critical, High, or Medium findings remain. All prior findings from r1–r6 are resolved. The two prior Low items have also been addressed:
| Finding | Resolution |
|---|---|
r6 L1 — Dead is not LegacyTransactionForRpc guard |
✅ Replaced with direct cast; comment updated to explain why the cast is safe |
Status: mergeable. No blocking findings.
|
@claude review again, I reverted it back you are actually wrong on your finding. The is not LegacyTransactionForRpc legacy line earns its keep by binding legacy for use below - its no-op branch is just a free safety net, no exception ceremony. |
|
Claude finished @svlachakis's task in 42s —— View job PR Review:
|
| Finding | Resolution |
|---|---|
r8 L2 — SignTransactionResult used = null! suppressors |
✅ Fixed: both properties now use required keyword — compiler enforces initialization at the single call site in eth_signTransaction |
| r8 L1 — Blob happy-path has no round-trip test | ℹ️ Acknowledged by author as too complex; error paths (ValidateLengths, ValidateHashes, null checks) are covered; correctness not affected |
Correctness cross-checks (final)
| Check | Result |
|---|---|
required byte[] Raw / required TransactionForRpc Tx — enforced by compiler |
✅ |
SignTransactionResult always fully initialized at call site (EthRpcModule.cs:406–410) |
✅ |
| All r1–r8 findings carried forward | ✅ |
| No new code added in this commit | ✅ |
Status: mergeable. No Critical, High, or Medium findings. All Low items from previous reviews are resolved or explicitly acknowledged with rationale.
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
just FYI: we deliberately didn't implement that in the past as we wanted to avoid managing user keys and security rusks that come from it
|
@LukaszRozmej fair point - looking more carefully, the signing capability itself is already exposed via
Maybe ship it as:
Geth ships this already without any of these guardrails, so we'd end up stricter than them. WDYT? |
+1 |
|
@claude re-review |
|
Claude finished @LukaszRozmej's task in 3m 56s —— View job PR Re-review:
|
| Suggestion | Status |
|---|---|
BlobTransactionForRpc copy constructor: populate new props |
✅ Blobs/Commitments/Proofs already populated from NetworkWrapper at BlobTransactionForRpc.cs:43–48 |
(ulong)1.Ether for RpcTxFeeCap default |
✅ Already in JsonRpcConfig.cs:71 |
FormatWeiAsEther alternative (ToDecimal path) |
✅ Acknowledged — F2 + InvariantCulture preserves Geth wire format across locales |
ErrorCodes.AccountLocked for SecurityException |
✅ Acknowledged — author chose InvalidInput for Geth parity |
New findings from discussion thread
The comment thread raised unresolved security concerns that were discussed and agreed upon but not yet implemented.
Medium
M1 — No opt-in config flag: eth_signTransaction is always enabled when the Eth module is active
src/Nethermind/Nethermind.JsonRpc/IJsonRpcConfig.cs
LukaszRozmej noted that signing was deliberately omitted historically due to key management security risks. svlachakis then proposed shipping it "off by default behind a config flag", and alexb5dh explicitly +1'd that specific guardrail. However, no EnableEthSignTransaction (or equivalent) config item was added. The method is unconditionally exposed to any caller who can reach the Eth module.
Concretely: a default Nethermind node with JsonRpc.Enabled = true exposes eth_signTransaction to anyone who can reach port 8545. Unlike eth_sendTransaction, the signed payload exits the node without being broadcast — it can be stored and replayed at any later time. svlachakis correctly identified this as the primary marginal risk over existing endpoints. An opt-out (or opt-in) flag was the agreed mitigation.
// IJsonRpcConfig.cs
[ConfigItem(Description = "Whether to enable eth_signTransaction. Disabled by default due to key management security risks.", DefaultValue = "false")]
bool EnableEthSignTransaction { get; set; }Then in EthRpcModule.eth_signTransaction:
if (!_rpcConfig.EnableEthSignTransaction)
return ResultWrapper<SignTransactionResult>.Fail("eth_signTransaction is disabled", ErrorCodes.MethodNotFound);Low
L1 — eth_signTransaction not added to MethodsLoggingFiltering
src/Nethermind/Nethermind.JsonRpc/JsonRpcConfig.cs:54–62
Engine methods (engine_newPayloadV*, engine_forkchoiceUpdated*) are filtered from Trace-level request logging to prevent payload bytes appearing in log files. svlachakis explicitly proposed the same treatment for eth_signTransaction ("auto-filtered from request/response logging like the engine methods"). The signed raw bytes (and the full transaction fields) currently appear in Trace logs when Trace is enabled. While Trace is off in production by default, the filter is standard practice for sensitive signing endpoints.
// JsonRpcConfig.cs MethodsLoggingFiltering default
"engine_newPayloadV1",
"engine_newPayloadV2",
"engine_newPayloadV3",
"engine_forkchoiceUpdatedV1",
"engine_forkchoiceUpdatedV2",
"flashbots_validateBuilderSubmissionV3",
"eth_signTransaction" // ← addL2 — No INFO-level audit log on sign
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs:407
eth_sign (line 325) logs at Trace. svlachakis proposed an INFO-level log entry per eth_signTransaction call (from address + tx hash) to create an auditable trail — especially meaningful since signed bytes leave the node without appearing in the mempool. A single line after tx.Hash = tx.CalculateHash() would suffice:
if (_logger.IsInfo) _logger.Info($"eth_signTransaction: signed tx {tx.Hash} for {tx.SenderAddress}");Correctness cross-checks (re-verified against current code)
| Check | Result |
|---|---|
| All prior technical findings (r1–r9) | ✅ Carried forward |
BlobTransactionForRpc copy constructor populates Blobs/Commitments/Proofs |
✅ BlobTransactionForRpc.cs:43–48 |
RpcTxFeeCap = (ulong)1.Ether |
✅ JsonRpcConfig.cs:71 |
FormatWeiAsEther uses F2 + InvariantCulture |
✅ Intentional Geth parity |
ErrorCodes.InvalidInput on SecurityException |
✅ Intentional Geth parity |
eth_sign uses ErrorCodes.AccountLocked — deliberate inconsistency accepted |
✅ Acknowledged |
Status: not yet mergeable. M1 (opt-in config flag) reflects explicit team consensus from the discussion that was never implemented. L1 and L2 are the remaining guardrails svlachakis proposed; both are Low and don't block merge on their own.
| if (result is not null) | ||
| { | ||
| result.IsTypeDefaulted = isDefaulted; | ||
| } |
There was a problem hiding this comment.
The downgrade is a pre-Berlin compatibility guard. ResolveType checks IsTypeDefaulted == true && !spec.IsEip2930Enabled and forces the type to Legacy, since pre-Berlin EVMs reject typed (≥ 0x01) txs outright. The flag is set whenever the JSON request omitted the type field - covers both the gasPrice-routing path (which is already Legacy → no-op downgrade) and the absolute fallback (EIP1559 default → actually downgraded to Legacy). Explicit type: "0x2" or discriminator-routed types (accessList/maxFeePerGas) keep IsTypeDefaulted == false and pass through unchanged. Pre-Berlin chains are mostly dead, but for eth_call/eth_estimateGas against historical heights this keeps the result spec-correct.
| try | ||
| { | ||
| _wallet.Sign(tx, chainId); | ||
| } | ||
| catch (SecurityException) | ||
| { | ||
| return ResultWrapper<SignTransactionResult>.Fail("authentication needed: password or unlock", ErrorCodes.InvalidInput); | ||
| } |
There was a problem hiding this comment.
Could we have exceptionless TrySign?
There was a problem hiding this comment.
I tried but it's too much for this PR.
Wallet API has Sign + TrySign duplication and the catch only handles SecurityException (Clef throws InvalidOperationException, NullWallet returns null differently). A proper fix needs an IWallet redesign.
One redesign approach is:
single Sign(Hash256, Address) returning null, TrySignTransaction virtual on the interface for Clef's account_signTransaction override, SignMessage as extension. That's a larger refactor with cascading impact across IWallet impls and other RPC handlers; better as a focused follow-up than bundled here.
Will open a separate issue/PR.
This reverts commit 441e37a.
|
@claude review again |
|
Claude finished @svlachakis's task in 5m 4s —— View job PR Re-review:
|
| Change | Status |
|---|---|
LukaszRozmej's HasFeeFields property-pattern refactor |
✅ Applied in e05bc24 — matches suggestion exactly |
LukaszRozmej's MultiplyOverflow overflow guard on fee cap |
✅ Applied in e05bc24 |
LukaszRozmej's TryAttachBlobSidecar + ValidateBlobSidecarFields split |
✅ Applied in e05bc24 — matches suggestion exactly |
TrySign on IWallet |
↩️ Applied in 441e37a, reverted in b956806 — acknowledged as too complex for this PR; follow-up issue planned |
| Exception catch expanded to cover all wallet failure modes | ✅ 0f37644 — now catches SecurityException, InvalidOperationException, CryptographicException |
| M1 (r10) — No opt-in config flag | ↓ Downgraded to Low — LukaszRozmej APPROVED on 2026-05-09 after the full config-flag discussion (2026-05-08), so his approval constitutes implicit acceptance of the current state |
Exception catch expansion: correct
EthRpcModule.cs:407
catch (Exception ex) when (ex is SecurityException or InvalidOperationException or CryptographicException)The three-way catch is correct and well-documented. Each arm maps to a specific wallet implementation:
SecurityException— keystore wallets when account is lockedInvalidOperationException— ClefWallet on remote-signer rejectionCryptographicException— defaultIWallet.Sign(Transaction, ulong)whenSign(Hash256, Address)returns null (e.g. NullWallet)
The ex variable is declared but not used (only the pattern is evaluated) — this is the correct C# idiom for a filtered catch without needing the exception value. ✅
Remaining findings
Low
L1 — No opt-in EnableEthSignTransaction config flag
The guardrail proposed by svlachakis in the discussion thread (off-by-default flag, +1'd by alexb5dh) is still unimplemented. However, LukaszRozmej APPROVED the PR with awareness of this proposal, so this is confirmed as a follow-up suggestion rather than a hard prerequisite. The account-unlock check (_wallet.IsUnlocked(from)) already gates access. The config flag is defense-in-depth.
L2 — eth_signTransaction not added to MethodsLoggingFiltering
JsonRpcConfig.cs:54–62
Engine methods are filtered from Trace-level logs to prevent payload bytes appearing in log files. eth_signTransaction was proposed to receive the same treatment (signed raw bytes appear in Trace when Trace is enabled). Adding "eth_signTransaction" to the default list would prevent accidental key-material exposure in verbose logs. Low priority since Trace is off in production.
L3 — No INFO-level audit log per sign
Proposed by svlachakis: a single INFO log line after tx.Hash = tx.CalculateHash() (from address + tx hash) would create an auditable trail, especially meaningful since signed bytes leave the node without entering the mempool. eth_sign (line 325) logs at Trace; a higher-visibility level would be appropriate for signing operations.
Correctness cross-checks (full re-verification)
| Check | Result |
|---|---|
| Exception catch covers all three wallet failure modes | ✅ EthRpcModule.cs:407 |
LukaszRozmej's HasFeeFields pattern (EIP1559 checks both MaxFeePerGas and MaxPriorityFeePerGas) |
✅ EthRpcModule.cs:423–425 |
MultiplyOverflow + totalFee > capWei (overflow treated as cap-exceeded) |
✅ EthRpcModule.cs:439 |
ValidateBlobSidecarFields extracted; null checks use switch expression |
✅ EthRpcModule.cs:472–479 |
IsTypeDefaulted = true only for gasPrice-only and absolute default — no pre-Berlin regression |
✅ TransactionForRpc.cs:189,201 |
PromoteToEip1559IfTypeDefaulted — is not LegacyTransactionForRpc legacy binding is load-bearing for GasPrice access below |
✅ TransactionForRpc.cs:84 |
| All prior technical findings from r1–r9 | ✅ Carried forward |
Status: mergeable. No Critical, High, or Medium findings. The three Low items above are unimplemented guardrails proposed in the discussion thread — given LukaszRozmej's APPROVE with full awareness of the discussion, these are confirmed as post-merge follow-ups rather than blockers.
|
@claude review again |
|
Claude finished @svlachakis's task in 2m 59s —— View job PR Re-review:
|
| Finding | Resolution |
|---|---|
r10 L1 — No opt-in EnableEthSignTransaction config flag |
✅ bool EnableEthSignTransaction { get; set; } added to IJsonRpcConfig.cs/JsonRpcConfig.cs (default false); guard added at top of eth_signTransaction returning MethodNotFound when disabled |
r10 L2 — eth_signTransaction not in MethodsLoggingFiltering |
✅ Added to default list in both IJsonRpcConfig.cs DefaultValue string and JsonRpcConfig.cs array initializer |
| r10 L3 — No INFO-level audit log per sign | ✅ _logger.Info($"eth_signTransaction signed tx {tx.Hash} from {tx.SenderAddress}") added at EthRpcModule.cs:417 after CalculateHash() |
Tests correctly updated to set ctx.Test.RpcConfig.EnableEthSignTransaction = true in all three test helpers — the new default-off behavior is properly reflected.
New findings
Low
L1 — PromoteToEip1559IfTypeDefaulted() is public API without a doc comment
src/Nethermind/Nethermind.Facade/Eth/RpcTransaction/TransactionForRpc.cs:79
The coding-style rules require doc comments for all public APIs. PromoteToEip1559IfTypeDefaulted is now public on TransactionForRpc, but IsTypeDefaulted is internal — meaning for any externally constructed instance the method is always a no-op (since IsTypeDefaulted defaults to false). Callers outside the assembly may be surprised that calling it has no effect. A <summary> and <remarks> would close that gap:
/// <summary>
/// If this transaction's type was inferred by fallback during JSON deserialization (gasPrice-only
/// or no discriminators), re-wraps it as an <see cref="EIP1559TransactionForRpc"/>.
/// Otherwise returns <c>this</c> unchanged.
/// </summary>
/// <remarks>
/// Only effective on instances deserialized by <see cref="TransactionJsonConverter"/>.
/// Programmatically constructed instances have <see cref="IsTypeDefaulted"/> = <c>false</c>
/// and will always be returned unchanged.
/// </remarks>
public TransactionForRpc PromoteToEip1559IfTypeDefaulted()L2 — Overflow branch in CheckTxFeeCap logs an undefined fee value
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs:444–448
if (UInt256.MultiplyOverflow(perGas, (UInt256)tx.GasLimit, out UInt256 totalFee) || totalFee > capWei)
{
return ResultWrapper<SignTransactionResult>.Fail(
$"tx fee ({FormatWeiAsEther(totalFee)} ether) exceeds the configured cap ...",When MultiplyOverflow returns true, totalFee holds the truncated (wrapped) result of the multiplication — a meaningless value. The error message will display that garbage number as if it were the actual fee, which confuses callers trying to understand how far they exceeded the cap. The other MultiplyOverflow call sites in the codebase (e.g. TransactionProcessor.cs:658, OptimismTransactionProcessor.cs:109) never use the out value in the overflow path. A minimal fix:
bool overflow = UInt256.MultiplyOverflow(perGas, (UInt256)tx.GasLimit, out UInt256 totalFee);
if (overflow || totalFee > capWei)
{
string feeStr = overflow ? "overflow" : FormatWeiAsEther(totalFee);
return ResultWrapper<SignTransactionResult>.Fail(
$"tx fee ({feeStr} ether) exceeds the configured cap ({FormatWeiAsEther(capWei)} ether)",
ErrorCodes.InvalidInput);
}Correctness cross-checks (full re-verification)
| Check | Result |
|---|---|
EnableEthSignTransaction = false by default — eth_signTransaction returns MethodNotFound when disabled |
✅ EthRpcModule.cs:362–363 |
eth_signTransaction in MethodsLoggingFiltering default — signed raw bytes suppressed from Trace logs |
✅ JsonRpcConfig.cs:62, IJsonRpcConfig.cs:143 |
| INFO-level audit log after signing: hash + sender | ✅ EthRpcModule.cs:417 |
Exception catch covers SecurityException, InvalidOperationException, CryptographicException |
✅ EthRpcModule.cs:410 |
DeriveTxType: isDefaulted = true only for gasPrice-only and absolute default; discriminator-matched types keep isDefaulted = false |
✅ TransactionForRpc.cs:187–202 |
PromoteToEip1559IfTypeDefaulted guard covers all current subclasses via AccessListTransactionForRpc |
✅ TransactionForRpc.cs:83 |
| Fee cap overflow treated as cap-exceeded | ✅ EthRpcModule.cs:444 |
Blob sidecar: ValidateBlobSidecarFields + ValidateLengths + ValidateHashes |
✅ EthRpcModule.cs:459–474 |
Sidecar attached before _wallet.Sign |
✅ EthRpcModule.cs:395–400 |
tx.Hash = tx.CalculateHash() after signing |
✅ EthRpcModule.cs:415 |
SkipTypedWrapping | InMempoolForm encode flags consistent with eth_sendRawTransaction decode path |
✅ |
Tests updated: all helpers set EnableEthSignTransaction = true |
✅ EthRpcModuleTests.SignTransaction.cs:180,188,196 |
| All r1–r10 findings carried forward | ✅ |
Status: mergeable. No Critical, High, or Medium findings. Both Low items are documentation/UX polish and don't affect correctness or safety.
|
@claude review again, L1 is no go, its not public api as you claim |
|
Claude finished @svlachakis's task in 1m 47s —— View job PR Re-review:
|
| Check | Result |
|---|---|
EnableEthSignTransaction = false by default; guard returns MethodNotFound when disabled |
✅ EthRpcModule.cs:362–363, IJsonRpcConfig.cs:175 |
eth_signTransaction in MethodsLoggingFiltering default — signed bytes suppressed from Trace logs |
✅ IJsonRpcConfig.cs:143 |
| INFO audit log after signing: hash + sender | ✅ EthRpcModule.cs:417 |
Fee cap overflow guard — wraparound multiplication treated as cap-exceeded, error string uses "overflow" |
✅ EthRpcModule.cs:444–449 (a7d0f5b) |
HasFeeFields property-pattern covers EIP1559 (both fields) and Legacy (GasPrice) |
✅ EthRpcModule.cs:428–430 |
Exception catch covers SecurityException / InvalidOperationException / CryptographicException |
✅ EthRpcModule.cs:410 |
SignTransactionResult uses required — compiler-enforced initialization |
✅ SignTransactionResult.cs:12,15 |
IsTypeDefaulted = true only for gasPrice-only and absolute default; discriminator-matched types keep false |
✅ TransactionForRpc.cs:189,201 |
PromoteToEip1559IfTypeDefaulted — is not LegacyTransactionForRpc legacy binding load-bearing for GasPrice |
✅ TransactionForRpc.cs:84 |
Blob sidecar: ValidateBlobSidecarFields null checks + ValidateLengths + ValidateHashes |
✅ EthRpcModule.cs:458–473 |
Sidecar attached before _wallet.Sign; signing only sets tx.Signature, wrapper survives |
✅ EthRpcModule.cs:395–400 |
tx.Hash = tx.CalculateHash() after signing |
✅ EthRpcModule.cs:415 |
SkipTypedWrapping | InMempoolForm flags consistent with eth_sendRawTransaction decode path |
✅ |
| All findings from r1–r11 carried forward | ✅ |
Status: mergeable. No Critical, High, or Medium findings. Both Low items from r11 are now closed — L1 disputed correctly, L2 fixed in a7d0f5b.
3 participants
Fixes Closes Resolves #11516
Changes
eth_signTransactionimplementationTypes of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
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.