fix: zero value nonce is invalid #881
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryanchriswhite
added
core
bryanchriswhite
force-pushed
the
fix/zero-value-nonce
branch
from
f598847 to
235e06c
Compare
dylanlott
reviewed
Jul 5, 2023
| @@ -374,6 +374,14 @@ func (m *p2pModule) handlePocketEnvelope(pocketEnvelopeBz []byte) error { | |||
| return fmt.Errorf("decoding network message: %w", err) | |||
| } | |||
|
|
|||
| if poktEnvelope.Nonce == 0 { | |||
dylanlott
reviewed
Jul 5, 2023
dylanlott
reviewed
Jul 5, 2023
dylanlott
reviewed
Jul 5, 2023
| @@ -21,6 +21,11 @@ func GetNonce() uint64 { | |||
| rand.Seed(time.Now().UTC().UnixNano()) //nolint:staticcheck // G404 - Weak random source is okay in unit tests | |||
| return rand.Uint64() //nolint:gosec // G404 - Weak source of random here is fallback | |||
| } | |||
|
|
|||
| // 0 is an invalid value | |||
| if bigNonce.Uint64() == 0 { | |||
Olshansk
requested changes
Jul 6, 2023
* pokt/main: [P2P] Integrate background router (#732) Update main README.md [Bug] Fix CI linter errors (#885) [Tooling] Block `IN_THIS_*` comments from passing CI (#889) [Utility] Update E2E feature path template doc (#870) [IBC] Add nil check on proof for membership and non-membership proof creation (#877) Added git diff state to devlog10 Devlog 10 (#872)
* pokt/main: chore: introduce `Submodule` interface (#855)
Olshansk
approved these changes
Jul 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Because
0is the zero (default) value foruint64, if it is valid to be used as a nonce, it becomes difficult to distinguish the scenario where sender did not set a nonce from one where they explicitly set it to0.I'm not confident whether the ability to make this distinction matters now or has the potential to later but was following a feeling.
Summary generated by Reviewpad on 11 Jul 23 11:56 UTC
This pull request includes the following changes:
p2p/module.gofile, thehandlePocketEnvelopefunction now checks ifpoktEnvelope.Nonceis zero and returns an error if it is. The error message includes the hex-encoded nonce value.p2p/module_test.gofile, new test cases have been added to test the handling of an invalid nonce.p2p/types/errors.gofile, a new error variableErrInvalidNoncehas been added to represent an invalid nonce value.shared/crypto/rand.gofile, a check has been added to ensure that the generated nonce value is not zero. If it is zero, the function recursively calls itself to generate a new nonce.Issue
N/A; observation made while working on #732.
Type of change
Please mark the relevant option(s):
List of changes
ErrInvalidNonceP2P error typeNonce(uint64(0)) is invalidGetNoncefunction never returns the zero valueTesting
make develop_test; if any code changes were mademake test_e2eon k8s LocalNet; if any code changes were madee2e-devnet-testpasses tests on DevNet; if any code was changedRequired Checklist
godocformat comments on touched members (see: tip.golang.org/doc/comment)If Applicable Checklist
shared/docs/*if I updatedshared/*README(s)