Goal: adding auth addr to signed txn if signer is passed #3459
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d#3237) * Three new globals for to help contract-to-contract usability * detritis * Check error * doc comments
* Three new globals for to help contract-to-contract usability * detritis * Check error * doc comments * opcode, docs, and tests * specs update
* ledger: fix `NextRewardsState()` (algorand#3403) ## Summary A modification of algorand#3336. Added a new test where the rewards pool overspends and proposed a fix in `NextRewardsState()` requiring a consensus upgrade. ## Test Plan This is mostly tests. * Fix a potential problem of committing non-uniform consensus versions (algorand#3453) If accountdb accumulates a large backlog, it is possible catchpoint tracker would attempt to commit a wider range than account updates tracker expects. * avoid generating log error on EnsureValidatedBlock / EnsureBlock (algorand#3424) In EnsureBlock,, do not log as error message if the error is ledgercore.ErrNonSequentialBlockEval and the block round is in the past (i.e. already in the ledger). * Fix typo Fulll to Full (algorand#3456) Fix typo * Fix worng message on restore crash db. (algorand#3455) When crash state is found but could not be restored, noCrashState variable is used to report a warning. However, this variable was set to false in a case where there was no crash state, and the wrong warning was reported. * Adding new scenario for feature networks (algorand#3451) Co-authored-by: Tolik Zinovyev <[email protected]> Co-authored-by: Pavel Zbitskiy <[email protected]> Co-authored-by: Shant Karakashian <[email protected]>
…-algorand into feature/contract-to-contract
* ledger: fix `NextRewardsState()` (algorand#3403) ## Summary A modification of algorand#3336. Added a new test where the rewards pool overspends and proposed a fix in `NextRewardsState()` requiring a consensus upgrade. ## Test Plan This is mostly tests. * Fix a potential problem of committing non-uniform consensus versions (algorand#3453) If accountdb accumulates a large backlog, it is possible catchpoint tracker would attempt to commit a wider range than account updates tracker expects. * avoid generating log error on EnsureValidatedBlock / EnsureBlock (algorand#3424) In EnsureBlock,, do not log as error message if the error is ledgercore.ErrNonSequentialBlockEval and the block round is in the past (i.e. already in the ledger). * Fix typo Fulll to Full (algorand#3456) Fix typo * Fix worng message on restore crash db. (algorand#3455) When crash state is found but could not be restored, noCrashState variable is used to report a warning. However, this variable was set to false in a case where there was no crash state, and the wrong warning was reported. * Adding new scenario for feature networks (algorand#3451) * Fixing telemetry ports (algorand#3497) Co-authored-by: Tolik Zinovyev <[email protected]> Co-authored-by: Pavel Zbitskiy <[email protected]> Co-authored-by: Shant Karakashian <[email protected]> Co-authored-by: Jack <[email protected]>
* Three new globals for to help contract-to-contract usability * detritis * Check error * doc comments * Impose limits on the entire "tree" of inner calls. This also increases the realism of testing of multiple app calls in a group by creating the EvalParams with the real constructor, thus getting the pooling stuff tested here without playing games manipulating the ep after construction. * Move appID tracking into EvalContext, out of LedgerForLogic This change increases the seperation between AVM execution and the ledger being used to lookup resources. Previously, the ledger kept track of the appID being executed, to offer a narrower interface to those resources. But now, with app-to-app calls, the appID being executed must change, and the AVM needs to maintain the current appID. * Stupid linter * Fix unit tests error messages * Allow access to resources created in the same transaction group The method will be reworked, but the tests are correct and want to get them visible to team. * Access to apps created in group Also adds some tests that are currently skipped for testing - access to addresses of newly created apps - use of gaid in inner transactions Both require some work to implement the thing being tested. * Remove tracked created mechanism in favor of examining applydata. * Allow v6 AVM code to use in-group created asas, apps (& their accts) One exception - apps can not mutate (put or del) keys from the app accounts, because EvalDelta cannot encode such changes. * lint docs * typo * The review dog needs obedience training. * Use one EvalParams for logic evals, another for apps in dry run We used to use one ep per transaction, shared between sig and and app. But the new model of ep usage is to keep using one while evaluating an entire group. The app ep is now built logic.NewAppEvalParams which, hopefully, will prevent some bugs when we change something in the EvalParams and don't reflect it in what was a "raw" EvalParams construction in debugger and dry run. * Use logic.NewAppEvalParams to decrease copying and bugs in debugger * Simplify use of NewEvalParams. No more nil return when no apps. This way, NewEvalParams can be used for all creations of EvalParams, whether they are intended for logicsig or app use, greatly simplifying the way we make them for use by dry run or debugger (where they serve double duty). * Remove explicit PastSideEffects handling in tealdbg * Always create EvalParams to evaluate a transaction group. We used to have an optimization to avoid creating EvalParams unless there was an app call in the transaction group. But the interface to allow transaction processing to communicate changes into the EvalParams is complicated by that (we must only do it if there is one!) This also allows us to use the same construction function for eps created for app and logic evaluation, simplifying dry-run and debugger. The optimization is less needed now anyway: 1) The ep is now shared for the whole group, so it's only one. 2) The ep is smaller now, as we only store nil pointers instead of larger scratch space objects for non-app calls. * Correct mistaken commit * Spec improvments * More spec improvments, including resource "availability" * Recursively return inner transaction tree * Lint * No need for ConfirmedRound, so don't deref a nil pointer! * license check * Shut up, dawg. * base64 merge cleanup * Remove the extraneous field type arrays. * bsqrt * acct_holding_get, a unified opcode for account field access * Thanks, dawg * CR and more spec simplification * e2e test for inner transaction appls * Give max group size * 16 inner txns, regardless of apps present * Adjust test for allowing 256 inners * NCC audit (except double credit) plus app depth limit * Fix for double credit from inner groups, found by NCC * Spec updates * Count inners properly, increase inner txn allocbound
…-algorand into feature/contract-to-contract
Fix pooled costs during app to app recursion Also adds divw, itxnas, gitxnas Makes accessing 0 accounts in dry-run ok.
* txn LastLog * Insulate test from the type of field
…3529) * Only allow access to txn effects for previous transactions * JP code review * Ok, dawg
tsachiherman
reviewed
Feb 2, 2022
| @@ -787,10 +787,17 @@ var signCmd = &cobra.Command{ | |||
|
|
|||
| for _, group := range groupsOrder { | |||
| txnGroup := []transactions.SignedTxn{} | |||
| for _, txn := range txnGroups[group] { | |||
| for idx, txn := range txnGroups[group] { | |||
There was a problem hiding this comment.
doesn't client.SignTransactionWithWalletAndSigner already does that ? I think that this would only be needed in the case of lsig.Logic != nil.
There was a problem hiding this comment.
Sorry, do you mean move this if case into the lsig != nil check?
There was a problem hiding this comment.
I think so. But I could be wrong.
I think that the correct way to approach this is to have this tested.
We have some expect tests for the goal commands. I think that extending those to cover this use case wound't be a bad idea.
There was a problem hiding this comment.
Tweaked the logic but I'm struggling to find where the tests mentioned live
There was a problem hiding this comment.
The tests lives in /go-algorand/test/e2e-go/cli/goal/expect
…-algorand into feature/contract-to-contract
* Test current state of inner clear state behavior * First half of clear state fixes. Prevents inner calls in CSP Should also implement the "CSP requires 700 budget and can't spend more than 700 budget" rule, but tests still required. * Match program version. Don't allow downgrade < 6. * partition test * typo Co-authored-by: Michael Diamant <[email protected]> * typo Co-authored-by: Michael Diamant <[email protected]> * Make the default clear_state program match versions to approval * Correct some tests that now need programs present * Fix test to actually test INNER clear state * Add tests for opcode budget and CSPs * Improve tracing for inner app calls * Error now reports what the cost *was* before the attempted inst. * Simplify type switch and maybe kick CI? * Fewer copies during inner processing Co-authored-by: Michael Diamant <[email protected]>
…-algorand into feature/contract-to-contract
* test for global modifications by various inner apps * CR adjustments to feeCredit * Test local state sharing too. * explain clear state details
Codecov Report
@@ Coverage Diff @@
## feature/contract-to-contract #3459 +/- ##
================================================================
- Coverage 47.70% 47.69% -0.01%
================================================================
Files 370 370
Lines 59993 59998 +5
================================================================
Hits 28617 28617
- Misses 28038 28045 +7
+ Partials 3338 3336 -2
Continue to review full report at Codecov.
|
barnjamin
changed the title
…-algorand into feature/contract-to-contract
tsachiherman
pushed a commit
that referenced
this pull request
Mar 18, 2022
…count (#3773) ## Summary When trying to sign a transaction file with goal, if the sender is a normal address but the signer is a logic program, the call fails with an error message like: `send.txn: txn[0] error LogicNot signed and not a Logic-only account` The error message comes from https://github.com/algorand/go-algorand/blob/33b87c432065d3be0ce1fdad682604f416676133/data/transactions/verify/txn.go#L314 It notes that a valid case is when Auth is set to the hash of the program but we aren't setting the auth on the transaction even if the -S flag is passed ## Test Plan Made a txn where sender is a regular account and signed it with ```sh goal clerk sign -i tmp.txn -o tmp.txn -S BJATCHES5YJZJ7JITYMVLSSIQAVAWBQRVGPQUDT5AZ2QSLDSXWWM46THOY -p tmp.teal ``` I borked the git history on the last one #3459 so just nuked it and starting over here. Left it at @tsachiherman wanting better tests for this #3459 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When trying to sign a transaction file with goal, if the sender is a normal address but the signer is a logic program, the call fails with an error message like:
The error message comes from
go-algorand/data/transactions/verify/txn.go
Line 314 in 33b87c4
It notes that a valid case is when Auth is set to the hash of the program but we aren't setting the auth on the transaction even if the -S flag is passed
Test Plan
Made a txn where sender is a regular account and signed it with