simulate: resource population #6015

joe-p · 2024-06-05T22:59:02Z

Summary

When a user calls simulate with UnnamedResources enabled, simulate should suggest to the user how they can populate the resource arrays in their transactions to properly send the transaction group to the network.

Test Plan

Test ResourcePopulator works with simple local (not group sharing) resources
Test ResourcePopulator with group sharing
Test ResourcePopulator resource limit detection with group sharing (ie. it is able to find the correct transaction to put a resource in)
Test Simulate with ResourcePopulator functionality
Test /simulate endpoint with ResourcePopulator functionality
Write smaller tests for better ledger/simulation/resources.go coverage

codecov · 2024-06-06T11:17:49Z

Codecov Report

Attention: Patch coverage is 76.13636% with 63 lines in your changes missing coverage. Please review.

Project coverage is 55.97%. Comparing base (97ab559) to head (ec7a36a).
Report is 10 commits behind head on master.

❗ Current head ec7a36a differs from pull request most recent head 351f915

Please upload reports for the commit 351f915 to get more accurate results.

Files	Patch %	Lines
ledger/simulation/resources.go	76.58%	42 Missing and 17 partials ⚠️
ledger/simulation/simulator.go	66.66%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6015      +/-   ##
==========================================
+ Coverage   55.88%   55.97%   +0.09%     
==========================================
  Files         482      482              
  Lines       68571    68835     +264     
==========================================
+ Hits        38320    38531     +211     
- Misses      27646    27680      +34     
- Partials     2605     2624      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joe-p · 2024-07-24T20:20:05Z

In 99abd2f I have added resource population to the simulate API but for some reason this struct is not being properly encoded in the response

type PopulatedResourceArrays struct {
	Accounts []basics.Address    `codec:"accounts"`
	Assets   []basics.AssetIndex `codec:"assets"`
	Apps     []basics.AppIndex   `codec:"apps"`
	Boxes    []logic.BoxRef      `codec:"boxes"`
}

Boxes is encoded properly, but Accounts, Assets, and Apps are coming up nil. This behavior can be seen when running TestSimulateWithUnnamedResources with the changes in this commit. This seems to be an issue with the handler or encoder because TestPopulateResources shows simulate itself returns references (in this case addresses) as expected.

@jasonpaulos Any idea where the data is getting lost? In general, what is the best way to debug these algod API tests?

jannotti · 2024-08-19T20:02:25Z

daemon/algod/api/server/v2/utils.go

@@ -576,6 +576,7 @@ func convertTxnGroupResult(txnGroupResult simulation.TxnGroupResult) PreEncodedS
 		AppBudgetAdded:           omitEmpty(txnGroupResult.AppBudgetAdded),
 		AppBudgetConsumed:        omitEmpty(txnGroupResult.AppBudgetConsumed),
 		UnnamedResourcesAccessed: convertUnnamedResourcesAccessed(txnGroupResult.UnnamedResourcesAccessed),
+		PopulatedResourceArrays:  txnGroupResult.PopulatedResourceArrays,


Probably needs omitEmpty?

jannotti · 2024-08-19T20:03:42Z

daemon/algod/api/algod.oas2.json

@@ -3948,6 +3948,10 @@
        "fix-signers": {
          "description": "If true, signers for transactions that are missing signatures will be fixed during evaluation.",
          "type": "boolean"
+        },
+        "populate-resource-arrays": {


I'd prefer we say "populate-resources" here. I don't think we need to mention the datatype in the name. (This will also lead to some smaller diffs below).

jannotti · 2024-08-19T20:04:17Z

daemon/algod/api/server/v2/handlers.go

+	ExtraOpcodeBudget      uint64                                      `codec:"extra-opcode-budget,omitempty"`
+	ExecTraceConfig        simulation.ExecTraceConfig                  `codec:"exec-trace-config,omitempty"`
+	FixSigners             bool                                        `codec:"fix-signers,omitempty"`
+	PopulateResourceArrays bool                                        `codec:"populate-resource-arrays,omitempty"`


Here's an example of where "PopulateResources" will make for a smaller diff.

jannotti · 2024-08-19T20:04:35Z

daemon/algod/api/server/v2/utils.go

+		ExtraOpcodeBudget:      request.ExtraOpcodeBudget,
+		TraceConfig:            request.ExecTraceConfig,
+		FixSigners:             request.FixSigners,
+		PopulateResourceArrays: request.PopulateResourceArrays,


Another potentially smaller diff.

jannotti · 2024-08-19T20:05:42Z

ledger/simulation/resources.go

+	// The static fields are resource arrays that were given in the txn group and thus cannot be removed
+	// The assumption is that these are prefilled because of one of the following reaons:
+	//   - This transaction has already been signed
+	//   - One of the foreign arrays is accessed on-chain


Reads funny. Do you mean this?

Suggested change

// - One of the foreign arrays is accessed on-chain

// - One of the foreign resources is accessed on-chain

jannotti · 2024-08-20T15:52:31Z

ledger/simulation/resources.go

+			return nil
+		}
+	}
+	return fmt.Errorf("no room for holding")


More info usually better:

Suggested change

return fmt.Errorf("no room for holding")

return fmt.Errorf("no room for holding %s : %d", addr, aid)

Lots of other similar.

jannotti · 2024-08-20T15:55:02Z

ledger/simulation/resources.go

+}
+
+func (r *txnResources) addApp(aid basics.AppIndex) {
+	r.apps[aid] = struct{}{}


I think you want to track that this adds availability of the app account too.

Suggested change

r.apps[aid] = struct{}{}

r.apps[aid] = struct{}{}

r.accounts[aid.Address()] = struct{}{}

jannotti · 2024-08-20T15:56:47Z

ledger/simulation/resources.go

+		}
+
+		for _, app := range txn.ForeignApps {
+			p.txnResources[groupIndex].staticApps[app] = struct{}{}


App account becomes accessible too.

Suggested change

p.txnResources[groupIndex].staticApps[app] = struct{}{}

p.txnResources[groupIndex].staticApps[app] = struct{}{}

p.txnResources[groupIndex].staticAccounts[app.Address()] = struct{}{}

jannotti · 2024-08-20T16:00:52Z

ledger/simulation/resources.go

+	return hasField || hasStatic || hasRef
+}
+
+func (r *txnResources) addAccount(addr basics.Address) {


I don't love the style of needing to call hasRoom() and then invoking add* unconditionally. Could the add* methods return error instead?
This would require you have two forms of some add* methods, or an extra argument that means "add this if you can do so by adding only one reference".

jannotti · 2024-08-20T16:02:24Z

ledger/simulation/resources.go

+	for asset := range r.assets {
+		assets = append(assets, asset)
+	}
+	for asset := range r.staticAssets {
+		assets = append(assets, asset)
+	}


These can use the ... form of append.

jasonpaulos

Finally submitting my review that's been in progress for a while. Note there's some overlap with what @jannotti has already posted

jasonpaulos · 2024-07-30T14:41:34Z

daemon/algod/api/algod.oas2.json

@@ -4265,6 +4269,10 @@
        },
        "unnamed-resources-accessed": {
          "$ref": "#/definitions/SimulateUnnamedResourcesAccessed"
+        },
+        "populated-resource-arrays": {
+          "description": "Present if populate-resource-arrays is true in the request. In that case, it will be a map of transaction index in the group to populated resource arrays. There may be moure resource arrays given than transaction in the group, which means more app call transactions would be needed for extra resources.",


Suggested change

"description": "Present if populate-resource-arrays is true in the request. In that case, it will be a map of transaction index in the group to populated resource arrays. There may be moure resource arrays given than transaction in the group, which means more app call transactions would be needed for extra resources.",

"description": "Present if populate-resource-arrays is true in the request. In that case, it will be a map of transaction index in the group to populated resource arrays. There may be more resource arrays given than transaction in the group, which means more app call transactions would be needed for extra resources.",

jasonpaulos · 2024-07-30T14:46:36Z

daemon/algod/api/algod.oas2.json

+        },
+        "populated-resource-arrays": {
+          "description": "Present if populate-resource-arrays is true in the request. In that case, it will be a map of transaction index in the group to populated resource arrays. There may be moure resource arrays given than transaction in the group, which means more app call transactions would be needed for extra resources.",
+          "type": "object"


Unless there is a very compelling reason, I would recommend against using a map with non-string keys in the response object. It makes JSON encoding much more difficult.

I'd instead suggest an array, or (my preference) adding this as a property to individual SimulateTransactionResult objects, with another array here for any extra txns.

jasonpaulos · 2024-07-30T16:19:17Z

ledger/simulation/resources.go

@@ -605,3 +605,488 @@ func (p *resourcePolicy) AvailableBox(app basics.AppIndex, name string, operatio
 	}
 	return p.tracker.addBox(app, name, readSize, *p.initialBoxSurplusReadBudget, p.ep.Proto.BytesPerBoxReference)
 }
+
+// txnResources tracks the resources being added to a tranasction during resource population


Suggested change

// txnResources tracks the resources being added to a tranasction during resource population

// txnResources tracks the resources being added to a transaction during resource population

jasonpaulos · 2024-07-30T16:19:39Z

ledger/simulation/resources.go

+// txnResources tracks the resources being added to a tranasction during resource population
+type txnResources struct {
+	// The static fields are resource arrays that were given in the txn group and thus cannot be removed
+	// The assumption is that these are prefilled because of one of the following reaons:


Suggested change

// The assumption is that these are prefilled because of one of the following reaons:

// The assumption is that these are prefilled because of one of the following reasons:

jasonpaulos · 2024-07-30T16:42:47Z

ledger/simulation/resources.go

+	staticAssets   map[basics.AssetIndex]struct{}
+	staticApps     map[basics.AppIndex]struct{}
+	staticAccounts map[basics.Address]struct{}
+	staticBoxes    []logic.BoxRef


FYI you can use logic.BoxRef as a map key

jasonpaulos · 2024-07-30T17:39:49Z

ledger/simulation/resources.go

+		accounts:           make(map[basics.Address]struct{}),
+		boxes:              []logic.BoxRef{},
+		maxTotalRefs:       consensusParams.MaxAppTotalTxnReferences,
+		maxAccounts:        consensusParams.MaxAppTxnAccounts,


Are you choosing to ignore the individual limits on assets, apps, and boxes because they happen to be the same as the total ref limit? I'd rather not have this code rely on that always being true

jasonpaulos · 2024-08-27T18:40:00Z

ledger/simulation/resources.go

+}
+
+// PopulatedResourceArrays is a struct that contains all the populated arrays for a txn
+type PopulatedResourceArrays struct {


To be consistent with the other structs in this package, I would prefer this struct not have codec tags and be directly exposed through the REST API. Instead a new model should be added to the oas2 spec and then daemon/algod/api/server/v2/utils.go can convert between this simulation.PopulatedResourceArrays and the generated model struct.

jasonpaulos · 2024-08-27T19:36:27Z

daemon/algod/api/server/v2/handlers.go

+	FailureMessage           *string                                    `codec:"failure-message,omitempty"`
+	UnnamedResourcesAccessed *model.SimulateUnnamedResourcesAccessed    `codec:"unnamed-resources-accessed,omitempty"`
+	Txns                     []PreEncodedSimulateTxnResult              `codec:"txn-results"`
+	PopulatedResourceArrays  map[int]simulation.PopulatedResourceArrays `code:"populated-resource-arrays,omitempty"`


Suggested change

PopulatedResourceArrays map[int]simulation.PopulatedResourceArrays `code:"populated-resource-arrays,omitempty"`

PopulatedResourceArrays map[int]simulation.PopulatedResourceArrays `codec:"populated-resource-arrays,omitempty"`

jasonpaulos · 2024-08-27T19:53:04Z

test/e2e-go/restAPI/simulate/simulateRestAPI_test.go

+			},
+		},
+	}
+	a.Equal(expectedResult, resp)


Re: the test failure that you're seeing. This is what the server sends back (decoded from the msgpack response):

"unnamed-resources-accessed": { "app-locals": [ { "account": "7WCFW7UO7NA3NTQWDPKMFT4NNTOU4A7M5IF5VGPB3OCH6TMLD4X6QX453M", "app": 1005 } ], "asset-holdings": [ { "account": "7WCFW7UO7NA3NTQWDPKMFT4NNTOU4A7M5IF5VGPB3OCH6TMLD4X6QX453M", "asset": 1002 } ], "boxes": [ { "app": 1006, "name:b64": "QQ==" } ], "extra-box-refs": 1 }

I don't think there's anything going wrong with the encoding or e2e aspect, but rather a logic error inside the simulate package when multiple different resource types are unspecified. I noticed that the test you're comparing against, TestPopulateResources, is much simpler and only tests accounts. I suggest replicating this in a unit test and debugging further there. Generally speaking the most complex test cases and corner cases should be unit tests in simulation_eval_test.go, then all the e2e test needs to do is make sure all inputs get picked up properly and all outputs are present as expected in the response (to catch encoding errors or fields being dropped).

joe-p changed the title ~~Feat/populate_resources~~ resource population Jun 5, 2024

joe-p mentioned this pull request Jun 5, 2024

ResourcePopulator joe-p/go-algorand#2

Closed

ResourcePopulator

5ba0a9a

joe-p force-pushed the feat/populate_resources branch from 466fd50 to 5ba0a9a Compare June 5, 2024 23:06

joe-p changed the title ~~resource population~~ simulate: resource population Jun 5, 2024

TestPopulatorWithGlobalResources (arrays only)

028b8c4

joe-p added 14 commits June 6, 2024 07:30

test addBox

941ba6e

use ElementsMatch

a69091c

addHolding

d9e77ef

test variable renaming

4667647

appLocals

cbd1d8a

restore default limits

d79780f

fix rekey field

e6d59ba

populate with static properties and remove zeroAddr

54cd38c

empty boxes

673b03d

ensure duplicates are removed

ceddaa2

overflow txn resources

62213a0

use ConsensusParams

f616449

fix empty box count

6a4f1ee

golangci-lint

0524c3c

algorandskiy added Enhancement external contribution labels Jun 7, 2024

algorandskiy requested review from jasonpaulos and jannotti June 7, 2024 17:13

joe-p added 5 commits June 14, 2024 16:31

Merge branch 'master' into feat/populate_resources

73e5b4c

PopulateResourceArrays in simulate (untested)

6d8c161

populate from ResourceTracker

022c565

initial TestPopulateResources

93dd4e9

group sharing and no group sharing TestPopulateResources

3f78752

joe-p added 5 commits June 19, 2024 07:58

test with non appl in group

359b5b0

modify visibility on resource pop stuff

2229b0a

remove duplicate partitiontest.PartitionTest(t)

ec7a36a

don't make RekeyTo address available

351f915

add PopulateResourceArrays to simulate API (WIP)

99abd2f

jannotti reviewed Aug 20, 2024

View reviewed changes

jasonpaulos reviewed Aug 27, 2024

View reviewed changes

jasonpaulos mentioned this pull request Aug 28, 2024

Simulate: Pack transaction reference arrays for unnamed resources #5684

Closed

gmalouf assigned joe-p Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simulate: resource population #6015

simulate: resource population #6015

joe-p commented Jun 5, 2024 •

edited

Loading

codecov bot commented Jun 6, 2024 •

edited

Loading

joe-p commented Jul 24, 2024

jannotti Aug 19, 2024

jannotti Aug 19, 2024

jannotti Aug 19, 2024

jannotti Aug 19, 2024

jannotti Aug 19, 2024

jannotti Aug 20, 2024

jannotti Aug 20, 2024

jannotti Aug 20, 2024

jannotti Aug 20, 2024

jannotti Aug 20, 2024

jasonpaulos left a comment

jasonpaulos Jul 30, 2024

jasonpaulos Jul 30, 2024

jasonpaulos Jul 30, 2024

jasonpaulos Jul 30, 2024

jasonpaulos Jul 30, 2024

jasonpaulos Jul 30, 2024

jasonpaulos Aug 27, 2024

jasonpaulos Aug 27, 2024

jasonpaulos Aug 27, 2024

	// - One of the foreign arrays is accessed on-chain
	// - One of the foreign resources is accessed on-chain

	return fmt.Errorf("no room for holding")
	return fmt.Errorf("no room for holding %s : %d", addr, aid)

	r.apps[aid] = struct{}{}
	r.apps[aid] = struct{}{}
	r.accounts[aid.Address()] = struct{}{}

	p.txnResources[groupIndex].staticApps[app] = struct{}{}
	p.txnResources[groupIndex].staticApps[app] = struct{}{}
	p.txnResources[groupIndex].staticAccounts[app.Address()] = struct{}{}

	"description": "Present if populate-resource-arrays is true in the request. In that case, it will be a map of transaction index in the group to populated resource arrays. There may be moure resource arrays given than transaction in the group, which means more app call transactions would be needed for extra resources.",
	"description": "Present if populate-resource-arrays is true in the request. In that case, it will be a map of transaction index in the group to populated resource arrays. There may be more resource arrays given than transaction in the group, which means more app call transactions would be needed for extra resources.",

	// txnResources tracks the resources being added to a tranasction during resource population
	// txnResources tracks the resources being added to a transaction during resource population

	// The assumption is that these are prefilled because of one of the following reaons:
	// The assumption is that these are prefilled because of one of the following reasons:

	PopulatedResourceArrays map[int]simulation.PopulatedResourceArrays `code:"populated-resource-arrays,omitempty"`
	PopulatedResourceArrays map[int]simulation.PopulatedResourceArrays `codec:"populated-resource-arrays,omitempty"`

simulate: resource population #6015

Are you sure you want to change the base?

simulate: resource population #6015

Conversation

joe-p commented Jun 5, 2024 • edited Loading

Summary

Test Plan

codecov bot commented Jun 6, 2024 • edited Loading

Codecov Report

joe-p commented Jul 24, 2024

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

jasonpaulos 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

joe-p commented Jun 5, 2024 •

edited

Loading

codecov bot commented Jun 6, 2024 •

edited

Loading