core/consensus: add support for pointer values on the wire #1838
Conversation
| @@ -73,150 +69,28 @@ func TestSigning(t *testing.T) { | |||
| require.False(t, ok) | |||
| } | |||
|
|
|||
| func TestBackwardsCompatibility(t *testing.T) { | |||
There was a problem hiding this comment.
this tests 2 updates ago, we already removed support, can remove now.
core/corepb/v1/consensus.proto
Outdated
| repeated QBFTMsg justification = 2; | ||
| QBFTMsg msg = 1; | ||
| repeated QBFTMsg justification = 2; | ||
| repeated google.protobuf.Any values = 3; |
There was a problem hiding this comment.
so the values are added here and can therefore be removed from QBFTMsg above once we remove support for v0.13.
There was a problem hiding this comment.
Nit: can we add a comment for future reference?
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1838 +/- ##
==========================================
+ Coverage 55.34% 55.45% +0.10%
==========================================
Files 170 170
Lines 22332 22254 -78
==========================================
- Hits 12360 12340 -20
+ Misses 8385 8337 -48
+ Partials 1587 1577 -10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -545,3 +568,22 @@ func newRoundTimer(round int64) (<-chan time.Time, func()) { | |||
| timer := time.NewTimer(roundStart + (time.Duration(round) * roundIncrease)) | |||
| return timer.C, func() { timer.Stop() } | |||
| } | |||
|
|
|||
| func valuesByHash(values []*anypb.Any) (map[[32]byte]*anypb.Any, error) { | |||
| resp := make(map[[32]byte]*anypb.Any) | |||
There was a problem hiding this comment.
Should this function return error if values is empty?
There was a problem hiding this comment.
nope, empty values result in a empty map, this is required for legacy v0.13 support.
core/corepb/v1/consensus.proto
Outdated
| repeated QBFTMsg justification = 2; | ||
| QBFTMsg msg = 1; | ||
| repeated QBFTMsg justification = 2; | ||
| repeated google.protobuf.Any values = 3; |
There was a problem hiding this comment.
Nit: can we add a comment for future reference?
|
@gsora I do not think we should get rid of |
|
Sorry, didn't mean to imply that I'm nitpicking though, because I really would love to avoid pointers when possible. |
|
Since the |
corverroos
added
the
merge when ready
Upgrades the QBFT wire protocol (without breaking backwards compatibility) by adding the whole value to the
ConsensusMsgenvelope and hashes of those values to theQBFTMsgitself. This will allow us to remove the values from the QBFT messages, which will reduce network bandwidth usage by a lot (90% for large values; beacon blocks).This increases the network bandwidth for next release v0.14, but once we remove support for v0.13, it will reduce it by a lot.
category: feature
ticket: #1552