Reconfig by yacovm · Pull Request #360 · ava-labs/Simplex

yacovm · 2026-03-23T14:20:32Z

No description provided.

samliok · 2026-03-30T15:45:06Z

msm/fake_node_test.go

+	}
+	require.Greater(t, node.Epoch(), uint64(1))
+
+	return


why do we return here?

oops, artifact from debugging 🤦‍♂️

Fixed.

samliok · 2026-03-30T15:53:56Z

msm/msm_test.go

+type outerBlock struct {
+	finalization *simplex.Finalization
+	block        metadata.StateMachineBlock
+}
+
+type blockStore map[uint64]*outerBlock


can we store helper structs like this in their own files? alongside functions like newStateMachine()?

it's hard to review since im bouncing around these large files trying to separate/review the helpers from the tests

samliok · 2026-03-30T15:55:02Z

msm/msm_test.go

+func (bs blockStore) clone() blockStore {
+	newStore := make(blockStore)
+	for k, v := range bs {
+		newStore[k] = v
+	}
+	return newStore
+}


Suggested change

func (bs blockStore) clone() blockStore {

newStore := make(blockStore)

for k, v := range bs {

newStore[k] = v

}

return newStore

}

func (bs blockStore) clone() blockStore {

newStore := make(blockStore)

maps.Copy(newStore, bs)

return newStore

}

samliok · 2026-04-01T18:03:21Z

msm/msm.go

+	return sm.wrapBlock(parentBlock, childBlock, simplexEpochInfo, pChainHeight, simplexMetadata, simplexBlacklist), nil
+}
+
+func (sm *StateMachine) createSealingBlock(ctx context.Context, parentBlock StateMachineBlock, simplexMetadata []byte, simplexBlacklist []byte, simplexEpochInfo SimplexEpochInfo, newApprovals *approvals, pChainHeight uint64) (*StateMachineBlock, error) {


newApprovals is unused

samliok · 2026-04-01T18:24:45Z

msm/encoding.go

+	})
+}
+
+func (nbms NodeBLSMappings) ForEach(selector func(int, NodeBLSMapping)) {


why can't we just do

for i, nbm := range NodeBLSMappings { }

samliok · 2026-04-01T18:33:21Z

msm/msm.go

+	// GetPChainHeight returns the latest known P-chain height.
+	GetPChainHeight func() uint64
+	// GetUpgrades returns the current upgrade configuration.
+	GetUpgrades func() UpgradeConfig


im confused by this function. The only time we set it is in a test, and it is set to a noop. If it is not necessary now, can we remove it from this pr?

It's needed by ICM epochs

samliok · 2026-04-01T20:41:01Z

msm/encoding.go

+			return
+		}
+		if selector(i, nbm) {
+			total, err = safeAdd(total, nbm.Weight)


why not just return the error right away?

this function feels so ai generated 😅 so much unnecessary complexity for a simple for loop + sum counter. Unless i am missing something?

samliok · 2026-04-01T20:45:54Z

msm/encoding.go

+
+type ValidatorSetApprovals []ValidatorSetApproval
+
+func (vsa ValidatorSetApprovals) ForEach(f func(int, ValidatorSetApproval)) {


samliok · 2026-04-01T20:46:09Z

msm/encoding.go

+		}
+	})
+	return result
+}


needs a new line at the end

samliok · 2026-04-01T20:49:46Z

msm/encoding.go

+func (nbms NodeBLSMappings) Equal(other NodeBLSMappings) bool {
+	if len(nbms) != len(other) {
+		return false
+	}
+
+	nbmsClone := nbms.Clone()
+	otherClone := other.Clone()
+
+	slices.SortFunc(nbmsClone, func(a, b NodeBLSMapping) int {
+		return slices.Compare(a.NodeID[:], b.NodeID[:])
+	})
+
+	slices.SortFunc(otherClone, func(a, b NodeBLSMapping) int {
+		return slices.Compare(a.NodeID[:], b.NodeID[:])
+	})
+
+	for i := range nbmsClone {
+		if !nbmsClone[i].Equals(&otherClone[i]) {
+			return false
+		}
+	}
+	return true
+}


Suggested change

func (nbms NodeBLSMappings) Equal(other NodeBLSMappings) bool {

if len(nbms) != len(other) {

return false

}

nbmsClone := nbms.Clone()

otherClone := other.Clone()

slices.SortFunc(nbmsClone, func(a, b NodeBLSMapping) int {

return slices.Compare(a.NodeID[:], b.NodeID[:])

})

slices.SortFunc(otherClone, func(a, b NodeBLSMapping) int {

return slices.Compare(a.NodeID[:], b.NodeID[:])

})

for i := range nbmsClone {

if !nbmsClone[i].Equals(&otherClone[i]) {

return false

}

}

return true

}

func (nbms NodeBLSMappings) Equal(other NodeBLSMappings) bool {

if len(nbms) != len(other) {

return false

}

sortByNodeID := func(a, b NodeBLSMapping) int {

return slices.Compare(a.NodeID[:], b.NodeID[:])

}

nbmsClone := nbms.Clone()

otherClone := other.Clone()

slices.SortFunc(nbmsClone, sortByNodeID)

slices.SortFunc(otherClone, sortByNodeID)

return slices.EqualFunc(nbmsClone, otherClone, func(a, b NodeBLSMapping) bool {

return a.Equals(&b)

})

}

also why don't we just keep these sets sorted? potentially we could require GetValidatorSet to return a sorted array, or create a wrapper like GetSortedValidatorSet which would save us a bunch of slices.SortFunc() calls as equal can be called multiple times

also we could just store the NodeBLSMappings as a map and then assert equality in linear time rather than sorting both functions and then iterating(O(nlogn) * 2 + n) ?

samliok · 2026-04-01T21:03:44Z

msm/encoding.go

+	canotoData canotoData_ValidatorSetApproval
+}
+
+type ValidatorSetApprovals []ValidatorSetApproval


why is this an array and not a map with the nodeIDs being keys? seems like UniqueByNodeID would benefit from this.

It's not unique. A node may send several approvals, for different payloads.

That's why we filter them:

newApprovals = newApprovals.Filter(func(i int, approval ValidatorSetApproval) bool { // Pick only approvals that agree with our candidate auxiliary info digest and P-Chain height return approval.PChainHeight == pChainHeight && approval.AuxInfoSeqDigest == candidateAuxInfoDigest })

samliok · 2026-04-02T18:43:43Z

msm/README.md

+- The genesis block does not contain the Simplex epoch information, and it is implicitly set to be the zero values of the fields above.
+
+
+### Auxiliary information encoding


alongside separating out the ICM stuff from this PR, I think we should also remove the auxillary information to reduce the diff of this PR. thoughts?

samliok · 2026-04-03T15:54:31Z

msm/fake_node_test.go

+func (fn *fakeNode) tryFinalizeNextBlock() {
+	nextIndex := len(fn.finalizedBlocks)
+
+	if fn.isNextBlockTelock() {
+		return
+	}
+
+	block := fn.notarizedBlocks[nextIndex]


it seems like we can some index out of bounds exceptions/nil panics if notarizedBlocks & finalizedBlocks are out of sync. For example this test panics

func TestTryFinalize(t *testing.T) { fn := newFakeNode(t) fn.tryFinalizeNextBlock() }

is there any benefit to having the block in two places? can we have something like

type blockState struct { block metadata.StateMachineBlock finalized bool }

and then remove the two arrays and just have a singular array in the node state?

samliok · 2026-04-03T16:01:15Z

msm/fake_node_test.go

+	Prev [32]byte
+}
+
+type fakeNode struct {


im not a fan of the name fakeNode, it is too generic and non-descriptive as it doesn't really tell me much about what the node actually does, i.e. the part being faked.

we already have BasicNode, LongRunningNode, ControlledNode. Maybe MultiEpochNode?

samliok · 2026-04-03T16:04:15Z

msm/fake_node_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestFakeNode(t *testing.T) {


this test was initially hard to follow. TestFakeNode makes it seem like we are trying to test the fake node implementation, even though we don't really care about testing the fake node implementation. Maybe TestStateMachineEpochTransition is better?

samliok · 2026-04-03T16:16:26Z

msm/fake_node_test.go

+	notarizedBlocks []metadata.StateMachineBlock
+	finalizedBlocks []metadata.StateMachineBlock


should we consider consolidating these into a single data structure?

samliok · 2026-04-03T18:38:05Z

msm/msm.go

+	sm.maybeInit()
+
+	// The zero sequence number is reserved for the genesis block, which should never be built.
+	if simplexMetadata.Seq == 0 {


should we add a test for this case?

samliok · 2026-04-03T19:12:34Z

msm/msm.go

+	return block.InnerBlock.Verify(ctx)
+}
+
+func (sm *StateMachine) identifyCurrentState(prevBlockSimplexEpochInfo SimplexEpochInfo) (state, error) {


Suggested change

func (sm *StateMachine) identifyCurrentState(prevBlockSimplexEpochInfo SimplexEpochInfo) (state, error) {

func epochState(epochInfo SimplexEpochInfo) (state, error) {

i dont think this function should be associated with StateMachine.

samliok · 2026-04-03T19:14:23Z

msm/msm.go

+	return block.InnerBlock.Verify(ctx)
+}
+
+func (sm *StateMachine) identifyCurrentState(prevBlockSimplexEpochInfo SimplexEpochInfo) (state, error) {


I think this suggestion is even better than the previous. Also we don't need to return error regardless

Suggested change

func (sm *StateMachine) identifyCurrentState(prevBlockSimplexEpochInfo SimplexEpochInfo) (state, error) {

func (i *SimplexEpochInfo) CurrentState() state {

we should also probably write tests for this function

samliok · 2026-04-03T19:17:52Z

msm/msm.go

+}
+
+func computePrevVMBlockSeq(parentBlock StateMachineBlock, prevBlockSeq uint64) uint64 {
+	// Either our parent block has no inner block, in which case we just inherit its previous VM block sequence,


Suggested change

// Either our parent block has no inner block, in which case we just inherit its previous VM block sequence,

// Either our parent block has no inner block, in which case we just inherit its previous VM block sequence.

samliok · 2026-04-03T19:23:57Z

msm/msm.go

+	case stateFirstSimplexBlock:
+		return sm.buildBlockZero(ctx, parentBlock, simplexMetadataBytes, simplexBlacklistBytes)
+	case stateBuildBlockNormalOp:
+		return sm.buildBlockNormalOp(ctx, parentBlock, simplexMetadataBytes, simplexBlacklistBytes, prevBlockSeq)
+	case stateBuildCollectingApprovals:
+		return sm.buildBlockCollectingApprovals(ctx, parentBlock, simplexMetadataBytes, simplexBlacklistBytes, prevBlockSeq)
+	case stateBuildBlockEpochSealed:
+		return sm.buildBlockEpochSealed(ctx, parentBlock, simplexMetadataBytes, simplexBlacklistBytes, prevBlockSeq)


if we pass in the simplexMetadata as simplex.Metadata instead of bytes, we can remove prevBlockSeq from all the function headers and just use simplexMetadata.Seq - 1. Then when we need the bytes, we should just call simplexMetadata.Bytes()

This commit adds an implementation of the Metadata State Machine (MSM). It intercepts block building and verification from the Simplex instance, and performs or verifies metadata state transitions according to the logic defined in the README.md. Contains: - MSM block building and verification logic - Epoch transition (sealing blocks, Telocks, approval collection) - Block encoding with canoto - Unit and integration tests (fake node test and full epoch lifecycle test) Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>

yacovm marked this pull request as draft March 23, 2026 14:20

yacovm force-pushed the reconfig branch 6 times, most recently from 4aa113e to ef4c05f Compare March 30, 2026 14:33

samliok reviewed Mar 30, 2026

View reviewed changes

samliok reviewed Apr 1, 2026

View reviewed changes

samliok reviewed Apr 3, 2026

View reviewed changes

yacovm added 5 commits April 6, 2026 18:02

fix alignment to document

d6c8cb0

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>

Improve build_decision

f4b7e0c

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>

Fix test

ff02e14

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>

Add some comments

eb3e1b2

Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>

yacovm force-pushed the reconfig branch from 47d5fba to eb3e1b2 Compare April 6, 2026 16:02


		type ValidatorSetApprovals []ValidatorSetApproval

		func (vsa ValidatorSetApprovals) ForEach(f func(int, ValidatorSetApproval)) {

		- The genesis block does not contain the Simplex epoch information, and it is implicitly set to be the zero values of the fields above.


		### Auxiliary information encoding

		notarizedBlocks []metadata.StateMachineBlock
		finalizedBlocks []metadata.StateMachineBlock

	func (sm *StateMachine) identifyCurrentState(prevBlockSimplexEpochInfo SimplexEpochInfo) (state, error) {
	func epochState(epochInfo SimplexEpochInfo) (state, error) {

	func (sm *StateMachine) identifyCurrentState(prevBlockSimplexEpochInfo SimplexEpochInfo) (state, error) {
	func (i *SimplexEpochInfo) CurrentState() state {

	// Either our parent block has no inner block, in which case we just inherit its previous VM block sequence,
	// Either our parent block has no inner block, in which case we just inherit its previous VM block sequence.

Conversation

yacovm commented Mar 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants