runtime: Stabilise NEP264

[austinabell]

Issue: #6150

This stabilization comes with refactors based on suggestion #6150 (comment) where append_action_function_call and internal_promise_batch_action_function_call are removed. These were only used to be able to have consistent and non-duplicate functionality with and without this protocol feature flag, so this functionally changes nothing functionally about the existing functionality but reduces the amount of code.

[matklad]

So this basically look good to me! But, while working on it, I realized that there's a class of behaviors not currently covered by tests. Namely, when we create a bunch of weighted promises, and then exceed either of the two gas limits (for burnt or for total gas). I think it might be best if I add those cases, as there we'll get interraction with vm's trickly logic of gas limit processing.

To clarify, I do think that the logic we have here is correct though!

[austinabell]

So this basically look good to me! But, while working on it, I realized that there's a class of behaviors not currently covered by tests. Namely, when we create a bunch of weighted promises, and then exceed either of the two gas limits (for burnt or for total gas). I think it might be best if I add those cases, as there we'll get interraction with vm's trickly logic of gas limit processing.

To clarify, I do think that the logic we have here is correct though!

Unless I'm misunderstanding what you're suggesting here, I don't see how that could be triggered. When the prepaid gas is consumed completely there would just be the error and the weight distribution would never happen? Do you just want to verify this is the case?

Feel free to push the tests to this branch if you'd like! Otherwise let me know what specifically you are trying to test for and I can push it along so this doesn't get blocked before the next release

[matklad]

When the prepaid gas is consumed completely there would just be the error and the weight distribution would never happen

I think it'll still happen -- even on exhausted gas, we still extract outcome out of VMLogic. And I think it might even be non-trivial -- if we exceeded max_burnt_gas, but not prepaied gas, we'd have some gas attached to the promises. Now, I don't think that this is going to be user visible behavior, but the code will get executed.

what specifically you are trying to test for and I can push it along so this doesn't get blocked before the next release

I am thinking about two tests, implementing via test-contract-rs:

One is:

function_call_weight(weight = 1)
gas(u64::MAX) // exceed max burn gas

Another is

function_call_weight(weight = 1)
function_call(gas=u64::MAX) // exceed prepaid gas

[austinabell]

When the prepaid gas is consumed completely there would just be the error and the weight distribution would never happen

I think it'll still happen -- even on exhausted gas, we still extract outcome out of VMLogic. And I think it might even be non-trivial -- if we exceeded max_burnt_gas, but not prepaied gas, we'd have some gas attached to the promises. Now, I don't think that this is going to be user visible behavior, but the code will get executed.

what specifically you are trying to test for and I can push it along so this doesn't get blocked before the next release

I am thinking about two tests, implementing via test-contract-rs:

One is:

function_call_weight(weight = 1)
gas(u64::MAX) // exceed max burn gas

Another is

function_call_weight(weight = 1)
function_call(gas=u64::MAX) // exceed prepaid gas

Oh, I see. Maybe it would be worth considering adding a check to short circuit if unused_gas == 0? Seems like it might be worth it to avoid logic on failed transactions?

[matklad]

Oh, I see. Maybe it would be worth considering adding a check to short circuit if unused_gas == 0? Seems like it might be worth it to avoid logic on failed transactions?

I believe this check won't help with the first use case. We have two gas limits: the total gas attached to tx (which can be spend either on immediate wasm execution or atttached to promises) and a config limit on how much a single tx can burn immediatelly. If we hit the second limit, we might still have gas to burn

[mina86]

I wouldn’t call it a chore. It’s a big feature and a protocol change.

This looks fine though I admit I don’t have deep understanding of what those functions are actually doing.

NayDuck: https://nayduck.near.org/#/run/2457

[mina86]

Could you describe a bit in the PR description what’s the deal with append_action_function_call and internal_promise_batch_action_function_call being removed and all the corresponding changes? IIUC this PR is really doing two things: stabilising the NEP and then also switches some of the code to use that NEP. That’s fine but it would be nice if we had a more detailed description of the changes.

[matklad]

@austinabell in the last commit, I've added the tests I was talking about.

While doing that, I've noticed a subtle thing. I think one motivation for attaching lefrover gas to the last promise was to avoid emitting a refund receipt. However, it seems that with dynamic gas pricing we need to generate a refund anyway.

[austinabell]

I wouldn’t call it a chore. It’s a big feature and a protocol change.

Yep, my thinking was just that the functionality wasn't added or changed here, but just removed the feature flag.

Could you describe a bit in the PR description what’s the deal with append_action_function_call and internal_promise_batch_action_function_call being removed and all the corresponding changes? IIUC this PR is really doing two things: stabilising the NEP and then also switches some of the code to use that NEP. That’s fine but it would be nice if we had a more detailed description of the changes.

Done

While doing that, I've noticed a subtle thing. I think one motivation for attaching lefrover gas to the last promise was to avoid emitting a refund receipt. However, it seems that with dynamic gas pricing we need to generate a refund anyway.

Yes, that was the motivation. I wasn't aware of this, what would be your suggestion here now that this is the case? Should gas just be refunded or keep the logic and gas splitting of the remainder? Who else should we get input on for this?

[matklad]

what would be your suggestion here now that this is the case?

Nothing, I think it's ok! Just something I personally haven't notice before.

@austinabell could you resolve conflicts here?

[austinabell]

@austinabell could you resolve conflicts here?

Yeah, it's a bit strange that the tests you've added cause unrelated tests to fail. Looking into that now

[austinabell]

So, not a perfect e2e testing solution (I will work to do a cleaner and easily reproducible/automated setup soon) but to verify all APIs are connected correctly I tested it against all near-sdk-rs examples using this new host function. near/near-sdk-rs#742 (commit 94cd0d36f6074b48a5fbf5a1492f5838fc847018) was used and to repro:

# From within nearcore directory
make sandbox-release

# From within near-sdk-rs directory
NEAR_SANDBOX_BIN_PATH=<absolute path to nearcore>/n
earcore/sandbox/release/neard ./examples/test_all.sh

Workspaces does not have support to use a compiled (not prebuilt binary) version of sandbox, which seems somewhat necessary to be able to add this into test suite of nearcore. Also unclear if workspaces execution is optimized enough to add in. cc @ChaoticTempest

Would be nice if there was a way to compile sandbox and run it directly from workspaces, but seems unlikely neard crate (or slight subset) would be released anytime soon.

[matklad]

This looks good to me now!

[matklad]

As this is a feature stabilization, I'd ask someone from the reviewers to give a second approval here (we discussed informally with bowenwang1996 that, as feature stabilizations are a big deal, we want a higher bar for reviews here). @mina86 could you give a second look here?