celo-org / celo-blockchain

Official repository for the golang Celo Blockchain
https://celo.org
GNU Lesser General Public License v3.0
560 stars 198 forks source link

Remove per-peer rlp.Encode calls of istanbul messages #2175

Closed hbandura closed 1 year ago

hbandura commented 1 year ago

Istanbul consensus is currently rlp encoding messages twice, and decoding them twice upon receiving one. The second encoding is done per peer, so if the same message is multicasted to 110 peers, it is encoded once, then 110 times more. This commit ensures the double encoding is done just once, reducing from 111 to 2 the calls to rlp.EncodeToBytes. A better way to handle this (but more disruptive) is completely drop the double encoding and double decoding, but would imply a protocol change.

github-actions[bot] commented 1 year ago
5862 passed, 45 skipped
codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 40.74% and project coverage change: -0.02% :warning:

Comparison is base (2d25832) 55.14% compared to head (278bf9a) 55.13%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2175 +/- ## ========================================== - Coverage 55.14% 55.13% -0.02% ========================================== Files 678 679 +1 Lines 114321 114336 +15 ========================================== - Hits 63047 63034 -13 - Misses 47391 47410 +19 - Partials 3883 3892 +9 ``` | [Files Changed](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org) | Coverage Δ | | |---|---|---| | [consensus/istanbul/announce/vc\_gossiper.go](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#diff-Y29uc2Vuc3VzL2lzdGFuYnVsL2Fubm91bmNlL3ZjX2dvc3NpcGVyLmdv) | `0.00% <0.00%> (ø)` | | | [consensus/istanbul/backend/handler.go](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#diff-Y29uc2Vuc3VzL2lzdGFuYnVsL2JhY2tlbmQvaGFuZGxlci5nbw==) | `32.36% <0.00%> (ø)` | | | [consensus/istanbul/core/handler.go](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#diff-Y29uc2Vuc3VzL2lzdGFuYnVsL2NvcmUvaGFuZGxlci5nbw==) | `82.28% <ø> (-0.11%)` | :arrow_down: | | [consensus/istanbul/proxy/val\_enodes\_share.go](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#diff-Y29uc2Vuc3VzL2lzdGFuYnVsL3Byb3h5L3ZhbF9lbm9kZXNfc2hhcmUuZ28=) | `51.00% <0.00%> (ø)` | | | [eth/protocols/eth/celo\_peer.go](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#diff-ZXRoL3Byb3RvY29scy9ldGgvY2Vsb19wZWVyLmdv) | `0.00% <0.00%> (ø)` | | | [eth/protocols/eth/peer.go](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#diff-ZXRoL3Byb3RvY29scy9ldGgvcGVlci5nbw==) | `28.27% <ø> (+0.22%)` | :arrow_up: | | [consensus/istanbul/backend/message\_senders.go](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org#diff-Y29uc2Vuc3VzL2lzdGFuYnVsL2JhY2tlbmQvbWVzc2FnZV9zZW5kZXJzLmdv) | `58.02% <73.33%> (+1.08%)` | :arrow_up: | ... and [32 files with indirect coverage changes](https://app.codecov.io/gh/celo-org/celo-blockchain/pull/2175/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=celo-org)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 1 year ago

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit d2a8d929f195c8e217c827c5db5d0d20f77a0757

coverage: 49.8% of statements across all listed packages
coverage:  63.0% of statements in consensus/istanbul
coverage:  40.1% of statements in consensus/istanbul/announce
coverage:  54.6% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  64.6% of statements in consensus/istanbul/core
coverage:  45.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.4% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random