Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Test current balance behaviour in recursive sends #52

Open
anorth opened this issue Aug 6, 2020 · 8 comments · May be fixed by #155
Open

Test current balance behaviour in recursive sends #52

anorth opened this issue Aug 6, 2020 · 8 comments · May be fixed by #155
Assignees
Labels
area/message-vector Areas: Message-class vector discomfort-factor/8 Discomfort factor: I touched my eyes after picking up a Jalapeño (10,000 SHU). kind/vector Kind: Vector

Comments

@anorth
Copy link
Member

anorth commented Aug 6, 2020

Actor A invokes a method on Actor B and supplies a nonzero value. Before Actor B’s execution is complete, it invokes a method on Actor A, but does NOT supply value. Within this context, CurrentBalance should factor in the amount initially sent to Actor B.

Finally, Actor B’s execution finishes successfully and Actor A continues execution where it left off. CurrentBalance should factor in the amount sent to Actor B.

Via @wadealexc

@raulk
Copy link
Member

raulk commented Aug 18, 2020

@anorth are these questions about which behaviour is correct (which would need to be discussed in the team)? Or are these test scenarios worded as questions?

@anorth
Copy link
Member Author

anorth commented Aug 20, 2020

I've removed the question marks - they reflect expected behaviour to be tested.

@raulk raulk transferred this issue from filecoin-project/chain-validation Aug 20, 2020
@willscott willscott self-assigned this Aug 25, 2020
@raulk
Copy link
Member

raulk commented Aug 25, 2020

@willscott We'll need the chaos actor to test this.

Suggestion: add a RecursiveSend method that takes a recursion level (big.Int) as an arg, and returns a slice of []abi.TokenAmount where we'd accumulate all observed balances in each recursion level.

  • ChaosActor#RecursiveSend(n)
    • records the observed balance in a local var.
    • if n-1 != 0, calls ChaosActor#RecursiveSend(n-1), else it returns the recorded balance immediately in a slice of len == 1.
    • prepend the recorded value to the returned array.

@anorth
Copy link
Member Author

anorth commented Aug 25, 2020

Sends from an actor to itself are a whole extra class additional to what I described originally, which should definitely be included. For a self-send with value, I would expect the observed balance to not change because it is subtracted and then added to the same actor.

To test the balance changes originally described, you'll need at least two instances of the chaos actor, one sending to the other.

@raulk raulk added area/message-vector Areas: Message-class vector kind/vector Kind: Vector hint/needs-scoring Hint: Needs scoring labels Aug 26, 2020
@anorth anorth added discomfort-factor/8 Discomfort factor: I touched my eyes after picking up a Jalapeño (10,000 SHU). and removed hint/needs-scoring Hint: Needs scoring labels Aug 27, 2020
@anorth anorth removed their assignment Aug 27, 2020
@raulk
Copy link
Member

raulk commented Aug 28, 2020

@anorth Gotcha. In the reflexive send scenario, wouldn't we expect the balance to have been deducted gas, though?

@raulk
Copy link
Member

raulk commented Aug 28, 2020

Based on @anorth's feedback, the way we would do this is by using ChaosActor#CreateActor to deploy multiple instances of the ChaosActor. Imagine we deploy 100 instances, on 100 addresses (one each).

RecursiveSend would then have this signature and pseudo-code:

type AddressSend struct {
    Address address.Address
    Value   abi.TokenAmount
}

type AddressBalance struct {
    Address        address.Address
    Before, After  abi.TokenAmount
}

func (a *ChaosActor) RecursiveSend(rt *Runtime, sends []*AddressSend) (balances []*AddressBalance) {
    rt.ValidateImmediateCallerAcceptAny()

    // We're the last one in the chain.
    if len(sends) == 0 {
        return []*AddressBalance{&AddressBalance{
            Address: rt.Message().Receiver(),
            Before:  rt.CurrentBalance(),
            After:   rt.CurrentBalance(),
        }}
    }

    before := rt.CurrentBalance()

    // next is the chaos actor address we are routing to, xs is the parameters we'll be sending it.
    next, xs := sends[0], sends[1:]
    
    res, err := rt.Send(next.Address, MethodRecursiveSend, xs, next.Value)
    if !err.IsSuccess() {
        panic(err)
    }

    // assume res is already []*AddressBalance; in practice you'll need to call res.Into() to deserialise
    after := rt.CurrentBalance()

    // prepend our readings to the incoming slice, and return.
    ret := append([]*AddressBalance{&AddressBalance{
        Address: rt.Message().Receiver(),
        Before:  before,
        After:   after,
    }}, res...)

    return ret
}

@anorth
Copy link
Member Author

anorth commented Aug 31, 2020

In the reflexive send scenario, wouldn't we expect the balance to have been deducted gas

Value covering the entire gas limit for a top-level message is deducted from the sender before the message is processed, and any un-used refunded right at the end. So whatever the observed balance is inside a top level call to actor A, if it sends value to itself then inside the nested call I expect the observed balance to be the same; all the gas was already handled before the top-level call started.

@raulk raulk assigned alanshaw and unassigned willscott Sep 8, 2020
@alanshaw alanshaw linked a pull request Sep 21, 2020 that will close this issue
@alanshaw
Copy link
Member

alanshaw commented Sep 21, 2020

@raulk I knocked up this PoC while trying to work out how to create multiple chaos actors: #155

I'm trying to avoid using the chaos CreateActor method because under the hood it uses Runtime.CreateActor which actually shouldn't allow chaos to use it. Not only that but Runtime.CreateActor restricts us to creating "builtin" actors - those defined in specs-actors which does not include chaos so we can't use chaos CreateActor directly right now anyway.

I looked into using the init actor to create a chaos actor but it is heavily restricted in what it can create and which actors are allowed to create.

So in #155 I'm using MessageVectorBuilder.Actors.CreateActor which works nicely and sidesteps the above issues! \o/

RecursiveSend would then have this signature and pseudo-code:

This looks really succinct and I'd be happy to switch to using it because the message construction code in #155 is a little mind bending. That said, I was trying to avoid adding even more functionality to the chaos actor - we've always said chaos should be primitives and this would be very specific to this test scenario...and I'm not certain it has high potential to be used in other tests that aren't just derivatives of this one. WDYT?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/message-vector Areas: Message-class vector discomfort-factor/8 Discomfort factor: I touched my eyes after picking up a Jalapeño (10,000 SHU). kind/vector Kind: Vector
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants