Skip to content

Commit 29561c2

Browse files
flcl42flcl42
committed
Reorder validations
1 parent 06ca96e commit 29561c2

6 files changed

Lines changed: 195 additions & 54 deletions

File tree

‎src/Nethermind/Ethereum.Test.Base/BlockchainTestBase.cs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -435,7 +435,6 @@ private static readonly (string ExpectedError, string Substring)[] ValidationErr
435435
("BlockException.INVALID_STATE_ROOT", "InvalidStateRoot: State root in header does not match"),
436436
("BlockException.GAS_USED_OVERFLOW", "Block gas limit exceeded"), // alternate error string
437437
("BlockException.BLOCK_ACCESS_LIST_GAS_LIMIT_EXCEEDED", "BlockAccessListGasLimitExceeded:"),
438-
("TransactionException.GAS_ALLOWANCE_EXCEEDED", "BlockAccessListGasLimitExceeded:"),
439438
];
440439

441440
private const RegexOptions ValidationErrorRegexOptions = RegexOptions.CultureInvariant | RegexOptions.Compiled;
@@ -457,8 +456,6 @@ private static readonly (string ExpectedError, Regex Pattern)[] ValidationErrorR
457456
("TransactionException.GAS_ALLOWANCE_EXCEEDED", ValidationErrorRegex(@"TxGasLimitCapExceeded:")),
458457
("BlockException.INVALID_BAL_EXTRA_ACCOUNT", ValidationErrorRegex(@"Error decoding block access list:.*Account changes were in incorrect order")),
459458
("BlockException.INVALID_BAL_MISSING_ACCOUNT", ValidationErrorRegex(@"InvalidBlockLevelAccessList: Suggested block-level access list missing account changes")),
460-
("BlockException.INVALID_DEPOSIT_EVENT_LAYOUT", ValidationErrorRegex(@"InvalidBlockLevelAccessList: Suggested block-level access list missing account changes")),
461-
("BlockException.SYSTEM_CONTRACT_CALL_FAILED", ValidationErrorRegex(@"InvalidBlockLevelAccessList: Suggested block-level access list missing account changes")),
462459
];
463460

464461
private static string[] MapValidationErrorsToEestExceptions(string validationError) =>

‎src/Nethermind/Nethermind.Blockchain.Test/BlockProcessorTests.cs‎

Lines changed: 159 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@
1818
using Nethermind.Core.Test.Blockchain;
1919
using Nethermind.Core.Test.Builders;
2020
using Nethermind.Evm.TransactionProcessing;
21+
using Nethermind.Int256;
2122
using Nethermind.JsonRpc.Test.Modules;
2223
using Nethermind.Logging;
2324
using Nethermind.Specs;
2425
using Nethermind.Specs.Forks;
26+
using Nethermind.Specs.Test;
2527
using Nethermind.Evm.State;
2628
using Nethermind.TxPool;
2729
using NSubstitute;
@@ -208,40 +210,132 @@ public void BranchProcessor_unsubscribes_from_TransactionsExecuted_after_process
208210
}
209211

210212
[Test, MaxTime(Timeout.MaxTestTime)]
211-
[TestCaseSource(nameof(BlockValidationTransactionsExecutor_bal_validation_cases))]
212-
public void BlockValidationTransactionsExecutor_validates_bal_only_when_validation_enabled(
213+
[TestCaseSource(nameof(BlockProcessor_bal_validation_cases))]
214+
public void BlockProcessor_validates_bal_after_execution_requests_only_when_validation_enabled(
213215
ProcessingOptions processingOptions,
214216
bool shouldValidateBlockAccessList)
215217
{
216218
TrackingBlockAccessListWorldState stateProvider = new(TestWorldStateFactory.CreateForTest());
217-
stateProvider.LoadSuggestedBlockAccessList(new BlockAccessList(), 37_568);
219+
List<string> events = [];
220+
stateProvider.OnValidate = (index, gasRemaining) => events.Add($"bal-{index}:{gasRemaining}");
218221

219222
ITransactionProcessorAdapter transactionProcessor = Substitute.For<ITransactionProcessorAdapter>();
220-
transactionProcessor.Execute(Arg.Any<Transaction>(), Arg.Any<ITxTracer>()).Returns(static callInfo =>
223+
transactionProcessor.Execute(Arg.Any<Transaction>(), Arg.Any<ITxTracer>()).Returns(callInfo =>
221224
{
225+
events.Add("tx");
222226
Transaction transaction = callInfo.Arg<Transaction>();
223227
transaction.SpentGas = 63_586;
224228
transaction.BlockGasUsed = 37_568;
225229
return TransactionResult.Ok;
226230
});
227231

228-
BlockProcessor.BlockValidationTransactionsExecutor txExecutor = new(transactionProcessor, stateProvider);
229-
Block block = Build.A.Block.WithTransactions(Build.A.Transaction.SignedAndResolved().TestObject).TestObject;
230-
BlockReceiptsTracer receiptsTracer = new();
231-
receiptsTracer.StartNewBlockTrace(block);
232+
IExecutionRequestsProcessor executionRequestsProcessor = Substitute.For<IExecutionRequestsProcessor>();
233+
executionRequestsProcessor
234+
.When(static x => x.ProcessExecutionRequests(Arg.Any<Block>(), Arg.Any<IWorldState>(), Arg.Any<TxReceipt[]>(), Arg.Any<IReleaseSpec>()))
235+
.Do(_ => events.Add("requests"));
236+
237+
OverridableReleaseSpec spec = new(London.Instance) { IsEip7928Enabled = true };
238+
BlockProcessor processor = CreateBalTestBlockProcessor(stateProvider, transactionProcessor, executionRequestsProcessor, spec);
239+
Block block = Build.A.Block
240+
.WithTransactions(Build.A.Transaction.SignedAndResolved().TestObject)
241+
.WithGasUsed(37_568)
242+
.WithBlockAccessList(new BlockAccessList())
243+
.TestObject;
232244

233-
txExecutor.ProcessTransactions(block, processingOptions, receiptsTracer, CancellationToken.None);
245+
using IDisposable _ = stateProvider.BeginScope(null);
246+
processor.ProcessOne(block, processingOptions, NullBlockTracer.Instance, spec, CancellationToken.None);
234247

235248
if (shouldValidateBlockAccessList)
236249
{
237-
Assert.That(stateProvider.ValidatedGasRemaining, Is.EqualTo(new long[] { 37_568L, 0L }));
250+
Assert.That(events, Is.EqualTo(new[] { "tx", "requests", "bal-0:37568", "bal-1:0" }));
238251
}
239252
else
240253
{
241-
Assert.That(stateProvider.ValidatedGasRemaining, Is.Empty);
254+
Assert.That(events, Is.EqualTo(new[] { "tx", "requests" }));
242255
}
243256
}
244257

258+
[Test, MaxTime(Timeout.MaxTestTime)]
259+
public void BlockProcessor_prioritizes_execution_request_error_over_bal_validation()
260+
{
261+
TrackingBlockAccessListWorldState stateProvider = new(TestWorldStateFactory.CreateForTest());
262+
ITransactionProcessorAdapter transactionProcessor = SuccessfulTransactionProcessor();
263+
IExecutionRequestsProcessor executionRequestsProcessor = Substitute.For<IExecutionRequestsProcessor>();
264+
executionRequestsProcessor
265+
.When(static x => x.ProcessExecutionRequests(Arg.Any<Block>(), Arg.Any<IWorldState>(), Arg.Any<TxReceipt[]>(), Arg.Any<IReleaseSpec>()))
266+
.Do(callInfo => throw new InvalidBlockException(callInfo.Arg<Block>(), "DepositsInvalid: Invalid deposit event layout: test"));
267+
268+
OverridableReleaseSpec spec = new(London.Instance) { IsEip7928Enabled = true };
269+
BlockProcessor processor = CreateBalTestBlockProcessor(stateProvider, transactionProcessor, executionRequestsProcessor, spec);
270+
Block block = Build.A.Block
271+
.WithTransactions(Build.A.Transaction.SignedAndResolved().TestObject)
272+
.WithGasUsed(37_568)
273+
.WithBlockAccessList(new BlockAccessList())
274+
.TestObject;
275+
276+
using IDisposable _ = stateProvider.BeginScope(null);
277+
InvalidBlockException exception = Assert.Throws<InvalidBlockException>(
278+
() => processor.ProcessOne(block, ProcessingOptions.None, NullBlockTracer.Instance, spec, CancellationToken.None))!;
279+
280+
Assert.That(exception.Message, Does.StartWith("DepositsInvalid: Invalid deposit event layout"));
281+
Assert.That(stateProvider.ValidatedGasRemaining, Is.Empty);
282+
}
283+
284+
[Test, MaxTime(Timeout.MaxTestTime)]
285+
public void BlockProcessor_prioritizes_transaction_error_over_bal_item_gas_limit()
286+
{
287+
TrackingBlockAccessListWorldState stateProvider = new(TestWorldStateFactory.CreateForTest());
288+
ITransactionProcessorAdapter transactionProcessor = Substitute.For<ITransactionProcessorAdapter>();
289+
transactionProcessor.Execute(Arg.Any<Transaction>(), Arg.Any<ITxTracer>()).Returns(TransactionResult.BlockGasLimitExceeded);
290+
IExecutionRequestsProcessor executionRequestsProcessor = Substitute.For<IExecutionRequestsProcessor>();
291+
292+
OverridableReleaseSpec spec = new(London.Instance) { IsEip7928Enabled = true };
293+
BlockProcessor processor = CreateBalTestBlockProcessor(stateProvider, transactionProcessor, executionRequestsProcessor, spec);
294+
Block block = Build.A.Block
295+
.WithTransactions(Build.A.Transaction.SignedAndResolved().TestObject)
296+
.WithGasLimit(21_000)
297+
.WithGasUsed(21_000)
298+
.WithBlockAccessList(BlockAccessListWithAccountReads(15))
299+
.TestObject;
300+
301+
using IDisposable _ = stateProvider.BeginScope(null);
302+
Nethermind.Blockchain.InvalidTransactionException exception = Assert.Throws<Nethermind.Blockchain.InvalidTransactionException>(
303+
() => processor.ProcessOne(block, ProcessingOptions.None, NullBlockTracer.Instance, spec, CancellationToken.None))!;
304+
305+
Assert.That(exception.Message, Does.Contain("Block gas limit exceeded"));
306+
Assert.That(stateProvider.ValidatedGasRemaining, Is.Empty);
307+
executionRequestsProcessor
308+
.DidNotReceive()
309+
.ProcessExecutionRequests(Arg.Any<Block>(), Arg.Any<IWorldState>(), Arg.Any<TxReceipt[]>(), Arg.Any<IReleaseSpec>());
310+
}
311+
312+
[Test, MaxTime(Timeout.MaxTestTime)]
313+
public void BlockProcessor_validates_bal_item_gas_limit_after_transactions()
314+
{
315+
TrackingBlockAccessListWorldState stateProvider = new(TestWorldStateFactory.CreateForTest());
316+
ITransactionProcessorAdapter transactionProcessor = SuccessfulTransactionProcessor(blockGasUsed: 21_000);
317+
IExecutionRequestsProcessor executionRequestsProcessor = Substitute.For<IExecutionRequestsProcessor>();
318+
319+
OverridableReleaseSpec spec = new(London.Instance) { IsEip7928Enabled = true };
320+
BlockProcessor processor = CreateBalTestBlockProcessor(stateProvider, transactionProcessor, executionRequestsProcessor, spec);
321+
Block block = Build.A.Block
322+
.WithTransactions(Build.A.Transaction.SignedAndResolved().TestObject)
323+
.WithGasLimit(21_000)
324+
.WithGasUsed(21_000)
325+
.WithBlockAccessList(BlockAccessListWithAccountReads(15))
326+
.TestObject;
327+
328+
using IDisposable _ = stateProvider.BeginScope(null);
329+
InvalidBlockException exception = Assert.Throws<InvalidBlockException>(
330+
() => processor.ProcessOne(block, ProcessingOptions.None, NullBlockTracer.Instance, spec, CancellationToken.None))!;
331+
332+
Assert.That(exception.Message, Does.StartWith("BlockAccessListGasLimitExceeded"));
333+
Assert.That(stateProvider.ValidatedGasRemaining, Is.Empty);
334+
executionRequestsProcessor
335+
.Received(1)
336+
.ProcessExecutionRequests(Arg.Any<Block>(), Arg.Any<IWorldState>(), Arg.Any<TxReceipt[]>(), Arg.Any<IReleaseSpec>());
337+
}
338+
245339
[TestCase(2_000, false)]
246340
[TestCase(1_999, true)]
247341
[MaxTime(Timeout.MaxTestTime)]
@@ -467,6 +561,7 @@ private sealed class TrackingBlockAccessListWorldState(IWorldState innerWorldSta
467561
public bool TracingEnabled { get; set; }
468562
public BlockAccessList GeneratedBlockAccessList { get; set; } = new();
469563
public List<long> ValidatedGasRemaining { get; } = [];
564+
public Action<ushort, long>? OnValidate { get; set; }
470565

471566
private long _gasUsed;
472567

@@ -480,7 +575,10 @@ public long GasUsed()
480575
=> _gasUsed;
481576

482577
public void ValidateBlockAccessList(BlockHeader block, ushort index, long gasRemaining)
483-
=> ValidatedGasRemaining.Add(gasRemaining);
578+
{
579+
OnValidate?.Invoke(index, gasRemaining);
580+
ValidatedGasRemaining.Add(gasRemaining);
581+
}
484582

485583
public void SetBlockAccessList(Block block, IReleaseSpec spec) { }
486584
public IDisposable BeginSystemAccountReadSuppression() => EmptyDisposable.Instance;
@@ -492,11 +590,57 @@ public void Dispose() { }
492590
}
493591
}
494592

495-
public static IEnumerable<TestCaseData> BlockValidationTransactionsExecutor_bal_validation_cases()
593+
private static BlockProcessor CreateBalTestBlockProcessor(
594+
IWorldState stateProvider,
595+
ITransactionProcessorAdapter transactionProcessor,
596+
IExecutionRequestsProcessor executionRequestsProcessor,
597+
IReleaseSpec spec)
598+
{
599+
ITransactionProcessor systemTransactionProcessor = Substitute.For<ITransactionProcessor>();
600+
return new(
601+
new TestSingleReleaseSpecProvider(spec),
602+
TestBlockValidator.AlwaysValid,
603+
NoBlockRewards.Instance,
604+
new BlockProcessor.BlockValidationTransactionsExecutor(transactionProcessor, stateProvider),
605+
stateProvider,
606+
NullReceiptStorage.Instance,
607+
new BeaconBlockRootHandler(systemTransactionProcessor, stateProvider),
608+
Substitute.For<IBlockhashStore>(),
609+
LimboLogs.Instance,
610+
new WithdrawalProcessor(stateProvider, LimboLogs.Instance),
611+
executionRequestsProcessor);
612+
}
613+
614+
private static ITransactionProcessorAdapter SuccessfulTransactionProcessor(long blockGasUsed = 37_568)
615+
{
616+
ITransactionProcessorAdapter transactionProcessor = Substitute.For<ITransactionProcessorAdapter>();
617+
transactionProcessor.Execute(Arg.Any<Transaction>(), Arg.Any<ITxTracer>()).Returns(callInfo =>
618+
{
619+
Transaction transaction = callInfo.Arg<Transaction>();
620+
transaction.SpentGas = blockGasUsed;
621+
transaction.BlockGasUsed = blockGasUsed;
622+
return TransactionResult.Ok;
623+
});
624+
625+
return transactionProcessor;
626+
}
627+
628+
private static BlockAccessList BlockAccessListWithAccountReads(int count)
629+
{
630+
BlockAccessList blockAccessList = new();
631+
for (int i = 0; i < count; i++)
632+
{
633+
blockAccessList.AddAccountRead(Address.FromNumber((UInt256)(ulong)(i + 1)));
634+
}
635+
636+
return blockAccessList;
637+
}
638+
639+
public static IEnumerable<TestCaseData> BlockProcessor_bal_validation_cases()
496640
{
497641
yield return new TestCaseData(ProcessingOptions.None, true)
498-
.SetName("BlockValidationTransactionsExecutor_uses_block_gas_for_bal_validation_budget");
642+
.SetName("BlockProcessor_uses_block_gas_for_bal_validation_budget");
499643
yield return new TestCaseData(ProcessingOptions.NoValidation, false)
500-
.SetName("BlockValidationTransactionsExecutor_skips_bal_validation_when_no_validation_requested");
644+
.SetName("BlockProcessor_skips_bal_validation_when_no_validation_requested");
501645
}
502646
}

‎src/Nethermind/Nethermind.Blockchain.Test/Validators/BlockValidatorTests.cs‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -262,17 +262,16 @@ public void ValidateSuggestedBlock_SuggestedBlockIsInvalid_CorrectErrorIsSet(Blo
262262
Assert.That(error, Does.StartWith(expectedError));
263263
}
264264

265-
[TestCase(30_000, true)]
266-
[TestCase(29_999, false)]
267-
public void ValidateSuggestedBlock_Enforces_bal_item_gas_limit_boundary(long gasLimit, bool expectedValid)
265+
[Test]
266+
public void ValidateSuggestedBlock_Defers_bal_item_gas_limit_boundary_to_processing()
268267
{
269-
BlockHeader parent = Build.A.BlockHeader.TestObject;
268+
BlockHeader parent = Build.A.BlockHeader.WithGasLimit(30_000).TestObject;
270269
BlockAccessList bal = Build.A.BlockAccessList.WithPrecompileChanges(parent.Hash!, timestamp: 12).TestObject;
271270
byte[] encodedBal = Rlp.Encode(bal).Bytes;
272271
Hash256 balHash = new(ValueKeccak.Compute(encodedBal).Bytes);
273272
Block suggestedBlock = Build.A.Block
274273
.WithParent(parent)
275-
.WithGasLimit(gasLimit)
274+
.WithGasLimit(29_999)
276275
.WithBlobGasUsed(0)
277276
.WithWithdrawals([])
278277
.WithBlockAccessList(bal)
@@ -284,14 +283,7 @@ public void ValidateSuggestedBlock_Enforces_bal_item_gas_limit_boundary(long gas
284283

285284
bool isValid = sut.ValidateSuggestedBlock(suggestedBlock, parent, out string? error);
286285

287-
Assert.That(isValid, Is.EqualTo(expectedValid));
288-
if (expectedValid)
289-
{
290-
Assert.That(error, Is.Null);
291-
}
292-
else
293-
{
294-
Assert.That(error, Does.StartWith("BlockAccessListGasLimitExceeded"));
295-
}
286+
Assert.That(isValid, Is.True);
287+
Assert.That(error, Is.Null);
296288
}
297289
}

‎src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.BlockValidationTransactionsExecutor.cs‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,6 @@ public TxReceipt[] ProcessTransactions(Block block, ProcessingOptions processing
3131
Metrics.ResetBlockStats();
3232

3333
bool shouldValidate = !processingOptions.ContainsFlag(ProcessingOptions.NoValidation);
34-
bool shouldValidateBlockAccessList = _balBuilder is not null && shouldValidate;
35-
long? gasRemaining = shouldValidateBlockAccessList ? _balBuilder!.GasUsed() : null;
36-
37-
if (shouldValidateBlockAccessList)
38-
{
39-
_balBuilder!.ValidateBlockAccessList(block.Header, 0, gasRemaining!.Value);
40-
}
4134

4235
for (int i = 0; i < block.Transactions.Length; i++)
4336
{
@@ -49,12 +42,6 @@ public TxReceipt[] ProcessTransactions(Block block, ProcessingOptions processing
4942
{
5043
ThrowInvalidBlockForGasLimit(block);
5144
}
52-
53-
if (shouldValidateBlockAccessList)
54-
{
55-
gasRemaining -= currentTx.BlockGasUsed;
56-
_balBuilder!.ValidateBlockAccessList(block.Header, (ushort)(i + 1), gasRemaining!.Value);
57-
}
5845
}
5946
_balBuilder?.GeneratedBlockAccessList.IncrementBlockAccessIndex();
6047

‎src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
using Nethermind.Consensus.Validators;
1616
using Nethermind.Consensus.Withdrawals;
1717
using Nethermind.Core;
18+
using Nethermind.Core.Messages;
1819
using Nethermind.Core.Specs;
1920
using Nethermind.Core.Threading;
2021
using Nethermind.Crypto;
@@ -144,6 +145,8 @@ protected virtual TxReceipt[] ProcessBlock(
144145

145146
executionRequestsProcessor.ProcessExecutionRequests(block, stateProvider, receipts, spec);
146147

148+
ValidateBlockAccessList(block, options);
149+
147150
_balBuilder?.SetBlockAccessList(block, spec);
148151

149152
ReceiptsTracer.EndBlockTrace();
@@ -167,6 +170,33 @@ protected virtual TxReceipt[] ProcessBlock(
167170
return receipts;
168171
}
169172

173+
private void ValidateBlockAccessList(Block block, ProcessingOptions options)
174+
{
175+
if (_balBuilder is null || options.ContainsFlag(ProcessingOptions.NoValidation))
176+
{
177+
return;
178+
}
179+
180+
if (block.BlockAccessList is not null)
181+
{
182+
int itemCount = block.BlockAccessList.ItemCount();
183+
if ((long)itemCount * GasCostOf.BlockAccessListItem > block.Header.GasLimit)
184+
{
185+
throw new InvalidBlockException(block, BlockErrorMessages.BlockAccessListGasLimitExceeded(itemCount, block.Header.GasLimit));
186+
}
187+
}
188+
189+
long gasRemaining = _balBuilder.GasUsed();
190+
_balBuilder.ValidateBlockAccessList(block.Header, 0, gasRemaining);
191+
192+
Transaction[] transactions = block.Transactions;
193+
for (int i = 0; i < transactions.Length; i++)
194+
{
195+
gasRemaining -= transactions[i].BlockGasUsed;
196+
_balBuilder.ValidateBlockAccessList(block.Header, (ushort)(i + 1), gasRemaining);
197+
}
198+
}
199+
170200
[MethodImpl(MethodImplOptions.NoInlining)]
171201
private static void CalculateBlooms(TxReceipt[] receipts) => ParallelUnbalancedWork.For(
172202
0,

‎src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -411,15 +411,6 @@ public virtual bool ValidateBlockLevelAccessList(Block block, IReleaseSpec spec,
411411

412412
if (block.BlockAccessList is not null)
413413
{
414-
int itemCount = block.BlockAccessList.ItemCount();
415-
if (itemCount * GasCostOf.BlockAccessListItem > block.Header.GasLimit)
416-
{
417-
error = BlockErrorMessages.BlockAccessListGasLimitExceeded(itemCount, block.Header.GasLimit);
418-
if (_logger.IsWarn) _logger.Warn($"Block level access list item count {itemCount} exceeds block gas limit bound in block {block.ToString(Block.Format.FullHashAndNumber)}.");
419-
420-
return false;
421-
}
422-
423414
if (!ValidateBlockLevelAccessListHashMatches(block, out Hash256 blockLevelAccessListHash))
424415
{
425416
error = BlockErrorMessages.InvalidBlockLevelAccessListHash(block.Header.BlockAccessListHash, blockLevelAccessListHash);

0 commit comments

Comments
 (0)