From e89e9be7d0bbe486a8d45cbb4d111a73dae719ea Mon Sep 17 00:00:00 2001 From: ak88 Date: Wed, 22 May 2024 11:44:15 +0200 Subject: [PATCH 1/8] introduce boundary checks SnapProvider --- .../SnapSync/SnapProvider.cs | 22 ++++++++++++++++--- .../SnapSync/SnapProviderHelper.cs | 4 +++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs index aa74716db65..9f4ee0f6234 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs @@ -51,7 +51,7 @@ public AddRangeResult AddAccountRange(AccountRange request, AccountsAndProofs re { AddRangeResult result; - if (response.PathAndAccounts.Count == 0 && response.Proofs.Count == 0) + if (!response.PathAndAccounts.Any()) { _logger.Trace($"SNAP - GetAccountRange - requested expired RootHash:{request.RootHash}"); @@ -59,7 +59,13 @@ public AddRangeResult AddAccountRange(AccountRange request, AccountsAndProofs re } else { - result = AddAccountRange(request.BlockNumber.Value, request.RootHash, request.StartingHash, response.PathAndAccounts, response.Proofs, hashLimit: request.LimitHash); + result = AddAccountRange( + request.BlockNumber.Value, + request.RootHash, + request.StartingHash, + response.PathAndAccounts, + response.Proofs, + hashLimit: request.LimitHash); if (result == AddRangeResult.OK) { @@ -73,8 +79,18 @@ public AddRangeResult AddAccountRange(AccountRange request, AccountsAndProofs re return result; } - public AddRangeResult AddAccountRange(long blockNumber, in ValueHash256 expectedRootHash, in ValueHash256 startingHash, IReadOnlyList accounts, IReadOnlyList proofs = null, in ValueHash256? hashLimit = null!) + public AddRangeResult AddAccountRange( + long blockNumber, + in ValueHash256 expectedRootHash, + in ValueHash256 startingHash, + IReadOnlyList accounts, + IReadOnlyList proofs = null, + in ValueHash256? hashLimit = null!) { + if (!accounts.Any()) + { + return AddRangeResult.OK; + } ITrieStore store = _trieStorePool.Get(); try { diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs index a3aaf5efa10..23131d0cc42 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; @@ -31,7 +32,8 @@ public static (AddRangeResult result, bool moreChildrenToRight, List(), new List()); ValueHash256 lastHash = accounts[^1].Path; (AddRangeResult result, List<(TrieNode, TreePath)> sortedBoundaryList, bool moreChildrenToRight) = From b3c86cf21527b488c7503f393a376e710fdb9faf Mon Sep 17 00:00:00 2001 From: ak88 Date: Wed, 22 May 2024 13:24:19 +0200 Subject: [PATCH 2/8] unnecessary await --- .../Nethermind.Synchronization/SnapSync/SnapSyncDownloader.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapSyncDownloader.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapSyncDownloader.cs index 28690a51013..32a6a0392dd 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapSyncDownloader.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapSyncDownloader.cs @@ -52,8 +52,6 @@ public async Task Dispatch(PeerInfo peerInfo, SnapSyncBatch batch, CancellationT Logger.Error($"DEBUG/ERROR Error after dispatching the snap sync request. Request: {batch}", e); } } - - await Task.CompletedTask; } } } From 2c3d044a7c088f5c476f594173f04b62b2aea79f Mon Sep 17 00:00:00 2001 From: ak88 Date: Wed, 22 May 2024 16:07:17 +0200 Subject: [PATCH 3/8] throw on emty accounts instead --- .../SnapSync/SnapProviderTests.cs | 58 +++++++++++++++++++ .../UnpooledReadOnlyList.cs | 37 ++++++++++++ .../SnapSync/SnapProvider.cs | 4 +- .../SnapSync/SnapProviderHelper.cs | 2 +- 4 files changed, 97 insertions(+), 4 deletions(-) create mode 100644 src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs create mode 100644 src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs diff --git a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs new file mode 100644 index 00000000000..97fbd0c0768 --- /dev/null +++ b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: 2024 Demerzel Solutions Limited +// SPDX-License-Identifier: LGPL-3.0-only + +using FluentAssertions; +using Nethermind.Blockchain; +using Nethermind.Core.Crypto; +using Nethermind.Db; +using Nethermind.Logging; +using Nethermind.State.Snap; +using Nethermind.Synchronization.SnapSync; +using Nethermind.Trie; +using NSubstitute; +using NUnit.Framework; +using System; +using System.Collections.Generic; + +namespace Nethermind.Synchronization.Test.SnapSync; + +[TestFixture] +public class SnapProviderTests +{ + + [Test] + public void AddAccountRange_AccountListIsEmpty_ThrowArgumentException() + { + IDbProvider dbProvider = new DbProvider(); + using ProgressTracker progressTracker = new(Substitute.For(), dbProvider.GetDb(DbNames.State), LimboLogs.Instance); + SnapProvider sut = new(progressTracker, dbProvider.CodeDb, new NodeStorage(dbProvider.StateDb), LimboLogs.Instance); + + Assert.That( + () => sut.AddAccountRange( + 0, + Keccak.Zero, + Keccak.Zero, + Array.Empty(), + Array.Empty().AsReadOnly()), Throws.ArgumentException); + } + + + [Test] + public void AddAccountRange_ResponseHasEmptyListOfAccountsAndOneProof_ReturnsExpiredRootHash() + { + MemDb db = new(); + IDbProvider dbProvider = new DbProvider(); + dbProvider.RegisterDb(DbNames.State, db); + using ProgressTracker progressTracker = new(Substitute.For(), dbProvider.GetDb(DbNames.State), LimboLogs.Instance); + dbProvider.RegisterDb(DbNames.Code, new MemDb()); + AccountRange accountRange = new(Keccak.Zero, Keccak.Zero, Keccak.MaxValue); + AccountsAndProofs accountsAndProofs = new (); + accountsAndProofs.PathAndAccounts = new List().ToUnpooledList(); + accountsAndProofs.Proofs = new List { new byte[] { 0x0 } }.ToUnpooledList(); + + SnapProvider sut = new(progressTracker, dbProvider.CodeDb, new NodeStorage(dbProvider.StateDb), LimboLogs.Instance); + + sut.AddAccountRange(accountRange, accountsAndProofs).Should().Be(AddRangeResult.ExpiredRootHash); + } + +} diff --git a/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs b/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs new file mode 100644 index 00000000000..f570587216c --- /dev/null +++ b/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs @@ -0,0 +1,37 @@ +// SPDX-FileCopyrightText: 2024 Demerzel Solutions Limited +// SPDX-License-Identifier: LGPL-3.0-only + +using Nethermind.Core.Collections; +using System; +using System.Collections; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Nethermind.Synchronization.Test; +/// +/// This is list that does not require disposing and is meant for test purposes. +/// +/// +internal class UnpooledReadOnlyList : List,IOwnedReadOnlyList +{ + public UnpooledReadOnlyList(IEnumerable enumerable) : base(enumerable) + {} + public UnpooledReadOnlyList(IReadOnlyList list) : base(list) + {} + public ReadOnlySpan AsSpan() + { + return this.AsSpan(); + } + + public void Dispose() + { + } +} + +internal static class IOwnedReadOnlyListExtensions +{ + public static IOwnedReadOnlyList ToUnpooledList(this IEnumerable enumerable) => new UnpooledReadOnlyList(enumerable); + public static IOwnedReadOnlyList ToUnpooledList(this IReadOnlyCollection collection) => new UnpooledReadOnlyList(collection); +} diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs index 9f4ee0f6234..df6d451c65b 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs @@ -88,9 +88,7 @@ public AddRangeResult AddAccountRange( in ValueHash256? hashLimit = null!) { if (!accounts.Any()) - { - return AddRangeResult.OK; - } + throw new ArgumentException("Cannot be empty.",nameof(accounts)); ITrieStore store = _trieStorePool.Get(); try { diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs index 23131d0cc42..fc3b872797a 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs @@ -33,7 +33,7 @@ public static (AddRangeResult result, bool moreChildrenToRight, List(), new List()); + throw new ArgumentException("Cannot be empty.",nameof(accounts)); ValueHash256 lastHash = accounts[^1].Path; (AddRangeResult result, List<(TrieNode, TreePath)> sortedBoundaryList, bool moreChildrenToRight) = From 51918602f3a99b04df87893ecccc0a60a46f6901 Mon Sep 17 00:00:00 2001 From: ak88 Date: Wed, 22 May 2024 16:09:17 +0200 Subject: [PATCH 4/8] format --- .../SnapSync/SnapProviderTests.cs | 2 +- .../Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs | 6 +++--- .../Nethermind.Synchronization/SnapSync/SnapProvider.cs | 2 +- .../SnapSync/SnapProviderHelper.cs | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs index 97fbd0c0768..f27df6c029a 100644 --- a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs +++ b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs @@ -46,7 +46,7 @@ public void AddAccountRange_ResponseHasEmptyListOfAccountsAndOneProof_ReturnsExp using ProgressTracker progressTracker = new(Substitute.For(), dbProvider.GetDb(DbNames.State), LimboLogs.Instance); dbProvider.RegisterDb(DbNames.Code, new MemDb()); AccountRange accountRange = new(Keccak.Zero, Keccak.Zero, Keccak.MaxValue); - AccountsAndProofs accountsAndProofs = new (); + AccountsAndProofs accountsAndProofs = new(); accountsAndProofs.PathAndAccounts = new List().ToUnpooledList(); accountsAndProofs.Proofs = new List { new byte[] { 0x0 } }.ToUnpooledList(); diff --git a/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs b/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs index f570587216c..00cda4c0280 100644 --- a/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs +++ b/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs @@ -14,12 +14,12 @@ namespace Nethermind.Synchronization.Test; /// This is list that does not require disposing and is meant for test purposes. /// /// -internal class UnpooledReadOnlyList : List,IOwnedReadOnlyList +internal class UnpooledReadOnlyList : List, IOwnedReadOnlyList { public UnpooledReadOnlyList(IEnumerable enumerable) : base(enumerable) - {} + { } public UnpooledReadOnlyList(IReadOnlyList list) : base(list) - {} + { } public ReadOnlySpan AsSpan() { return this.AsSpan(); diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs index df6d451c65b..b2468420fdb 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs @@ -88,7 +88,7 @@ public AddRangeResult AddAccountRange( in ValueHash256? hashLimit = null!) { if (!accounts.Any()) - throw new ArgumentException("Cannot be empty.",nameof(accounts)); + throw new ArgumentException("Cannot be empty.", nameof(accounts)); ITrieStore store = _trieStorePool.Get(); try { diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs index fc3b872797a..00fba6761b7 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs @@ -33,7 +33,7 @@ public static (AddRangeResult result, bool moreChildrenToRight, List sortedBoundaryList, bool moreChildrenToRight) = From 09ff69ced8f741376cdc2a9ce704d8be52e3b915 Mon Sep 17 00:00:00 2001 From: ak88 Date: Wed, 22 May 2024 16:16:58 +0200 Subject: [PATCH 5/8] fix test --- .../SnapSync/SnapProviderTests.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs index f27df6c029a..8bc522400ca 100644 --- a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs +++ b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs @@ -23,8 +23,11 @@ public class SnapProviderTests [Test] public void AddAccountRange_AccountListIsEmpty_ThrowArgumentException() { + MemDb db = new(); IDbProvider dbProvider = new DbProvider(); + dbProvider.RegisterDb(DbNames.State, db); using ProgressTracker progressTracker = new(Substitute.For(), dbProvider.GetDb(DbNames.State), LimboLogs.Instance); + dbProvider.RegisterDb(DbNames.Code, new MemDb()); SnapProvider sut = new(progressTracker, dbProvider.CodeDb, new NodeStorage(dbProvider.StateDb), LimboLogs.Instance); Assert.That( From d366187ce7facee557d5e6d0a2769dbf8cf001f6 Mon Sep 17 00:00:00 2001 From: ak88 Date: Thu, 23 May 2024 09:42:38 +0200 Subject: [PATCH 6/8] Update src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs Co-authored-by: Lukasz Rozmej --- .../SnapSync/SnapProviderTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs index 8bc522400ca..4048227dde9 100644 --- a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs +++ b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs @@ -49,7 +49,7 @@ public void AddAccountRange_ResponseHasEmptyListOfAccountsAndOneProof_ReturnsExp using ProgressTracker progressTracker = new(Substitute.For(), dbProvider.GetDb(DbNames.State), LimboLogs.Instance); dbProvider.RegisterDb(DbNames.Code, new MemDb()); AccountRange accountRange = new(Keccak.Zero, Keccak.Zero, Keccak.MaxValue); - AccountsAndProofs accountsAndProofs = new(); + using AccountsAndProofs accountsAndProofs = new(); accountsAndProofs.PathAndAccounts = new List().ToUnpooledList(); accountsAndProofs.Proofs = new List { new byte[] { 0x0 } }.ToUnpooledList(); From 281f35d168176046d72456509f422e8add39d771 Mon Sep 17 00:00:00 2001 From: ak88 Date: Thu, 23 May 2024 10:36:58 +0200 Subject: [PATCH 7/8] review comments --- .../Nethermind.Synchronization/SnapSync/SnapProvider.cs | 4 ++-- .../Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs index b2468420fdb..9e902ded317 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProvider.cs @@ -51,7 +51,7 @@ public AddRangeResult AddAccountRange(AccountRange request, AccountsAndProofs re { AddRangeResult result; - if (!response.PathAndAccounts.Any()) + if (response.PathAndAccounts.Count == 0) { _logger.Trace($"SNAP - GetAccountRange - requested expired RootHash:{request.RootHash}"); @@ -87,7 +87,7 @@ public AddRangeResult AddAccountRange( IReadOnlyList proofs = null, in ValueHash256? hashLimit = null!) { - if (!accounts.Any()) + if (accounts.Count == 0) throw new ArgumentException("Cannot be empty.", nameof(accounts)); ITrieStore store = _trieStorePool.Get(); try diff --git a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs index 00fba6761b7..ea9dfc1f70a 100644 --- a/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs +++ b/src/Nethermind/Nethermind.Synchronization/SnapSync/SnapProviderHelper.cs @@ -32,7 +32,7 @@ public static (AddRangeResult result, bool moreChildrenToRight, List Date: Thu, 23 May 2024 11:00:38 +0200 Subject: [PATCH 8/8] removed UnpooledReadOnlyList --- .../SnapSync/SnapProviderTests.cs | 5 ++- .../UnpooledReadOnlyList.cs | 37 ------------------- 2 files changed, 3 insertions(+), 39 deletions(-) delete mode 100644 src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs diff --git a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs index 4048227dde9..58e0a808b06 100644 --- a/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs +++ b/src/Nethermind/Nethermind.Synchronization.Test/SnapSync/SnapProviderTests.cs @@ -4,6 +4,7 @@ using FluentAssertions; using Nethermind.Blockchain; using Nethermind.Core.Crypto; +using Nethermind.Core.Extensions; using Nethermind.Db; using Nethermind.Logging; using Nethermind.State.Snap; @@ -50,8 +51,8 @@ public void AddAccountRange_ResponseHasEmptyListOfAccountsAndOneProof_ReturnsExp dbProvider.RegisterDb(DbNames.Code, new MemDb()); AccountRange accountRange = new(Keccak.Zero, Keccak.Zero, Keccak.MaxValue); using AccountsAndProofs accountsAndProofs = new(); - accountsAndProofs.PathAndAccounts = new List().ToUnpooledList(); - accountsAndProofs.Proofs = new List { new byte[] { 0x0 } }.ToUnpooledList(); + accountsAndProofs.PathAndAccounts = new List().ToPooledList(); + accountsAndProofs.Proofs = new List { new byte[] { 0x0 } }.ToPooledList(); SnapProvider sut = new(progressTracker, dbProvider.CodeDb, new NodeStorage(dbProvider.StateDb), LimboLogs.Instance); diff --git a/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs b/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs deleted file mode 100644 index 00cda4c0280..00000000000 --- a/src/Nethermind/Nethermind.Synchronization.Test/UnpooledReadOnlyList.cs +++ /dev/null @@ -1,37 +0,0 @@ -// SPDX-FileCopyrightText: 2024 Demerzel Solutions Limited -// SPDX-License-Identifier: LGPL-3.0-only - -using Nethermind.Core.Collections; -using System; -using System.Collections; -using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks; - -namespace Nethermind.Synchronization.Test; -/// -/// This is list that does not require disposing and is meant for test purposes. -/// -/// -internal class UnpooledReadOnlyList : List, IOwnedReadOnlyList -{ - public UnpooledReadOnlyList(IEnumerable enumerable) : base(enumerable) - { } - public UnpooledReadOnlyList(IReadOnlyList list) : base(list) - { } - public ReadOnlySpan AsSpan() - { - return this.AsSpan(); - } - - public void Dispose() - { - } -} - -internal static class IOwnedReadOnlyListExtensions -{ - public static IOwnedReadOnlyList ToUnpooledList(this IEnumerable enumerable) => new UnpooledReadOnlyList(enumerable); - public static IOwnedReadOnlyList ToUnpooledList(this IReadOnlyCollection collection) => new UnpooledReadOnlyList(collection); -}