[ADR] 0002 - Separate consensus and P2P IDs

[bryanchriswhite]

Summary

This pull request adds an Architecture Decision Record (ADR) for simplifying and consolidating node identity in our system by keeping consensus NodeId and P2P identity separate. The ADR explains the context, problem statement, decision drivers, considered options, and the decision outcome, along with the positive and negative consequences of the chosen option.

The chosen option is to keep consensus NodeId and P2P identity separate, as it prevents unintended interference between consensus and P2P layers while maintaining a clear separation of concerns.

Please review the ADR and provide feedback on:

• The clarity of the problem statement and context
• The thoroughness of the decision drivers and considered options
• The appropriateness of the chosen option given the decision drivers
• Any additional considerations that may have been overlooked
• Any changes in formatting, grammar, or language for better readability

Related Issues

• [P2P][LibP2P][SPIKE] Consolidate and Refactor Node Identity pocket#348
• [TECHDEBT][Consensus] Review Hotstuff Node IDs pocket#434

[Olshansk]

Pocket Network Protocol Team

[Olshansk]

See the PR for my updated template and lmk if you want to adopt it here (if we merge it).

Totally cool with not applying it retroactively, though.

[Olshansk]

Without examples, references or links, it's really hard to understand what the decision was actually entailing.

[Olshansk]

Suggested change

	* Preventing unintended interference between different node identity types
	* Preventing unintended interference between different node identity types
	* Maintenance, tracking and defining multiple type of IDs

[Olshansk]

This ADR does not define where/how Peer Identity is used and where/how NodeID is used.

[Olshansk]

I did a bit more research in how we're using NodeID, and here's what I found.

How we define it (changes with every height where the valset is modified):

func NewActorMapper(validators []*coreTypes.Actor) *actorMapper {
	am := &actorMapper{
		valAddrToIdMap: make(ValAddrToIdMap, len(validators)),
		idToValAddrMap: make(IdToValAddrMap, len(validators)),
		validatorMap:   make(ValidatorMap, len(validators)),
	}

	valAddresses := make([]string, 0, len(validators))
	for _, val := range validators {
		addr := val.GetAddress()
		valAddresses = append(valAddresses, addr)
		am.validatorMap[addr] = val
	}
	sort.Strings(valAddresses)

	for i, addr := range valAddresses {
		nodeId := NodeId(i + 1) // TODO(#434): Improve the use of NodeIDs
		am.valAddrToIdMap[addr] = nodeId
		am.idToValAddrMap[nodeId] = addr
	}

	return am
}

How we use it (for deterministic round robin):

func (m *leaderElectionModule) electNextLeaderDeterministicRoundRobin(message *typesCons.HotstuffMessage) (typesCons.NodeId, error) {
	height := int64(message.Height)
	readCtx, err := m.GetBus().GetPersistenceModule().NewReadContext(height)
	if err != nil {
		return typesCons.NodeId(0), err
	}
	defer readCtx.Release()

	vals, err := readCtx.GetAllValidators(height)
	if err != nil {
		return typesCons.NodeId(0), err
	}

	value := int64(message.Height) + int64(message.Round) + int64(message.Step) - 1
	numVals := int64(len(vals))

	return typesCons.NodeId(value%numVals + 1), nil
}

If we leverage the PeerID, we can just use the sorting mechanism there and avoid a separate "NodeId".

It feels like it would cause less cognitive load for new entrants to the codebase. Wdyt?

[Olshansk]

@bryanchriswhite wanted to follow up on my comments in this ADR.