fix: Add error message for failed TFHE.req() calls [#91]

[0xf333]

This basically addresses the need for more informative error handling in our TFHE library.

Description:

• Modified TFHE.req() in Impl.sol to return a tuple (bool, string).
  • bool for the status of the TFHE.req() call.
  • string for corresponding log messages to imporve debuging experience.
• Adjusted req() functions in TFHE.sol to handle this new implementation.

Test Case:

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.13 <0.8.20;

import "./lib/TFHE.sol";


contract TestContract {
    
    event ResultLogged(string message);


    // Test values
    euint8 constant TEST_VALUE_PASS = euint8.wrap(123); 
    euint8 constant TEST_VALUE_FAIL = euint8.wrap(0); 


    modifier logsResult(euint8 value) {
        bool _result;
        string memory _message; 

        (_result, _message) = TFHE.req(value);

        if (_result){
            emit ResultLogged(_message);
        } else {
            revert(_message);
        }
        _;
    }


    // This will pass and log TFHE.req() success message
    function testReqPass() public logsResult(TEST_VALUE_PASS){}

    // Once the precompiled contract 69 is setup, This will fail 
    // and revert with the desired error message `TFHE.req() fail`.
    function testReqFail() public logsResult(TEST_VALUE_FAIL){}

}

Impact:

This commit addresses issue #91

[0xf333]

@immortal-tofu / @dartdart26 /@tremblaythibaultl

Hi guys, feel free to let me know if you have any questions regarding the PR.
I'm open to any feedback you might have.

Thanks

[dartdart26]

@immortal-tofu / @dartdart26 /@tremblaythibaultl

Hi guys, feel free to let me know if you have any questions regarding the PR. I'm open to any feedback you might have.

Thanks

Thank you for your PR @0xf333! We will have a look and get back to you!

[immortal-tofu]

Hey @0xf333 !
Thank you for your contribution to this project. Before we can merge your work, we kindly ask you to sign our Contributor License Agreement (CLA). This is just a necessary step to ensure we're all on the same page regarding licensing. You can find the CLA document and sign it using the following link: CLA. Thank you so much for your understanding and support!

[0xf333]

Hey @0xf333 ! Thank you for your contribution to this project. Before we can merge your work, we kindly ask you to sign our Contributor License Agreement (CLA). This is just a necessary step to ensure we're all on the same page regarding licensing. You can find the CLA document and sign it using the following link: CLA. Thank you so much for your understanding and support!

Done, and on the GitHub part I assume you meant username.

Last but not least, you can consider this chat thread as the public record of when I agreed for you guys to use my contributions in your project.

[0xf333]

The license identifier is required by the Solidity compiler, which is why I'm including it in the TestContract for the test case I provided.
I've now updated it to SPDX-License-Identifier: MIT for more flexibility.

[dartdart26]

@0xf333 We are working on an explicit decrypt functionality that will be merged soon: #89

Essentially, one would be able to first decrypt and then emit an event (i.e. an if statement). I think that is very similar to what your are doing, but I think will be clearer, because require typically implies a revert, whereas an explicit decrypt doesn’t.

Maybe what we originally wanted is to have an overload of TFHE.req(ebool b, string message) or TFHE.req(ebool b, error e) such that if it fails during txn processing, an event with the user-provided error is automatically emitted. However, not sure if that can be done with both emitting an event and reverting. Maybe could somehow be done in go-ethereum, but not in Solidity. What do you think?

[mortendahl]

Thanks!

[0xf333]

Maybe what we originally wanted is to have an overload of TFHE.req(ebool b, string message) or TFHE.req(ebool b, error e) such that if it fails during txn processing, an event with the user-provided error is automatically emitted. However, not sure if that can be done with both emitting an event and reverting. Maybe could somehow be done in go-ethereum, but not in Solidity. What do you think?

@dartdart26

I think you can achieve something similar by handling the success event message on the caller contract side. Basically, the TFHE.req function main task will be to check a condition and reverting with a custom error message when it fails; however success message will no longer be builtin.

Basically, you will have something like this.

inside Impl.sol

function req(uint256 ciphertext, string memory customErrorMessage) internal view {
    bytes32[1] memory input;
    input[0] = bytes32(ciphertext);
    uint256 inputLen = 32;

    // Call the require precompile.
    uint256 precompile = Precompiles.Require;
    assembly {
        if iszero(staticcall(gas(), precompile, input, inputLen, 0, 0)) {
            // Reverting with the custom error message here
            revert(ciphertext, customErrorMessage) 
        }
    }
}

inside TFHE.sol

function req(euint8 value, string memory errorMessage) internal view {
    Impl.req(euint8.unwrap(value), errorMessage);
}

function req(euint16 value, string memory errorMessage) internal view {
    Impl.req(euint16.unwrap(value), errorMessage);
}

function req(euint32 value, string memory errorMessage) internal view {
    Impl.req(euint32.unwrap(value), errorMessage);
}

function req(ebool b, string memory errorMessage) internal view {
    Impl.req(ebool.unwrap(b), errorMessage);
}

---

And then inside the caller contract

This is what I mentioned about handling it on the caller contract side

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.13 <0.8.20;

import "./lib/TFHE.sol";

contract TestContract {
    
    // Test values
    euint8 constant TEST_VALUE_PASS = euint8.wrap(123); 
    euint8 constant TEST_VALUE_FAIL = euint8.wrap(0); 

    event ReqPassed(string message);

    
    modifier req(euint8 value) {
        // You'll provide your custom error message here
        // mine is `TFHE.req() failed`.
        TFHE.req(value, "TFHE.req() failed");
        
        // If TFHE.req() doesn't revert, it means the check passed
        // So, we emit a success event here
        emit ReqPassed("TFHE.req() passed");
        _;
    }

    // This will pass and not revert
    function testReqPass() public req(TEST_VALUE_PASS){}

    // Once the precompiled contract 69 is setup, This will fail 
    // and revert with the desired error message `TFHE.req() failed`.
    function testReqFail() public req(TEST_VALUE_FAIL){}
}

Is this close to what you have in mind?

[dartdart26]

Is this close to what you have in mind?

Yes, that is what I had in mind. However, now that I think of it, there might be two issues, but I haven’t investigated fully:

1. On gas estimation, we assume encrypted requires are true without decrypting (as we need consensus to trigger the threshold network to decrypt). In that case, the message is not going to be shown.
2. If a txn gets reverted during execution after consensus and a decryption, I think the message will also not show in a block explorer or in a txn receipt just due to the reversal. I don’t see the message in a txn receipt. I think the way explorers could show it is using the tracing Eth RPCs. However, we don’t support them as of now because of decryptions - we need to sort out how to get decrypted values during tracing. Need to double check all of that, though. Any thoughts on it?

[0xf333]

Is this close to what you have in mind?

Yes, that is what I had in mind. However, now that I think of it, there might be two issues, but I haven’t investigated fully:

1. On gas estimation, we assume encrypted requires are true without decrypting (as we need consensus to trigger the threshold network to decrypt). In that case, the message is not going to be shown.

2. If a txn gets reverted during execution after consensus and a decryption, I _think_ the message will also not show in a block explorer or in a txn receipt just due to the reversal. I don’t see the message in a txn receipt. I think the way explorers could show it is using the tracing Eth RPCs. However, we don’t support them as of now because of decryptions - we need to sort out how to get decrypted values during tracing. Need to double check all of that, though. Any thoughts on it?

Makes senses, thank you for your feedback, I get your point regarding gas, reversion, and error messages visibility.

As you know, due to how Ethereum blockchain currently works,the error message from the revert statement will be included in the transaction receipt, but informally not in the event logs. Perhaps providing the option to choose how to handle req from the caller contract side would be a more flexible approach?

That's the reason why in my initial and current commit, the req function does not immediately revert the transaction if the condition is not met. Instead, it returns a boolean value indicating the result of the check and a string message. The idea is to allow the caller contract to decide how to handle the result from req function.

Wouldn't this be the more flexible approach due to current limitations?
I appreciate you guys feedback, let me know what you think.

[dartdart26]

Is this close to what you have in mind?

Yes, that is what I had in mind. However, now that I think of it, there might be two issues, but I haven’t investigated fully:

1. On gas estimation, we assume encrypted requires are true without decrypting (as we need consensus to trigger the threshold network to decrypt). In that case, the message is not going to be shown.

2. If a txn gets reverted during execution after consensus and a decryption, I _think_ the message will also not show in a block explorer or in a txn receipt just due to the reversal. I don’t see the message in a txn receipt. I think the way explorers could show it is using the tracing Eth RPCs. However, we don’t support them as of now because of decryptions - we need to sort out how to get decrypted values during tracing. Need to double check all of that, though. Any thoughts on it?

Makes senses, thank you for your feedback, I get your point regarding gas, reversion, and error messages visibility.

As you know, due to how Ethereum blockchain currently works,the error message from the revert statement will be included in the transaction receipt, but informally not in the event logs. Perhaps providing the option to choose how to handle req from the caller contract side would be a more flexible approach?

That's the reason why in my initial and current commit, the req function does not immediately revert the transaction if the condition is not met. Instead, it returns a boolean value indicating the result of the check and a string message. The idea is to allow the caller contract to decide how to handle the result from req function.

Wouldn't this be the more flexible approach due to current limitations? I appreciate you guys feedback, let me know what you think.

Yes, we might want to be explicit about decryption and, potentially, even remove encrypted requires. Instead, you explicitly decrypt and then decide what to do, potentially reverting. That is why I was mentioning #89 instead of calling it req().

In terms of reverting, emitted events before the revert would also be reverted by default. That is why I was thinking we might have to change go-ethereum for it to work. Do I miss something?

In terms of receipts, AFAIK, there is no information about the revert/require string/error there: https://docs.infura.io/networks/ethereum/json-rpc-methods/eth_gettransactionreceipt. Do I miss something?

In general, I think we might have to emit events outside of the EVM (but inside go-ethereum) due to the default reversal behaviour of events to be able to preserve the information, but I haven't gotten into the details yet. Or another approach might be to use the tracing RPCs, but then a question remains on whether these can run on a node that doesn't have the FHE key (or a share of the FHE key). Maybe the go-ethereum change is most promising at that point, but we haven't spent time looking into it. Overall, we'd like to get the information in a block explorer or a receipt or an event, without changing the tools, APIs or infrastructure too much.

I know there are a lot of considerations that are not apparent from the Solidity library itself or the go-ethereum codebase. We are still working on it and we really appreciate your PRs, feedback, suggestions and discussions on all of it!

[0xf333]

In terms of receipts, AFAIK, there is no information about the revert/require string/error there: https://docs.infura.io/networks/ethereum/json-rpc-methods/eth_gettransactionreceipt. Do I miss something?

---

Oh, I see there's a keyboard auto corrected word in my previous reply that may have skewed what I was talking about, I meant "unfortunately" not informally :

As you know, due to how Ethereum blockchain currently works,the error message from the revert statement will be included in the transaction receipt, but informally unfortunately not in the event logs.

---

As for the rest, I get your point

[dartdart26]

I think we will soon use explicit decryptions instead of require statements on encrypted booleans. Nevertheless, thank you for your PR and the discussion @0xf333!