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

Hotfix to solve a backup stream limit Issue #816

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Hotfix to solve a backup stream limit Issue #816

merged 3 commits into from
Nov 17, 2022

Conversation

eric-gmendes
Copy link
Contributor

@eric-gmendes eric-gmendes commented Oct 19, 2022

Description

We're trying to backup our blockchain but at some point we get the following error:
rpc error: code = ResourceExhausted desc = grpc: received message larger than max (4194312 vs. 4194304)

We did some tests and we found out that an interval of blocks, 32000 to 36981, every single time returns this error.
Looking through the code we find out that the read limit for a stream is 4MB, but the write limit is 21MB. Checking the error message we understand that the backup process is receiving more than 4MB.

We check the appending block function, it checks the size of blocks that will be written to the stream:

	if uint64(w.buf.Len()+len(data)) >= w.maxPayload {
		// send buffered data to client first
		if err := w.flush(); err != nil {
			return err
		}
	}

This code considers only the sum of block's size to be less than 4MB. If you check the ExportEvent Protobuf struct besides the blocks we also send 3 uint64 fields (I'm calling header) in the stream.

Using the golang document, we could realize that these 3 fields add to the stream payload 24 bytes (3 * 8) (I know that polygon edge uses Protobuf and this is the maximum size possible for this 3 fields). So the previous validation may let out 24 bytes and may accept appending more blocks than the stream will support.

The solution that I propose here was to add a const maxHeaderInfoSize with the max size of the fields (that I'm calling header) and change the validation to use it

if uint64(int(maxHeaderInfoSize)+w.buf.Len()+len(data)) >= w.maxPayload

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

The manual test that we did, was to retry the backup process on our blockchain and see if it would finish without problems.
This solution work like charm!

@github-actions
Copy link

github-actions bot commented Oct 19, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@eric-gmendes eric-gmendes marked this pull request as draft October 19, 2022 13:22
@eric-gmendes eric-gmendes changed the title Adding maxHeaderSize const and changing the appendBlock validatation … Hotfix to solve a backup stream limit Issue Oct 19, 2022
@eric-gmendes eric-gmendes marked this pull request as ready for review October 19, 2022 13:28
@eric-gmendes eric-gmendes marked this pull request as draft October 19, 2022 13:29
@eric-gmendes eric-gmendes reopened this Oct 19, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2022
@eric-gmendes eric-gmendes marked this pull request as ready for review October 19, 2022 13:33
@0xPolygon 0xPolygon unlocked this conversation Oct 25, 2022
@vcastellm
Copy link
Contributor

Can you please Please provide a detailed description of what was done in this PR?

@eric-gmendes
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@eric-gmendes
Copy link
Contributor Author

recheck

@eric-gmendes
Copy link
Contributor Author

Sorry @vcastellm, I had a problem with my PR and lost permission to update it with my comments and description. A restore this permission yesterday, and today I provide a description for it.
Please let me know if I need to supply a better description or more details!

server/system_service.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #816 (fb2a666) into develop (337f90a) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head fb2a666 differs from pull request most recent head b530f9e. Consider uploading reports for the commit b530f9e to get more accurate results

@@             Coverage Diff             @@
##           develop     #816      +/-   ##
===========================================
+ Coverage    52.45%   52.46%   +0.01%     
===========================================
  Files          132      132              
  Lines        17520    17520              
===========================================
+ Hits          9190     9192       +2     
+ Misses        7661     7659       -2     
  Partials       669      669              
Impacted Files Coverage Δ
network/server.go 75.79% <0.00%> (+0.42%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@vcastellm vcastellm merged commit 44c2ab3 into 0xPolygon:develop Nov 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants