fix: zero value nonce is invalid (#881)

## Description

Because `0` is the zero (default) value for `uint64`, 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
to `0`.

I'm not confident whether the ability to make this distinction matters
now or has the potential to later but was following a feeling.

<!-- reviewpad:summarize:start -->
### Summary generated by Reviewpad on 11 Jul 23 11:56 UTC
This pull request includes the following changes:

- In the `p2p/module.go` file, the `handlePocketEnvelope` function now
checks if `poktEnvelope.Nonce` is zero and returns an error if it is.
The error message includes the hex-encoded nonce value.
- In the `p2p/module_test.go` file, new test cases have been added to
test the handling of an invalid nonce.
- In the `p2p/types/errors.go` file, a new error variable
`ErrInvalidNonce` has been added to represent an invalid nonce value.
- In the `shared/crypto/rand.go` file, 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.
<!-- reviewpad:summarize:end -->

## Issue

N/A; observation made while working on #732.

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [x] Bug fix
- [ ] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Adds `ErrInvalidNonce` P2P error type
- Ensures the P2P message handler rejects the zero value `Nonce`
(`uint64(0)`) is invalid
- Ensures the `GetNonce` function never returns the zero value

## Testing

- [ ] `make develop_test`; if any code changes were made
- [ ] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [ ] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [ ] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [ ] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made


## Required Checklist

- [ ] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added, or updated, [`godoc` format
comments](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)

p2p/module.go

@@ -495,6 +495,14 @@ func (m *p2pModule) handlePocketEnvelope(pocketEnvelopeBz []byte) error {
 		return fmt.Errorf("decoding network message: %w", err)
 	}
 
+	if poktEnvelope.Nonce == 0 {
+		return fmt.Errorf(
+			"handling pocket envelope: %w (hex-encoded): %x",
+			typesP2P.ErrInvalidNonce,
+			poktEnvelope.Nonce,
+		)
+	}
+
 	if m.isNonceAlreadyObserved(poktEnvelope.Nonce) {
 		// skip passing redundant message to application layer
 		return nil

p2p/module_test.go

@@ -1,3 +1,5 @@
+//go:build test
+
 package p2p
 
 import (
@@ -9,11 +11,15 @@ import (
 	libp2pCrypto "github.com/libp2p/go-libp2p/core/crypto"
 	libp2pHost "github.com/libp2p/go-libp2p/core/host"
 	mocknet "github.com/libp2p/go-libp2p/p2p/net/mock"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/anypb"
+
 	typesP2P "github.com/pokt-network/pocket/p2p/types"
 	"github.com/pokt-network/pocket/p2p/utils"
 	"github.com/pokt-network/pocket/runtime/configs"
 	"github.com/pokt-network/pocket/runtime/defaults"
 	cryptoPocket "github.com/pokt-network/pocket/shared/crypto"
+	"github.com/pokt-network/pocket/shared/messaging"
 	"github.com/pokt-network/pocket/shared/modules"
 	mockModules "github.com/pokt-network/pocket/shared/modules/mocks"
 	"github.com/stretchr/testify/require"
@@ -109,6 +115,8 @@ func Test_Create_configureBootstrapNodes(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			libp2pMockNet := mocknet.New()
+
 			ctrl := gomock.NewController(t)
 			mockRuntimeMgr := mockModules.NewMockRuntimeMgr(ctrl)
 			mockBus := createMockBus(t, mockRuntimeMgr, nil)
@@ -136,7 +144,7 @@ func Test_Create_configureBootstrapNodes(t *testing.T) {
 				ServiceURL: testLocalServiceURL,
 			}
 
-			host := newLibp2pMockNetHost(t, privKey, peer)
+			host := newMockNetHost(t, libp2pMockNet, privKey, peer)
 			p2pMod, err := Create(mockBus, WithHost(host))
 			if (err != nil) != tt.wantErr {
 				t.Errorf("p2pModule.Create() error = %v, wantErr %v", err, tt.wantErr)
@@ -151,9 +159,84 @@ func Test_Create_configureBootstrapNodes(t *testing.T) {
 }
 
 func TestP2pModule_WithHostOption_Restart(t *testing.T) {
-	ctrl := gomock.NewController(t)
+	privKey := cryptoPocket.GetPrivKeySeed(1)
+
+	peer := &typesP2P.NetworkPeer{
+		PublicKey:  privKey.PublicKey(),
+		Address:    privKey.Address(),
+		ServiceURL: testLocalServiceURL,
+	}
+
+	libp2pMockNet := mocknet.New()
+	host := newMockNetHost(t, libp2pMockNet, privKey, peer)
+
+	mod := newP2PModule(t, privKey, WithHost(host))
+
+	// start the module; should not create a new host
+	err := mod.Start()
+	require.NoError(t, err)
 
+	// assert initial host matches the one provided via `WithHost`
+	require.Equal(t, host, mod.host, "initial hosts don't match")
+
+	// stop the module; destroys module's host
+	err = mod.Stop()
+	require.NoError(t, err)
+
+	// assert host does *not* match after restart
+	err = mod.Start()
+	require.NoError(t, err)
+	require.NotEqual(t, host, mod.host, "post-restart hosts don't match")
+}
+
+func TestP2pModule_InvalidNonce(t *testing.T) {
 	privKey := cryptoPocket.GetPrivKeySeed(1)
+
+	peer := &typesP2P.NetworkPeer{
+		PublicKey:  privKey.PublicKey(),
+		Address:    privKey.Address(),
+		ServiceURL: testLocalServiceURL,
+	}
+
+	libp2pMockNet := mocknet.New()
+	host := newMockNetHost(t, libp2pMockNet, privKey, peer)
+
+	mod := newP2PModule(
+		t, privKey,
+		WithHost(host),
+	)
+	err := mod.Start()
+	require.NoError(t, err)
+
+	// Use zero value nonce
+	poktEnvelope := &messaging.PocketEnvelope{
+		Content: &anypb.Any{},
+	}
+	poktEnvelopeBz, err := proto.Marshal(poktEnvelope)
+	require.NoError(t, err)
+
+	err = mod.handlePocketEnvelope(poktEnvelopeBz)
+	require.ErrorIs(t, err, typesP2P.ErrInvalidNonce)
+
+	// Explicitly set the nonce to 0
+	poktEnvelope = &messaging.PocketEnvelope{
+		Content: &anypb.Any{},
+		// 0 should be an invalid nonce value
+		Nonce: 0,
+	}
+	poktEnvelopeBz, err = proto.Marshal(poktEnvelope)
+	require.NoError(t, err)
+
+	err = mod.handlePocketEnvelope(poktEnvelopeBz)
+	require.ErrorIs(t, err, typesP2P.ErrInvalidNonce)
+}
+
+// TECHDEBT(#609): move & de-duplicate
+func newP2PModule(t *testing.T, privKey cryptoPocket.PrivateKey, opts ...modules.ModuleOption) *p2pModule {
+	t.Helper()
+
+	ctrl := gomock.NewController(t)
+
 	mockRuntimeMgr := mockModules.NewMockRuntimeMgr(ctrl)
 	mockBus := createMockBus(t, mockRuntimeMgr, nil)
 
@@ -176,46 +259,30 @@ func TestP2pModule_WithHostOption_Restart(t *testing.T) {
 		},
 	}).AnyTimes()
 	mockBus.EXPECT().GetRuntimeMgr().Return(mockRuntimeMgr).AnyTimes()
-
-	peer := &typesP2P.NetworkPeer{
-		PublicKey:  privKey.PublicKey(),
-		Address:    privKey.Address(),
-		ServiceURL: testLocalServiceURL,
-	}
-
-	mockNetHost := newLibp2pMockNetHost(t, privKey, peer)
-	p2pMod, err := Create(mockBus, WithHost(mockNetHost))
+	p2pMod, err := Create(mockBus, opts...)
 	require.NoError(t, err)
 
 	mod, ok := p2pMod.(*p2pModule)
 	require.Truef(t, ok, "unknown module type: %T", mod)
 
-	// start the module; should not create a new host
-	err = mod.Start()
-	require.NoError(t, err)
-
-	// assert initial host matches the one provided via `WithHost`
-	require.Equal(t, mockNetHost, mod.host, "initial hosts don't match")
-
-	// stop the module; destroys module's host
-	err = mod.Stop()
-	require.NoError(t, err)
-
-	// assert host does *not* match after restart
-	err = mod.Start()
-	require.NoError(t, err)
-	require.NotEqual(t, mockNetHost, mod.host, "post-restart hosts don't match")
+	return mod
 }
 
 // TECHDEBT(#609): move & de-duplicate
-func newLibp2pMockNetHost(t *testing.T, privKey cryptoPocket.PrivateKey, peer *typesP2P.NetworkPeer) libp2pHost.Host {
+func newMockNetHost(
+	t *testing.T,
+	libp2pMockNet mocknet.Mocknet,
+	privKey cryptoPocket.PrivateKey,
+	peer *typesP2P.NetworkPeer,
+) libp2pHost.Host {
+	t.Helper()
+
 	libp2pPrivKey, err := libp2pCrypto.UnmarshalEd25519PrivateKey(privKey.Bytes())
 	require.NoError(t, err)
 
 	libp2pMultiAddr, err := utils.Libp2pMultiaddrFromServiceURL(peer.ServiceURL)
 	require.NoError(t, err)
 
-	libp2pMockNet := mocknet.New()
 	host, err := libp2pMockNet.AddPeer(libp2pPrivKey, libp2pMultiAddr)
 	require.NoError(t, err)
 
@@ -224,6 +291,8 @@ func newLibp2pMockNetHost(t *testing.T, privKey cryptoPocket.PrivateKey, peer *t
 
 // TECHDEBT(#609): move & de-duplicate
 func baseTelemetryMock(t *testing.T, _ modules.EventsChannel) *mockModules.MockTelemetryModule {
+	t.Helper()
+
 	ctrl := gomock.NewController(t)
 	telemetryMock := mockModules.NewMockTelemetryModule(ctrl)
 	timeSeriesAgentMock := baseTelemetryTimeSeriesAgentMock(t)
@@ -240,6 +309,8 @@ func baseTelemetryMock(t *testing.T, _ modules.EventsChannel) *mockModules.MockT
 
 // TECHDEBT(#609): move & de-duplicate
 func baseTelemetryTimeSeriesAgentMock(t *testing.T) *mockModules.MockTimeSeriesAgent {
+	t.Helper()
+
 	ctrl := gomock.NewController(t)
 	timeSeriesAgentMock := mockModules.NewMockTimeSeriesAgent(ctrl)
 	timeSeriesAgentMock.EXPECT().CounterRegister(gomock.Any(), gomock.Any()).AnyTimes()
@@ -249,6 +320,8 @@ func baseTelemetryTimeSeriesAgentMock(t *testing.T) *mockModules.MockTimeSeriesA
 
 // TECHDEBT(#609): move & de-duplicate
 func baseTelemetryEventMetricsAgentMock(t *testing.T) *mockModules.MockEventMetricsAgent {
+	t.Helper()
+
 	ctrl := gomock.NewController(t)
 	eventMetricsAgentMock := mockModules.NewMockEventMetricsAgent(ctrl)
 	eventMetricsAgentMock.EXPECT().EmitEvent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()

p2p/types/errors.go

@@ -6,7 +6,8 @@ import (
 )
 
 var (
-	ErrUnknownPeer = errors.New("unknown peer")
+	ErrUnknownPeer  = errors.New("unknown peer")
+	ErrInvalidNonce = errors.New("invalid nonce")
 )
 
 func ErrUnknownEventType(msg any) error {

shared/crypto/rand.go

@@ -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 {
+		return GetNonce()
+	}
 	return bigNonce.Uint64()
 }