Unify types with Geth#11937
Conversation
…aturating subtraction
|
I disagree with cutting. Yes it is cumbersome but IMO it is all-or-nothing refactor. |
just to clarify, I meant to say cutting it up into smaller PRs is cumbersome, not this PR itself. I'm also in favour of keeping this single PR |
# Conflicts: # src/Nethermind/Nethermind.Synchronization/FastSync/StateSyncPivot.cs # src/Nethermind/Nethermind.Synchronization/FastSync/StateSyncRunner.cs
…imit and fix sync test
# Conflicts: # src/Nethermind/Nethermind.Consensus/Stateless/WitnessGeneratingHeaderFinder.cs # src/Nethermind/Nethermind.JsonRpc.Test/Modules/DebugRpcModuleTests.ExecutionWitness.cs
…y check failures
# Conflicts: # src/Nethermind/Nethermind.Db/FlatDbConfig.cs # src/Nethermind/Nethermind.JsonRpc.Test/Modules/DebugRpcModuleTests.TraceCallMany.cs # src/Nethermind/Nethermind.Merge.Plugin.Test/SszRest/SszMiddlewareTests.cs # src/Nethermind/Nethermind.Merge.Plugin/IEngineRpcModule.Amsterdam.cs # src/Nethermind/Nethermind.Merge.Plugin/SszRest/Handlers/GetPayloadBodiesByRangeSszHandler.cs # src/Nethermind/Nethermind.TxPool/ITxPool.cs
…style lint warnings
| .WithType(TxType.SetCode) | ||
| .WithTo(signer.Address) | ||
| .WithGasLimit(GasCostOf.Transaction + GasCostOf.NewAccount * count) | ||
| .WithGasLimit(GasCostOf.Transaction + GasCostOf.NewAccount * (ulong)count) |
Deep reviewScope. 1,265 files, +10,160/-9,028 — signed→unsigned migration of block-number/gas counters. I ran five parallel passes (BlockTree, EVM/gas, sync/migration, JSON-RPC/RLP/SSZ, broad pattern sweep) and verified the highest-impact claims against the PR branch. CI is fully green but does not exercise the failure modes below — the pattern is that the original signed code silently no-op'd on out-of-range inputs, the tests were written against that, and post-migration the same inputs wrap rather than no-op. I agree with keeping this as one PR — the type-graph is too interconnected to bisect cleanly. HIGH — please fix before merge1.
|
| // has the marker atomically with the swap — a crash in between cannot leave | ||
| // the new DB live without the floor. | ||
| _stateBoundary.OldestStateBlock = stateToCopy; | ||
| _stateBoundary.OldestStateBlock = (long)stateToCopy; |
There was a problem hiding this comment.
_stateBoundary.OldestStateBlock should be ulong
| // Delete old tx index | ||
| if (_receiptConfig.TxLookupLimit > 0 && newMain.Number > _receiptConfig.TxLookupLimit.Value) | ||
| // safe: TxLookupLimit is checked to be > 0, so it is safe to cast to ulong | ||
| if (_receiptConfig.TxLookupLimit > 0 && newMain.Number > (ulong)_receiptConfig.TxLookupLimit.Value) |
There was a problem hiding this comment.
_receiptConfig.TxLookupLimit should be ulong?
| Task<LevelVisitOutcome> IBlockTreeVisitor.VisitLevelEnd(ChainLevelInfo chainLevelInfo, ulong levelNumber, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| int expectedVisitedBlocksCount = _currentLevel?.BlockInfos.Length ?? 0; |
There was a problem hiding this comment.
Cast once
| int expectedVisitedBlocksCount = _currentLevel?.BlockInfos.Length ?? 0; | |
| ulong expectedVisitedBlocksCount = (ulong)_currentLevel?.BlockInfos.Length ?? 0ul; |
| private ulong _minBlock = ulong.MaxValue; | ||
| private Task _pruningTask = Task.CompletedTask; | ||
|
|
||
| public Hash256? GetHash(BlockHeader headBlock, int depth) => |
There was a problem hiding this comment.
| public Hash256? GetHash(BlockHeader headBlock, int depth) => | |
| public Hash256? GetHash(BlockHeader headBlock, ulong depth) => |
| : _flatCache.TryGet(headBlock.ParentHash!, out Hash256[] array) ? array[depth - 2] | ||
| : Load(headBlock, depth, out _)?.Hash; | ||
|
|
||
| private CacheNode? Load(BlockHeader blockHeader, int depth, out Hash256[]? hashes, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
| private CacheNode? Load(BlockHeader blockHeader, int depth, out Hash256[]? hashes, CancellationToken cancellationToken = default) | |
| private CacheNode? Load(BlockHeader blockHeader, ulong depth, out Hash256[]? hashes, CancellationToken cancellationToken = default) |
| public Hash256? FindHash(long number) => GetBlockHashOnMainOrBestDifficultyHash(number); | ||
| public Hash256? FindHash(ulong number) => GetBlockHashOnMainOrBestDifficultyHash(number); | ||
|
|
||
| public IOwnedReadOnlyList<BlockHeader> FindHeaders(Hash256? blockHash, int numberOfBlocks, int skip, bool reverse) |
There was a problem hiding this comment.
Maybe - not sure if better?
| public IOwnedReadOnlyList<BlockHeader> FindHeaders(Hash256? blockHash, int numberOfBlocks, int skip, bool reverse) | |
| public IOwnedReadOnlyList<BlockHeader> FindHeaders(Hash256? blockHash, ulong numberOfBlocks, int skip, bool reverse) |
| result[responseIndex] = current; | ||
| responseIndex++; | ||
| long nextNumber = startHeader.Number + directionMultiplier * (responseIndex * skip + responseIndex); | ||
| long nextNumber = (long)startHeader.Number + directionMultiplier * (responseIndex * skip + responseIndex); |
There was a problem hiding this comment.
| long nextNumber = (long)startHeader.Number + directionMultiplier * (responseIndex * skip + responseIndex); | |
| ulong nextNumber = startHeader.Number + directionMultiplier * (responseIndex * skip + responseIndex); |
| // Guard against underflow: currentNumber is ulong, so only subtract if > 0 | ||
| BestKnownNumber = currentNumber > 0 | ||
| ? Math.Min(BestKnownNumber, currentNumber - 1) | ||
| : 0; |
There was a problem hiding this comment.
Maybe we should have this guards centralized in some helper methods?
| using BatchWrite batch = _chainLevelInfoRepository.StartBatch(); | ||
|
|
||
| long repairedAbove = ClearStaleMarkersAbove(start.Number, batch); | ||
| long repairedAbove = ClearStaleMarkersAbove(start.Number, batch); // start.Number is ulong (BlockHeader) |
There was a problem hiding this comment.
| long repairedAbove = ClearStaleMarkersAbove(start.Number, batch); // start.Number is ulong (BlockHeader) | |
| long repairedAbove = ClearStaleMarkersAbove(start.Number, batch); |
| .AssertHeadBlockIs(keys[i], 1); | ||
| } | ||
|
|
||
| for (int i = 1; i <= 10; i++) |
There was a problem hiding this comment.
| for (ulong i = 1; i <= 10; i++) |
| { | ||
| bool hasHead = blockTree.Head is not null; | ||
| long level = hasHead ? blockTree.Head!.Number + 1 : 0; | ||
| long level = hasHead ? (long)blockTree.Head!.Number + 1 : 0; |
There was a problem hiding this comment.
| long level = hasHead ? (long)blockTree.Head!.Number + 1 : 0; | |
| ulong level = hasHead ? blockTree.Head!.Number + 1 : 0; |
| // as everything before pivot should be finalized | ||
| long blocksAfter = _blockTree.BestKnownNumber - level + 1; | ||
| if (blocksAfter >= minSealersForFinalization) | ||
| long blocksAfter = (long)_blockTree.BestKnownNumber - (long)level + 1; |
There was a problem hiding this comment.
| long blocksAfter = (long)_blockTree.BestKnownNumber - (long)level + 1; | |
| ulong blocksAfter = _blockTree.BestKnownNumber + 1 - level; |
| { | ||
| long mod = parentStep - currentStep + emptyStepsCount; | ||
| if (mod > 0) | ||
| if (parentStep + emptyStepsCount >= currentStep) |
There was a problem hiding this comment.
Save parentStep + emptyStepsCount to variable?
| StepDurationMilliseconds = (long)stepDuration * millisecondsInSecond; | ||
| TransitionTimestampMilliseconds = (long)transitionTimestamp * millisecondsInSecond; |
There was a problem hiding this comment.
change StepDurationMilliseconds and TransitionTimestampMilliseconds to ulong?
| // more or less at the constant component | ||
| // prevents attack on all Nethermind nodes at once | ||
| _sealValidationInterval = SealValidationIntervalConstantComponent - 8 + _cryptoRandom.NextInt(16); | ||
| _sealValidationInterval = (uint)(SealValidationIntervalConstantComponent - 8 + _cryptoRandom.NextInt(16)); |
There was a problem hiding this comment.
SealValidationIntervalConstantComponent - uint?
| && (orphaned || ValidateBlockNumber(header, parent, ref error)) | ||
| && (orphaned || ValidateTimestamp(header, parent, ref error)) | ||
| && (orphaned || ValidateGasLimitRange(header, parent, spec, ref error)) |
There was a problem hiding this comment.
Won't order break any tests?
| protected virtual bool ValidateFieldLimit(BlockHeader blockHeader, ref string? error) => | ||
| // Number, GasLimit and GasUsed are ulong — they can never be negative. | ||
| // This method is kept for subclass extensibility; no checks are needed at this level. | ||
| true; |
There was a problem hiding this comment.
Is this overriden anywhere (maybe Arbitrum plugin?) If not remove the whole method.
|
|
||
| protected virtual bool ValidateExtraData(BlockHeader header, IReleaseSpec spec, bool isUncle, ref string? error) | ||
| { | ||
| bool extraDataValid = header.ExtraData.Length <= spec.MaximumExtraDataSize | ||
| && (isUncle | ||
| || _daoBlockNumber is null | ||
| || header.Number < _daoBlockNumber | ||
| || header.Number >= _daoBlockNumber + 10 | ||
| // Safe cast: DaoBlockNumber is a known small constant (~1.9M), always fits ulong |
There was a problem hiding this comment.
| // Safe cast: DaoBlockNumber is a known small constant (~1.9M), always fits ulong |
| @@ -105,7 +105,7 @@ private static bool IsKin(BlockHeader header, BlockHeader uncle, int relationshi | |||
| { | |||
| int maxDepth = Math.Min(Math.Min(ancestorsCount, relationshipLevel), (int)header.Number); | |||
There was a problem hiding this comment.
Cast early
| int maxDepth = Math.Min(Math.Min(ancestorsCount, relationshipLevel), (int)header.Number); | |
| ulong maxDepth = (ulong)Math.Min(Math.Min(ancestorsCount, relationshipLevel), (int)header.Number); |
| // Safe cast: ElasticityMultiplier is always a small positive constant (e.g. 2) | ||
| adjustedGasLimit *= (ulong)releaseSpec.ElasticityMultiplier; |
There was a problem hiding this comment.
Make ElasticityMultiplier ulong
| return currentMasternodes[currentLeaderIndex]; | ||
| } | ||
|
|
||
| private static bool IsMasternode(EpochSwitchInfo epochInfo, Address node) => | ||
| epochInfo.Masternodes.AsSpan().IndexOf(node) != -1; | ||
|
|
||
| // TODO: consider using a another sync indicator | ||
| private bool IsSynced() => !_blockTree.IsSyncing().isSyncing && _blockTree.Head is not null; | ||
| private bool IsSynced() |
| public Hash256 ParentHash { get; set; } | ||
| public Hash256 ReceiptsRoot { get; set; } | ||
| public Hash256 Sha3Uncles { get; set; } | ||
| public byte[]? Signature { get; set; } | ||
| public long Size { get; set; } | ||
| public Hash256 StateRoot { get; set; } | ||
| [JsonConverter(typeof(NullableRawLongConverter))] | ||
| public long? Step { get; set; } | ||
| [JsonConverter(typeof(NullableRawULongConverter))] |
There was a problem hiding this comment.
Is it custom converter just for such fields or it's a default one that does not need special mention per property?
| @@ -1373,11 +1377,6 @@ protected virtual CallResult RunByteCode<TTracingInst, TCancelable>( | |||
| Revert: | |||
| // Return a CallResult indicating a revert. | |||
| return new CallResult((byte[])ReturnData, null, shouldRevert: true, exceptionType); | |||
|
|
|||
| OutOfGas: | |||
There was a problem hiding this comment.
Weird thing to remove, why?
There was a problem hiding this comment.
it was likely unreachable code since ulong < 0 is always false
| long cost = (newActiveWords - activeWords) * GasCostOf.Memory + | ||
| ((newActiveWords * newActiveWords) >> 9) - | ||
| ((activeWords * activeWords) >> 9); | ||
| ulong cost = (ulong)(newActiveWords - activeWords) * GasCostOf.Memory + |
There was a problem hiding this comment.
Before the change long allowed it to be negative, all here looks a bit dangerous
flcl42
left a comment
There was a problem hiding this comment.
Focused pass on signed-to-unsigned behavior changes and the EIP-8037 gas-accounting path. I left comments only where the changed line creates a concrete consensus/correctness regression or silently reinterprets negative ranges.
Conflict resolution highlights: - BlockTree.cs: keep master's TryUpdateMainChain refactor, apply ulong types - ISnapshotRepository / PersistenceManager / SnapshotRepository: take master's head-ancestor / committed-head tracking, switch new APIs to ulong, keep PR's PreGenesis-aware fallback in DetermineSnapshotToPersist and FlushToPersistence - BlockTreeTestDouble: switch master's new test double to ulong end-to-end - TestBlockTree / BlockTreeCallSpy: collapse to BlockTreeTestDouble base - MemoryHintMan: switch master's 1.GiB/32.MB int literals to 1UL.GiB / 32UL.MB - Other test files: adopt master's structural changes, keep PR ulong types Build-fixes triggered by the merge (not pre-existing in either branch): - DebugModuleTests: GetBundleTraces ulong?, MigrateReceipts ulong, BlockParameter(0UL) - ClearAllColumnsBatchingTests: StateId.PreGenesis sentinel instead of (-1) - TestingRpcModuleBlockchainTests: WithNonce ulong overload Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…and dead checks HIGH (consensus / correctness): - EvmInstructions.Storage SSTORE refund: saturating subtract to prevent the ulong wrap that would silently grant the maximum post-cap refund - StateSyncPivot.Diff: precedence bug returned the absolute number instead of the difference; reroute through SaturatingSub - BlockTree.AcceptVisitor: bail on empty/inverted range instead of looping ~2^64 - BlockTreeSuggestPacer.WaitForQueue: guard the subtract so a head overtake doesn't pause the pacer indefinitely - BinarySearchBlockNumber: bail at index==0 in the Down branch - WitnessGeneratingHeaderFinder: count-driven loop; old i-- looped forever when _lowestRequestedHeader == 0 MEDIUM: - SaturatingSub promoted to a shared UInt64Extensions; TransactionProcessor and EthereumGasPolicy now use it - SszNumericChecks deleted; ulong fields no longer narrow to long - ReceiptMigration.tx-index expiry: explicit-guard intent, no more (long?) wrap trick - EraStore.GetEpochNumber: throw on blockNumber < FirstBlock - ReceiptFixMigration: SaturatingSub Head?.Number - 2 - EraAdminRpcModule / EraE.AdminEraService: reject negative start/end before the (ulong) cast turns -1 into MaxValue - E2StoreReader: reject negative starting_number from on-disk int64 - BlockParameter JSON Read: TryGetUInt64 path instead of throwing FormatException Xdc (flcl42): - SubnetPenaltyHandler.minBlockNumber / startRange: guard underflows before clamp - EpochSwitchManager.estBlockNum: SaturatingSub before Math.Max - Eip8037BlockGasInclusionCheck.CalculateBlockRegularGas: saturate the multi-step subtraction so a bookkeeping bug doesn't silently blow the block gas limit Cleanups: - NewPayloadHandler emoji: explicit if-tree (the < 0 branch was dead on ulong) - DebugRpcModule.debug_getBlockRlp: remove dead blockNumber >= 0 check - TraceStorePruner: early-return on block.Number <= _blockToKeep - XdcRpcModule: dead BlockNumber < 0 checks → BlockNumber is null - StateSyncPivot.TrySetNewBestHeader: drop the no-op Math.Max(x, 0) - PowForwardHeaderProvider: drop the no-op outer Math.Max and the stale comment - BlockDownloaderTests: drop the no-op Math.Max(0UL, …) noise - SurgeGasPriceOracle.GetAverageGasUsagePerBlock: dead currentBlockNumber >= 0 and the unguarded i-- at i==0 - BlockhashStore.GetBlockHashFromState: drop dead requiredBlockNumber < 0 - ISurgeConfig.FeeHistoryBlockCount, L2GasUsageWindowSize, MaxGasLimitRatio: int → ulong (eliminates the casts at call sites) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Type changes that eliminate casts at multiple sites:
- HeadersSyncBatch.RequestSize: ulong → int (14 (int) casts dropped in callers;
one (ulong) cast added in EndNumber). Sync request sizes are inherently small.
- EraPathUtils.Filename (Era1 + EraE): long epoch → ulong epoch (4 cast sites)
- IOpcodeTracingConfig.{StartBlock,EndBlock,RecentBlocks}: long? → ulong?,
cascading through TraceConfiguration / FromConfig / BlockRangeValidator and
the recorder's currentChainTip (~12 cast sites)
- ProcessingStats: _chunkBlobs and BlockData.BlobCount long → ulong
- MinBlockInCachePruneStrategyTests / MaxBlockInCachePruneStrategyTests:
const long → const ulong for PruneBoundary and {Min,Max}BlockFromPersisted
Per-site cleanups:
- BlockHeaderTests: hex literals (0x2fefbaUL) instead of
(ulong)Bytes.FromHexString(...).ToUnsignedBigInteger()
- Int64Extensions.ToLongFromBigEndianByteArrayWithoutLeadingZeros: cast
internally once so call sites drop their (long) cast
- ProcessedTransactionsDbCleaner: switch to ToULongFromBigEndian... directly
- SyncConfig / RocksDbConfigFactoryTests / SyncPeerProtocolHandlerBase:
N.MB / N.MiB ulong-extension forms drop the outer (ulong) cast
- BlockAccessListsSyncFeedTests: ToULongFromBigEndian... directly
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nt cleanups - Nethermind.Stateless.Executor.csproj: drop link to deleted SszRest/SszNumericChecks.cs - Nethermind.OpcodeTracing.Plugin/OpcodeTracingConfig.cs: restore space lost in long?→ulong? replace_all - Nethermind.Taiko.Test/SurgeGasPriceOracleTests.cs: drop unused System using left over after Math.Max(0UL, …) removal Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PR's Validate signature dropped the intrinsicRegular/intrinsicState
subtractions, making the worst-case dimension calculation strictly
match `min(TX_MAX_GAS_LIMIT, tx.gas)` and `tx.gas`. Master subtracts the
opposite-dimension intrinsic before clamping, yielding a smaller worst-
case that lets txs through when one dimension is tight but the tx's
intrinsic mass lives in the other dimension. The Amsterdam test
`test_block_2d_gas_valid_when_cumulative_exceeds_limit` fails under the
PR's stricter check; master is green.
Port master's behavior to the PR's ulong signature using SaturatingSub
for the txGas - intrinsic{State,Regular} subtractions. Also revert the
CalculateBlockRegularGas + Storage SSTORE Refund SaturatingSub changes
back to raw subtracts so they match the PR's tested behavior.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ormula The PR-added unit / integration tests asserted the "raw tx.gas" inclusion predicate (worst-case state = tx.gas, no intrinsicRegular subtraction). That contradicts the Pyspec fixture `tests/amsterdam/eip8037_state_creation_gas_cost_increase/.../test_block_2d_gas_valid_when_cumulative_exceeds_limit`, which passes on master because master's Validate subtracts the opposite- dimension intrinsic before clamping. Drop the two PR-added cases that required the raw-tx.gas predicate; rewrite the parameterized Boundary_state case to assert the worst-case-after-intrinsic-subtract semantics. - Eip8037BlockGasInclusionCheckTests: drop Creation_tx_regular_check_without_subtraction_rejects; update Boundary_state deltas so the rejection lands when (tx.gas - intrinsicRegular) > stateAvailable - Eip8037BlockGasIntegrationTests: drop Eip8037_creation_tx_regular_check_without_subtraction_rejects Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HIGH: - BeaconPivot.GetLowestBlockToFinalize: guard `Head.Number - MaxDepth + 1` — wrapped to a near-ulong.MaxValue safe number when head < 64. - Optimism BatchV1.DecodeTxs: drop (ulong) cast on UInt256 tx values; the Transaction.Value / DecodedMaxFeePerGas fields are UInt256 and the cast silently truncated any tx > ~18.4 ETH. - EthereumGasPolicy.CreateAvailableFromIntrinsic: Debug.Assert on the two unguarded subtracts (gasLimit - intrinsicRegular - intrinsicState and TX_MAX_GAS_LIMIT - intrinsicRegular). Callers validate today; this catches future eth_call paths that skip validation. - EvmInstructions.Storage SSTORE refund: Debug.Assert + SaturatingSub on `Refund -= sClearRefunds`. Per EIP-2200/3529 the matching `+= sClearRefunds` ran earlier so the invariant holds; the saturating subtract guards against an out-of-order wrap silently granting the maximum post-cap refund. - ReceiptMigration.MigrationPointerTracker.ReportCompleted: guard `_nextToConfirm--` at 0 so a fully-migrated chain doesn't wrap the tracker into ulong.MaxValue. Reports `MigratedBlockNumber = 0` when genesis is reached. MEDIUM: - VmState.CommitToParent: `checked` the parentState.Refund += Refund. Gas- bounded so unreachable in practice, but the unsigned add no longer caught a buggy negative-child propagation that master's signed long would have. - Eip8037BlockGasInclusionCheck.Validate: early-return RegularDimensionExceeded / StateDimensionExceeded when cumulative > limit. The prior SaturatingSub silently floored to 0 and the worst-case check could falsely succeed. - CompactionSchedule.NextFullCompactionAfter: clamp `from + distance` at ulong.MaxValue. Unreachable at any realistic chain height; explicit. - AuRaBlockFinalizationManager.LoadInitialLastFinalizedBlockLevel and GetFinalizationLevel: drop the long round-trip and use ulong arithmetic with explicit zero/underflow guards. Avoids the 2^63-1 wrap that was only theoretical but exposed by the signed→unsigned migration. NIT: - BlockTree.GetBlockHashOnMainOrBestDifficultyHash: drop dead `blockNumber < 0` check on ulong parameter. - ReceiptsSyncFeed: drop stale "_barrier is long" comment (now ulong). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # src/Nethermind/Nethermind.AuRa.Test/Validators/MultiValidatorTests.cs # src/Nethermind/Nethermind.Blockchain/IBlockFinalizationManager.cs # src/Nethermind/Nethermind.Blockchain/ManualFinalizationManager.cs # src/Nethermind/Nethermind.Core.Test/Builders/BlockTreeTestDouble.cs # src/Nethermind/Nethermind.Merge.AuRa/AuRaMergeFinalizationManager.cs # src/Nethermind/Nethermind.Merge.Plugin.Test/MergeFinalizationManagerTests.cs # src/Nethermind/Nethermind.Merge.Plugin/MergeFinalizationManager.cs
…lean up XDC comments
Closes #9685
Changes
ulong.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?