refactor(gnovm): make IsOriginCall/AssertOriginCall rely on PrevRealm
#1048
Conversation
github-actions
bot
added
🧾 package/realm
tbruyelle
changed the title
IsOriginCall/AssertOriginCall rely on PrevRealmIsOriginCall/AssertOriginCall rely on PrevRealm
| t.Errorf("expected IsOriginCall=false but got true") | ||
| } | ||
| AssertOriginCall() | ||
| }() |
There was a problem hiding this comment.
No longer need to add an other frame (the function literal) to cheat the AssertOriginCall algorithm, since it doesn't use the number of frames any more.
gnovm/stdlibs/frame.go
Outdated
| addr: m.Context.(ExecContext).OrigCaller, | ||
| pkgPath: "", // empty for users | ||
| } | ||
| } |
There was a problem hiding this comment.
Having a standalone function also improves the readability of the algorithm, which used to be quite confusing IMO :)
|
Hey @tbruyelle, Can you check why the CI is acting freaky with this PR? |
|
I don’t see we need to relate the prevRealm() with IsOriginCall() on the Frame. On chain VM runtim, the calls are initiated from user EOA account to the realm function calls directly. No main function involved. 1st machine.Frame[0] the call to a realm pkg method Foo() through VM Keeper to machine.RunFunc() For filetest, function test call starting from main() in a main package. The main package address is used to fake a user caller (EOA) 1st machine.Fram[0] the call to gno main package main() through file test to machine.RunMain() That is why the injected package method need to overwrite on IsOrginCall() for file tests. These are different usage cases. They are not tight to realm, no need to unify them on the frame. Here are some other suggestions related to this. I will create a separate RFC for these.
|
This is because of #1036, we probably need to fix that issue before being able to merge this PR. |
|
@piux2 I understand your point, the goal is to base all functions (OriginCall and PrevRealm) to a single algorithm, and also to avoid test overriding. Test overriding is really annoying, it's not just about changing the number of frames from 2 to 3, because this is true only for "_filetest.gno". For "_test.gno", the number of frames is 7. Since we need to make that distinction, there's also an other exception that need to be handled ( isOriginCall := func(m *gno.Machine) bool {
tname := m.Frames[0].Func.Name
switch tname {
case "main": // test is a _filetest
return len(m.Frames) == 3
case "runtest": // test is a _test
return len(m.Frames) == 7
}
// support init() in _filetest
// XXX do we need to distinguish from 'runtest'/_test?
// XXX pretty hacky even if not.
if strings.HasPrefix(string(tname), "init.") {
return len(m.Frames) == 3
}
panic("unable to determine if test is a _test or a _filetest")
}
The PrevRealm algorithm returns the user if there's no frames or no realm inside the frames, so even if one day a user can call a package directly (which is not possible if I'm not wrong),
Interesting point, I'm not aware of this reentry issue. |
gnovm/stdlibs/frame.go
Outdated
| // isOriginCall returns true if the std.OrigCaller == std.PrevRealm | ||
| func isOriginCall(m *gno.Machine) bool { | ||
| return prevRealm(m).addr == m.Context.(ExecContext).OrigCaller | ||
| } |
There was a problem hiding this comment.
Could we not, at this point, implement IsOriginCall() as Gno code, simply return std.PrevRealm().Addr() == std.GetOrigCaller()?
There was a problem hiding this comment.
Yeah good idea, and I realize it can be even simpler like return std.PrevRealm().IsUser()
There was a problem hiding this comment.
@thehowl so I tried but it seems I fall into the same issue than #875, it's not possible to call a native function from gno files located in stdlibs/std/. If true let's delay your idea to when Improved native bindings is merged.
There was a problem hiding this comment.
@tbruyelle FYI, #875 doesn't have this kind of issue any more. But it looks like isOriginCall has more complex conditions which can't be solved at this point.
|
@moul I had to fix a couple of other tests, but this was for the same reason as So for instance:
|
There was a problem hiding this comment.
it also surprised me, then I realized this is because the package name has changed, and the test is no longer a realm. So I assumed this is why the // Realm: data is so different than before.
There was a problem hiding this comment.
Can you check what's inside if it's not a realm anymore? shouldn't be shorter?
Maybe we should disable the //Realm directive for this test.
There was a problem hiding this comment.
Wow sry I didn't realized the change added so many lines! We definitively need to fix that, thanks for raising!
So I dived a little bit more to understand, so the key differences come from the number of operations added in the store, and there's 4 kinds of operation (switchrealm, create, update and delete). Here is the report :
z_4_filetest.gno as a realm (before this PR)
- total number of ops: 30
- switchrealm 15
[switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/test"]] - create 10
- update 5
- delete 0
z_4_filetest.gno not a realm (in this PR)
- total number of ops: 13691
- switchrealm 49
[switchrealm["strconv"] switchrealm["errors"] switchrealm["std"] switchrealm["internal/os"] switchrealm["time"] switchrealm["gno.land/p/demo/avl"] switchrealm["unicode"] switchrealm["sort"] switchrealm["unicode/utf8"] switchrealm["io"] switchrealm["internal/bytealg"] switchrealm["strings"] switchrealm["regexp/syntax"] switchrealm["bytes"] switchrealm["regexp"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/boards"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/users"] switchrealm["gno.land/r/demo/boards"]] - create 13615
- update 27
- delete 0
So as we can notice there's many more switchrealm in the second version, which also implies a lot more creates operations. Seems like in this case the store records also non-realm packages like strconv, errors, std, etc, whereas the first case limits the record to realm packages only.
I tried to understand what's the code involved internally, but that's a difficult task. I can dive further, or else we can just remove the Realm: instruction from this test and move on.
There was a problem hiding this comment.
Here is more detailed report, where the number of create/update/delete are associated with their realm. The 3 numbers behind the switchrealm are respectively the number of create, update and delete.
z_4_filetest.gno as a realm (before this PR)
0 {switchrealm["gno.land/r/demo/users"] 0 0 0}
1 {switchrealm["gno.land/r/demo/users"] 0 0 0}
2 {switchrealm["gno.land/r/demo/boards"] 10 5 0}
3 {switchrealm["gno.land/r/demo/boards"] 0 0 0}
4 {switchrealm["gno.land/r/demo/users"] 0 0 0}
5 {switchrealm["gno.land/r/demo/users"] 0 0 0}
6 {switchrealm["gno.land/r/demo/users"] 0 0 0}
7 {switchrealm["gno.land/r/demo/users"] 0 0 0}
8 {switchrealm["gno.land/r/demo/users"] 0 0 0}
9 {switchrealm["gno.land/r/demo/users"] 0 0 0}
10 {switchrealm["gno.land/r/demo/users"] 0 0 0}
11 {switchrealm["gno.land/r/demo/users"] 0 0 0}
12 {switchrealm["gno.land/r/demo/users"] 0 0 0}
13 {switchrealm["gno.land/r/demo/boards"] 0 0 0}
14 {switchrealm["gno.land/r/test"] 0 0 0}
z_4_filetest.gno not a realm (in this PR)
0 {switchrealm["strconv"] 3 0 0}
1 {switchrealm["errors"] 3 0 0}
2 {switchrealm["std"] 8 0 0}
3 {switchrealm["internal/os"] 3 0 0}
4 {switchrealm["time"] 18 0 0}
5 {switchrealm["gno.land/p/demo/avl"] 4 0 0}
6 {switchrealm["unicode"] 13261 0 0} <---- culprit
7 {switchrealm["sort"] 4 0 0}
8 {switchrealm["unicode/utf8"] 21 0 0}
9 {switchrealm["io"] 13 0 0}
10 {switchrealm["internal/bytealg"] 8 0 0}
11 {switchrealm["strings"] 8 0 0}
12 {switchrealm["regexp/syntax"] 72 0 0}
13 {switchrealm["bytes"] 9 0 0}
14 {switchrealm["regexp"] 13 0 0}
15 {switchrealm["gno.land/r/demo/users"] 0 0 0}
16 {switchrealm["gno.land/r/demo/users"] 0 0 0}
17 {switchrealm["gno.land/r/demo/users"] 46 0 0}
18 {switchrealm["gno.land/r/demo/boards"] 0 0 0}
19 {switchrealm["gno.land/r/demo/boards"] 0 0 0}
20 {switchrealm["gno.land/r/demo/boards"] 79 0 0}
21 {switchrealm["gno.land/r/demo/users"] 3 3 0}
22 {switchrealm["gno.land/r/demo/users"] 0 0 0}
23 {switchrealm["gno.land/r/demo/users"] 0 0 0}
24 {switchrealm["gno.land/r/demo/boards"] 6 3 0}
25 {switchrealm["gno.land/r/demo/users"] 0 0 0}
26 {switchrealm["gno.land/r/demo/users"] 0 0 0}
27 {switchrealm["gno.land/r/demo/boards"] 7 2 0}
28 {switchrealm["gno.land/r/demo/users"] 0 0 0}
29 {switchrealm["gno.land/r/demo/users"] 0 0 0}
30 {switchrealm["gno.land/r/demo/boards"] 8 3 0}
31 {switchrealm["gno.land/r/demo/users"] 0 0 0}
32 {switchrealm["gno.land/r/demo/users"] 0 0 0}
33 {switchrealm["gno.land/r/demo/boards"] 8 7 0}
34 {switchrealm["gno.land/r/demo/boards"] 0 0 0}
35 {switchrealm["gno.land/r/demo/users"] 0 0 0}
36 {switchrealm["gno.land/r/demo/users"] 0 0 0}
37 {switchrealm["gno.land/r/demo/boards"] 10 9 0}
38 {switchrealm["gno.land/r/demo/boards"] 0 0 0}
39 {switchrealm["gno.land/r/demo/users"] 0 0 0}
40 {switchrealm["gno.land/r/demo/users"] 0 0 0}
41 {switchrealm["gno.land/r/demo/users"] 0 0 0}
42 {switchrealm["gno.land/r/demo/users"] 0 0 0}
43 {switchrealm["gno.land/r/demo/users"] 0 0 0}
44 {switchrealm["gno.land/r/demo/users"] 0 0 0}
45 {switchrealm["gno.land/r/demo/users"] 0 0 0}
46 {switchrealm["gno.land/r/demo/users"] 0 0 0}
47 {switchrealm["gno.land/r/demo/users"] 0 0 0}
48 {switchrealm["gno.land/r/demo/boards"] 0 0 0}
so we can see that this is the unicode package that implies so many create, logically because the code contains many variable declarations.
|
Just found out this PR will fix another issue in the current behaviour of IsOriginCall/AssertOriginCall. Because the old behaviour was to rely on func TestNewGame(t *testing.T) {
g := NewGame(std.DerivePkgAddr("xx").String())
println(g)
}func TestNewGame(t *testing.T) {
for range [...]string{""} {
g := NewGame(std.DerivePkgAddr("xx").String())
println(g)
}
}The reason seems to be that when we enter a for loop we add a new frame (for handling break/continue) |
|
@thehowl cool, and yes that demonstrates that using the number of frames is really too weak! |
|
This PR is essential for the upcoming gnochess event. @tbruyelle, could you please focus on reducing the substantial changes, which currently amount to 600k lines? @piux2, we would appreciate it if you could conduct another round of reviews. Your previous concerns extended beyond the PR itself and should be addressed separately, outside the scope of this PR. |
tbruyelle
force-pushed
the
refac/assertOriginCaller
branch
from
7fbfe14 to
27d1379
Compare
TODO see if that passes the tests
|
Hey @tbruyelle, are you aware of this discussion? EDIT: Ah, I see that you've already discussed with @piux2. My two cents on this is that we shouldn't push developers to require OriginCalls to their code, since it simply limits functionality. I would keep the option in, but mainly promote Ethereum has the same concept ( The real question is this (@piux2): why would you want to limit your realm/function to be called only directly by an EOA (wallet)? What is a good reason for not allowing a "middleman" realm/package? IMHO, it only blocks useful functionality like account abstraction, and is generally not very well accepted by the community (ie you're not fully permissionless/you are in a way "censoring" usage). I don't think security should be reason why we should block non-origin calls. |
Nuclear bomb button dapp was mentionned as a usecase for Other than that, I agree with you, I would prefer to rely only on |
|
@piux2 Do you think it's acceptable for This way it will ignore all stdlibs packages, which will make it work in tests without having to override it in But it will still panic if there's an additional |
Here is the lastest we can discuss. a. MsgRun behaves the same as MsgCall; the main contract in MsgRun is trusted, just as in MsgCall. b. p/ is treated the same as r/. (However, because p/ can no longer import r/, the p/ package will not act as a middleman. Does this allow rules 7 and 9 to pass directly?) c. 1 results in a PANIC because AssertOrigin kicks in when AssertOrigin protects the entry point of the entire realm when it is in the call path
|
|
formatted A/B/C functions being talked about: // realm pkg: gno.lang/r/myrealm
package myrealm
import (
"std"
)
func A() string {
C()
return "return from A()"
}
func B() string {
if false {
C()
}
return "return from B()"
}
func C() string {
std.AssertOriginCall() // same as AssertIsUser()
return "return from C()"
} |
|
Following. |
tl;drWe have a decision on the design:
the tableFrom the table written by @piux2, this entails only one change (line 12):
A summary of the discussionThere are two main use cases brought up for AssertOriginCall:
The first one is not actually "solved" by AssertOriginCall, as it is possible There is one notable concern in providing a mechanism like AssertOriginCall: AssertOriginCall, however, explicitly does not solve the issue of Therefore, the reason why you should use AssertOriginCall is for safety in the It makes sense for claiming an airdrop to be an explicit action that On the other side, the input of m_call is much more visible and understandable To the extent of enforcing realm authors to use safe mechanisms, we're removing A note on closures. In the "scribbles" below I make an example of ClaimAirdrop ScribblesHere are the notes I was writing down during the meeting: package MsgRun
func main() {
attacker.CallClosure(func() { c.ClaimAirdrop() })
}
package attacker
func CallClosure(f func()) { f() }
package c // realm
func ClaimAirdrop() {
std.AssertOriginCall() // UX choice (can only use MsgCall)
REMOVE std.GetOrigCaller
std.PrevRealm() // => avoids impersonation, enforced security mechanism for realms
std.PrevRealm().IsUser() // MsgRun -> true; MsgCall -> true
std.PrevRealm().Address() // MsgRun -> user addr; MsgCall -> user addr
std.PrevRealm().PkgPath() // MsgRun -> ""; MsgCall -> ""
// TBD section, for future consideration:
// PkgPath(): "" -> gno.land/u/g13312asdkjask?
// std.IsSingleMessageTransaction() // -> true enforcer of "mempool-fairness"
}For future considerationA couple of discussion points left for the future. Fair MsgCallsOne feature that would be good to have is a "fairness" mechanism for calling a
To avoid this, there could be a system to ensure that the transaction being A
|
Turn back AssertOriginCall to original implementation. Unfortunately, this still requires to override it for tests (wip).
|
Many thanks @thehowl for your last comment which summarizes everything. I think we should close this PR, the title is not accurate (and changing it will make the discussions obsoletes) and many changes in the code aren't relevant any more. Regarding the immediate work that needs to be done given your comment, I suggest to split into different PRs/issues:
|
Fix gnolang#1663 `AssertOriginCall` used to panic on `MsgRun` because it involves more than 2 frames. But we want to reinforce that property, with an additional check. Added an improved version of the unit and txtar tests from gnolang#1048. In particular, the txtar adds 2 more cases : 19) MsgCall invokes std.AssertOriginCall directly: pass 20) MsgRun invokes std.AssertOriginCall directly: PANIC Note that 19) involves a change in the AssertOriginCall algorithm, because in that situation there's only a single frame. Even if there's no reason to call `std.AssertOriginCall()` directly from the command line, I think it's logic to make it pass, because that's a origin call.
Fix gnolang#1663 `AssertOriginCall` used to panic on `MsgRun` because it involves more than 2 frames. But we want to reinforce that property, with an additional check. Added an improved version of the unit and txtar tests from gnolang#1048. In particular, the txtar adds 2 more cases : 19) MsgCall invokes std.AssertOriginCall directly: pass 20) MsgRun invokes std.AssertOriginCall directly: PANIC Note that 19) involves a change in the AssertOriginCall algorithm, because in that situation there's only a single frame. Even if there's no reason to call `std.AssertOriginCall()` directly from the command line, I think it's logic to make it pass, because that's a origin call.
Fix gnolang#1663 `AssertOriginCall` used to panic on `MsgRun` because it involves more than 2 frames. But we want to reinforce that property, with an additional check. Added an improved version of the unit and txtar tests from gnolang#1048. In particular, the txtar adds 2 more cases : 19) MsgCall invokes std.AssertOriginCall directly: pass 20) MsgRun invokes std.AssertOriginCall directly: PANIC Note that 19) involves a change in the AssertOriginCall algorithm, because in that situation there's only a single frame. Even if there's no reason to call `std.AssertOriginCall()` directly from the command line, I think it's logic to make it pass, because that's a origin call.
…#1665) Fix #1663 `AssertOriginCall` used to panic on `MsgRun` because it involves more than 2 frames. But we want to reinforce that property, with an additional check. Added an improved version of the unit and txtar tests from #1048. In particular, the txtar adds 2 more cases : 19) MsgCall invokes std.AssertOriginCall directly: pass 20) MsgRun invokes std.AssertOriginCall directly: PANIC Note that 19) involves a change in the AssertOriginCall algorithm, because in that situation there's only a single frame. Even if there's no reason to call `std.AssertOriginCall()` directly from the command line, I think it's logic to make it pass, because that's a origin call. To run the txtar test: ``` $ go test ./gno.land/cmd/gnoland/ -v -run TestTestdata/assert ``` <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Co-authored-by: Leon Hudak <[email protected]> Co-authored-by: Morgan Bazalgette <[email protected]>
…gnolang#1665) Fix gnolang#1663 `AssertOriginCall` used to panic on `MsgRun` because it involves more than 2 frames. But we want to reinforce that property, with an additional check. Added an improved version of the unit and txtar tests from gnolang#1048. In particular, the txtar adds 2 more cases : 19) MsgCall invokes std.AssertOriginCall directly: pass 20) MsgRun invokes std.AssertOriginCall directly: PANIC Note that 19) involves a change in the AssertOriginCall algorithm, because in that situation there's only a single frame. Even if there's no reason to call `std.AssertOriginCall()` directly from the command line, I think it's logic to make it pass, because that's a origin call. To run the txtar test: ``` $ go test ./gno.land/cmd/gnoland/ -v -run TestTestdata/assert ``` <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Co-authored-by: Leon Hudak <[email protected]> Co-authored-by: Morgan Bazalgette <[email protected]>
|
Closing in favour of #1577 and #2906 (the remaining issues pointed out by @tbruyelle are now solved, and IIRC we have txtars which match the "table" pointed out in my comment). |
Now that
PrevRealmworks on tests files (#896), we can updateIsOriginCallandAssertOriginCallso they rely on the same algorithm.IsOriginCallreturns true if the previous caller is the one that signed the tx, aka the original caller.AssertOriginCalltriggers a panic isIsOriginCallreturn false.Initially, those 2 functions were using the number of frames of the
Machineto determine if the previous caller was the original caller. In production, if the number of frames is 2, thenIsOriginCallreturns true. Also, in test environment, they had to be overrided because the number of frames is different than production (and also different between*_test.gnoand*_filetest.gnofiles).Using the number of frames is kinda weak, so I propose to rely on the
PrevRealmalgoirthm instead. Because by definition, ifPrevRealmreturns the original caller, that means the previous caller is the original caller.List of changes:
PrevRealmalgorithm out ofDefineNative()into a standalone function, so it can be easily tested and reused.isOriginCallstandalone function that callPrevRealm.isOriginCallandprevRealminDefineNative()IsOriginCallandAssertOriginCall, because they're no longer needed thanks to the fact they are not relying on the number of frames any more.Required additional fix:
r/demo/boards, the*_filetest.gnofiles were badly configured, so I had to update most of them :Considering all
r/demo/boardspublic functions start withAssertOriginCall, the*_filetest.gnofiles cannot introduce a secondary realm likegno.land/r/boards_testbecause else that means the previous caller isn't the original caller. By removing that secondary realm from the frames, we ensure the previous caller is the original one.Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description