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

Fix randomly failing receipt deserialization #5120

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Fix randomly failing receipt deserialization #5120

merged 3 commits into from
Jan 10, 2023

Conversation

asdacap
Copy link
Contributor

@asdacap asdacap commented Jan 10, 2023

Fix #5116

  • Occationally there are unexplainable run with lots of dropping peers.
  • Closer inspection shows that the node is disconnecting peers due to error in forward sync, specifically empty receipt received.
  • Anyway, it turns out that there is a bug in the receipt deserialization logic which can get triggered if the first element of the byte buffer allocated in ZeroNettyP2PHandler is equal to Rlp.OfEmptySequence[0].

Changes

  • Fix deserialization bug.
  • Added extra check in ProtocolHandlerBase that helped me debug this issue.

Types of changes

  • Bugfix (a non-breaking change that fixes an issue)

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

  • Can by triggered manually by something like this in ZeroNettyP2PHandler.
                IByteBuffer output = PooledByteBufferAllocator.Default.Buffer(uncompressedLength + 1);
                output.WriteByte(Rlp.OfEmptySequence[0]);
                output.ReadByte();

LukaszRozmej
LukaszRozmej approved these changes Jan 10, 2023
View reviewed changes
src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/ProtocolHandlerBase.cs
T result = _serializer.Deserialize<T>(data);
if (data.IsReadable())
{
throw new IncompleteDeserializationException(
Copy link
Member

Choose a reason for hiding this comment

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

What does that exception achieve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make it easier to detect deserialization issue.

LukaszRozmej
LukaszRozmej reviewed Jan 10, 2023
View reviewed changes
...Nethermind/Nethermind.Network/P2P/Subprotocols/Eth/V63/Messages/ReceiptsMessageSerializer.cs Outdated
if (byteBuffer.Array.Length == 0 || byteBuffer.Array.First() == Rlp.OfEmptySequence[0]) return new ReceiptsMessage(null);
if (byteBuffer.ReadableBytes == 0)
{
return new ReceiptsMessage(null);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to have some ReceiptsMessage.Empty ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure enough.

@asdacap asdacap merged commit 1edfd97 into master Jan 10, 2023
@asdacap asdacap deleted the fix/random-receipt-deserialization-failure branch January 10, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@kamilchodola kamilchodola Awaiting requested review from kamilchodola

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@asdacap @LukaszRozmej