Add Verification + Tracking Logic To Simplex Blocks #4052

samliok · 2025-07-03T21:34:00Z

Why this should be merged

In addition to calling Verify on the blocks inner vmBlock, we need to do some additional checks to ensure a simplex block can be verified. Furthermore, we need to ensure that any verified blocks are eventually accepted or rejected.

How this works

In simplex we do some pre-verification logic. However this is not enough, as we do not have access to the inner vmBlock pointed to by the prev field in the ProtocolMetadata. This means our verification logic in avalanchego must verify that prev for the block we are currently verifying points to that blocks parent. To understand more about why this important, see this test.

This PR also introduces a blockTracker struct to keep track of blocks that have been verified. When index is called on a digest, the blockTracker calls Accept on that block and Reject on any competing blocks.

How this was tested

Need to be documented in RELEASES.md?

Copilot

Pull Request Overview

This PR refactors change tracking in the Merkle DB view and history layers, replacing the old keyChanges+sortedKeys approach with a unified values map and a new recordValueChange method. It also updates the view iterator to reflect the map-based storage and overhauls history retrieval functions to collapse and filter changes via the new map structure.

Replaces keyChanges and sortedKeys with values map and recordValueChange in view.go
Updates NewIteratorWithStartAndPrefix to iterate over values and then sort the results in view_iterator.go
Refactors history functions (getValueChanges, getChangesToGetToRoot) to use the new map-based change summary in history.go and updates tests

Reviewed Changes

Copilot reviewed 89 out of 91 changed files in this pull request and generated 1 comment.

File	Description
x/merkledb/view.go	Introduced `recordValueChange` method and `values` map
x/merkledb/view_iterator.go	Changed iterator to range over `values` and sort KeyChanges
x/merkledb/history.go	Refactored history change accumulation to use `changeSummary`
x/merkledb/history_test.go	Updated tests to assert on `values` instead of slices

Comments suppressed due to low confidence (2)

x/merkledb/view_iterator.go:27

[nitpick] The local variable changes shadows v.changes and may be confusing. Consider renaming it to something like result or filteredChanges.

		changes   = make([]KeyChange, 0, len(v.changes.values))

x/merkledb/view_iterator.go

yacovm · 2025-07-08T20:36:48Z

simplex/block.go


 	"github.com/ava-labs/simplex"

 	"github.com/ava-labs/avalanchego/snow/consensus/snowman"
 	"github.com/ava-labs/avalanchego/snow/engine/snowman/block"
 	"github.com/ava-labs/avalanchego/utils/hashing"
+	"github.com/ava-labs/avalanchego/vms/proposervm/tree"


Can we move vms/proposervm/tree to somewhere like snow/tree ? It's a bit insanitary that Simplex uses something that is in proposervm

moved to utils

yacovm · 2025-07-08T21:14:37Z

simplex/block.go

+	}
+
+	// if we do not have it in the map, it's possible for it to be the last accepted block
+	lastID, err := b.blockTracker.vm.LastAccepted(ctx)


Instead of doing that can't we load the last accepted to simplexDigestsToBlock upon creation? This way we also don't depend on the implementation of the VM's LastAccepted in this method, for instance if the VM doesn't implement this efficiently.

But the bigger benefit is that we should always find the previous block this way (unless it's the genesis block) and we have less special handling.

yacovm · 2025-07-08T21:23:06Z

simplex/block.go

+		return fmt.Errorf("failed to get last accepted block: %w", err)
+	}
+
+	if lastID != b.vmBlock.Parent() {


I would very much like us to explicitly make sure that the chain induced by Simplex is aligned with the chain induced by the VM, and not only the inner VMs are aligned:

prevBlock.BlockHeader().Digest == b.metadata.Prev && prevBlock.vmBlock.ID() == b.vmBlock.Parent()

yacovm · 2025-07-08T21:24:55Z

simplex/block.go

+
+	bd, exists := bt.simplexDigestsToBlock[digest]
+	if !exists {
+		return fmt.Errorf("%w: %s", errDigestNotFound, digest)


this case isn't covered by tests. Can we add a test for this?

yacovm · 2025-07-09T17:00:52Z

simplex/block.go

+	simplexDigestsToBlock map[simplex.Digest]*Block
+
+	// lastAcceptedID is the ID of the last accepted block at the time of the block tracker creation.
+	lastAcceptedID ids.ID


shouldn't we update these two values once we we call indexBlock?

Can we make a test that is like TestVerifyInnerBlockBreaksHashChain but goes through several blocks?

i dont think we need to update them, since we will have always have the last accepted id/digest in the block trackers memory. I do think the name is misleading though. Added to this test

simplex/block.go

yacovm · 2025-07-14T23:11:36Z

simplex/block.go

+	if b.blockTracker.isBlockAlreadyVerified(b.vmBlock) {
+		return b, nil
+	}
+


I think we need to grab the context lock here, because we're executing this function asynchronously.

I think that there is something I overlooked, please see comment.

StephenButtolph · 2025-07-16T14:45:05Z

simplex/block.go

+		return fmt.Errorf("%w: %s", errDigestNotFound, b.metadata.Prev)
+	}
+
+	if b.vmBlock.Parent() != prevBlock.vmBlock.ID() || b.metadata.Prev != prevBlock.digest {


When can b.metadata.Prev != prevBlock.digest ever be true? Removing this check doesn't break any tests.

StephenButtolph · 2025-07-16T14:46:33Z

simplex/block.go

+	simplexDigestsToBlock := make(map[simplex.Digest]*Block)
+	simplexDigestsToBlock[latestBlock.digest] = latestBlock
+
+	return &blockTracker{
+		tree:                  tree.New(),
+		simplexDigestsToBlock: simplexDigestsToBlock,
+	}


Suggested change

simplexDigestsToBlock := make(map[simplex.Digest]*Block)

simplexDigestsToBlock[latestBlock.digest] = latestBlock

return &blockTracker{

tree: tree.New(),

simplexDigestsToBlock: simplexDigestsToBlock,

}

return &blockTracker{

tree: tree.New(),

simplexDigestsToBlock: map[simplex.Digest]*Block{

latestBlock.digest: latestBlock,

},

}

StephenButtolph · 2025-07-16T14:52:58Z

simplex/util_test.go

+// newBlockWithDigest is a helper function that creates a new block and sets its digest.
+// This is helpful since otherwise we would need the blockDeserializer to create the block.
+func newBlockWithDigest(t *testing.T, vmBlock snowman.Block, tracker *blockTracker, round, seq uint64, prev simplex.Digest) *Block {


I have found in other tests that having various newBlockWithXXX(, newBlock( ect scales really annoyingly.

If we don't foresee adding more things here, then feel free to ignore... But it might make sense to make a single newBlock function which takes in a config with reasonable zero values.

That has proven to be much more flexible in my experience.

StephenButtolph · 2025-07-16T14:54:07Z

simplex/block.go

+	if b.blockTracker.isBlockAlreadyVerified(b.vmBlock) {
+		return b, nil
+	}


Don't we still need to add b to the simplexDigestsToBlock map here?

StephenButtolph · 2025-07-16T14:55:43Z

simplex/block.go

+	err := b.verifyParentMatchesPrevBlock()
+	if err != nil {


For simple checks like these (where err shouldn't be referenced later) I think we typically try to use inline if statements. The only exception to this is with very long (multi-line) statements, which are hard to read imo.

Suggested change

err := b.verifyParentMatchesPrevBlock()

if err != nil {

if err := b.verifyParentMatchesPrevBlock(); err != nil {

StephenButtolph · 2025-07-16T14:56:04Z

simplex/block.go

+	err = b.vmBlock.Verify(ctx)
+	if err != nil {


See above comment

Suggested change

err = b.vmBlock.Verify(ctx)

if err != nil {

if err := b.vmBlock.Verify(ctx); err != nil {

StephenButtolph · 2025-07-16T14:58:15Z

simplex/block.go

+	prevBlock := b.blockTracker.getBlockByDigest(b.metadata.Prev)
+	if prevBlock == nil {


I know that maps return the zero value when looking up something fails, and that we never expect to actually place a nil value in the map. But in avalanchego we typically do explicit success checks (so prevBlock, ok := ...)

Copilot AI review requested due to automatic review settings July 3, 2025 21:34

samliok requested review from joshua-kim, rrazvan1, maru-ava, StephenButtolph and meaghanfitzgerald as code owners July 3, 2025 21:34

github-project-automation bot added this to avalanchego Jul 3, 2025

samliok marked this pull request as draft July 3, 2025 21:34

Copilot AI reviewed Jul 3, 2025

View reviewed changes

x/merkledb/view_iterator.go Outdated Show resolved Hide resolved

samliok force-pushed the simplex-tracker branch from 39129f0 to 99463d4 Compare July 3, 2025 21:36

samliok marked this pull request as ready for review July 3, 2025 21:36

samliok marked this pull request as draft July 3, 2025 21:36

samliok force-pushed the simplex-tracker branch from 99463d4 to 217daff Compare July 3, 2025 21:45

samliok added 4 commits July 7, 2025 15:43

add verification + tracking logic

5dc8508

add testing

6bed3a0

unit tests passing

da9bdfc

comments + use test helper

152bc99

samliok force-pushed the simplex-tracker branch from 24727f8 to 152bc99 Compare July 7, 2025 19:44

update comment

66330d7

samliok marked this pull request as ready for review July 7, 2025 19:45

better comments

5b15e69

yacovm reviewed Jul 8, 2025

View reviewed changes

samliok added 4 commits July 8, 2025 17:50

check prev digest

68532e3

test errDigestnotfound

1f7609b

add last accepted block and digest

566a549

mv tree to utils

e5db65f

yacovm reviewed Jul 9, 2025

View reviewed changes

samliok added 2 commits July 9, 2025 14:40

add test

46e2bdd

latest block

a94ea87

yacovm reviewed Jul 9, 2025

View reviewed changes

simplex/block.go Outdated Show resolved Hide resolved

samliok and others added 2 commits July 10, 2025 11:18

return early on double verify

a9b6a3f

Merge branch 'master' into simplex-tracker

9121363

yacovm previously approved these changes Jul 10, 2025

View reviewed changes

joshua-kim moved this to Ready 🚦 in avalanchego Jul 14, 2025

yacovm reviewed Jul 14, 2025

View reviewed changes

Merge branch 'master' into simplex-tracker

e985f2a

StephenButtolph reviewed Jul 16, 2025

View reviewed changes

	err := b.verifyParentMatchesPrevBlock()
	if err != nil {
	if err := b.verifyParentMatchesPrevBlock(); err != nil {

	err = b.vmBlock.Verify(ctx)
	if err != nil {
	if err := b.vmBlock.Verify(ctx); err != nil {

		prevBlock := b.blockTracker.getBlockByDigest(b.metadata.Prev)
		if prevBlock == nil {

Add Verification + Tracking Logic To Simplex Blocks #4052

Are you sure you want to change the base?

Add Verification + Tracking Logic To Simplex Blocks #4052

Conversation

samliok commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

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!

yacovm Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

samliok commented Jul 3, 2025 •

edited

Loading

yacovm Jul 9, 2025 •

edited

Loading