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.

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

[Consensus] Penalize validators for double signing #432

Open
8 tasks
Tracked by #890
Olshansk opened this issue Jan 7, 2023 · 3 comments
Open
8 tasks
Tracked by #890

[Consensus] Penalize validators for double signing #432

Olshansk opened this issue Jan 7, 2023 · 3 comments
Assignees
Labels
consensus Consensus specific changes utility Utility specific changes

Comments

@Olshansk
Copy link
Member

Olshansk commented Jan 7, 2023

Objective

Penalize validators that double-signed competing state transitions.

Origin Document

This was discussed during a core team protocol hour. Due to Pocket's application-specific stack, we can eliminate the need for Tendermint's Evidence in exchange for a simpler solution.

Some of the ideas discussed included looking at getQuorumCertificate and shown in the comments below:

func (m *consensusModule) getQuorumCertificate(height uint64, step typesCons.HotstuffStep, round uint64) (*typesCons.QuorumCertificate, error) {
	var pss []*typesCons.PartialSignature
	for _, msg := range m.MessagePool[step] {
	  // ...

	hasDoubleSign :=  /* pseudo-code: group by msg.SenderAddress + check for multiplicity > 1 */
    if hasDoubleSign {
       // Maybe: Send a message to award the validator who caught double sign
       // Send message to slash the signer that did a double sign
    }

	  // ...
	return &typesCons.QuorumCertificate{
		Height:             m.Height,
		Step:               step,
		Round:              m.Round,
		Block:              m.Block,
		ThresholdSignature: thresholdSig,
	}, nil
}

Goals

  • Identify how double signing can happen and how it should be handled in Pocket Network v1
  • Identify other missing elements in the codebase that need to be accounted for in lieu of Tendermint's evidence

Deliverable

  • Implementation that penalizes validators for double-signing state transitions
  • Sufficient tests for the implementation above
  • Add a new README explaining how double signing attacks are handled
  • Update the appropriate CHANGELOG(s)
  • Creating Github issues for subsequent work that is handled by Tendermint's evidence and is not done in Pocket network V1

Non-goals / Non-deliverables

  • Reimplenting everything that Evidence does in Tendermint

Testing Methodology

  • make target for a new set of tests
  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

Creator: @Olshansk
Co-Owners: @gokutheengineer

@Olshansk Olshansk added the consensus Consensus specific changes label Jan 7, 2023
@Olshansk
Copy link
Member Author

Extra context: while votes are stored in-memory of the validator's running process, when byzantine behaviour is detected, the proposer/replica submits an Evidence Transaction (with supporting evidence), which is later stored on-chain. More context here: https://github.com/pokt-network/pocket-network-protocol/blob/issues/23/aat/utility/README.md#382-evidence

Though there is still a chance some byzantine signatures may be lost, it is a "best effort" process.

@Olshansk
Copy link
Member Author

Olshansk commented Jan 26, 2023

We're not currently handling any logic for double signing but have a few data structures in place.

Going to delete them but wanted to document what we have for a future reference.

In message.proto

message MessageDoubleSign {
  utility.LegacyVote vote_a = 1;
  utility.LegacyVote vote_b = 2;
  optional bytes reporter_address = 3;
}

// TECHDEBT: Consolidate this with consensus
message LegacyVote {
  bytes public_key = 1;
  int64 height = 2;
  uint32 round = 3;
  uint32 type = 4;
  bytes block_hash = 5;
}

In vote.go

package types

const (
	DoubleSignEvidenceType = 1
)

// NOTE: there's no signature validation on the vote because we are unsure the current mode of vote signing
// TODO *Needs to add signatures to vote structure*
func (v *LegacyVote) ValidateBasic() Error {
	if err := ValidatePublicKey(v.PublicKey); err != nil {
		return err
	}
	if err := ValidateHash(v.BlockHash); err != nil {
		return err
	}
	if v.Height < 0 {
		return ErrInvalidBlockHeight()
	}
	if v.Type != DoubleSignEvidenceType {
		return ErrInvalidEvidenceType()
	}
	return nil
}

And in vote_test.go:

package types

import (
	"testing"

	"github.com/pokt-network/pocket/shared/crypto"
	"github.com/stretchr/testify/require"
)

func TestVoteValidateBasic(t *testing.T) {
	publicKey, err := crypto.GeneratePublicKey()
	require.NoError(t, err)
	testHash := crypto.SHA3Hash([]byte("fake_hash"))
	v := LegacyVote{
		PublicKey: publicKey.Bytes(),
		Height:    1,
		Round:     2,
		Type:      DoubleSignEvidenceType,
		BlockHash: testHash,
	}
	require.NoError(t, v.ValidateBasic())
	// bad public key
	v2 := v
	v2.PublicKey = []byte("not_a_public_key")
	badPkLen := len(v2.PublicKey)
	require.Equal(t, v2.ValidateBasic(), ErrInvalidPublicKeyLen(crypto.ErrInvalidPublicKeyLen(badPkLen)))
	// no public key
	v2.PublicKey = nil
	require.Equal(t, v2.ValidateBasic(), ErrEmptyPublicKey())
	// bad hash
	v3 := v
	v3.BlockHash = []byte("not_a_hash")
	badBlockHashLen := len(v3.BlockHash)
	require.Equal(t, v3.ValidateBasic(), ErrInvalidHashLength(crypto.ErrInvalidHashLen(badBlockHashLen)))
	// no hash
	v3.BlockHash = nil
	require.Equal(t, v3.ValidateBasic(), ErrEmptyHash())
	// negative height
	v4 := v
	v4.Height = -1
	require.Equal(t, v4.ValidateBasic(), ErrInvalidBlockHeight())
	// bad type
	v5 := v
	v5.Type = 0
	require.Equal(t, v5.ValidateBasic(), ErrInvalidEvidenceType())
}
func TestMessage_DoubleSign_ValidateBasic(t *testing.T) {
	pk, err := crypto.GeneratePublicKey()
	require.NoError(t, err)

	hashA := crypto.SHA3Hash(pk.Bytes())
	hashB := crypto.SHA3Hash(pk.Address())
	voteA := &LegacyVote{
		PublicKey: pk.Bytes(),
		Height:    1,
		Round:     2,
		Type:      DoubleSignEvidenceType,
		BlockHash: hashA,
	}
	voteB := &LegacyVote{
		PublicKey: pk.Bytes(),
		Height:    1,
		Round:     2,
		Type:      DoubleSignEvidenceType,
		BlockHash: hashB,
	}
	reporter, _ := crypto.GenerateAddress()
	msg := &MessageDoubleSign{
		VoteA:           voteA,
		VoteB:           voteB,
		ReporterAddress: reporter,
	}
	er := msg.ValidateBasic()
	require.NoError(t, er)

	pk2, err := crypto.GeneratePublicKey()
	require.NoError(t, err)
	msgUnequalPubKeys := new(MessageDoubleSign)
	msgUnequalPubKeys.VoteA = proto.Clone(msg.VoteA).(*LegacyVote)
	msgUnequalPubKeys.VoteB = proto.Clone(msg.VoteB).(*LegacyVote)
	msgUnequalPubKeys.VoteA.PublicKey = pk2.Bytes()
	er = msgUnequalPubKeys.ValidateBasic()
	require.Equal(t, ErrUnequalPublicKeys().Code(), er.Code())

	msgUnequalHeights := new(MessageDoubleSign)
	msgUnequalHeights.VoteA = proto.Clone(msg.VoteA).(*LegacyVote)
	msgUnequalHeights.VoteB = proto.Clone(msg.VoteB).(*LegacyVote)
	msgUnequalHeights.VoteA.Height = 2
	er = msgUnequalHeights.ValidateBasic()
	require.Equal(t, ErrUnequalHeights().Code(), er.Code())

	msgUnequalRounds := new(MessageDoubleSign)
	msgUnequalRounds.VoteA = proto.Clone(msg.VoteA).(*LegacyVote)
	msgUnequalRounds.VoteB = proto.Clone(msg.VoteB).(*LegacyVote)
	msgUnequalRounds.VoteA.Round = 1
	er = msgUnequalRounds.ValidateBasic()
	require.Equal(t, ErrUnequalRounds().Code(), er.Code())

	msgEqualVoteHash := new(MessageDoubleSign)
	msgEqualVoteHash.VoteA = proto.Clone(msg.VoteA).(*LegacyVote)
	msgEqualVoteHash.VoteB = proto.Clone(msg.VoteB).(*LegacyVote)
	msgEqualVoteHash.VoteB.BlockHash = hashA
	er = msgEqualVoteHash.ValidateBasic()
	require.Equal(t, ErrEqualVotes().Code(), er.Code())
}

And utility/message_handler.go had the following logic:

func (u *utilityContext) HandleMessageDoubleSign(message *typesUtil.MessageDoubleSign) typesUtil.Error {
	latestHeight, err := u.GetLatestBlockHeight()
	if err != nil {
		return err
	}
	evidenceAge := latestHeight - message.VoteA.Height
	maxEvidenceAge, err := u.GetMaxEvidenceAgeInBlocks()
	if err != nil {
		return err
	}
	if evidenceAge > int64(maxEvidenceAge) {
		return typesUtil.ErrMaxEvidenceAge()
	}
	pk, er := crypto.NewPublicKeyFromBytes(message.VoteB.PublicKey)
	if er != nil {
		return typesUtil.ErrNewPublicKeyFromBytes(er)
	}
	doubleSigner := pk.Address()
	// burn validator for double signing blocks
	burnPercentage, err := u.GetDoubleSignBurnPercentage()
	if err != nil {
		return err
	}
	if err := u.BurnActor(coreTypes.ActorType_ACTOR_TYPE_VAL, burnPercentage, doubleSigner); err != nil {
		return err
	}
	return nil
}

Olshansk added a commit that referenced this issue Feb 14, 2023
…/ 2) (#503)

## Description

This is the first of several changes necessary to refactor and improve the utility module to enable implementation of future, more complex, interfaces.

## Issue

Fixes #475

## Type of change

Please mark the relevant option(s):

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

## List of changes

### Changes

**Utility**
- Add a `Validatable` type for basic validation
- Split business logic specific to certain actors (e.g. validator reward, app relays, message handling) into separate files
- Reduce the scope of functions and types that shouldn’t be exposed
- Upgraded the actors tests - a lot went here to help with understanding what’s going on but it’s still just a start
- Remove the `Context` struct; unnecessary abstraction
- Added comments and guidance on message, transaction and signature validation
- Added `ITransaction`, an interface for the `Transaction` protocol to help capture the functionality it adds to the core type

**Code Confusion**
- Remove `IUnstakingActor` and use `UnstakingActor` directly. Guideline for removing future unnecessary types (e.g. TxResult)
- Delineate between unstaking & unbonding in a few places throughout the codebase

**Bugs**
- Avoid unstaking all actors when intending to UnstakeMsg paused actors only (major bug in persistence sql query)
- `tx.Equals` was comparing the same transaction against itself (major bug)
- Staking status enums in utility did not reflect the same protocol as in persistence (needs to be consolidated)

**Code optimization**
- Avoid redundant `byte` <-> `string` conversions in several places
- Avoid redundant `bigInt` <-> `string` conversions in several places

**Code health**
- Add comments explaining the use/responsibility of various types
- Consolidate BigInt/String converters used in other parts of the codebase
- Consolidate some types used everywhere (e.g. `actorTypes`)
- Reduce the code footprint of the `codec` package & add some TODOs
- Removed unused double sign code (moved to #432 for reference)

### Files focused on

utility
- [x] ├── account.go
- [x] ├── account_test.go
- [x] ├── actor.go
- [x] ├── actor_test.go
- [ ] ├── application.go
- [ ] ├── application_test.go
- [ ] ├── block.go
- [ ] ├── block_test.go
- [ ] ├── context.go
- [ ] ├── doc
- [ ] │   ├── CHANGELOG.md
- [ ] │   ├── PROTOCOL_RELAY.md
- [ ] │   ├── PROTOCOL_SESSION.md
- [ ] │   └── README.md
- [ ] ├── gov.go
- [ ] ├── gov_test.go
- [ ] ├── message_handler.go
- [ ] ├── message_test.go
- [ ] ├── module.go
- [ ] ├── module_test.go
- [ ] ├── service
- [ ] │   └── service.go
- [ ] ├── session.go
- [ ] ├── transaction.go
- [ ] ├── transaction_test.go
- [ ] ├── types
- [x] │   ├── constants.go
- [x] │   ├── error.go
- [x] │   ├── gov.go
- [x] │   ├── message.go
- [x] │   ├── message_staking.go
- [x] │   ├── message_test.go
- [ ] │   ├── proto
- [ ] │   │   ├── message.proto
- [ ] │   │   ├── stake_status.proto
- [ ] │   │   ├── transaction.proto
- [ ] │   │   ├── tx_result.proto
- [ ] │   │   └── utility_messages.proto
- [x] │   ├── relay_chain.go
- [x] │   ├── relay_chain_test.go
- [x] │   ├── signature.go
- [ ] │   ├── transaction.go
- [ ] │   ├── transaction_test.go
- [x] │   ├── tx_fifo_mempool.go
- [x] │   ├── tx_fifo_mempool_test.go
- [ ] │   ├── tx_result.go
- [x] │   └── validatable.go
- [ ] └── validator.go

## Testing

- [x] `make develop_test`
- [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) w/ all of the steps outlined in the `README`

## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [x] I have updated the corresponding README(s); local and/or global
- [ ] 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)


Co-authored-by: Alessandro De Blasis <[email protected]>
@Olshansk Olshansk self-assigned this Apr 19, 2023
@Olshansk Olshansk added the utility Utility specific changes label Apr 19, 2023
@Olshansk
Copy link
Member Author

@red-0ne Just a heads up that we'll need to create an E2E feature ticket for this [1] following [2] when we get around to this task

[1] https://github.com/pokt-network/pocket/blob/main/utility/doc/E2E_FEATURE_LIST.md#1-validator---double-sign-burn-
[2] https://github.com/pokt-network/pocket/blob/main/utility/doc/E2E_FEATURE_PATH_TEMPLATE.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus Consensus specific changes utility Utility specific changes
Projects
Status: Rescope
Development

No branches or pull requests

4 participants