Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Trace/db traces #4535

Merged
merged 81 commits into from
Dec 6, 2022
Merged

Trace/db traces #4535

merged 81 commits into from
Dec 6, 2022

Conversation

LukaszRozmej
Copy link
Member

@LukaszRozmej LukaszRozmej commented Sep 7, 2022 •
edited
Loading

Resolves #4205, #4143

This is based on 1.14.6 version. Merge ready and Gnosis merge ready if someone wants to use it.

Changes:

  • Introduces TraceStore plugin
  • Allows of saving parity like traces (trace module) to database and later serving them without having the state data
  • Requires block to be processed to store the trace, currently no sync options
  • Node can support pruning both InMemory and Full, so while blocks need to be processed, state can be pruned.
  • Supports pruning of old traces, has a limit of how many block traces needs to be kept (Head - limit ... Head], but can also keep all traces from genesis (keep in mind that's a LOT of storage!)
  • Supports configuring what traces we want to keep
  • Replaces Trace RPC module to try to grab traces from database first and fallback to state only when they are not present
  • Extends Nethedmind to be able to replace JSON RPC module
  • Fixes bug with EndBlockTrace being called too early - before applying rewards

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Testing

Requires testing

  • Yes
  • No

In case you checked yes, did you write tests??

  • Yes
  • No

Comments about testing , should you have some (optional)

Further comments (optional)

I've tested by syncing chunk of Gnosis chain. And sending RPC requests. Testing further...

@LukaszRozmej LukaszRozmej marked this pull request as draft September 8, 2022 14:48
asdacap
asdacap reviewed Sep 9, 2022
View reviewed changes
Copy link
Contributor

@asdacap asdacap left a comment

Choose a reason for hiding this comment

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

This is a very large PR, its very hard to review. Please try to separate what can be separated next time.

Overall, I don't see any problem, but I can't approve it for master without @MarekM25 or @dceleda blessing.

@asdacap
Copy link
Contributor

asdacap commented Sep 9, 2022

Also, got an estimation of storage usage?

@LukaszRozmej LukaszRozmej changed the base branch from master to release/1.14.6 November 28, 2022 12:10
@LukaszRozmej LukaszRozmej changed the base branch from release/1.14.6 to master November 28, 2022 12:10
@LukaszRozmej LukaszRozmej marked this pull request as ready for review November 28, 2022 12:10
@LukaszRozmej
Ignore ParityResult.IsEmpty in JSON
a458eb9
@LukaszRozmej
Fix remove empty results
08207dc
@LukaszRozmej
Merge 1.14.7
a9a05fe
@LukaszRozmej
Merge remote-tracking branch 'origin/master' into trace/db-traces
778780f 
# Conflicts:
#	src/Nethermind/Directory.Build.props
#	src/Nethermind/Nethermind.Consensus/Processing/BlockRef.cs
#	src/Nethermind/Nethermind.Core.Test/Collections/ArrayPoolListTests.cs
#	src/Nethermind/Nethermind.Core/BlockInfo.cs
#	src/Nethermind/Nethermind.Core/Collections/ArrayPoolList.cs
#	src/Nethermind/Nethermind.Core/Collections/IListExtensions.cs
#	src/Nethermind/Nethermind.Core/Extensions/Bytes.cs
#	src/Nethermind/Nethermind.Core/Resettables/ResettableList.cs
#	src/Nethermind/Nethermind.Db/RocksDbInitializer.cs
#	src/Nethermind/Nethermind.Evm/Tracing/BlockTracerBase.cs
#	src/Nethermind/Nethermind.Evm/Tracing/GethStyle/GethLikeBlockTracer.cs
#	src/Nethermind/Nethermind.Evm/Tracing/IBlockTracer.cs
#	src/Nethermind/Nethermind.Evm/Tracing/ITxTracer.cs
#	src/Nethermind/Nethermind.Evm/Tracing/ParityStyle/ParityResult.cs
#	src/Nethermind/Nethermind.Evm/Tracing/Proofs/ProofBlockTracer.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/RpcModuleProviderTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/TestRpcModuleProvider.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/Trace/ParityTxTraceFromReplayConverterTest.cs
#	src/Nethermind/Nethermind.JsonRpc/JsonRpcService.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/BoundedModulePool.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/IRpcModulePool.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/IRpcModuleProvider.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/NullModuleProvider.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/RpcModuleProvider.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/SingletonModulePool.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/Subscribe/SubscribeRpcModule.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/Trace/ParityTxTraceFromReplay.cs
#	src/Nethermind/Nethermind.JsonRpc/Modules/Trace/TraceModuleFactory.cs
#	src/Nethermind/Nethermind.Serialization.Json/EthereumJsonSerializer.cs
#	src/Nethermind/Nethermind.Serialization.Json/KeccakConverter.cs
#	src/Nethermind/Nethermind.Serialization.Rlp/RlpDecoderExtensions.cs
#	src/Nethermind/Nethermind.State/IStateTracer.cs
#	src/Nethermind/Nethermind.State/IStorageTracer.cs
#	src/Nethermind/Nethermind.State/NullStorageTracer.cs
@LukaszRozmej
fix licensing header
87f1b50
@LukaszRozmej
fix whitespace
3952d3e
@LukaszRozmej
Final touches
73b1071
asdacap
asdacap reviewed Dec 5, 2022
View reviewed changes
src/Nethermind/Nethermind.JsonRpc.TraceStore/TraceStoreRpcModule.cs
if (trace is not null)
{
FilterTrace(trace, traceTypes);
result = ResultWrapper<T>.Success(map(trace));
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: Another idea is to create a Map method to ResultWrapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if I understood, but I think like this it is better separated to map just trace types instead of JSON RPC types

asdacap
asdacap reviewed Dec 5, 2022
View reviewed changes
src/Nethermind/Nethermind.JsonRpc.TraceStore/TraceStoreRpcModule.cs
Span<byte> tracesSerialized = _traceStore.GetSpan(block.Hash!);
try
{
if (!tracesSerialized.IsEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: Prefer early return. Keep code to the left, easier to read and probably change easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nested if makes this hard, so there are multiple conditions to return from this path.

asdacap
asdacap reviewed Dec 5, 2022
View reviewed changes
src/Nethermind/Nethermind.JsonRpc.TraceStore/TraceStoreRpcModule.cs
}

traces = null;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: I'm guessing returning trace directly and checking traces is null would also work.

asdacap
asdacap approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@asdacap asdacap left a comment

Choose a reason for hiding this comment

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

Don't see any significant issue.

@sambacha
Copy link

sambacha commented Dec 6, 2022

awesome

marcindsobczak
marcindsobczak approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@marcindsobczak marcindsobczak left a comment

Choose a reason for hiding this comment

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

Looks correct, I didn't find any bug

src/Nethermind/Nethermind.JsonRpc.TraceStore.Tests/DbPersistingBlockTracerTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc.TraceStore.Tests/TraceStorePrunerTests.cs Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/Modules/RpcModuleProvider.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/Modules/Trace/ParityTxTraceFromReplay.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/Modules/Trace/TraceModuleFactory.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Serialization.Json/EthereumJsonSerializer.cs Show resolved Hide resolved
@LukaszRozmej LukaszRozmej merged commit 2e8630b into master Dec 6, 2022
@LukaszRozmej LukaszRozmej deleted the trace/db-traces branch December 6, 2022 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asdacap asdacap asdacap approved these changes

@marcindsobczak marcindsobczak marcindsobczak approved these changes

@rubo rubo Awaiting requested review from rubo rubo is a code owner

@flcl42 flcl42 Awaiting requested review from flcl42

@MarekM25 MarekM25 Awaiting requested review from MarekM25

@dceleda dceleda Awaiting requested review from dceleda

@kamilchodola kamilchodola Awaiting requested review from kamilchodola

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support tracing of synced, not-processed blocks