Simulate: Make optional signatures an opt-in feature #5335

jasonpaulos · 2023-04-25T00:14:37Z

Summary

To be more consistent with other simulation add-ons such as higher log limits (#5247), this PR makes it so that simulated transactions must have correct and present signatures by default. Now, if the caller wishes to allow empty signatures, they must explicitly do so by including the allow-empty-signatures parameter.

The simulation response type has also been revised with this in mind:

Removed would-succeed. Prior to this, this field summarized both whether all transactions succeeded and whether all transactions has valid signatures. Now that empty signatures are not allowed by default, there is not much use in would-succeed, since it's nearly as easy to see if the failure-message field is present for any transaction group.
Removed missing-signatures from each transaction. Since callers must opt into allowing empty signatures and we no longer report would-succeed, there's no need to be as explicit about whether transactions are missing signatures. This information can be found by looking at the signed transaction and noticing that no signature fields are present.

Additionally, I increased the types of verification errors that simulate can report in its HTTP 200 response. Now for nearly every type of verification error (a notable exception is invalid signatures, which are tricker to pinpoint because we use batch verification), simulate will report this error using the failure-message and failed-at fields. This is nice because were this not the case, submitting a transaction with a missing signature without allow-empty-signatures would result in an HTTP 400 and a not as useful error message.

Test Plan

New tests added and existing tests updated

jasonpaulos · 2023-04-25T21:38:02Z

ledger/simulation/trace.go

@@ -51,9 +50,6 @@ type TxnGroupResult struct {

 	// AppBudgetConsumed is the total opcode cost used for this group
 	AppBudgetConsumed uint64
-
-	// FeeCredit is the fees left over after covering fees for this group
-	FeeCredit uint64


I discovered this field was unused

Is this unused even before this PR?

Yes. I believe it was introduced in #5221. I discovered this field is set by the tracer, but not used in any other way. @algochoi do you have any thoughts about this?

Yes, I am okay with this being removed - it wasn't being serialized because I wasn't convinced enough that reporting the fee credit would be useful at the moment.

Ok thanks. This likely will be useful at some point, but until then I'd like to remove it just to keep things clean

jannotti

Very nice that this reduced total line count while adding functionality, clarity, and tests.

jannotti · 2023-04-26T15:05:03Z

cmd/goal/clerk.go

+	simulateCmd.Flags().BoolVar(&signaturesOptional, "signatures-optional", false, "Allow transactions without signatures to be evaluated as if they had correct signatures")
 	simulateCmd.Flags().BoolVar(&liftLogLimits, "lift-log-limits", false, "Lift the limits on log opcode during simulation")


I wonder if we should try to get consistency among the "power pack" arguments, like
allow-empty-signatures, and allow-more-logging.

@ahangsu

sure, I think prefixing with allow-* or relax-* both sounds fine with me for consistent cmdline arg name.

We don't want to change internal var names, right?

I'm open to allow-empty-signatures -- this was almost what I chose to call it anyway.

@jannotti are you suggesting changes for goal and for the REST API definitions? Right now they are the same

I guess I'm suggesting it as widely as you'd like to make the changes. I can understand not changing the equivalent underlying variables, but also, I guess it would be nice. Same for REST.

Renamed in 1e1fdbd to allow-empty-signatures and allow-more-logging everywhere

data/transactions/verify/txn.go

ledger/simulation/simulation_eval_test.go

ledger/simulation/tracer.go

daemon/algod/api/algod.oas2.json

daemon/algod/api/algod.oas3.yml

ahangsu

Mostly looking good, just some minor nits here and there.

I have a gap in thinking, help me fill this up: How is the improvement in txn.go and txnBatch.go connect to the simulation one? Seems it is reporting more info for real txn-verification error?

ahangsu · 2023-04-26T14:21:57Z

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

@@ -391,15 +390,15 @@ func convertSimulationResult(result simulation.Result) PreEncodedSimulateRespons
 	var evalOverrides *model.SimulationEvalOverrides
 	if result.EvalOverrides != (simulation.ResultEvalOverrides{}) {
 		evalOverrides = &model.SimulationEvalOverrides{
-			MaxLogSize:  result.EvalOverrides.MaxLogSize,
-			MaxLogCalls: result.EvalOverrides.MaxLogCalls,
+			SignaturesOptional: trueOrNil(result.EvalOverrides.SignaturesOptional),


nit: I guess this can be just passing from result.EvalOverrides.SignaturesOptional, for both are *bool with nil considered as false?

At one point I had thought that our encoder would include values if they were non-nil pointers to zero values. But after local tests it seems that's not the case and that changing this line to SignaturesOptional: &result.EvalOverrides.SignaturesOptional would have the same effect.

Since this is surprising to me, I'd rather leave this code as-is and investigate more whether what I described was ever true, or I'm misremembering something

test/scripts/e2e_subs/e2e-app-simulate.sh

ahangsu · 2023-04-26T14:39:30Z

ledger/simulation/trace.go

@@ -51,9 +50,6 @@ type TxnGroupResult struct {

 	// AppBudgetConsumed is the total opcode cost used for this group
 	AppBudgetConsumed uint64
-
-	// FeeCredit is the fees left over after covering fees for this group
-	FeeCredit uint64


Is this unused even before this PR?

ahangsu · 2023-04-26T14:45:07Z

ledger/simulation/simulator.go

@@ -111,30 +112,27 @@ var proxySigner = crypto.PrivateKey{
 // check verifies that the transaction is well-formed and has valid or missing signatures.
 // An invalid transaction group error is returned if the transaction is not well-formed or there are invalid signatures.
 // To make things easier, we support submitting unsigned transactions and will respond whether signatures are missing.
-func (s Simulator) check(hdr bookkeeping.BlockHeader, txgroup []transactions.SignedTxn, debugger logic.EvalTracer) ([]int, error) {
+func (s Simulator) check(hdr bookkeeping.BlockHeader, txgroup []transactions.SignedTxn, signaturesOptional bool, tracer logic.EvalTracer) error {


nit: I somehow want to negotiate with you on not to add signatureOptional bool argument to s.check signature, for you can infer from tracer.Result's EvalOverrides field about signatureOptional. I point this out for that is how I approach similar problem in lift-log-limits PR, see if it align with your thoughts.

renaming debugger to tracer looks good.

Good idea, but I don't want to change the tracer argument from logic.EvalTracer to *evalTracer. Because it's defined as the more generic tracer, the test TestSimulateWithTrace is able to work. This test uses the mock tracer to ensure our simulated environment still calls tracer hooks properly.

I did something similar to what you suggested in ea3372e and 4a4fae0 though, which uses the ResultEvalOverrides struct instead of signaturesOptional bool to pass this option. Let me know what you think.

ouch, didn't see it coming, I thought s.simulateWithTracer is not taking interface, now it seems a bit involved...

ledger/simulation/simulator.go

ahangsu · 2023-04-26T15:24:49Z

data/transactions/verify/txn.go

The change in this file is mostly passing groupIndex in TxGroupError to tell if specific txn in group is erroring, or the group itself is ill-formed (under which case id = -1).

ledger/simulation/simulation_eval_test.go

daemon/algod/api/algod.oas3.yml

bbroder-algo · 2023-04-26T15:46:21Z

would-succeed was an ambiguous predicate and I won't miss it at all!

jasonpaulos · 2023-04-26T19:50:06Z

I have a gap in thinking, help me fill this up: How is the improvement in txn.go and txnBatch.go connect to the simulation one? Seems it is reporting more info for real txn-verification error?

@ahangsu the connection is that without the changes to how simulate handles verification errors, submitting a txn without signatures (and without allow-empty-signatures) would result in an HTTP 400 error, which I think is a bit too severe and less useful.

Make optional signatures an opt-in feature

9516241

jasonpaulos added the Enhancement label Apr 25, 2023

Add goal e2e test and catch more verification errors in the response

25a0630

jasonpaulos commented Apr 25, 2023

View reviewed changes

jasonpaulos marked this pull request as ready for review April 25, 2023 21:38

jasonpaulos requested review from ahangsu, jannotti and bbroder-algo April 25, 2023 21:40

jannotti reviewed Apr 26, 2023

View reviewed changes

bbroder-algo reviewed Apr 26, 2023

View reviewed changes

daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved

bbroder-algo reviewed Apr 26, 2023

View reviewed changes

daemon/algod/api/algod.oas2.json Outdated Show resolved Hide resolved

bbroder-algo reviewed Apr 26, 2023

View reviewed changes

daemon/algod/api/algod.oas3.yml Outdated Show resolved Hide resolved

ahangsu reviewed Apr 26, 2023

View reviewed changes

bbroder-algo reviewed Apr 26, 2023

View reviewed changes

daemon/algod/api/algod.oas3.yml Outdated Show resolved Hide resolved

jannotti self-requested a review April 26, 2023 16:46

jasonpaulos added 5 commits April 26, 2023 11:44

Review feedback

a56938b

Merge branch 'master' into simulate-signatures-optional

e504868

Suggestions from Hang

ea3372e

didn't make it into previous commit

4a4fae0

The great renaming

1e1fdbd

jasonpaulos requested review from ahangsu and bbroder-algo April 26, 2023 19:50

jannotti approved these changes Apr 26, 2023

View reviewed changes

bbroder-algo approved these changes Apr 26, 2023

View reviewed changes

bbroder-algo merged commit 1ff6da8 into algorand:master Apr 26, 2023

algochoi mentioned this pull request Apr 26, 2023

api: Regenerate Client Interfaces and implement cucumber tests. algorand/java-algorand-sdk#555

Merged

jasonpaulos deleted the simulate-signatures-optional branch April 26, 2023 21:29

Algo-devops-service mentioned this pull request May 9, 2023

go-algorand 3.16.0-beta Release PR #5370

Closed

This was referenced May 9, 2023

go-algorand 3.16.0-beta Release PR #5371

Closed

go-algorand 3.16.0-beta Release PR #5382

Closed

Algo-devops-service mentioned this pull request May 22, 2023

go-algorand 3.16.0-beta Release PR #5406

Merged

This was referenced May 24, 2023

go-algorand 3.16.0-beta Release PR #5417

Merged

go-algorand 3.16.0-beta Release PR #5430

Merged

go-algorand 3.16.0-beta Release PR #5434

Merged

Algo-devops-service mentioned this pull request Jun 12, 2023

go-algorand 3.16.1-stable Release PR #5465

Merged

onetechnical mentioned this pull request Jun 14, 2023

go-algorand 3.16.2-stable Release PR #5469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simulate: Make optional signatures an opt-in feature #5335

Simulate: Make optional signatures an opt-in feature #5335

jasonpaulos commented Apr 25, 2023 •

edited

Loading

jasonpaulos Apr 25, 2023

ahangsu Apr 26, 2023

jasonpaulos Apr 26, 2023

algochoi Apr 26, 2023

jasonpaulos Apr 26, 2023

jannotti left a comment

jannotti Apr 26, 2023

ahangsu Apr 26, 2023

jasonpaulos Apr 26, 2023

jannotti Apr 26, 2023

jasonpaulos Apr 26, 2023

ahangsu left a comment

ahangsu Apr 26, 2023

jasonpaulos Apr 26, 2023

ahangsu Apr 26, 2023

ahangsu Apr 26, 2023

jasonpaulos Apr 26, 2023 •

edited

Loading

ahangsu Apr 26, 2023

ahangsu Apr 26, 2023

bbroder-algo commented Apr 26, 2023

jasonpaulos commented Apr 26, 2023

		simulateCmd.Flags().BoolVar(&signaturesOptional, "signatures-optional", false, "Allow transactions without signatures to be evaluated as if they had correct signatures")
		simulateCmd.Flags().BoolVar(&liftLogLimits, "lift-log-limits", false, "Lift the limits on log opcode during simulation")

Simulate: Make optional signatures an opt-in feature #5335

Simulate: Make optional signatures an opt-in feature #5335

Conversation

jasonpaulos commented Apr 25, 2023 • edited Loading

Summary

Test Plan

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

jannotti 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

ahangsu 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

jasonpaulos Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbroder-algo commented Apr 26, 2023

jasonpaulos commented Apr 26, 2023

jasonpaulos commented Apr 25, 2023 •

edited

Loading

jasonpaulos Apr 26, 2023 •

edited

Loading