Atomic block extension #981

ceyonur · 2025-05-23T15:36:45Z

Why this should be merged

Seperates atomic block verification, accept, reject from block by introducing a general block extender/extension interface

How this works

This pull request introduces significant changes to the EVM plugin to support block extensions, refactor block verification, and improve atomic transaction handling. The most notable updates include replacing the snowman.Block interface with a new extension.ExtendedBlock, introducing a new blockExtension for handling block extensions. These changes aim to enhance modularity, maintainability, and support for future upgrades.

Added a new blockExtension implementation in plugin/evm/atomic_block_extension.go to handle block extensions, including atomic transaction extraction, syntactic/semantic verification, and UTXO validation.

These changes pave the way for a more extensible and robust EVM implementation, aligning with upcoming protocol upgrades and improving the handling of atomic transactions.

How this was tested

Existing UTs/e2e tests

Need to be documented?

no

Need to update RELEASES.md?

No

plugin/evm/block.go

alarso16 · 2025-06-02T14:06:43Z

plugin/evm/block.go

-		if atomicState, err := b.vm.atomicBackend.GetVerifiedAtomicState(b.ethBlock.Hash()); err == nil {
-			_ = atomicState.Reject() // ignore this error so we can return the original error instead.
-		}
+	if b.extension != nil && (err != nil || !writes) {


I think a comment here about what might be cleaned up would be helpful.

The same outcome can be achieved with a slight refactor, which IMO is preferable to a comment. Comments should be reserved for why, not how nor what. "How" and "what" can be communicated in code, and if they're not then it suggests that the code needs to be changed. "Why" can't be communicated in code, so it requires a comment.

if noWrite := err != nil || !writes; noWrite && b.extension != nil { b.extension.CleanupVerified() }

alarso16 · 2025-06-02T14:08:22Z

plugin/evm/block.go

 	return err
 }

+// semanticVerify verifies that a *Block is internally consistent.


I think this section could still go in another file. It's relatively tangential to the operations here.

semanticVerify does not do much other than calling the extension. I find it a bit weird to split a type's functions into different files unless it leads to a gigantic file (like VM)

Weren't these just moved from block_verification.go into here and the methods renamed? Or were they changed beyond that? The move adds a significant amount of unnecessary review.

plugin/evm/extension/config.go

plugin/evm/vm.go

plugin/evm/vm_extensible.go

plugin/evm/block.go

plugin/evm/atomic/tx_semantic_verifier.go

ARR4N · 2025-06-05T10:05:21Z

This was merged while I was still reviewing (sorry it's taken so long) as I'm trying to get my head around the ultimate goal of block extensions. Are there use cases beyond adding atomic transactions?

Abstractions add complexity (through indirection) and a requirement to maintain 2x the amount of code (implementation and interface definition), which I think are only justified when there is a need for more than one implementation.

EDIT: to clarify, I like the abstraction of the atomic functionality as standalone methods, and think it simplifies things for SAE integration. What I'm still concerned about is possibly premature abstraction and YAGNI.

ARR4N · 2025-06-05T10:15:08Z

plugin/evm/block.go


-// Block implements the snowman.Block interface
-type Block struct {
+// wrappedBlock implements the snowman.wrappedBlock interface


snowman.Block interface

ARR4N · 2025-06-05T10:17:40Z

plugin/evm/block.go

-// Block implements the snowman.Block interface
-type Block struct {
+// wrappedBlock implements the snowman.wrappedBlock interface
+type wrappedBlock struct {


What makes this a wrapped block? If I understand correctly it's actually wrapping something (the extension), not being wrapped. What am I misunderstanding?

ARR4N · 2025-06-05T10:22:12Z

plugin/evm/block.go

-		vm:        vm,
-		atomicTxs: atomicTxs,
-	}, nil
+	if vm.extensionConfig.BlockExtender != nil {


(subjective, readability) I would return early if it is nil, signalling to the reader what they need to remember and concentrate on:

extender := vm.extensionConfig.BlockExtender if extender == nil { return b, nil } ext, err := extender.NewBlockExtension(b) if err != nil { return nil, ... } b.extension = ext return b, nil

Extensive nesting adds complexity that adds cognitive load. Effectively our brains have to keep a stack for which every level of indentation is a push. The shorter the stack, the easier it is for the reader.

ARR4N · 2025-06-05T10:28:38Z

plugin/evm/block.go

-		if atomicState, err := b.vm.atomicBackend.GetVerifiedAtomicState(b.ethBlock.Hash()); err == nil {
-			_ = atomicState.Reject() // ignore this error so we can return the original error instead.
-		}
+	if b.extension != nil && (err != nil || !writes) {


The same outcome can be achieved with a slight refactor, which IMO is preferable to a comment. Comments should be reserved for why, not how nor what. "How" and "what" can be communicated in code, and if they're not then it suggests that the code needs to be changed. "Why" can't be communicated in code, so it requires a comment.

if noWrite := err != nil || !writes; noWrite && b.extension != nil { b.extension.CleanupVerified() }

ARR4N · 2025-06-05T10:35:49Z

plugin/evm/block.go

 	return err
 }

+// semanticVerify verifies that a *Block is internally consistent.


Weren't these just moved from block_verification.go into here and the methods renamed? Or were they changed beyond that? The move adds a significant amount of unnecessary review.

ARR4N · 2025-06-05T10:41:39Z

plugin/evm/mempool_atomic_gossiping_test.go

+			blockExtension, ok := wrappedBlock.GetBlockExtension().(atomic.AtomicBlockContext)
+			assert.True(ok, "unknown block extension type")
+
+			assert.Equal(txID, blockExtension.AtomicTxs()[0].ID(), "block does not include expected transaction")


Why only check the first one?

ARR4N · 2025-06-05T10:44:15Z

plugin/evm/vm.go

 		extDataHashes = fujiExtDataHashes
 	}
-	vm.syntacticBlockValidator = NewBlockValidator(extDataHashes)
+	vm.extensionConfig.BlockExtender = newBlockExtender(extDataHashes, vm)


Hard-coding the specific concrete implementation of an abstraction suggests the premature abstraction I was concerned about in my overall PR comment.

ARR4N · 2025-06-05T10:45:12Z

plugin/evm/vm.go


 	// Note: the status of block is set by ChainState
-	blk, err := vm.newBlock(block)
+	blk, err := wrapBlock(block, vm)


Why was this changed from a method to a function that accepts the original method's receiver value anyway?

ARR4N · 2025-06-05T10:48:36Z

plugin/evm/vm_extensible.go

+		return nil, err
+	}
+
+	return blk.(*wrappedBlock), nil


This isn't safe in production code as it could panic.

It's also another instance of a hard-coded concrete implementation of the abstraction.

ARR4N · 2025-06-05T10:48:42Z

plugin/evm/vm_extensible.go

+	if lastAcceptedBlock == nil {
+		return nil
+	}
+	return lastAcceptedBlock.(*wrappedBlock)


ceyonur changed the base branch from master to atomic-sync-pkg May 23, 2025 15:37

ceyonur commented May 23, 2025

View reviewed changes

plugin/evm/block.go Show resolved Hide resolved

ceyonur marked this pull request as ready for review May 27, 2025 20:58

ceyonur requested a review from a team as a code owner May 27, 2025 20:58

ceyonur added the atomic vm refactor label Jun 2, 2025

alarso16 reviewed Jun 2, 2025

View reviewed changes

Base automatically changed from atomic-sync-pkg to master June 3, 2025 22:44

ceyonur added 5 commits June 4, 2025 11:31

introduce atomic block extension

09297f8

rename block

8305c8e

implement atomicblockcontext

06c646f

remove params dep

c6e0fe6

reviews

d477ea2

ceyonur force-pushed the atomic-block-extension branch from 2ddb618 to d477ea2 Compare June 4, 2025 09:01

alarso16 approved these changes Jun 4, 2025

View reviewed changes

ceyonur enabled auto-merge June 4, 2025 23:57

ceyonur added this pull request to the merge queue Jun 4, 2025

Merged via the queue into master with commit 3d7a2c8 Jun 5, 2025
9 checks passed

ceyonur deleted the atomic-block-extension branch June 5, 2025 00:10

ARR4N reviewed Jun 5, 2025

View reviewed changes

ARR4N mentioned this pull request Jun 5, 2025

move block extender to vm #984

Merged

JonathanOppenheimer mentioned this pull request Aug 7, 2025

sync: coreth PR #963,981,1009: sync package extension and related items ava-labs/subnet-evm#1679

Draft

Atomic block extension #981

Atomic block extension #981

Uh oh!

Conversation

ceyonur commented May 23, 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?

Need to update RELEASES.md?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ARR4N Jun 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ARR4N commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ARR4N Jun 5, 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!

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!

4 participants

ceyonur commented May 23, 2025 •

edited

Loading

ARR4N Jun 5, 2025 •

edited

Loading

ARR4N commented Jun 5, 2025 •

edited

Loading

ARR4N Jun 5, 2025 •

edited

Loading