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.

Sign up for GitHub

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

Jump to bottom

update secret connection to use a little endian encoded nonce #2264

Merged
merged 4 commits into from
Aug 28, 2018
Merged

update secret connection to use a little endian encoded nonce #2264

merged 4 commits into from
Aug 28, 2018

Conversation

liamsi
Copy link
Contributor

@liamsi liamsi commented Aug 23, 2018 •
edited
Loading

Minor change that implements #2263.

Looks like circelci fails because this comes from a fork?

ValarDragon
ValarDragon approved these changes Aug 23, 2018
View reviewed changes
Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, efficiency loss for not doing this in place should be negligible, since it's int64 parsing.

@liamsi liamsi mentioned this pull request Aug 24, 2018
Merged
melekes
melekes reviewed Aug 25, 2018
View reviewed changes
p2p/conn/secret_connection.go
return
}
}
counter := binary.LittleEndian.Uint64(nonce[4:])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the first 4 bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not use them / they will be all zero. We use chacha20poly1305 which expects a 12 byte nonce though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of weird, but only using the last 8 bytes has become the standard for cha cha nonces in stream mode. (This is what is done in noise)

This will be sufficient for our purposes tho, since 2^64 messages will be more than well have. (We can't use random nonces, since that only gave a 2^48 bound, which is in the reachable zone)

p2p/conn/secret_connection.go Outdated
@@ -334,11 +334,7 @@ func shareAuthSignature(sc *SecretConnection, pubKey crypto.PubKey, signature []

// increment nonce big-endian by 1 with wraparound.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Thanks!

@melekes
Copy link
Contributor

melekes commented Aug 26, 2018

@liamsi can you commit a dummy change (add whitespace or something) to restart CircleCI? It looks like they have fixed the above problem.

@liamsi
Copy link
Contributor Author

liamsi commented Aug 26, 2018

Looks like this needs to land first? #2279
Usually, you can also trigger a rebuild by closing and reopening a PR. Let me try.

@liamsi liamsi closed this Aug 26, 2018
@liamsi liamsi reopened this Aug 26, 2018
@melekes melekes closed this Aug 27, 2018
@melekes melekes reopened this Aug 27, 2018
@melekes
Copy link
Contributor

melekes commented Aug 27, 2018

@liamsi you're right! I've just merged #2279.

btw. closing/reopening does not force CircleCI to rebuild.

@melekes melekes added C:p2p Component: P2P pkg T:breaking Type: Breaking Change labels Aug 27, 2018
@liamsi
Copy link
Contributor Author

liamsi commented Aug 27, 2018

Looks like there are still some issues with circelci? e.g:

#!/bin/bash -eo pipefail
bash test/app/test.sh
bash: test/app/test.sh: No such file or directory
Exited with code 127

@melekes
Copy link
Contributor

melekes commented Aug 27, 2018

You need to rebase your branch against latest develop.

https://stackoverflow.com/questions/7244321/how-do-i-update-a-github-forked-repository#7244456

@liamsi liamsi requested a review from zramsay as a code owner August 27, 2018 13:29
@liamsi
Copy link
Contributor Author

liamsi commented Aug 27, 2018

Meh, should have done that using the command line :-D One second.

@liamsi
Copy link
Contributor Author

liamsi commented Aug 27, 2018 •
edited
Loading

Thanks @melekes! Rebased (now properly). Should I squash the commit messages, too?

@codecov-io
Copy link

Codecov Report

Merging #2264 into develop will increase coverage by 0.07%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2264      +/-   ##
===========================================
+ Coverage    62.72%   62.79%   +0.07%     
===========================================
  Files          215      215              
  Lines        17584    17536      -48     
===========================================
- Hits         11029    11012      -17     
+ Misses        5648     5608      -40     
- Partials       907      916       +9
Impacted Files Coverage Δ
p2p/conn/secret_connection.go 73.93% <100%> (-0.16%) ⬇️
p2p/pex/pex_reactor.go 74.17% <0%> (-1%) ⬇️
libs/db/prefix_db.go 55.75% <0%> (-0.8%) ⬇️
blockchain/pool.go 65.73% <0%> (-0.7%) ⬇️
config/toml.go 53.19% <0%> (-0.66%) ⬇️
p2p/pex/addrbook.go 69.58% <0%> (-0.49%) ⬇️
consensus/reactor.go 73.55% <0%> (-0.27%) ⬇️
p2p/peer.go 57.95% <0%> (ø) ⬆️
consensus/state.go 77.56% <0%> (+0.6%) ⬆️
consensus/replay.go 62.62% <0%> (+7.76%) ⬆️

@melekes melekes merged commit 9d06d7e into tendermint:develop Aug 28, 2018
@ebuchman
Copy link
Contributor

This needs to be reflected in changelog (breaking)

@melekes
Copy link
Contributor

melekes commented Aug 31, 2018

Added.

melekes added a commit that referenced this pull request Aug 31, 2018
@melekes
add missing changelog entry
029694e 
#2264 (comment)
@melekes melekes mentioned this pull request Aug 31, 2018
Merged
melekes added a commit that referenced this pull request Aug 31, 2018
@melekes
add missing changelog entry (#2303)
7b2f709 
#2264 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValarDragon ValarDragon ValarDragon approved these changes

@melekes melekes melekes approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@xla xla Awaiting requested review from xla

@zramsay zramsay Awaiting requested review from zramsay

Assignees
No one assigned
Labels
C:p2p Component: P2P pkg T:breaking Type: Breaking Change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@liamsi @melekes @codecov-io @ebuchman @ValarDragon