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

Fix max op not taken in account #3587

Merged
merged 3 commits into from
Feb 20, 2023
Merged

Fix max op not taken in account #3587

merged 3 commits into from
Feb 20, 2023

Conversation

AurelienFT
Copy link
Contributor

Currently someone can create a block with more than 5000 operations (it happens when the last blocks are not produced on the same thread) as there is no limit on the number of ops but on maximum size of a block (in bytes) or maximum gas.

But in the workflow, of block propagation when we receive a list of operation from a block we assume we can have only 5k operations (to avoid deserializing a too big message and OOM). So the block producers of these blocks were disconnected from their peers and their blocks not propagated.

@AurelienFT AurelienFT requested a review from Eitu33 February 19, 2023 23:00
@AurelienFT AurelienFT changed the title Fix max op not taken in account. Fix max op not taken in account Feb 19, 2023
massa-pool-worker/src/operation_pool.rs Outdated Show resolved Hide resolved
@aoudiamoncef
Copy link
Contributor

aoudiamoncef commented Feb 20, 2023

@AurelienFT, @modship
Could we improve this error message in the future ? IMHO, it's not clear and may be logging the payload in it's string format could be better ?

massa_network_worker::binders: error deserializing message: Error(Failed Message deserialization / Failed ReplyForBlocks deserialization / Count / Failed blockId deserialization / Failed Vec<OperationId> deserialization / Failed length deserialization / Failed u32 deserialization / Fail / Input: [188, 57, 0, 1, 204, 108, 122, 232, 57, 65, 226, 65, 224, 87, 18, 122, 205, 141, 49, 238, 93, 153, 196, 12, 161, 167, 74, 34, 220, 213, 17, 223, 51, 169, 0, 3, 133, 47, 33, 233, 225, 200, 28, 126, 47, 99, 50, 66, 223, 248, 204, 53, 103, 159, 208, 34, 59, 176, 228, 219, 150, 251, 138, 102, 167, 176, 0, 5, 3, 213, 27, 156, 227, 98, 88, 87, 88, 48, 129, 7, 233, 177, 190, 239, 189, 5, 45, 46, 77, 178, 200, 179, 243, 75, 148, 148, 235, 80, 0, 8, 37, 164, 70, 247, 114, 2, 72, 182, 42, 214, 210, 176, 82, 74, 189, 228, 144, 161, 93, 6, 13, 94, 29, 66, 180, 190, 188, 227, 54, 188, 0, 8, 106, 172, 200, 13, 93, 137, 150, 11, 108, 60, 222, 83, 87, 101, 95, 147, 127, 248, 115, 167, 129, 121, 149, 209, 162, 65, 194, 57, 182, 93, 0, 8, 239, 19, 58, 151, 235, 126, 18, 125, 51, 66, 219, 35, 21, 251, 13, 217, 147, 36, 126, 212, 72, 23, 94, 17, 73, 29, 154, 23, 215, 157, 0, 11, 191, 56, 33, 198, 174, 211, 246, 189, 1, 163, 88, 39, 156, 143, 76, 71, 64, 102, 38, 33, 169, 92, 193, 76, 80, 21, 200, 50, 200, 47, 0, 12, 58, 5, 36, 25, 93, 88, 241, 81, 121, 247, 151, 215, 11, 14, 235, 153, 61, 244, 102, 14, 47, 138, 240, 255, 61, 118, 145, 61, 77, 39, 0, 15, 49, 203, 141, 188, 5, 39, 58, 210, 142, 164, 238, 218, 32, 20, 106, 249, 5, 201, 161, 183, 208, 255, 146, 213, 201, 65, 141, 239, 90, 184, 0, 17, 186, 251, 210, 153, 148, 21, 194, 159, 1, 143, 246, 0, 4, 161, 199, 49, 172, 255, 201, 134, 1, 146, 88, 108, 37, 103, 206, 92, 54, 109, 0, 19, 17, 28, 87, 229, 74, 188, 4, 8, 98, 30, 245, 62, 112, 130, 117, 25, 133, 116, 138, 115, 151, 0, 124, 131, 221, 136, 185, 209, 147, 178, 0, 36, 47, 160, 17, 151, 60, 26, 10, 205, 187, 83, 131, 75, 147, 136, 227, 128, 90, 29, 0, 86, 145, 140, 22, 154, 87, 217, 89, 35, 82, 8, 0, 38, 63, 140, 156, 123, 184, 183, 191, 225, 99, 29, 49, 90, 165, 46, 68, 31, 58, 255, 242, 71, 190, 168, 175, 114, 52, 13, 174, 121, 119, 18, 0, 63, 73, 162, 89, 225, 20, 117, 10, 27, 110, 201, 251, 59, 141, 25, 148, 120, 72, 101, 25, 196, 78, 160, 106, 243, 73, 77, 14, 110, 198, 7, 0, 75, 202, 212, 74, 161, 160, 15, 142, 190, 165, 42, 15, 90, 181, 10, 229, 21, 232, 185, 227, 40, 82, 178, 143, 74, 18, 12, 251, 72, 73, 37, 0, 75, 230, 217, 232, 155, 76, 242, 16, 118, 72, 157, 74, 85, 164, 170, 12, 6, 8, 51, 232, 221, 192, 145, 219, 97, 140, 134, 35, 165, 185, 158, 0, 80, 225, 198, 181, 10, 23, 58, 122, 238, 59, 43, 50, 133, 200, 86, 22, 45, 184, 62, 219, 78, 125, 203, 148, 142, 210, 132, 51, 40, 222, 68, 0, 81, 113, 128, 229, 87, 227, 115, 169, 120, 238, 47, 136, 113, 28, 58, 148, 159, 182, 72, 82, 146, 23, 105, 18, 230, 141, 12, 123, 119, 238, 55, 0, 92, 53, 218, 159, 224, 19

@AurelienFT
Copy link
Contributor Author

It's bytes serialized there is no string representation that means anything. I find the error pretty clear except the fact that we don't have the upper limit directly on the error but could be fetched easily in the code

@aoudiamoncef
Copy link
Contributor

It's bytes serialized there is no string representation that means anything. I find the error pretty clear except the fact that we don't have the upper limit directly on the error but could be fetched easily in the code

Given the max size could help us, as we rely on error messages to detect bugs.

@AurelienFT
Copy link
Contributor Author

I'm not fully sure it's possible with nom

@Ben-PH
Copy link
Contributor

Ben-PH commented Feb 20, 2023

I thought I opened an issue about this at some point, but if I have, it's lost.

When I was working on the block header deserializer, I noticed some weird issues. The deserializer assumes a certain number of a collection that is being deserialized, and noms on the bytes under that assumption. By the time it gets to the next stage of the deserializer, it has under/over shot the data being nom'd on.

...because the under/over shoot just parses data as-is (e.g. it's just going into a byte-vec), it won't fail until it reaches the next part of deserialization. It triggers an error after the cause of an error, and gets confused about the source.

It seems to be the same kind of error?

@AurelienFT
Copy link
Contributor Author

@Ben-PH It could be a problem yes but here the error was triggered at the right place.

@AurelienFT AurelienFT merged commit 91aadd3 into testnet_20 Feb 20, 2023
@AurelienFT AurelienFT mentioned this pull request Feb 27, 2023
bors bot added a commit that referenced this pull request Mar 1, 2023
3489: Testnet 20 r=AurelienFT a=AurelienFT

Merged in this testnet:

- #3475 
- #3549
- #3562 
- #3462 
- #3492 
- #3502 
- #3495 
- #3556 
- #3511 
- #3498 
- #3566 
- #3557 
- #3576 
- #3579 
- #3507 
- #3585 
- #3587 
- #3580 
- #3590 
- #3549 
- #3455 
- #3601 
- #3602 
- #3606 
- #3588 
- #3603 
- #3554

Co-authored-by: AurelienFT <aurelien.foucault@epitech.eu>
Co-authored-by: Ben PHL <benphawke@gmail.com>
Co-authored-by: Modship <yeskinokay@gmail.com>
Co-authored-by: Ben <benphawke@gmail.com>
Co-authored-by: Eitu33 <89928840+Eitu33@users.noreply.github.com>
Co-authored-by: Sydhds <sylvain.delhomme@gmail.com>
Co-authored-by: modship <lu@massa.net>
Co-authored-by: Moncef AOUDIA <22281426+aoudiamoncef@users.noreply.github.com>
Co-authored-by: Moncef AOUDIA <ma@massa.net>
@Ben-PH Ben-PH mentioned this pull request Mar 20, 2023
28 tasks
@AurelienFT AurelienFT deleted the fix_max_op branch May 29, 2023 08:13
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.

4 participants