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.

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

ERA history import/export start #6173

Closed
wants to merge 45 commits into from

Conversation

ak88
Copy link
Contributor

@ak88 ak88 commented Oct 9, 2023

R#eceipt decoder

Fixes Closes Resolves #

Please choose one of the keywords above to refer to the issue this PR solves followed by the issue number (e.g. Fixes #000). If no issue number, remove the line. Also, remove everything marked optional that is not applicable. Remove this note after reading.

Changes

  • List the changes

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

  • Yes
  • No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

  • Yes
  • No

If yes, fill in the details here. Remove if not applicable.

Remarks

!!Work in progress!!

Copy link
Contributor

Choose a reason for hiding this comment

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

it shouldn't be updated in this PR, please revert


namespace Nethermind.Serialization.Rlp
{
public class BlockBodyDecoder : IRlpValueDecoder<BlockBody>, IRlpStreamDecoder<BlockBody>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between these decoders and existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will refactor this.

Copy link
Member

Choose a reason for hiding this comment

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

You could use it in BlockDecoder, also we have a BlockBodyDecoder in the networking part, maybe they are similar.

@LukaszRozmej
Copy link
Member

Related to: ethereum/go-ethereum#26621

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not finished. This will be used when integrating with Node CLI.

@ak88 ak88 marked this pull request as ready for review November 6, 2023 13:02
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Some initial comments

@@ -13,6 +13,7 @@
<PackageVersion Include="Colorful.Console" Version="1.2.15" />
<PackageVersion Include="CommandLineParser" Version="2.9.1" />
<PackageVersion Include="ConcurrentHashSet" Version="1.3.0" />
<PackageVersion Include="Cortex.SimpleSerialize" Version="0.2.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Not newer Nethermind.Serialization.Ssz?
@flcl42 Can you advise?

@@ -68,6 +69,7 @@
<PackageVersion Include="Seq.Api" Version="2023.3.0" />
<PackageVersion Include="SharpCompress" Version="0.34.1" />
<PackageVersion Include="Shouldly" Version="4.2.1" />
<PackageVersion Include="Snappier" Version="1.1.1" />
Copy link
Member

Choose a reason for hiding this comment

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

Is it faster or better than then already used Snappy.Standard?

Copy link
Contributor Author

@ak88 ak88 Nov 7, 2023

Choose a reason for hiding this comment

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

The problem is that era1 uses framing format which the existing lib does not support

src/Nethermind/Nethermind.Core.Test/Builders/TestItem.cs Outdated Show resolved Hide resolved
using SnappyStream snappyStream = new(memoryStream, CompressionMode.Compress, true);
await snappyStream.WriteAsync(bytes, cancellation);
await snappyStream.FlushAsync();
bytes = memoryStream.ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

lot of allocations

src/Nethermind/Nethermind.Era1/E2Store.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Era1/E2Store.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Era1/E2Store.cs Outdated Show resolved Hide resolved
var buf = ArrayPool<byte>.Shared.Rent(HeaderSize);
try
{
_stream!.Position = offset;
Copy link
Member

Choose a reason for hiding this comment

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

Are we doing a lot of jumping/seeking on the stream or is it more of linear scans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every iteration will read the offset from the block index, which is appended to the file.
That will probably trigger a lot of random reads.
I thought about loading the whole index instead at the expense of memory WDYT?

commit 836b4aa29eb31f8b903696eef4edfa3f96fb7a05
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Thu Nov 9 13:35:34 2023 +0100

    in params

commit 4c7104adfdfc8cf485b7bfe5b23a02e61daeb98c
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Thu Nov 9 13:15:50 2023 +0100

    update snappier

commit 6d97e87a211f559d1263a4f7a6caf97977a6d25d
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Thu Nov 9 12:57:11 2023 +0100

    small optimization

commit 8d9e39b0f85eb23bf05b1b7cc47acafe7f684108
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Wed Nov 8 15:46:44 2023 +0100

    format

commit d47b9839866676feff5ae2352af793f698f4cd97
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Wed Nov 8 15:41:57 2023 +0100

    changes for PR

commit 5c4614c2f68bf104220451d4c945579adfe32575
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Wed Nov 8 00:28:58 2023 +0100

    changes for PR
commit e4bbfed2577d4189637c86a92e6d2848820701fa
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Thu Nov 9 22:57:06 2023 +0100

    load bock index

commit 9e65cd99544d97610dd96fe5fd406e40be792455
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Thu Nov 9 22:05:48 2023 +0100

    small change

commit 8d207c6a7fc0e55f375fd4f169d04f0fa5ac85f7
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Thu Nov 9 21:43:37 2023 +0100

    load whole index

commit 126cc0c4ad42dae97a9fdf3951c3e670b71ad2b3
Author: Anders Holmbjerg Kristiansen <anders.holmbjerg@hotmail.com>
Date:   Thu Nov 9 16:37:38 2023 +0100

    small fix
@ak88 ak88 closed this Jan 16, 2024
@smartprogrammer93
Copy link
Contributor

Closing in favor of #6547

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants