Refactor sign tx by ak88 · Pull Request #11507 · NethermindEth/nethermind

ak88 · 2026-05-06T13:17:26Z

Refactor sign tx out of the consensus module and instead just listen for event inside the manager.

ak88 · 2026-05-06T13:17:44Z

claude · 2026-05-06T13:24:24Z

Claude finished @ak88's task in 2m 57s —— View job

Review: Refactor sign tx

Gather context
Read changed files
Analyze changes for correctness, security, and performance
Post findings

The refactor moves signing-transaction logic out of the XdcHotStuff consensus loop and into SignTransactionManager which now listens directly on BlockAddedToMain. The architecture change is sound — hooking the blockchain event is a better place for this than the tight consensus loop. However there is one critical behavioral regression introduced during the move, plus a few high/medium issues.

Critical

#	Location	Issue
1	`SignTransactionManager.cs:76`	Inverted `MergeSignRange` condition — old code signs when `Number % MergeSignRange == 0`; new code `return`s at that point and signs on every other block. Confirmed against `PenaltyHandler` and `XdcRewardCalculator` which both treat multiples of `MergeSignRange` as the action boundary. This is a pure refactor, so behavior must not change.

High

#	Location	Issue
2	`SignTransactionManager.cs:86`	Fire-and-forget swallows async exceptions. `_ = SubmitTransactionSign(...)` discards the returned `Task`. Signer failures, tx-pool rejections, and any exception inside will be silently lost. `robustness.md`: "Only omit await when fire-and-forget is the documented* intent."* Fix: chain `.ContinueWith(t => _logger.Error(...), OnlyOnFaulted)` or use a small async-safe wrapper.

Medium

#	Location	Issue
3	`SignTransactionManager.cs:71`	`ExtraConsensusData` not null-checked before `.BlockRound`. An unhandled `NullReferenceException` thrown from an event callback propagates into blockchain core code. `XdcHotStuff.OnNewHeadBlock` guards the same field explicitly — this handler should too.
4	`SignTransactionManager.cs:73`	`spec == null` should be `spec is null` per `coding-style.md`.

Low

#	Location	Issue
5	`SignTransactionManager.cs:31`	`_alreadySigned` is assigned once; should be `readonly`.
6	`XdcModule.cs:136`	`CreateSignTransactionManager` factory method is now dead code — the registration changed to `.AddSingleton<ISignTransactionManager, SignTransactionManager>()` which resolves the constructor automatically.

What's good

Subscribing to BlockAddedToMain is the right event for this — fires once per canonical block, cleanly decoupled from the consensus round loop.
Using AssociativeKeyCache<ValueHash256> for dedup is an improvement over the old monotonic _highestSignTxNumber (handles reorg scenarios correctly; the cache is thread-safe by design).
IDisposable.Dispose correctly unsubscribes the event handler, preventing leaks.
XdcHotStuff is now simpler and has one fewer dependency.

claude · 2026-05-06T13:26:06Z

Claude finished @ak88's task in 2m 7s —— View job

Review: Refactor sign tx

Read rule files
Read changed files
Analyze diff and post findings

The refactor moves signing-transaction logic out of XdcHotStuff and into SignTransactionManager via a BlockAddedToMain event subscription. The direction is good — the consensus runner shouldn't own this concern — but there are issues in the implementation.

High

SignTransactionManager.cs:86 — fire-and-forget poisons the deduplication cache

_alreadySigned.Set(xdcHeader.Hash) is called before the async sign attempt, then the task is discarded with _ = SubmitTransactionSign(...). If the signing throws (key unavailable, signer error, etc.) the exception is silently swallowed and the hash is already cached, so the block can never be retried on a subsequent BlockAddedToMain event. This is a silent, permanent miss. See inline comment for a fix sketch.

Medium

XdcModule.cs:136 — CreateSignTransactionManager factory is dead code

The registration was changed to .AddSingleton<ISignTransactionManager, SignTransactionManager>() (Autofac constructor injection), so this private factory is never invoked. Delete it.

ISignTransactionManager interface is now external-facing dead weight

After this refactor, nothing outside SignTransactionManager resolves ISignTransactionManager from the DI container or calls SubmitTransactionSign. The interface and its registration serve no abstraction purpose. Either:

Remove it and register SignTransactionManager as a concrete singleton (so the container constructs and manages its lifetime including IDisposable cleanup), or
Keep it if testability/mocking of the external call is still desired, but document why.

Low

SignTransactionManager.cs:73 — == null should be is null

Repo style requires is null / is not null. See inline comment.

Verdict: not yet mergeable. The High finding (silent exception + cache poisoning on sign failure) and the dead-code Medium findings should be addressed first.

claude · 2026-05-06T13:26:57Z

+        if (spec == null)
+            return;
+
+        if (xdcHeader.Number % spec.MergeSignRange == 0)


Critical: inverted signing condition

The old code in XdcHotStuff signed when Number % MergeSignRange == 0:

if (_highestSignTxNumber < roundParent.Number && (roundParent.Number % spec.MergeSignRange == 0)) { // sign }

PenaltyHandler and XdcRewardCalculator both confirm the semantic: actions occur at multiples of MergeSignRange. The new code does the opposite — it returns early (skips) at multiples and signs on every other block, which is a complete behavioral inversion.

Suggested change

if (xdcHeader.Number % spec.MergeSignRange == 0)

if (xdcHeader.Number % spec.MergeSignRange != 0)

return;

claude · 2026-05-06T13:27:02Z

+        if (IsMasternode(snapshot, _signer.Address))
+        {
+            _alreadySigned.Set(xdcHeader.Hash);
+            _ = SubmitTransactionSign(xdcHeader, spec);


High: fire-and-forget swallows async exceptions

_ = SubmitTransactionSign(...) discards the Task, so any exception thrown inside SubmitTransactionSign (signer failure, tx-pool error, etc.) is silently lost. Per robustness.md: "A missing await on an async call silently discards the result. Only omit await when fire-and-forget is the documented intent."

This handler fires on an event callback so it cannot be async directly — the conventional fix is a helper that logs on failure:

Suggested change

_ = SubmitTransactionSign(xdcHeader, spec);

_ = SubmitTransactionSign(xdcHeader, spec).ContinueWith(

t => _logger.Error("Failed to submit sign transaction", t.Exception),

System.Threading.Tasks.TaskContinuationOptions.OnlyOnFaulted);

claude · 2026-05-06T13:27:04Z

+        if (_alreadySigned.Contains(xdcHeader.Hash))
+            return;
+
+        ulong round = xdcHeader.ExtraConsensusData.BlockRound;


Medium: potential NullReferenceException

ExtraConsensusData is not null-checked before accessing .BlockRound. XdcHotStuff.OnNewHeadBlock throws explicitly when it's null; this event handler would throw an unhandled exception from the blockchain core instead.

Suggested change

ulong round = xdcHeader.ExtraConsensusData.BlockRound;

if (xdcHeader.ExtraConsensusData is null)

return;

ulong round = xdcHeader.ExtraConsensusData.BlockRound;

It is expected to be a xdc header

claude · 2026-05-06T13:27:06Z

+
+        ulong round = xdcHeader.ExtraConsensusData.BlockRound;
+        IXdcReleaseSpec spec = _specProvider.GetXdcSpec(xdcHeader, round);
+        if (spec == null)


Medium: use is null instead of == null

Per coding-style.md: "Use is null and is not null instead of == null and != null".

Suggested change

if (spec == null)

if (spec is null)

return;

claude · 2026-05-06T13:27:07Z

+    private readonly IBlockTree _blockTree;
+    private readonly ISnapshotManager _snapshotManager;
+    private readonly ISpecProvider _specProvider;
+    private AssociativeKeyCache<ValueHash256> _alreadySigned = new (128);


Low: field should be readonly

_alreadySigned is assigned once in the constructor and never reassigned; it should be readonly.

Suggested change

private AssociativeKeyCache<ValueHash256> _alreadySigned = new (128);

private readonly AssociativeKeyCache<ValueHash256> _alreadySigned = new(128);

claude · 2026-05-06T13:27:09Z


    private ISnapshotManager CreateSnapshotManager([KeyFilter(XdcRocksDbConfigFactory.XdcSnapshotDbName)] IDb db, IBlockTree blockTree, IMasternodeVotingContract votingContract, ISpecProvider specProvider) => new SnapshotManager(db, blockTree, votingContract, specProvider);
-    private ISignTransactionManager CreateSignTransactionManager(ISigner signer, ITxPool txPool, ILogManager logManager) => new SignTransactionManager(signer, txPool, logManager.GetClassLogger<SignTransactionManager>());
+    private ISignTransactionManager CreateSignTransactionManager(ISigner signer, ITxPool txPool, IBlockTree blockTree, ISnapshotManager snapshotManager, ISpecProvider specProvider, ILogManager logManager) => new SignTransactionManager(signer, txPool, blockTree, snapshotManager, specProvider, logManager.GetClassLogger<SignTransactionManager>());


Low: dead code

CreateSignTransactionManager is no longer called — the registration changed to .AddSingleton<ISignTransactionManager, SignTransactionManager>() which lets Autofac resolve the constructor directly. This method can be deleted.

claude · 2026-05-06T13:27:51Z

+        if (IsMasternode(snapshot, _signer.Address))
+        {
+            _alreadySigned.Set(xdcHeader.Hash);
+            _ = SubmitTransactionSign(xdcHeader, spec);


High — fire-and-forget swallows exceptions; failed sign is silently skipped forever

_alreadySigned.Set is called before the async sign attempt. If SubmitTransactionSign throws (signer unavailable, key error, etc.), the exception is swallowed and the hash remains in the cache — so this block can never be retried.

Because OnBlockAddedToMain is a synchronous event handler you cannot await here directly, but you should at minimum log the failure and avoid poisoning the cache:

Suggested change

_ = SubmitTransactionSign(xdcHeader, spec);

if (IsMasternode(snapshot, _signer.Address))

{

_alreadySigned.Set(xdcHeader.Hash);

_ = SubmitTransactionSign(xdcHeader, spec).ContinueWith(t =>

{

if (t.IsFaulted)

{

_alreadySigned.Delete(xdcHeader.Hash); // allow retry on next event

_logger.Error($"SubmitTransactionSign failed for block {xdcHeader.Hash}", t.Exception!.InnerException);

}

}, TaskContinuationOptions.OnlyOnFaulted);

}

(Or factor the fire-and-forget into a small async Task SignAsync(...) method with a try/catch that handles removal and logging, which avoids the lambda capturing overhead.)

if we fail to submit our transaction it is fine, just ignore it. Dont want to retry it.

claude · 2026-05-06T13:27:53Z

+
+        ulong round = xdcHeader.ExtraConsensusData.BlockRound;
+        IXdcReleaseSpec spec = _specProvider.GetXdcSpec(xdcHeader, round);
+        if (spec == null)


Low — style: use is null not == null

Per the repo coding style (coding-style.md): "Use is null and is not null instead of == null and != null".

Suggested change

if (spec == null)

if (spec is null)

claude · 2026-05-06T13:28:04Z


    private ISnapshotManager CreateSnapshotManager([KeyFilter(XdcRocksDbConfigFactory.XdcSnapshotDbName)] IDb db, IBlockTree blockTree, IMasternodeVotingContract votingContract, ISpecProvider specProvider) => new SnapshotManager(db, blockTree, votingContract, specProvider);
-    private ISignTransactionManager CreateSignTransactionManager(ISigner signer, ITxPool txPool, ILogManager logManager) => new SignTransactionManager(signer, txPool, logManager.GetClassLogger<SignTransactionManager>());
+    private ISignTransactionManager CreateSignTransactionManager(ISigner signer, ITxPool txPool, IBlockTree blockTree, ISnapshotManager snapshotManager, ISpecProvider specProvider, ILogManager logManager) => new SignTransactionManager(signer, txPool, blockTree, snapshotManager, specProvider, logManager.GetClassLogger<SignTransactionManager>());


Medium — dead code: this factory method is no longer wired to any DI registration

After the refactor, line 104 registers SignTransactionManager directly via Autofac constructor injection:

.AddSingleton<ISignTransactionManager, SignTransactionManager>()

This CreateSignTransactionManager factory is never called. Delete it.

Similarly, ISignTransactionManager is now only used internally within SignTransactionManager itself — nothing external resolves it from the container. Consider whether the interface (and the DI registration) still serves a purpose, or if SignTransactionManager can simply be registered as a concrete IDisposable singleton activated by the container. Fix this →

ak88 · 2026-05-06T14:08:25Z

@claude again

claude · 2026-05-06T14:09:03Z

Claude finished @ak88's task in 3m 19s —— View job

Re-review: Refactor sign tx

Gather context and author responses
Re-read changed files
Re-analyze remaining issues
Post findings

The previous review issues have been addressed — the inverted condition is fixed, is null is used, _alreadySigned is readonly, the dead factory method is deleted, and the fire-and-forget now logs on faulted (per ak88: no retry wanted). Both author clarifications are accepted.

However, a silent regression slipped in during the refactor of SubmitTransactionSign itself:

Critical: missing `transaction.Hash` assignment

File: SignTransactionManager.cs:49

The old code in master:

await signer.Sign(transaction);

transaction.Hash = transaction.CalculateHash();   // ← REMOVED

AcceptTxResult added = txPool.SubmitTx(transaction, TxHandlingOptions.PersistentBroadcast);

The new code drops that line entirely. Signer.Sign(Transaction tx) only sets tx.Signature; it does not compute or assign tx.Hash. TxPool.SubmitTx then dereferences tx.Hash! with null-forgiving operators at several points (_retryCache.Received(tx.Hash!), relevantPool.TryInsert(tx.Hash!, ...), etc.), so submitting with a null hash will throw a NullReferenceException and the pool insertion will fail entirely. TxSealer.Seal is the normal component that sets tx.Hash = tx.CalculateHash(), but it is not in this code path.

Fix — restore the assignment after signing:

await _signer.Sign(transaction);
transaction.Hash = transaction.CalculateHash();   // must recalculate after signature is set

AcceptTxResult added = _txPool.SubmitTx(transaction, TxHandlingOptions.PersistentBroadcast);

Fix this →

Everything else looks good. Once this one line is restored the PR is otherwise clean.

ak88 added the xdc label May 6, 2026

ak88 requested a review from batrr May 6, 2026 13:17

github-actions Bot added the refactoring label May 6, 2026

claude Bot reviewed May 6, 2026

View reviewed changes

ak88 added 5 commits May 6, 2026 16:03

refactor

5e005c4

merged

114b24d

fix

dd0d71e

fix

e5113b3

review comments

fee923a

ak88 force-pushed the refactor/sign-tx branch from 42e6dc8 to fee923a Compare May 6, 2026 14:07

ak88 added 2 commits May 7, 2026 16:17

DI hookup

9aa2b46

Merge branch 'master' into sign-tx2

8174551

	if (xdcHeader.Number % spec.MergeSignRange == 0)
	if (xdcHeader.Number % spec.MergeSignRange != 0)
	return;

-            _ = SubmitTransactionSign(xdcHeader, spec);
+            _ = SubmitTransactionSign(xdcHeader, spec).ContinueWith(
+                t => _logger.Error("Failed to submit sign transaction", t.Exception),
+                System.Threading.Tasks.TaskContinuationOptions.OnlyOnFaulted);

	private AssociativeKeyCache<ValueHash256> _alreadySigned = new (128);
	private readonly AssociativeKeyCache<ValueHash256> _alreadySigned = new(128);

-            _ = SubmitTransactionSign(xdcHeader, spec);
+            if (IsMasternode(snapshot, _signer.Address))
+            {
+                _alreadySigned.Set(xdcHeader.Hash);
+                _ = SubmitTransactionSign(xdcHeader, spec).ContinueWith(t =>
+                {
+                    if (t.IsFaulted)
+                    {
+                        _alreadySigned.Delete(xdcHeader.Hash); // allow retry on next event
+                        _logger.Error($"SubmitTransactionSign failed for block {xdcHeader.Hash}", t.Exception!.InnerException);
+                    }
+                }, TaskContinuationOptions.OnlyOnFaulted);
+            }

Conversation

ak88 commented May 6, 2026

Uh oh!

ak88 commented May 6, 2026

Uh oh!

claude Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: Refactor sign tx

Critical

High

Medium

Low

What's good

Uh oh!

claude Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review: Refactor sign tx

High

Medium

Low

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

ak88 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

ak88 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

ak88 commented May 6, 2026

Uh oh!

claude Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Re-review: Refactor sign tx

Critical: missing transaction.Hash assignment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

claude Bot commented May 6, 2026 •

edited

Loading

claude Bot commented May 6, 2026 •

edited

Loading

claude Bot commented May 6, 2026 •

edited

Loading

Critical: missing `transaction.Hash` assignment