Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ public XdcBlockHeaderBuilder WithExtraConsensusData(ExtraFieldsV2 extraFieldsV2)
return this;
}

public new XdcBlockHeaderBuilder WithTimestamp(ulong timestamp)
{
TestObjectInternal.Timestamp = timestamp;
return this;
}

public XdcBlockHeaderBuilder WithValidator(Signature signature)
{
XdcTestObjectInternal.Validator = signature.Bytes.ToArray();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ protected override ContainerBuilder ConfigureContainer(ContainerBuilder builder,
.AddSingleton((_) => BlockProducer)
//.AddSingleton((_) => BlockProducerRunner)
.AddSingleton<IRewardCalculator, ZeroRewardCalculator>()
.AddSingleton<ITimestamper>((_) => Timestamper)
.AddSingleton<ITimestamper>((ctx) => ctx.Resolve<ManualTimestamper>())
.AddSingleton<IBlockProducerRunner, XdcHotStuff>()

.AddSingleton<ITxPool>((ctx) =>
Expand Down
72 changes: 72 additions & 0 deletions src/Nethermind/Nethermind.Xdc.Test/SignTransactionManagerTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
// SPDX-License-Identifier: LGPL-3.0-only

using System;
using Nethermind.Blockchain;
using Nethermind.Consensus;
using Nethermind.Core;
using Nethermind.Core.Crypto;
using Nethermind.Core.Specs;
using Nethermind.Core.Test.Builders;
using Nethermind.Logging;
using Nethermind.TxPool;
using Nethermind.Xdc.Spec;
using Nethermind.Xdc.Types;
using NSubstitute;
using NUnit.Framework;

namespace Nethermind.Xdc.Test;

[Parallelizable(ParallelScope.All)]
internal class SignTransactionManagerTests
{
// window = MergeSignRange(15) * MinePeriod(2) * MaxSignableBlockPeriods(2) = 60s.
[TestCase(0L, true)]
[TestCase(60L, true)]
[TestCase(61L, false)]
[TestCase(86_400L, false)]
public void OnBlockAddedToMain_SignsOnlyRecentHeadBlocks(long secondsBehind, bool shouldSign)
{
IXdcReleaseSpec spec = Substitute.For<IXdcReleaseSpec>();
spec.MergeSignRange.Returns(15);
spec.MinePeriod.Returns(2);

ISpecProvider specProvider = Substitute.For<ISpecProvider>();
specProvider.GetSpec(Arg.Any<ForkActivation>()).Returns(spec);

ManualTimestamper timestamper = new();

XdcBlockHeader header = Build.A.XdcBlockHeader()
.WithNumber(spec.MergeSignRange)
.WithTimestamp((ulong)(timestamper.UnixTime.SecondsLong - secondsBehind))
.WithExtraConsensusData(new ExtraFieldsV2(1, new QuorumCertificate(new BlockRoundInfo(Hash256.Zero, 0, 0), null, 0)))
.TestObject;
Block block = new(header);

ISigner signer = Substitute.For<ISigner>();
signer.Address.Returns(TestItem.AddressA);
signer.TrySign(Arg.Any<Transaction>()).Returns(true);

ITxPool txPool = Substitute.For<ITxPool>();
txPool.SubmitTx(Arg.Any<Transaction>(), Arg.Any<TxHandlingOptions>()).Returns(AcceptTxResult.Accepted);

IBlockTree blockTree = Substitute.For<IBlockTree>();
blockTree.WasProcessed(block.Number, block.Hash!).Returns(true);
blockTree.FindBestSuggestedHeader().Returns(header); // bestSuggested == head => not syncing

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — since blockTree is an NSubstitute mock, IsSyncing() returns default (i.e. isSyncing: false) without any explicit setup, so the FindBestSuggestedHeader().Returns(header) call doesn't actually influence the test outcome. The comment // bestSuggested == head => not syncing implies a behavioral dependency that doesn't exist for a mock. The line could be removed, or if you want to document the intent, add a comment explaining IsSyncing() returns false by default on a mock.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to previous review — the previous comment called this a no-op, but it is load-bearing. IBlockTree.IsSyncing() is a static extension method that calls blockTree.FindBestSuggestedHeader()?.Number ?? 0. Without this stub, the mock returns null, giving bestSuggestedNumber = 0, and isSyncing = (0 == 0) = true — all shouldSign = true cases would fail. The comment here is accurate; the stub should stay.

blockTree.Head.Returns(block);

ISnapshotManager snapshotManager = Substitute.For<ISnapshotManager>();
snapshotManager.GetSnapshotByBlockNumber(Arg.Any<long>(), Arg.Any<IXdcReleaseSpec>())
.Returns(new Snapshot(block.Number, TestItem.KeccakA, [TestItem.AddressA]));

SignTransactionManager manager = new(
new Lazy<ISigner>(() => signer),
new Lazy<ITxPool>(() => txPool),
blockTree, snapshotManager, specProvider, timestamper, LimboLogs.Instance);
manager.Start();

blockTree.BlockAddedToMain += Raise.EventWith(new BlockReplacementEventArgs(block));

txPool.Received(shouldSign ? 1 : 0).SubmitTx(Arg.Any<Transaction>(), Arg.Any<TxHandlingOptions>());
}
}
7 changes: 7 additions & 0 deletions src/Nethermind/Nethermind.Xdc/SignTransactionManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ internal class SignTransactionManager(
IBlockTree blockTree,
ISnapshotManager snapshotManager,
ISpecProvider specProvider,
ITimestamper timestamper,
ILogManager logManager) : ISignTransactionManager, IStartable, IDisposable
{
// Lazy: ISigner and ITxPool are registered during InitializeBlockchain, after this class is instantiated.
Expand All @@ -34,6 +35,7 @@ internal class SignTransactionManager(
private readonly IBlockTree _blockTree = blockTree;
private readonly ISnapshotManager _snapshotManager = snapshotManager;
private readonly ISpecProvider _specProvider = specProvider;
private readonly ITimestamper _timestamper = timestamper;
private readonly ILogger _logger = logManager.GetClassLogger<SignTransactionManager>();
private readonly AssociativeKeyCache<ValueHash256> _alreadySigned = new(128);

Expand Down Expand Up @@ -76,6 +78,11 @@ private void OnBlockAddedToMain(object? sender, BlockReplacementEventArgs e)
if (spec is null)
return;

// Sign only recent head blocks; older ones are replayed during catch-up.
long window = spec.MergeSignRange * spec.MinePeriod * XdcConstants.MaxSignableBlockPeriods;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lowwindow == 0 is a silent failure mode: if either MergeSignRange or MinePeriod is ever 0, timestamp + 0 < now is true for every past block and signing is silently disabled. Both are always positive on XDC mainnet so this is not a production risk, but worth a brief guard or a comment to protect against future misconfiguration:

Suggested change
long window = spec.MergeSignRange * spec.MinePeriod * XdcConstants.MaxSignableBlockPeriods;
// Sign only recent head blocks; older ones are replayed during catch-up.
// window > 0 guard: if MinePeriod or MergeSignRange is 0, skip would match everything.
long window = spec.MergeSignRange * spec.MinePeriod * XdcConstants.MaxSignableBlockPeriods;
if (window > 0 && (long)xdcHeader.Timestamp + window < _timestamper.UnixTime.SecondsLong)
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we can probably extend the window here. All headers in current epoch should be signable.

if ((long)xdcHeader.Timestamp + window < _timestamper.UnixTime.SecondsLong)
return;

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

Expand Down
3 changes: 3 additions & 0 deletions src/Nethermind/Nethermind.Xdc/XdcConstants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,7 @@ internal static class XdcConstants

// 4-byte selector + 32-byte block number + 32-byte block hash
public const int SignTransactionDataLength = 68;

// Only sign recent head blocks.
public const int MaxSignableBlockPeriods = 2;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low — the comment says "during sync" but the IsSyncing() check on line 70 of SignTransactionManager already handles in-progress sync. This constant guards the subtler case: a node that was offline for a while restarts, IsSyncing() settles to false, but BlockAddedToMain fires for the trailing backlog of historical blocks. Suggest:

Suggested change
public const int MaxSignableBlockPeriods = 2;
// Guard against signing stale blocks replayed on restart (IsSyncing() handles active sync separately).
public const int MaxSignableBlockPeriods = 2;

}
Loading