[factory] add parent to workingset #4539

dustinxie · 2025-01-09T06:39:30Z

Description

Prep for pipelined ValidateBlock/CommitBlock.

Fixes #(issue)

Type of change

Please delete options that are not relevant.

[] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[] Code refactor or improvement
[] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
[] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

[] make test
[] fullsync
[] Other test (please specify)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

[] My code follows the style guidelines of this project
[] I have performed a self-review of my code
[] I have commented my code, particularly in hard-to-understand areas
[] I have made corresponding changes to the documentation
[] My changes generate no new warnings
[] I have added tests that prove my fix is effective or that my feature works
[] New and existing unit tests pass locally with my changes
[] Any dependent changes have been merged and published in downstream modules

dustinxie · 2025-01-09T06:42:12Z

state/factory/factory.go

@@ -356,7 +360,8 @@ func (sf *factory) Validate(ctx context.Context, blk *block.Block) error {
 		if err := ws.ValidateBlock(ctx, blk); err != nil {
 			return errors.Wrap(err, "failed to validate block with workingset in factory")
 		}
-		sf.putIntoWorkingSets(key, ws)
+		sf.workingsets.Add(key, ws)
+		sf.workingsets.Add(ws.height, ws)


this is success validation for block n, validation of the next block may continue which needs the state changes in this block, so adding ws to cache, which will become parent of next block's workingset

dustinxie · 2025-01-09T06:43:33Z

state/factory/factory.go

@@ -398,7 +403,8 @@ func (sf *factory) NewBlockBuilder(

 	blkCtx := protocol.MustGetBlockCtx(ctx)
 	key := generateWorkingSetCacheKey(blkBuilder.GetCurrentBlockHeader(), blkCtx.Producer.String())
-	sf.putIntoWorkingSets(key, ws)
+	sf.workingsets.Add(key, ws)
+	sf.workingsets.Add(ws.height, ws)


this is used by new block proposal, so for the same reason adding ws to cache

dustinxie · 2025-01-09T06:46:12Z

state/factory/factory.go

 	}
+	ws.detachParent()


this is used by CommitBlock(), at this time parent of current ws must have successfully committed his state changes into stateDB, so the current ws must only rely on itself when processing the block

dustinxie · 2025-01-09T06:47:10Z

state/factory/factory.go

-func (sf *factory) putIntoWorkingSets(key hash.Hash256, ws *workingSet) {
-	sf.mutex.Lock()
-	defer sf.mutex.Unlock()
-	sf.workingsets.Add(key, ws)


sf.workingsets is thread-safe cache, so no need to lock

quick clean-up not relevant to primary PR

dustinxie · 2025-01-09T06:49:59Z

state/factory/util.go

+			break
+		}
+	}
+}


both factory and statedb have the same workingset cache.LRUCache, these can become a new small component WorkingSetCollection later

dustinxie · 2025-01-09T06:50:40Z

state/factory/workingset.go

-		}
-	}
-	if fCtx.CorrectTxLogIndex {
-		updateReceiptIndex(receipts)


this func is not used any more

dustinxie · 2025-01-09T06:52:14Z

state/factory/workingset.go

@@ -322,6 +318,9 @@ func (ws *workingSet) freshAccountConversion(ctx context.Context, actCtx *protoc

 // Commit persists all changes in RunActions() into the DB
 func (ws *workingSet) Commit(ctx context.Context) error {
+	if err := ws.verifyParent(); err != nil {
+		return err
+	}


if parent is abandoned, the workingset is not valid, abort here

dustinxie · 2025-01-09T06:52:22Z

state/factory/workingset.go

@@ -508,6 +519,9 @@ func (ws *workingSet) Process(ctx context.Context, actions []*action.SealedEnvel
 }

 func (ws *workingSet) processWithCorrectOrder(ctx context.Context, actions []*action.SealedEnvelope) error {
+	if err := ws.verifyParent(); err != nil {
+		return err
+	}


if parent is abandoned, the workingset is not valid, abort here

envestcc · 2025-01-15T01:44:29Z

state/factory/workingset.go

@@ -346,6 +345,11 @@ func (ws *workingSet) State(s interface{}, opts ...protocol.StateOption) (uint64
 	if cfg.Keys != nil {
 		return 0, errors.Wrap(ErrNotSupported, "Read state with keys option has not been implemented yet")
 	}
+	if ws.parent != nil {


Should first query from the current ws, and if not found, then query from the parent

good point, but this is a little complicated, for example, assume

in the DB, state(k) = 100

in the parent workingset state(k) = 98 (there's a transfer of 2 token in pending block)

in this case, we should honor the value in parent workingset
assume there's another transfer of 3 token in current block, then state(k) = 95 now, and things get even more complicated: for next read, should honor the value in current workingset
i think the return value of State/Read should indicate if the value is from workingset cache, and honor the value in cache over that from DB

maybe we can replace db store with parent

envestcc · 2025-01-15T01:44:44Z

state/factory/workingset.go

@@ -361,6 +365,13 @@ func (ws *workingSet) States(opts ...protocol.StateOption) (uint64, state.Iterat
 	if cfg.Key != nil {
 		return 0, nil, errors.Wrap(ErrNotSupported, "Read states with key option has not been implemented yet")
 	}
+	if ws.parent != nil {


envestcc · 2025-01-15T01:55:38Z

state/factory/statedb.go

+	if height > 0 {
+		parent = getWorkingSetByHeight(sdb.workingsets, height-1)
+	}
+	return newWorkingSet(height, store, parent), nil


Is it only necessary to set the parent when height-1 is not yet confirmed?

and should NOT newWorkingSet at hanging height, that is height-1 is either already confirmed, or there is already an existing working set.

envestcc · 2025-01-15T01:59:01Z

state/factory/statedb.go

 	}
+	if err = ws.verifyParent(); err != nil {


only valid when parent already confirmed

sonarqubecloud · 2025-01-17T01:10:07Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dustinxie requested review from CoderZhi, Liuhaai and envestcc as code owners January 9, 2025 06:39

dustinxie marked this pull request as draft January 9, 2025 06:39

dustinxie commented Jan 9, 2025

View reviewed changes

state/factory/workingset.go

}

}

if fCtx.CorrectTxLogIndex {

updateReceiptIndex(receipts)

Copy link

Member Author

dustinxie Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this func is not used any more

dustinxie commented Jan 9, 2025

View reviewed changes

envestcc reviewed Jan 15, 2025

View reviewed changes

state/factory/statedb.go

}

if err = ws.verifyParent(); err != nil {

Copy link

Member

envestcc Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only valid when parent already confirmed

dustinxie added 2 commits January 16, 2025 14:34

[workingset] add GetDirty() to retrieve latest value

1a4069f

[factory] add parent to workingset

23f26f2

dustinxie force-pushed the wsparent branch from ecde994 to 23f26f2 Compare January 17, 2025 01:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[factory] add parent to workingset #4539

[factory] add parent to workingset #4539

dustinxie commented Jan 9, 2025

dustinxie Jan 9, 2025 •

edited

Loading

dustinxie Jan 9, 2025

dustinxie Jan 9, 2025

dustinxie Jan 9, 2025

dustinxie Jan 9, 2025

dustinxie Jan 9, 2025

dustinxie Jan 9, 2025

dustinxie Jan 9, 2025

dustinxie Jan 9, 2025

envestcc Jan 15, 2025

dustinxie Jan 16, 2025 •

edited

Loading

envestcc Jan 17, 2025

envestcc Jan 15, 2025

envestcc Jan 15, 2025

envestcc Jan 15, 2025 •

edited

Loading

envestcc Jan 15, 2025

sonarqubecloud bot commented Jan 17, 2025

[factory] add parent to workingset #4539

Are you sure you want to change the base?

[factory] add parent to workingset #4539

Conversation

dustinxie commented Jan 9, 2025

Description

Type of change

How Has This Been Tested?

Checklist:

dustinxie Jan 9, 2025 • 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

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

dustinxie Jan 16, 2025 • 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

envestcc Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 17, 2025

Quality Gate passed

dustinxie Jan 9, 2025 •

edited

Loading

dustinxie Jan 16, 2025 •

edited

Loading

envestcc Jan 15, 2025 •

edited

Loading