Claude finished @svlachakis's task in 5m 40s —— View job

PR Review: Wallet Refactoring

• Gather context and read changed files
• Review IWallet.cs interface changes
• Review wallet implementations (DevWallet, DevKeyStoreWallet, ProtectedKeyStoreWallet, NullWallet)
• Review ClefSigner / ClefWallet changes
• Review EthRpcModule and PersonalRpcModule changes
• Review test changes
• Post findings

Summary

Good refactoring. The shift from exception-based Sign to the TrySign / TrySignTransaction / TrySignMessage pattern eliminates expensive exception-path allocations and makes failure handling explicit at every call site. Using ValueHash256 / ValueKeccak throughout avoids one Hash256 heap allocation per sign call. The logic is preserved correctly—the EIP-155 V computation (v + 35 + 2 * chainId for legacy, RecoveryId + 27 for typed) is identical to the old code.

One Medium finding (nullability contract gap in ClefWallet) and a couple of Low observations below.

Medium

ClefWallet is missing [NotNullWhen(true)] on both signing overrides

IWallet declares:

bool TrySign(in ValueHash256 message, Address address, [NotNullWhen(true)] out Signature signature);
bool TrySignMessage(byte[] message, Address address, [NotNullWhen(true)] out Signature signature)

ClefWallet implements both without the attribute:

// ClefWallet.cs:67
public bool TrySign(in ValueHash256 message, Address address, out Signature signature)

// ClefWallet.cs:83
public bool TrySignMessage(byte[] message, Address address, out Signature signature)

The [NotNullWhen(true)] contract established by the interface is not inherited by the concrete implementation for callsites that hold a ClefWallet reference directly. Every other concrete implementation (DevWallet, DevKeyStoreWallet, ProtectedKeyStoreWallet, NullWallet) correctly carries the attribute; ClefWallet is the odd one out.

Fix this →

Low

eth_sign error message is less informative than before

Old code propagated the SecurityException.Message to the client on AccountLocked failures:

catch (SecurityException e)
{
    return ResultWrapper<Signature>.Fail(e.Message, ErrorCodes.AccountLocked);
}

New code always returns a fixed message ("Unable to sign as {addressData}") with ErrorCodes.AccountLocked for all failures. This is more consistent, but callers that displayed the account-locked error message verbatim will now get a generic string. Acceptable for a refactoring PR, but worth noting.

Passphrase-based atomic sign path silently dropped (breaking API change not called out in PR description)

DevKeyStoreWallet.Sign(Hash256, Address, SecureString passphrase) previously allowed callers to unlock-and-sign atomically without a prior UnlockAccount call. The new TrySign requires the account to be pre-unlocked. Same for DevWallet.

This is a deliberate and reasonable design decision (the personal_sign / eth_sign flows always call UnlockAccount first, so no behaviour regression for RPC consumers). However, as a public IWallet API the passphrase overload was reachable from plugin code. It would be worth adding a line to the PR description noting the intentional removal.

Positive observations

• IWallet.TrySignTransaction / TrySignMessage as default interface implementations is clean; only ClefWallet overrides what it needs (transaction signing via account_signTransaction, message signing via raw bytes to let Clef handle EIP-191).
• [DoesNotReturn, StackTraceHidden, AggressiveInlining] on ThrowSignFailed is consistent with the existing pattern in ClefSigner.
• WalletTxSigner now throws InvalidOperationException instead of CryptographicException—this is a better fit since the failure is an operational state error, not a crypto algorithm failure.
• Test improvements (Assert.That(signed, Is.True, ...)) make failures self-describing.

@claude review again, eth_sign error message is fine, it's matching the geth and our own eth_signTransaction.

Claude finished @svlachakis's task in 2m 11s —— View job

PR Review: Wallet Refactoring

• Acknowledge eth_sign error message is intentional (matches geth / eth_signTransaction behaviour)
• Re-read changed files and check Medium finding status
• Post updated review

Summary

Good shape. The shift from exception-based Sign to TrySign/TrySignTransaction/TrySignMessage is clean, failure handling is explicit at every call site, and ValueHash256/ValueKeccak cuts one heap allocation per sign path. The EIP-155 V computation is preserved correctly.

Previous Medium finding resolved: ClefWallet now carries [NotNullWhen(true)] on both TrySign(in ValueHash256…) (line 67) and TrySignMessage(byte[]…) (line 83), matching the interface contract and the other implementations. ✓

eth_sign error message: Intentional — "authentication needed: password or unlock" matches geth and the existing eth_signTransaction response. ✓

Passphrase-based atomic sign removed: Deliberate breaking change, called out in the PR description. ✓

No remaining findings. PR is mergeable.

Claude finished @svlachakis's task in 5m 8s —— View job

PR Review: Wallet Refactoring

• Gather context and understand PR scope
• Read changed files and diff
• Review IWallet interface changes
• Review wallet implementations (DevWallet, DevKeyStoreWallet, ProtectedKeyStoreWallet, NullWallet)
• Review ClefSigner / ClefWallet changes
• Review EthRpcModule and PersonalRpcModule changes
• Review test changes
• Post findings

Summary

Clean refactoring. The shift from exception-based Sign to the TrySign/TrySignMessage/TrySignTransaction pattern is well-executed across all implementations. Failure handling is now explicit at every call site, and ValueHash256/ValueKeccak avoids one Hash256 heap allocation per sign call.

No blocking findings. Previous Medium finding (ClefWallet missing [NotNullWhen(true)]) is resolved — both TrySign(in ValueHash256...) (line 67) and TrySignMessage(byte[]...) (line 83) carry the attribute. EIP-155 V computation is preserved correctly: recid + 35 + 2 * chainId for legacy, recid + 27 for typed.

Correctness Checks

EIP-155 V adjustment (IWallet.TrySignTransaction, line 36) — SecP256k1.SignCompact returns recovery id 0/1; new Signature(rs, v) stores it as V = recid + 27. So sig.V + 8 + 2 * chainId = recid + 35 + 2 * chainId. Matches EIP-155. ✓

ClefWallet V handling — legacy path takes Clef's raw EIP-155 V as-is; typed path adds VOffset to Clef's 0/1 recid. Identical to old code. ✓

ClefWallet TrySignMessage override — sends raw bytes to Clef (not pre-hashed), letting Clef apply EIP-191 internally. Correct for the Clef protocol. ✓

No remaining callers of old Sign/SignMessage methods — all call sites updated. ✓

Low

null vs null! inconsistency across projects

DevWallet, DevKeyStoreWallet, and NullWallet assign signature = null on the failure branch; ClefWallet uses signature = null!. This is explained by the nullable context: Nethermind.Wallet does not opt into <Nullable>enable</Nullable>, while Nethermind.ExternalSigner.Plugin does. Not a correctness issue, but worth enabling nullable in Nethermind.Wallet at some point for consistency.

Rlp.Encode allocates in TrySignTransaction

// IWallet.cs:34
ValueHash256 hash = ValueKeccak.Compute(Rlp.Encode(tx, true, true, chainId).Bytes);

Rlp.Encode(…).Bytes allocates a byte[]. This was inherited from the old implementation. Transaction signing is not a hot loop, but if this path is ever profiled as a hotspot, a stack-allocated RlpStream would eliminate the allocation. Fine as-is for this PR.

Positive observations

• Default interface methods for TrySignMessage / TrySignTransaction keep concrete implementations focused — only ClefWallet overrides what needs Clef-specific behavior.
• [DoesNotReturn, StackTraceHidden, AggressiveInlining] on ThrowSignFailed in WalletTxSigner is consistent with the existing pattern in ClefSigner.
• WalletTxSigner now throws InvalidOperationException instead of relying on CryptographicException from the wallet layer — semantically correct, since this is an operational state failure, not a cryptographic algorithm failure.
• Assert.That(signed, Is.True, …) in test improvements makes assertion failures self-describing without any extra cost.
• eth_sign error message ("authentication needed: password or unlock") correctly matches geth and the existing eth_signTransaction response, as confirmed by @svlachakis.

PR is mergeable. No Critical, High, or Medium findings.

@claude review again, extended trysign/tryseal even more

Claude finished @svlachakis's task in 5m 2s —— View job

PR Review: Wallet Refactoring

• Read changed files and diff
• Review IWallet / ISigner interface changes
• Review wallet implementations
• Review ClefSigner / ClefWallet changes
• Review WalletTxSigner / TxPool changes
• Review RPC module changes
• Post findings

Summary

Good extension — the TrySign / exceptionless pattern is now cascaded all the way through ISigner : ITxSigner, ITxSealer, and the Xdc consensus layer. Architecture is sound.

One Medium finding and a couple of Low observations below.

Medium

eth_sendTransaction error message has an unexpected "SignFailed, " code prefix

AcceptTxResult.SignFailed.ToString() returns "SignFailed, authentication needed: password or unlock" because ToString() is implemented as $"{Code}, {Message}". Every other sign-failure path in EthRpcModule and PersonalRpcModule returns the bare string "authentication needed: password or unlock". The inconsistency breaks clients that check the exact message text.

See inline comment on EthRpcModule.cs:522. Fix: use the message literal directly for the SignFailed case instead of acceptTxResult.ToString().

Low

CliqueSealer.cs — redundant || signature is null after !signed

Per [NotNullWhen(true)], when TrySign returns false, signature is null! by contract (all implementations confirm this). The || signature is null guard adds noise without adding safety; !signed alone is sufficient.

// Simplified:
if (!signed)
    throw new InvalidOperationException($"Clique signer {_signer.Address} could not sign block {block.Number}.");

NullSigner.TrySign(BlockHeader...) is unreachable via interface

NullSigner implements ISigner, ISignerStore — it does not declare IHeaderSigner. The TrySign(BlockHeader header, ...) method therefore cannot be dispatched through any interface. It is only callable on a concrete NullSigner reference, making it effectively dead code in the current class hierarchy. Either add IHeaderSigner to the class declaration or remove the method.

Correctness Checks

ISigner : ITxSigner chain — Signer.TrySign(Transaction) is now required by ITxSigner (via ISigner). ClefSigner.TrySign(Transaction) still throws NotImplementedException, which was the pre-existing behavior and is correct since ClefSigner is a block signer, not a tx signer. ✓

AcceptTxResult.SignFailed ID — id 19 does not collide with any existing AcceptTxResult value (0–18, 503). ✓

TxPoolSender.SendTransaction with null hash — tx.Hash! null-forgiving operator is safe: callers only use txHash in the accepted path, never the SignFailed path. ✓

Eip191Hasher.HashMessageValue — HashMessage now wraps it, so existing callers are unaffected. The ValueHash256 return on HashMessageValue avoids one heap allocation per message sign. ✓

ClefWallet.TrySignMessage override — sends raw bytes to Clef (EIP-191 applied internally by Clef), while the default IWallet.TrySignMessage hashes locally. Behavior is preserved from the old SignMessage. ✓

SignTransactionManager async removal — correctly converts to sync Task.CompletedTask; the old await signer.Sign(tx) never actually awaited real async work. ✓

EIP-155 V — sig.V + 8 + 2 * _chainId for legacy (= recid + 35 + 2 * chainId) and recid + 27 for typed. Identical to pre-refactor. ✓

Positive Observations

• ISigner : ITxSigner cascade is clean — Signer and NullSigner implement TrySign(Transaction) naturally, no separate wrapper needed.
• Eip191Hasher.HashMessageValue returning ValueHash256 is a nice stack-allocation win.
• XdcSealer, VotesManager, TimeoutCertificateManager all throw InvalidOperationException on sign failure with actionable context (signer address, block number / round). Correct failure semantics for consensus paths.
• Tests updated to use Assert.That(TrySign(...), Is.True) — failures are now self-describing.

@claude review again

Claude finished @svlachakis's task in 4m 45s —— View job

PR Review: Wallet Refactoring

• Gather context and read changed files
• Check if prior findings were addressed
• Review latest diff for new issues
• Post findings

Summary

All three findings from the previous review round are resolved. No new Critical, High, or Medium findings.

Prior Findings — All Resolved

Medium: eth_sendTransaction error message had code prefix ✓

AcceptTxResult.SignFailed is now checked explicitly before falling through to ToString():

return acceptTxResult.Equals(AcceptTxResult.SignFailed)
    ? ResultWrapper<Hash256>.Fail("authentication needed: password or unlock", ErrorCodes.AccountLocked)
    : ResultWrapper<Hash256>.Fail(acceptTxResult?.ToString() ?? string.Empty, ErrorCodes.TransactionRejected);

The message is the bare "authentication needed: password or unlock" string, consistent with eth_sign and eth_signTransaction. ✓

Low: CliqueSealer redundant || signature is null guard ✓

Removed — only !signed is checked now, which is correct given the [NotNullWhen(true)] contract. ✓

Low: NullSigner.TrySign(BlockHeader...) was unreachable via interface ✓

NullSigner now declares IHeaderSigner directly: public class NullSigner : IHeaderSigner, ISignerStore. The TrySign(BlockHeader, ...) override is fully reachable. ✓

Correctness Checks (New Code)

Signer.TrySign(Transaction) — ValueKeccak.Compute(Rlp.Encode(...).Bytes) gives the same hash as the old Keccak.Compute(...). EIP-155 V adjustment (sig.V + 8 + 2 * _chainId for legacy, RecoveryId + 27 for typed) is identical to the old implementation. ✓

TxSealer.TrySeal — virtual bool TrySeal preserves overrideability; short-circuits with return false before CalculateHash() and Timestamp assignment when signing fails, so no partial state is written. ✓

TxPoolSender — AcceptTxResult.SignFailed (id 19) does not collide with existing ids (0–18, 503). EthRpcModule maps it to ErrorCodes.AccountLocked, consistent with the old SecurityException path. ✓

OptimismEthRpcModule — await sealer.Seal(...) correctly converted to if (!sealer.TrySeal(...)) return Fail(...) since TrySeal is now synchronous. ✓

PersonalRpcModule.personal_sign — Unlock-then-sign flow preserved; if the account is already unlocked UnlockAccount is skipped and TrySignMessage is called directly. A failed TrySignMessage (locked or unknown address) returns AccountLocked correctly. ✓

ClefWallet.TrySign(in ValueHash256) / TrySignMessage — Both maintain the same address.ToString() / address serialization difference as the old Sign(Hash256, Address) / SignMessage(byte[], Address) methods, preserving wire behavior with Clef. ✓

Observation (Pre-existing, not introduced by this PR)

Signer declares public bool CanSignHeader => false; but does not implement IHeaderSigner — the property is dead code on concrete Signer references. This predates the PR; no action needed here.

No Critical, High, or Medium findings. PR is mergeable.

@ak88 @cicr99 please review & confirm I didn't regressed anything in XDC

@ak88 @cicr99 please review & confirm I didn't regressed anything in XDC

Seems most consumers wants to throw if signing fails. What is the reason for the refactor?

@claude does completely exceptionless approach make sense? please review.

Claude finished @svlachakis's task in 4m 9s —— View job

PR Review: Wallet Refactoring — Completely Exceptionless Approach

• Gather context on the question
• Read current state of changed files
• Evaluate the exceptionless design question
• Review XdcSealer, VotesManager, TimeoutCertificateManager (consensus path)
• Review ISigner / IHeaderSigner / ITxSigner changes
• Review ClefSigner changes
• Check correctness of the failure propagation chain
• Post findings

Does completely exceptionless make sense? Yes.

The key architectural question is whether signing failures in consensus and wallet code are expected operational states or programmer errors. They are operational states — a locked account, missing key, or temporarily unavailable remote signer is not a bug. The TrySign / TrySeal pattern is correct here.

The previous concern ("if we're throwing in the end, was the refactor worth it?") was valid for the pre-b268681 state, where XdcSealer still threw InvalidOperationException. In the current state (b268681), none of the consensus paths throw on signing failure — they log and skip gracefully.

Correctness of the null-return path in XdcSealer

The critical question: XdcSealer.SealBlock now returns Task.FromResult<Block>(null) on failure. Does the block producer handle that correctly?

Yes. XdcBlockProducer extends BlockProducerBase without overriding ProduceNewBlock or the sealing continuation. BlockProducerBase at line 154 explicitly handles this:

if (t.Result is not null)
{
    Metrics.BlocksSealed++;
    return t.Result;
}
else
{
    Logger.Info($"Failed to produce block ... (null seal)");
    Metrics.FailedBlockSeals++;
}

The null return is handled, the metric is tracked, and the Info log records it. This matches how AuRaSealer and CliqueSealer already behave — they both return null on failure and the base class handles it. ✓

Where exceptions still appear (and why that's correct)

These are all legitimate "this-should-not-happen" scenarios — the right type of case for exceptions.

Findings

Low — XdcSealerTests has no coverage for the new failure path

The only test (SealBlock_ShouldSignXdcBlockHeader) covers the success path. The new behavior — "signer has no key → SealBlock returns null rather than throwing" — has no test. Since this is a behavior change from the pre-refactor code (which threw), a regression test would be valuable:

[Test]
public async Task SealBlock_WithNoKey_ReturnsNull()
{
    XdcSealer sealer = new(new Signer(0, (PrivateKey?)null, NullLogManager.Instance), NullLogManager.Instance);
    Block block = Build.A.Block.WithHeader(Build.A.XdcBlockHeader().TestObject).TestObject;
    Block? result = await sealer.SealBlock(block, CancellationToken.None);
    Assert.That(result, Is.Null);
}

Add test →

Low — VotesManager.CastVote silently returns on sign failure

// VotesManager.cs:70
if (!TrySign(vote)) return Task.CompletedTask;

No log at Warn or higher when a vote cannot be signed. For XDC consensus participation, a silent skip means the node silently stops voting. At minimum a Warn log (matching XdcSealer's pattern) would help operators diagnose a key configuration problem.

Positive observations

• BlockProducerBase null-seal handling is correct: FailedBlockSeals metric + Info log.
• VotesManager.TrySign and TimeoutCertificateManager.SendTimeout correctly skip without throwing.
• SignTransactionManager.SubmitTransactionSign logs at Warn on failure — good.
• Convenience Sign() wrappers retained on ISigner / IHeaderSigner / ITxSealer for callers that assert must-succeed — this is the correct C# TryXxx / assertion wrapper pattern.
• AcceptTxResult.SignFailed → ErrorCodes.AccountLocked correctly propagates signing failure to RPC callers.

No Critical, High, or Medium findings. PR is mergeable. The exceptionless approach is well-designed — it matches the existing AuRa/Clique sealer pattern, is handled correctly by the base block producer, and eliminates exception allocations on the failure path without losing observability.

@claude review again

Claude finished @svlachakis's task in 2m 40s —— View job

PR Review: Wallet Refactoring

• Gather context and read prior review history
• Read changed files and latest diff
• Check if prior findings were addressed
• Review latest changes for new issues
• Post findings

Summary

All findings from the previous round are resolved. The latest commit (02cd2a4) addresses both Low findings and extends the graceful-degradation pattern to two more components.

Prior Findings — All Resolved

Low: XdcSealerTests had no test for the null-key failure path ✓

SealBlock_WhenSignerHasNoKey_ReturnsNull added in XdcSealerTests.cs. The test correctly creates a Signer with a null PrivateKey, calls SealBlock, and asserts the result is null, matching the BlockProducerBase null-seal contract.

Low: VotesManager.CastVote silently returned on sign failure ✓

Warn log added: $"XDC signer {_signer.Address} could not sign vote for block {blockInfo.Hash} (round {blockInfo.Round}) — skipping broadcast." Operators will now see a diagnostic when the node stops voting due to a key problem.

New Changes (Not Prior Findings)

TimeoutCertificateManager — constructor now takes ILogManager; SendTimeout logs Warn on sign failure: "XDC signer … could not sign timeout for round … — skipping broadcast." Same pattern as VotesManager. DI resolution is automatic via AddSingleton<ITimeoutCertificateManager, TimeoutCertificateManager>() in XdcModule — Autofac constructor-scans the concrete type and injects ILogManager from the container. ✓

GeneratedTxSource — changed from Select(…) + Seal() to Where(…) + TrySeal(). Failed-seal generated transactions are now skipped with a Warn log instead of propagating an exception up through the block producer. Correct. ✓

VotesManagerTests / VoteTests — both updated to pass NullLogManager.Instance to the new VotesManager constructor parameter. GeneratedTxSourceTests updated to mock TrySeal instead of Seal. ✓

No Critical, High, or Medium findings. PR is mergeable.