Investigate stack state when entering finalize_transaction

[igamigo]

What should be done?

As part of finalize_transaction, this set of commands is done for truncating the stack. However, following the stack, this should not have to be done unless the procedure is being executed with more elements than expected. If these commeands are commented out, execution will fail for multiple tests.

How should it be done?

Investigate why we are entering the procedure with more elements in the stack than expected.

When is this task done?

When we understand why this is happening and possibly have removed these extra commands.

Additional context

#897 (comment)

[PhilippGackstatter]

I looked into this issue and specifically this comment:

Separately, I'm actually wondering if we need swapdw dropw dropw on line 331. Could you try removing these and see if things still work? If it doesn't work, we enter this procedure with a dirty stack and we should probably understand why. In this case, let's create an issue for this.
#897 (comment).

I added these assertions and in most* tests, none of them panic:

• We start with 16 items on the stack and all of them are zero. We can't have less than 16 items on the stack and all of them are zero, so I guess this is as a "clean" stack.

export.finalize_transaction
    sdepth push.16 assert_eq.err=9999
    dupw push.0.0.0.0 assert_eqw.err=8888
    dupw.1 push.0.0.0.0 assert_eqw.err=7777
    dupw.2 push.0.0.0.0 assert_eqw.err=6666
    dupw.3 push.0.0.0.0 assert_eqw.err=5555

• After the final and init hash are on the stack we have 16 + 2 words = 24 elements on the stack which makes sense because we didn't drop anything yet. Similarly, before the truncate operation there are still 2 "extra" words on the stack:

# copy output note data to the advice map
exec.copy_output_notes_to_advice_map
# => [OUTPUT_NOTES_COMMITMENT, FINAL_ACCOUNT_HASH]
sdepth push.24 assert_eq.err=88

• Then we drop 2 empty words so we have 16 elements:

# truncate stack
swapdw dropw dropw
# => [OUTPUT_NOTES_COMMITMENT, FINAL_ACCOUNT_HASH]
sdepth push.16 assert_eq.err=55

• Finally we exchange get_expiration_block_num for one existing zero element by dropping that one. So we end up with 16 items at the very end.

So all in all: We start with 16 zero elements, push two words and one element for a total of 25 elements on the stack. We have two dropw instructions and one drop instruction so we drop 9 elements bringin the sdepth back to 16 elements.
If we removed the swapdw dropw dropw that would leave the stack at sdepth = 24. So is there actually a problem with finalize_transaction? It seems it doesn't matter whether the stack is dirty or not for this function specifically. It would have to drop elements one way or another, which isn't to say that a dirty stack is not a problem per se (see below*).

One related question that came up is what the ABI of exec-utable procedures looks like or if there even is an agreed upon one. Contrary to call and syscall they can basically do whatever and do not have to be padded or leave the stack at exactly depth 16 upon returning. Right now finalize_transaction says #! Stack: [] suggesting it does not consume anything, but it does actually consume (and drop) 9 elements from the stack. Do we have a structured way to describe that via the docs, similar to how we specify inputs and outputs? I guess the closest thing is saying Stack: [pad(16)] to indicate that the function will treat the inputs as garbage.
Because I can't find anything like that I don't know if finalize_transaction is supposed to leave the stack at depth 16 or whether the callers would be responsible for truncating. Then we could remove the two dropw and one drop instruction and add them wherever we call it. But overall I guess finalize_transaction may be somewhat special as the last function to be called in the kernel, so it just happens to also take care of stack truncation - but I think it should probably say so in its docs.

*In some epilogue tests we do not properly stick to (or have migrated to) the ABI from #685 when calling test::account procedures so we end up with more than 16 items on the stack and possibly non-zero ones. That makes the first assertion(s) in this post fail. Fixing the ABI problem makes the assertions pass there too. I guess that is best handled as part of that issue though.

cc @bobbinth

[bobbinth]

So all in all: We start with 16 zero elements, push two words and one element for a total of 25 elements on the stack. We have two dropw instructions and one drop instruction so we drop 9 elements bringin the sdepth back to 16 elements.
If we removed the swapdw dropw dropw that would leave the stack at sdepth = 24. So is there actually a problem with finalize_transaction?

Ah - makes sense! I think then the main issue is making clear what happens and using pad(16) in the comments (both doc comments for the function and inline comments), maybe be enough here.

One related question that came up is what the ABI of exec-utable procedures looks like or if there even is an agreed upon one.

There isn't really an ABI for exec-ed procedures. These are considered to be "trusted" code and so can do w/e with the stack (but hopefully the docs describe the behavior of such procedures correctly). The way I've been going about this is to write expected stack state as something like Stack: [x, y, z, ...] where ... means that the rest of the stack doesn't matter (and is not touched) by the procedure.

In the case of finalize_transaction procedure, I think it should be something like:

#! Stack: [pad(16)]
#! Output: [OUTPUT_NOTES_COMMITMENT, FINAL_ACCOUNT_HASH, tx_expiration_block_num, pad(7)]

So, maybe to close this issue all we need to do is to update some comments for this procedure.

[Fumuran]

I agree that an approach with adding pads to the stack description will work, but it looks strange to me that we make an exception just for that procedure, taking into account that there are a lot of other exec procedures which put some additional data on the stack without truncating it (about half of all our procedures do so).

I thought that truncating the stack after some procedure was executed is a concern of the procedure which were invoking it. So I'm not sure why should we come up with something new for this procedure, when we can just remove drops from the procedure code and truncate the stack after its execution if necessary? For the main we could truncate the stack right after the execution, and for the tests it's even less a problem: most of them doesn't cate about the stack, so we can just clean it, and for those who care we could just truncate it properly.

I tested it: with this small changes all tests are passing, and we don't need to make an exception for this proc.

[bobbinth]

I agree that an approach with adding pads to the stack description will work, but it looks strange to me that we make an exception just for that procedure, taking into account that there are a lot of other exec procedures which put some additional data on the stack without truncating it (about half of all our procedures do so).

I thought that truncating the stack after some procedure was executed is a concern of the procedure which were invoking it. So I'm not sure why should we come up with something new for this procedure, when we can just remove drops from the procedure code and truncate the stack after its execution if necessary? For the main we could truncate the stack right after the execution, and for the tests it's even less a problem: most of them doesn't cate about the stack, so we can just clean it, and for those who care we could just truncate it properly.

Good point. Maybe that's a better solution (i.e., truncating the stack after this procedure is called).

[bobbinth]

Closed by #971.