contract: implement sign fn with yield/resume

[ppca]

in this new design, we don't store the SignatureResponse in contract state. We still 1) won't allow repeated request if it was already requested and not completed; 2) only allow 8 requests concurrently

Note this new sign fn will return nothing, but you could still use it as if it returns Promise in any cross contract calls.

[volovyks]

Nice!

[ailisp]

Is it better to still return Promise? Without -> Promise, env::promise_return(yield_promise); will return yield_promise, but if yield_promise executes to fail, the transaction (of calling sign) is still success.

[ppca]

yeah,I noticed during my load tests with this code that the sign() tx result will show success until signature request timed out, which I think happens when it's past the number of blocks timeout yield/resume allows and still no result.
I tried load testing again, so the explorer will always show the status of tx as success, one example tx: one example tx: https://testnet.nearblocks.io/txns/BgkgGo3Xt4iWk9FdGeiJfSYtdQnVexzQS8DsnpaDUBqT?tab=execution.
But if we request status of transaction with this code

let outcome_view = ctx
            .jsonrpc_client
            .call(RpcTransactionStatusRequest {
                transaction_info: TransactionInfo::TransactionId {
                    tx_hash,
                    sender_account_id: ctx.nodes.ctx().mpc_contract.id().clone(),
                },
                wait_until: near_primitives::views::TxExecutionStatus::Final,
            })
            .await?;

        let Some(outcome) = outcome_view.final_execution_outcome else {
            anyhow::bail!("final execution outcome not available");
        };

        let outcome = outcome.into_outcome();

        let FinalExecutionStatus::SuccessValue(payload) = outcome.status else {
            anyhow::bail!("tx finished unsuccessfully: {:?}", outcome.status);
        };

as we did in integration tests, if the sign_on_finish fn did panic, this will show outcome.status as Failure(ActionError(ActionError { index: Some(0), kind: FunctionCallError(ExecutionError("Smart contract panicked: signature request timed out")) })).

So I guess if yield_promise executes to fail, which will result in sign_on_finish fn do env::panic_str(), then we'd be able to find out the transaction status is failure, although not through explorer.

[ailisp]

Nice Implementation!

I noticed in doc that:

If the contract does not trigger a resume after 200 blocks - around 4 minutes - the yielded function will resume receiving a "timeout error" as input.

Not sure if this need to be considered, if so combine the previous recursive call approach with yield can extend the limit to 400, 600, ... blocks with a bit more gas

[volovyks]

@ailisp that can be a good idea if we will create an infinite or long queue of signature requests. At the moment we are using different strategy where we allow max 8 simultaneous requests and return signature fast (<200blocks).

We may change this strategy in the future.

[ppca]

Nice Implementation!

I noticed in doc that:

If the contract does not trigger a resume after 200 blocks - around 4 minutes - the yielded function will resume receiving a "timeout error" as input.

Not sure if this need to be considered, if so combine the previous recursive call approach with yield can extend the limit to 400, 600, ... blocks with a bit more gas

@ailisp What Serhii said about limiting request count at the same time is right. I've personally load tested the contract in this code without the request_counter < 8 limit, and we do timeout a lot, I didn't keep precise count, but with the existing load test default setup, at least half are timing out.
I personally think extending the timeout window over 4 min would probably not be what we want users to experience. Our nodes currently function well with ~10 signature requests per min, beyond that it cannot generate triples fast enough to cover signatures. Ideally we improve our backend so we can function with more requests at the same time, but right now we might have to live with the request counter :(

[ppca]

tx finished unsuccessfully: Failure(ActionError(ActionError { index: Some(0), kind: FunctionCallError(ExecutionError("Smart contract panicked: Signature for this payload already requested")) }))
-- I'll look into the failed integration test

[ppca]

tx finished unsuccessfully: Failure(ActionError(ActionError { index: Some(0), kind: FunctionCallError(ExecutionError("Smart contract panicked: Signature for this payload already requested")) })) -- I'll look into the failed integration test

Turns out it's because the sign_on_finish fn removes pending request, but then does env::panic_str if the callback_result is Err, in which case all prev state changes in sign_on_finish will be reverted, thus pending request was not removed.

Also refactored signaure_responded a bit so it's easier to tell status of sign tx.

[ChaoticTempest]

Initial testing of this, I found that if the test fails and the signature is not provided in time at all, we just get "Smart contract panicked: Signature for this payload already requested". Which does not seem correct for an error message if it the MPC network never responded.

If you need the test code, I'll make a separate PR.
here's the execution details:

ExecutionFinalResult {
    total_gas_burnt: NearGas {
        inner: 2605429106161,
    },
    transaction: ExecutionOutcome {
        transaction_hash: 2rr3HUCqWPh9F9gt3grxR96mdrYpSazSJRCSWa89UdDk,
        block_hash: 63EEVkkJukUoJGqX1hKqcLPWmuRSyQ4pkcSu12aW3Jkk,
        logs: [],
        receipt_ids: [
            AAsdX4sEPrXRRaCB54946N3nYz6W66ecq9WiLnTguLtu,
        ],
        gas_burnt: NearGas {
            inner: 308444080648,
        },
        tokens_burnt: NearToken {
            inner: 30844408064800000000,
        },
        executor_id: AccountId(
            "dev-20240713010504-74071972890982.test.near",
        ),
        status: SuccessReceiptId(AAsdX4sEPrXRRaCB54946N3nYz6W66ecq9WiLnTguLtu),
    },
    receipts: [
        ExecutionOutcome {
            transaction_hash: AAsdX4sEPrXRRaCB54946N3nYz6W66ecq9WiLnTguLtu,
            block_hash: 63EEVkkJukUoJGqX1hKqcLPWmuRSyQ4pkcSu12aW3Jkk,
            logs: [],
            receipt_ids: [
                6sZJKXNtkC4wZVABmYsmTQv7fBhZ4XN1Ntnadw8G5tjn,
                7tG7dXEnc9WCCBkkgNgv7HX6FBGWyWb2VJxYEnZrDRTa,
            ],
            gas_burnt: NearGas {
                inner: 1850619900513,
            },
            tokens_burnt: NearToken {
                inner: 185061990051300000000,
            },
            executor_id: AccountId(
                "dev-20240713010504-74071972890982.test.near",
            ),
            status: Failure(ActionError(ActionError { index: Some(0), kind: FunctionCallError(ExecutionError("Smart contract panicked: Signature for this payload already requested")) })),
        },
        ExecutionOutcome {
            transaction_hash: 6sZJKXNtkC4wZVABmYsmTQv7fBhZ4XN1Ntnadw8G5tjn,
            block_hash: C1ej1k8wDtGn4ohLJGMv8c8UPres3eyX4QMGX73nJTkB,
            logs: [],
            receipt_ids: [],
            gas_burnt: NearGas {
                inner: 223182562500,
            },
            tokens_burnt: NearToken {
                inner: 0,
            },
            executor_id: AccountId(
                "dev-20240713010504-74071972890982.test.near",
            ),
            status: SuccessValue(''),
        },
        ExecutionOutcome {
            transaction_hash: 7tG7dXEnc9WCCBkkgNgv7HX6FBGWyWb2VJxYEnZrDRTa,
            block_hash: C1ej1k8wDtGn4ohLJGMv8c8UPres3eyX4QMGX73nJTkB,
            logs: [],
            receipt_ids: [],
            gas_burnt: NearGas {
                inner: 223182562500,
            },
            tokens_burnt: NearToken {
                inner: 0,
            },
            executor_id: AccountId(
                "dev-20240713010504-74071972890982.test.near",
            ),
            status: SuccessValue(''),
        },
    ],
    status: Failure(ActionError(ActionError { index: Some(0), kind: FunctionCallError(ExecutionError("Smart contract panicked: Signature for this payload already requested")) })),
}

[ChaoticTempest]

Seems like if we submit the same request for a signature after it has already completed the first, it will error out with Smart contract panicked: Signature for this payload already requested

[ChaoticTempest]

Seems like also getting "Signature has timed out" after multiple successive signature requests.

Take a look at all the testing I added in branch phuong/feat/contract-testing-without-nodes

[ppca]

Seems like also getting "Signature has timed out" after multiple successive signature requests.

Take a look at all the testing I added in branch phuong/feat/contract-testing-without-nodes

Hey @ChaoticTempest I've checked that code out and tried it. I've tried local unit tests myself too, but I did it by returning String as a signature. When I tested locally, I found out after many attempts that I needed to add in some wait time between sending async sign request and sending respond, likely because of transact_async() and yield/resume combined.

The test_contract_sign_request test in your branch passes 100% after I add in tokio::time::sleep(Duration::from_secs(2)).await; between transact_async of sign and respond.

I tried to reproduce the failed cases you mentioned:

1. "Signature has timed out" will happen on the very fist sign request without the sleep, no other sign requests will be sent.

2. I'm not able to reproduce if we submit the same request for a signature after it has already completed the first, it will error out with Smart contract panicked: Signature for this payload already requested locally. I used to see the error in the integration test test_signature_offline_node_back_online, and that happened previously because in prev code `sign_on_finish fn removes pending request, but then does env::panic_str if the callback_result is Err, in which case all prev state changes in sign_on_finish will be reverted, thus pending request was not removed. But it does not happen any more in the integration test, but it can still happen if we submit the same request before yield/resume returns a result or fail with timeout.

3. I also did not manage to reproduce: I found that if the test fails and the signature is not provided in time at all, we just get "Smart contract panicked: Signature for this payload already requested". This likely is similar to the situation mentioned in the above paragraph, but shouldn't happen with the current code and adding in the sleep.

[ailisp]

Did not have a chance to locally test it, but the code looks good to me overall. Nice work!

[ChaoticTempest]

It's weird that we require the sleep between the sign and respond, but we can resolve it later in a future PR

[ChaoticTempest]

Is gas weight supposed to be zero here. The docs mention the default is GasWeight(1). Tested and changed it to 1, but doesn't seem to effect much

[ppca]

Confirming with Saketh

[github-actions]

Terraform Feature Environment Destroy (dev-628)

Terraform Initialization ⚙️success

Terraform Destroy success

Show Destroy Plan

No changes. No objects need to be destroyed.

Either you have not created any objects yet or the existing objects were
already deleted outside of Terraform.

Destroy complete! Resources: 0 destroyed.

Pusher: @ChaoticTempest, Action: pull_request, Working Directory: ``, Workflow: Terraform Feature Env (Destroy)