perf(p2p/conn): Use a read buffer on the secret connection by ValarDragon · Pull Request #3419 · cometbft/cometbft

ValarDragon · 2024-07-03T22:48:47Z

Similar to #3346 , buffers the secret connection reads. This is a notable savings to CPU time. (25% of recvRoutine time, on Osmosis' version)

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

ValarDragon · 2024-07-03T22:53:17Z

p2p/conn/evil_secret_connection_test.go

 		{"share bad ethimeral key", newEvilConn(true, true, false, false), func(err error) bool { return assert.Contains(t, err.Error(), "wrong wireType") }},
 		{"refuse to share auth signature", newEvilConn(true, false, false, false), func(err error) bool { return errors.Is(err, io.EOF) }},
 		{"share bad auth signature", newEvilConn(true, false, true, true), func(err error) bool { return errors.As(err, &ErrDecryptFrame{}) }},
-		{"all good", newEvilConn(true, false, true, false), func(err error) bool { return err == nil }},


I have spent over 12 hours attempting to fix this test, and it is incredibly complicated to do so. (The hardest test change out of everything I've done in comment) I do not think its worth investing in fixing the test.

And this is because this is mocking an invalid secret connection, and then a "final check" that we mocked the whole evil secret connection correctly. Which involves simulating all reads/writes to something internal.

After talking with @zmanian we both think this test is unneeded. This test is basically testing an evil secret connection peer, who sent bad signatures /deviated from protocol could not authenticate. We check the exact error at every step, so we are still matching every error case. So it seems fine to delete the test that the evil secret connection mocking, would work for the full connection.

We test that good handshakes complete in many ways (All E2E tests, other secret connection tests, many switch tests, etc) I have also tested this on mainnet on multiple occasions.

cc @melekes if this sounds good to you. Your the one who wrote this set of tests!

The produced error, which should be nil is:

SecretConnection: failed to decrypt the frame: chacha20poly1305: message authentication failed

It is produced here:

cometbft/p2p/conn/secret_connection.go

Line 437 in 033f7bb

_, err = protoio.NewDelimitedReader(sc, 1024*1024).ReadMsg(&pba)

Anyway, do you mind commenting this line out instead of removing it? And put a comment "fails with the introduction of changes PR #3419"?

It's okay to comment out the test.

ValarDragon · 2024-07-03T23:49:18Z

This is an improvement, but not nearly as good as the write buffer.

Encryption open time vs IO.Read time
OLD: 5.6 to 14.5
NEW: 5 to 12.6

So an ~8% speedup?

cason

Ok with the changes, buffering is good in general.

cason · 2024-07-04T07:29:21Z

p2p/conn/evil_secret_connection_test.go

 		{"share bad ethimeral key", newEvilConn(true, true, false, false), func(err error) bool { return assert.Contains(t, err.Error(), "wrong wireType") }},
 		{"refuse to share auth signature", newEvilConn(true, false, false, false), func(err error) bool { return errors.Is(err, io.EOF) }},
 		{"share bad auth signature", newEvilConn(true, false, true, true), func(err error) bool { return errors.As(err, &ErrDecryptFrame{}) }},
-		{"all good", newEvilConn(true, false, true, false), func(err error) bool { return err == nil }},


The produced error, which should be nil is:

SecretConnection: failed to decrypt the frame: chacha20poly1305: message authentication failed

cason · 2024-07-04T07:38:50Z

p2p/conn/evil_secret_connection_test.go

 		{"share bad ethimeral key", newEvilConn(true, true, false, false), func(err error) bool { return assert.Contains(t, err.Error(), "wrong wireType") }},
 		{"refuse to share auth signature", newEvilConn(true, false, false, false), func(err error) bool { return errors.Is(err, io.EOF) }},
 		{"share bad auth signature", newEvilConn(true, false, true, true), func(err error) bool { return errors.As(err, &ErrDecryptFrame{}) }},
-		{"all good", newEvilConn(true, false, true, false), func(err error) bool { return err == nil }},


It is produced here:

cometbft/p2p/conn/secret_connection.go

Line 437 in 033f7bb

_, err = protoio.NewDelimitedReader(sc, 1024*1024).ReadMsg(&pba)

cason · 2024-07-04T07:42:15Z

p2p/conn/evil_secret_connection_test.go

 		{"share bad ethimeral key", newEvilConn(true, true, false, false), func(err error) bool { return assert.Contains(t, err.Error(), "wrong wireType") }},
 		{"refuse to share auth signature", newEvilConn(true, false, false, false), func(err error) bool { return errors.Is(err, io.EOF) }},
 		{"share bad auth signature", newEvilConn(true, false, true, true), func(err error) bool { return errors.As(err, &ErrDecryptFrame{}) }},
-		{"all good", newEvilConn(true, false, true, false), func(err error) bool { return err == nil }},


Anyway, do you mind commenting this line out instead of removing it? And put a comment "fails with the introduction of changes PR #3419"?

melekes

Thanks @ValarDragon ❤️

and comment it out

melekes · 2024-07-10T12:09:40Z

@mergify backport v1.x

mergify · 2024-07-10T12:09:48Z

backport v1.x

✅ Backports have been created

Details

#3489 perf(p2p/conn): Use a read buffer on the secret connection (backport #3419) has been created for branch v1.x but encountered conflicts

Closes #3198 Similar to #3346 , buffers the secret connection reads. This is a notable savings to CPU time. (25% of recvRoutine time, on Osmosis' version) --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit e1eabe0) # Conflicts: # p2p/conn/evil_secret_connection_test.go

…3419) (#3489) Closes #3198 Similar to #3346 , buffers the secret connection reads. This is a notable savings to CPU time. (25% of recvRoutine time, on Osmosis' version) --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3419 done by [Mergify](https://mergify.com). --------- Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

Closes #3198 Similar to #3346 , buffers the secret connection reads. This is a notable savings to CPU time. (25% of recvRoutine time, on Osmosis' version) --- - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

perf(p2p/conn): Use a read buffer on the secret connection

13170fb

ValarDragon requested a review from a team as a code owner July 3, 2024 22:48

ValarDragon requested a review from a team July 3, 2024 22:48

ValarDragon commented Jul 3, 2024

View reviewed changes

Add changelog

033f7bb

cason approved these changes Jul 4, 2024

View reviewed changes

melekes approved these changes Jul 5, 2024

View reviewed changes

bring back failing test case

6a89100

and comment it out

melekes enabled auto-merge July 5, 2024 06:31

melekes added this pull request to the merge queue Jul 5, 2024

Merged via the queue into main with commit e1eabe0 Jul 5, 2024

melekes deleted the dev/secret_conn_readbuffer branch July 5, 2024 06:41

mergify bot mentioned this pull request Jul 10, 2024

perf(p2p/conn): Use a read buffer on the secret connection (backport #3419) #3489

Merged

4 tasks

mattac21 mentioned this pull request Sep 12, 2025

chore: backport perf #5360

Merged

3 tasks

Conversation

ValarDragon commented Jul 3, 2024 • edited by cason Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValarDragon commented Jul 3, 2024

Uh oh!

cason left a comment

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!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

melekes commented Jul 10, 2024

Uh oh!

mergify bot commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ValarDragon commented Jul 3, 2024 •

edited by cason

Loading

mergify bot commented Jul 10, 2024 •

edited

Loading