chore(state-transition): flipped transaction context attributes logic #2451

abi87 · 2025-01-31T20:58:07Z

most of the attributes in transaction.Context have negated logic which does not feel natural to me.
This PR flips that, so that we would have VerifyPayload instead of SkipPayloadVerification etc

codecov · 2025-01-31T21:00:40Z

Codecov Report

Attention: Patch coverage is 20.51282% with 124 lines in your changes missing coverage. Please review.

Project coverage is 32.28%. Comparing base (7abf2d8) to head (245c368).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
primitives/transition/context.go	0.00%	50 Missing ⚠️
beacon/blockchain/finalize_block.go	0.00%	35 Missing ⚠️
beacon/validator/block_builder.go	0.00%	16 Missing ⚠️
beacon/blockchain/process_proposal.go	0.00%	14 Missing ⚠️
state-transition/core/state_processor_payload.go	50.00%	2 Missing and 2 partials ⚠️
node-core/components/state_processor.go	0.00%	2 Missing ⚠️
state-transition/core/state_processor.go	75.00%	1 Missing and 1 partial ⚠️
state-transition/core/state_processor_randao.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2451      +/-   ##
==========================================
- Coverage   32.35%   32.28%   -0.08%     
==========================================
  Files         350      350              
  Lines       15597    15634      +37     
  Branches       20       20              
==========================================
  Hits         5047     5047              
- Misses      10187    10224      +37     
  Partials      363      363

Files with missing lines	Coverage Δ
beacon/blockchain/service.go	`0.00% <ø> (ø)`
beacon/validator/service.go	`0.00% <ø> (ø)`
node-core/components/api.go	`0.00% <ø> (ø)`
node-core/components/chain_service.go	`0.00% <ø> (ø)`
node-core/components/validator_service.go	`0.00% <ø> (ø)`
state-transition/core/state_processor_genesis.go	`41.07% <100.00%> (ø)`
state-transition/core/state_processor_signature.go	`0.00% <ø> (ø)`
state-transition/core/state_processor_staking.go	`62.79% <100.00%> (+0.88%)`	⬆️
...tate-transition/core/state_processor_validators.go	`70.96% <100.00%> (ø)`
...ate-transition/core/state_processor_withdrawals.go	`50.61% <ø> (ø)`
... and 10 more

shotes

nits, otherwise LGTM

primitives/transition/context.go

state-transition/core/types.go

beacon/blockchain/finalize_block.go

primitives/transition/context.go

calbera · 2025-01-31T22:20:19Z

state-transition/core/state_processor.go

@@ -37,7 +37,7 @@ import (

 // StateProcessor is a basic Processor, which takes care of the
 // main state transition for the beacon chain.
-type StateProcessor[ContextT Context] struct {
+type StateProcessor struct {


If we're fixing this I love the removal of generics. But I would prefer using the transition.Context as an interface so that the fields of the struct are not mutable in the stateProcessor. Thoughts?

I have a slight preference for the current proposal (exported attributes).
If we keep the getter, I think we should unexport the attributes and so introduce a constructor too.
Happy to go with the majority here

cc @berachain/core

+1 for un-exporting the struct fields initialized via NewXXX method and having simple read only getters.

Added encapsulation, LMK how do you like this

rezbera

This is net improvement as is - lgtm

abi87 · 2025-02-04T01:22:14Z

primitives/transition/context.go

+func (c *Context) WithMeterGas(meter bool) *Context {
+	c.meterGas = meter
+	return c
+}
+
+func (c *Context) WithOptimisticEngine(optimistic bool) *Context {
+	c.optimisticEngine = optimistic
+	return c
+}
+
+func (c *Context) WithVerifyPayload(verifyPayload bool) *Context {
+	c.verifyPayload = verifyPayload
+	return c
+}
+
+func (c *Context) WithVerifyRandao(verifyRandao bool) *Context {
+	c.verifyRandao = verifyRandao
+	return c
+}
+
+func (c *Context) WithVerifyResult(verifyResult bool) *Context {
+	c.verifyResult = verifyResult
+	return c


I would rather pass attributes with these options to avoid a whole bunch of unreadable booleans in the constructor.
I would be down to do that if golang had proper typed enum. I think the solution I propose here is all in all the ones with the least code written (that does not pass booleans in the ctor)

In this case we may use opts pattern instead as otherwise we could update Context after initialization. Not a big deal though if we use readonly interface for accessing fields

abi87 · 2025-02-04T01:23:01Z

primitives/transition/context.go

+		verifyPayload: true,
+		verifyRandao:  true,
+		verifyResult:  true,


I rather set all validators to true by default. I think the safest is to explicitly switch them off

calbera · 2025-02-04T01:58:54Z

state-transition/core/types.go

@@ -33,32 +33,6 @@ import (
 	"github.com/karalabe/ssz"
 )

-// Context defines an interface for managing state transition context.
-type Context interface {


I think this Context interface should still be used so that only the read-only methods are usable in this package. Currently the Setters can still be called from within state-transition

Added a transition.ReadOnlyContext interface

abi87 · 2025-02-04T03:23:29Z

testing/state-transition/state-transition.go

+	).
+		WithVerifyPayload(false).
+		WithVerifyRandao(false).
+		WithVerifyResult(false).


technically WithVerifyRandao should be true cause SkiValidateRandao was set to the default value of false.
However the intention of the tests was to skip these checks at all and focus on the state transition, so it's ok to not verify randao (and this PR makes it more explicit that it currently is)

calbera · 2025-02-04T16:45:08Z

primitives/transition/context.go

@@ -26,73 +26,130 @@ import (
 	"github.com/berachain/beacon-kit/primitives/math"
 )

+// ReadOnlyContext defines an interface for managing state transition context.
+type ReadOnlyContext interface {


I actually prefer the pattern of the package that uses a type defines the interface in their own package. Rather than exporting a global interface

Is this really necessary when we have only packages and not modules? I think a single interface reduced code duplication

not absolutely necessary. I generally like following this pattern and is used throughout the repo https://medium.com/@mbinjamil/using-interfaces-in-go-the-right-way-99384bc69d39

I am fine with this

From https://go.dev/wiki/CodeReviewComments:

Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values... Do not define interfaces before they are used: without a realistic example of usage, it is too difficult to see whether an interface is even necessary, let alone what methods it ought to contain.

So I dropped the interface from transition.Context file and ended up having a single one in state-processor.
Basically the only use of this interface is using this as parameter in Transition method, so I think this is fine

fridrik01 · 2025-02-04T16:59:03Z

primitives/transition/context.go

@@ -26,73 +26,130 @@ import (
 	"github.com/berachain/beacon-kit/primitives/math"
 )

+// ReadOnlyContext defines an interface for managing state transition context.
+type ReadOnlyContext interface {


I am fine with this

fridrik01 · 2025-02-04T17:03:16Z

primitives/transition/context.go

+func (c *Context) WithMeterGas(meter bool) *Context {
+	c.meterGas = meter
+	return c
+}
+
+func (c *Context) WithOptimisticEngine(optimistic bool) *Context {
+	c.optimisticEngine = optimistic
+	return c
+}
+
+func (c *Context) WithVerifyPayload(verifyPayload bool) *Context {
+	c.verifyPayload = verifyPayload
+	return c
+}
+
+func (c *Context) WithVerifyRandao(verifyRandao bool) *Context {
+	c.verifyRandao = verifyRandao
+	return c
+}
+
+func (c *Context) WithVerifyResult(verifyResult bool) *Context {
+	c.verifyResult = verifyResult
+	return c


In this case we may use opts pattern instead as otherwise we could update Context after initialization. Not a big deal though if we use readonly interface for accessing fields

…erachain/beacon-kit into flip-transaction-context-logic

calbera

LGTM

flipped transaction context attributes logic

7ce1d75

abi87 self-assigned this Jan 31, 2025

abi87 marked this pull request as ready for review January 31, 2025 21:14

abi87 requested a review from a team as a code owner January 31, 2025 21:14

abi87 added the Ready for Review label Jan 31, 2025

nit

e473864

shotes reviewed Jan 31, 2025

View reviewed changes

primitives/transition/context.go Outdated Show resolved Hide resolved

state-transition/core/types.go Outdated Show resolved Hide resolved

abi87 added 2 commits January 31, 2025 16:43

dropped Context generic from state processor

7d680f2

dropped transition Context getters

f18866c

rezbera reviewed Jan 31, 2025

View reviewed changes

beacon/blockchain/finalize_block.go Outdated Show resolved Hide resolved

abi87 requested review from rezbera and shotes January 31, 2025 22:04

abi87 enabled auto-merge (squash) January 31, 2025 22:04

abi87 disabled auto-merge January 31, 2025 22:05

rezbera reviewed Jan 31, 2025

View reviewed changes

primitives/transition/context.go Outdated Show resolved Hide resolved

extended context

76d6b93

abi87 requested a review from rezbera January 31, 2025 22:16

calbera reviewed Jan 31, 2025

View reviewed changes

Merge branch 'main' into flip-transaction-context-logic

8bbf311

calbera deployed to e2e-test January 31, 2025 22:33 — with GitHub Actions Active

abi87 added 2 commits January 31, 2025 17:53

Merge branch 'main' into flip-transaction-context-logic

7783eca

Merge branch 'main' into flip-transaction-context-logic

40d1f29

abi87 temporarily deployed to test-e2e January 31, 2025 23:47 — with GitHub Actions Inactive

rezbera approved these changes Feb 1, 2025

View reviewed changes

Merge branch 'main' into flip-transaction-context-logic

5cea07e

rezbera temporarily deployed to test-e2e February 1, 2025 21:20 — with GitHub Actions Inactive

Merge branch 'main' into flip-transaction-context-logic

337523a

abi87 temporarily deployed to test-e2e February 2, 2025 15:02 — with GitHub Actions Inactive

Merge branch 'main' into flip-transaction-context-logic

90c93f9

abi87 had a problem deploying to test-e2e February 3, 2025 21:00 — with GitHub Actions Error

some more generics cleanup

8ff8e9a

abi87 had a problem deploying to test-e2e February 3, 2025 21:31 — with GitHub Actions Error

improved context encapsulation

de178af

abi87 had a problem deploying to test-e2e February 3, 2025 22:02 — with GitHub Actions Error

restored comments

dca0a0f

abi87 temporarily deployed to test-e2e February 3, 2025 22:08 — with GitHub Actions Inactive

abi87 requested review from fridrik01, rezbera and calbera February 4, 2025 01:20

abi87 commented Feb 4, 2025

View reviewed changes

calbera reviewed Feb 4, 2025

View reviewed changes

abi87 commented Feb 4, 2025

View reviewed changes

introduced ReadOnlyContext

45b051e

abi87 temporarily deployed to test-e2e February 4, 2025 16:39 — with GitHub Actions Inactive

abi87 requested a review from calbera February 4, 2025 16:40

calbera reviewed Feb 4, 2025

View reviewed changes

fridrik01 approved these changes Feb 4, 2025

View reviewed changes

abi87 added 2 commits February 4, 2025 12:16

reduced interface scope

712d68e

Merge branch 'main' into flip-transaction-context-logic

d430515

abi87 had a problem deploying to test-e2e February 4, 2025 17:23 — with GitHub Actions Error

Merge branch 'flip-transaction-context-logic' of https://github.com/b…

28d5631

…erachain/beacon-kit into flip-transaction-context-logic

abi87 temporarily deployed to test-e2e February 4, 2025 17:27 — with GitHub Actions Inactive

abi87 requested a review from calbera February 4, 2025 17:29

calbera approved these changes Feb 4, 2025

View reviewed changes

Merge branch 'main' into flip-transaction-context-logic

245c368

calbera temporarily deployed to test-e2e February 4, 2025 19:30 — with GitHub Actions Inactive

abi87 merged commit 8bd60c6 into main Feb 4, 2025
19 checks passed

abi87 deleted the flip-transaction-context-logic branch February 4, 2025 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(state-transition): flipped transaction context attributes logic #2451

chore(state-transition): flipped transaction context attributes logic #2451

abi87 commented Jan 31, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading

shotes left a comment •

edited

Loading

calbera Jan 31, 2025

abi87 Jan 31, 2025

abi87 Jan 31, 2025

fridrik01 Feb 3, 2025

abi87 Feb 3, 2025

rezbera left a comment

abi87 Feb 4, 2025

fridrik01 Feb 4, 2025

abi87 Feb 4, 2025

calbera Feb 4, 2025

abi87 Feb 4, 2025

abi87 Feb 4, 2025

calbera Feb 4, 2025

abi87 Feb 4, 2025

calbera Feb 4, 2025

fridrik01 Feb 4, 2025

calbera Feb 4, 2025

abi87 Feb 4, 2025

fridrik01 Feb 4, 2025

fridrik01 Feb 4, 2025

calbera left a comment

chore(state-transition): flipped transaction context attributes logic #2451

chore(state-transition): flipped transaction context attributes logic #2451

Conversation

abi87 commented Jan 31, 2025 • edited Loading

codecov bot commented Jan 31, 2025 • edited Loading

Codecov Report

shotes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rezbera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calbera left a comment

Choose a reason for hiding this comment

abi87 commented Jan 31, 2025 •

edited

Loading

codecov bot commented Jan 31, 2025 •

edited

Loading

shotes left a comment •

edited

Loading