-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only allow access to txn effects for previous transactions #3529
Only allow access to txn effects for previous transactions #3529
Conversation
data/transactions/logic/eval.go
Outdated
|
||
itxn := &cx.Txn.EvalDelta.InnerTxns[len(cx.Txn.EvalDelta.InnerTxns)-1] | ||
sv, err := cx.txnFieldToStack(itxn, fs, arrayFieldIdx, 0, true) | ||
sv, err := cx.opTxnImpl(0, srcGroup, field, ai, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be srcInner
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes!
data/transactions/logic/eval.go
Outdated
const srcGroup txnSource = 0 | ||
const srcInner txnSource = 1 | ||
const srcInnerGroup txnSource = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const srcGroup txnSource = 0 | |
const srcInner txnSource = 1 | |
const srcInnerGroup txnSource = 2 | |
const ( | |
srcGroup txnSource = 0 | |
srcInner = 1 | |
srcInnerGroup = 2 | |
) |
nit: this would be more recognizable as an enum like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll even go full on iota
on it
data/transactions/logic/eval.go
Outdated
tx := &group[gi] | ||
|
||
// int(gi) is safe because gi < len(group). Slices in Go cannot exceed `int` | ||
sv, err = cx.txnFieldToStack(tx, fs, ai, int(gi), src != srcGroup) | ||
if err != nil { | ||
cx.err = err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there's inconsistent use of cx.err
. This check sets it, but all other checks in this function don't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks. oddly enough that ends up having the exact same effect, but was certainly not intended
Next up, eliminate this cx.err assignment thing in all the opcodes.
txn opcodes should not be able to look at the current or future "effects" fields