network: proposal payload compression #4589

algorandskiy · 2022-09-26T16:46:00Z

Summary

Inproduce proposal payload compression in protocol version 2.2.

Acceptance:

Unit tests incl 2.1 and 2.2 peers mix
All e2e tests pass
Manual test master and feature nodes on TwoNodes50Each template

Testing

Unit test checking old + new ws protocol combinations
Manual TwoNotes50Each test with master (m) + feature (f)
Manual ThreeNodesEvenDist test with [m]m+f, m+f[f] where [r] is a relay node
Perf test

Possible optimization

If all peers support compressed proposals, do not allocate memory for non-compressed data batch. Possibly in another PR.

jannotti · 2022-09-26T16:47:40Z

zstd seems like a very good choice, since we can seed the dictionary it uses. it ought to do a great job on our repetitive msgpack txns after that.

codecov · 2022-09-26T17:10:27Z

Codecov Report

Merging #4589 (3f7d72f) into master (bb7c59f) will increase coverage by 0.07%.
The diff coverage is 82.26%.

@@            Coverage Diff             @@
##           master    #4589      +/-   ##
==========================================
+ Coverage   54.34%   54.42%   +0.07%     
==========================================
  Files         402      403       +1     
  Lines       51793    51917     +124     
==========================================
+ Hits        28149    28255     +106     
- Misses      21276    21284       +8     
- Partials     2368     2378      +10

Impacted Files	Coverage Δ
network/wsPeer.go	`68.93% <76.31%> (+3.42%)`	⬆️
network/msgCompressor.go	`82.35% <82.35%> (ø)`
network/wsNetwork.go	`65.52% <86.53%> (+0.95%)`	⬆️
crypto/merkletrie/trie.go	`66.42% <0.00%> (-2.19%)`	⬇️
crypto/merkletrie/node.go	`91.62% <0.00%> (-1.87%)`	⬇️
data/transactions/verify/txn.go	`76.19% <0.00%> (-0.96%)`	⬇️
ledger/tracker.go	`77.87% <0.00%> (-0.86%)`	⬇️
catchup/service.go	`68.14% <0.00%> (-0.75%)`	⬇️
ledger/acctupdates.go	`70.19% <0.00%> (+0.59%)`	⬆️
... and 2 more

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

network/wsPeer.go

algorandskiy · 2022-10-12T20:27:13Z

Some refactoring, unit tests and rebase to master

jannotti

I'm not so familiar with wsNetwork, but the parts I understood seemed good.

I wonder, since we have to support uncompressed senders anyway, why should we ever accept a compression that is bigger than the message? We might as well use the uncompressed form in that case. This simplifies the compression routine slightly, because we don't need to ask for the bound, we can just insist that it compresses in less than len(d) or refuse.

go.sum

network/msgCompressor.go

jannotti · 2022-10-13T14:39:01Z

network/msgCompressor.go

+func (dec zstdProposalDecompressor) convert(data []byte) ([]byte, error) {
+	r := zstd.NewReader(bytes.NewReader(data))
+	defer r.Close()
+	b := make([]byte, 0, 1024)


I though we are only compressing large messages, so maybe start higher? We could define a constant minCompressedMsgSize?

we compressing all proposals, small and large. I think I like this idea, the magic check above would handle both compressed and non-compressed

oh, right, mspacked, my bad

You could make the compressed encoding start with an additional 4 bytes to include the length up front?

I was considering suggesting that too. Then you could io.ReadFull on the known size (allocated all at once). And the check for exploding messages isn't even needed (it'll just fail to decompress into the buffer, which presumably you've prechecked against the maxsize)

Or, how about we start with a buffer 5x the size of the compressed message?

maybe 2.5x would be a good guess since we see 2-3x compression ratio

Sounds good. That should eliminate most copies.

network/msgCompressor.go

algorandskiy

This simplifies the compression routine slightly, because we don't need to ask for the bound, we can just insist that it compresses in less than len(d) or refuse.

I thought about it, and the only way is to compare the post-compression result (since the bound is always greater, and internally zstd re-allocates).

network/msgCompressor.go

algorandskiy · 2022-10-13T15:54:07Z

network/msgCompressor.go

+func (dec zstdProposalDecompressor) convert(data []byte) ([]byte, error) {
+	r := zstd.NewReader(bytes.NewReader(data))
+	defer r.Close()
+	b := make([]byte, 0, 1024)


we compressing all proposals, small and large. I think I like this idea, the magic check above would handle both compressed and non-compressed

network/msgCompressor.go

…to version

zeldovich · 2022-10-13T20:14:12Z

A common pattern for algod is to take an incoming message and relay it to others. Should we save the compressed message in network.IncomingMessage so that we don't have to re-compress it if we decide to re-broadcast it to other peers?

algorandskiy · 2022-10-13T21:21:57Z

A common pattern for algod is to take an incoming message and relay it to others. Should we save the compressed message in network.IncomingMessage so that we don't have to re-compress it if we decide to re-broadcast it to other peers?

outmsg := wn.handlers.Handle(msg)
...
case Broadcast:
	err := wn.Broadcast(wn.ctx, msg.Tag, msg.Data, false, msg.Sender)

Incoming msg gets converted into Outgoing msg, so re-broadcasting is a case only for some messages. Looks like we might indeed store the compressed part in Incoming and let the handler to propagate it into Outgoing. The only issue we start working with bytes at broadsting level, not with messages. I'll look close into it.

cce · 2022-10-13T21:48:24Z

@zeldovich the issue with proposals is that the relayAction constructs a new transmittedPayload by combining a PriorVote and unauthenticatedProposal that were pulled separately from the voteTracker and proposalStore systems — I ran into this when doing a POC to avoid the re-msgp-encoding of proposals when relaying a proposal in #4568.

I ran into the issue where I believe it isn't totally guaranteed that the PriorVote that was sent to you in the PP/transmittedPayload message (that you fed into agreement by splitting it in two messageEvents) would be the same PriorVote as you send out when relaying the PP/transmittedPayload message. Or maybe it is always the same when you are relaying for period 0?

In any case you can see in my POC I ended up adding references to the original msgp encoding of the unauthenticatedProposal and PriorVote separately, and then do a byte-comparison on PriorVote just to be extra sure it is the same before re-using the encoding...
#4568

But perhaps it would be possible to similarly feed the compressed proposal message into agreement and back out again to avoid re-compressing.

zeldovich · 2022-10-13T22:02:14Z

Hmm, yeah, on closer examination it seems like it will be tricky to do this optimization. You might be right that, in period 0, the prior vote that accompanied the proposal on arrival is the same as (or just as good as?) the prior vote we pull out of the voteTracker, but this also seems a subtle invariant to think about..

I was thinking it would be doable because we try to track some information about the original incoming message (e.g., agreement.MessageHandle), but it seems not sufficient for what we're talking about.

Not worth delaying this PR for this second-order optimization.

algorandskiy · 2022-10-14T14:54:47Z

According to perf tests, PP traffic went down to about 30%, and round time shows less fluctuations.

network/msgCompressor.go

cce · 2022-10-14T17:07:32Z

@zeldovich I guess one thing we could do, but would require a consensus upgrade, is move the compression into the networkAction.do code where we are calling protocol.Encode before sending out a proposal. We could have a new transmittedPayload encoding that was something like

type transmittedPayload struct {
   CompressedEncodedProposal []byte `codec:"pp"`
   PriorVote unauthenticatedVote `codec:"pv"`
}

and it would be agreement's job to decompress the proposal, and also keep a reference to the original compressed and msgp-encoded version in the unauthenticatedProposal type like I had in #4568.

Then we would only be compressing the proposal and not the PriorVote, so we wouldn't have to worry about mixing them up.

algorandskiy added Enhancement Team Carbon-11 labels Sep 26, 2022

algorandskiy self-assigned this Sep 26, 2022

algorandskiy requested review from brianolson and cce September 26, 2022 16:47

cce reviewed Sep 26, 2022

View reviewed changes

network/wsPeer.go Outdated Show resolved Hide resolved

zeldovich reviewed Sep 28, 2022

View reviewed changes

network/wsPeer.go Outdated Show resolved Hide resolved

algorandskiy requested review from cce and zeldovich October 10, 2022 00:21

algorandskiy force-pushed the pavel/pp-compression branch from 3fa1553 to 5418774 Compare October 10, 2022 00:30

zeldovich reviewed Oct 10, 2022

View reviewed changes

network/wsPeer.go Outdated Show resolved Hide resolved

zeldovich reviewed Oct 10, 2022

View reviewed changes

network/wsPeer.go Outdated Show resolved Hide resolved

algorandskiy changed the title ~~WIP: proposal payload compression~~ Proposal payload compression Oct 11, 2022

algorandskiy changed the title ~~Proposal payload compression~~ network: proposal payload compression Oct 11, 2022

algorandskiy added 9 commits October 12, 2022 16:24

WIP: proposal payload compression

1696294

WIP: test

425ee2f

Fixes and basic test

25a2157

Add unit tests

be82b01

Fix tests

2f3b220

Use reader to prevent zip bombing

bea1deb

Fix CR

b6916ec

refactoring and unit tests

a4d5442

Fix linter

b4f8360

algorandskiy force-pushed the pavel/pp-compression branch from 3c8e5c8 to b4f8360 Compare October 12, 2022 20:26

algorandskiy requested review from AlgoAxel and zeldovich October 12, 2022 20:27

add zstdProposalDecompressor

916de2d

jannotti previously approved these changes Oct 13, 2022

View reviewed changes

cce reviewed Oct 13, 2022

View reviewed changes

network/msgCompressor.go Outdated Show resolved Hide resolved

algorandskiy commented Oct 13, 2022

View reviewed changes

CR feedback: fast comp + bound

4b1b4ca

algorandskiy dismissed jannotti’s stale review via 4b1b4ca October 13, 2022 16:30

algorandskiy added 2 commits October 13, 2022 15:24

CR fixes: prio outside canCompress

f8fb0fb

CR: move PP compresion feature to the HTTP headers instead of the pro…

a625572

…to version

algorandskiy requested review from cce and jannotti October 13, 2022 20:12

Add metrics

aaaa19c

algorandskiy added 2 commits October 14, 2022 11:42

Rename method

0c37d7d

Start decompression with 3x buffer

3f7d72f

jannotti reviewed Oct 14, 2022

View reviewed changes

network/msgCompressor.go Show resolved Hide resolved

jannotti approved these changes Oct 14, 2022

View reviewed changes

algorandskiy merged commit d60dc6d into algorand:master Oct 14, 2022

This was referenced Oct 14, 2022

go-algorand 3.11.0-beta Release PR #4648

Closed

go-algorand 3.11.0-beta Release PR #4651

Merged

go-algorand 3.11.0-stable Release PR #4655

Merged

algobarb mentioned this pull request Oct 21, 2022

go-algorand 3.11.2-stable Release PR #4683

Merged

cce mentioned this pull request Mar 19, 2025

network: stateless vote compression #6276

Merged

5 tasks

network: proposal payload compression #4589

network: proposal payload compression #4589

Uh oh!

Conversation

algorandskiy commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Possible optimization

Uh oh!

jannotti commented Sep 26, 2022

Uh oh!

codecov bot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

algorandskiy commented Oct 12, 2022

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cce Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannotti Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zeldovich commented Oct 13, 2022

Uh oh!

algorandskiy commented Oct 13, 2022

Uh oh!

cce commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeldovich commented Oct 13, 2022

Uh oh!

algorandskiy commented Oct 14, 2022

Uh oh!

Uh oh!

cce commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

algorandskiy commented Sep 26, 2022 •

edited

Loading

codecov bot commented Sep 26, 2022 •

edited

Loading

cce Oct 13, 2022 •

edited

Loading

jannotti Oct 14, 2022 •

edited

Loading

cce commented Oct 13, 2022 •

edited

Loading