Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make message retryable and not spendable on gas #443

Merged
merged 17 commits into from
Feb 27, 2023

Conversation

pixelcircuits
Copy link
Contributor

@pixelcircuits pixelcircuits commented Dec 9, 2022

Input messages are retryable until included in a script transaction that doesn't revert. The amount on an input message can no longer be spent on gas. Messages with no data are now spendable as an input coin (and only spendable as an input coin).

@pixelcircuits pixelcircuits self-assigned this Dec 9, 2022
@pixelcircuits pixelcircuits linked an issue Dec 9, 2022 that may be closed by this pull request
@@ -83,39 +85,49 @@ If this check passes, the UTXO ID `(txID, outputIndex)` fields of each contract
For each asset ID `asset_id` in the input and output set:

```py
def sum_messages(tx, asset_id) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also provide type hints for the parameters

@pixelcircuits pixelcircuits marked this pull request as ready for review December 10, 2022 06:00
Copy link
Contributor

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good.

| `predicateData` | `byte[]` | Predicate input data (parameters). |
| name | type | description |
|-----------------------|----------------------------------------|------------------------------------------------------------------------|
| `txID` | `byte[32]` | Hash of transaction or ID of originating message ID. |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the case of a retried message, this is the tx id from the initial attempt to spend/use the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this scenario (message had no data and just has an amount) there isn't really a concept of retry. The amount would just move along to whatever outputs were specified. A retried message is just the same InputMessage until it is successfully spent.

@Voxelot Voxelot mentioned this pull request Feb 1, 2023
10 tasks
@Voxelot Voxelot removed their request for review February 15, 2023 02:28
@Voxelot
Copy link
Member

Voxelot commented Feb 15, 2023

Updated this design to keep messages separate from coins, while still allowing messages with non-zero dataLength to be retryable

@xgreenx
Copy link
Contributor

xgreenx commented Feb 16, 2023

Honestly, I don't understand why we need MessagePointer. Maybe someone can explain use cases for me=) Because it doesn't participate in the MessageId calculation, so we don't need it for uniqueness. How can it be used in fraud proofs? Right now, we have only MessageRoot in the block, so we will use the MessageId to do checks, not MessagePointer(we don't include messages from the relayer into this root, but maybe we should).

The same question I have for TxPointer. Do we really use it? I tried to find the reason for it, but it seems it is unclear from the commit history. We added TXOPointer in #249. But it seems the reason was that we already added it for the compressed transaction format in #135. And it seems the primary motivation for the compressed transaction format was the issue #111 to make the verification cheaper on the L1 side. But is that issue still actual? Because we still have UtxoId in the InputCoin and this TxPointer, so instead of 32 or 7/6 bytes, we have 32 + 7/6=)

It seems to me like we decided to optimize the verification on the L1 side with the usage of other primitives, but we stopped at some point and kept both variants. Also, maybe we can improve something in the design of fraud proofs after our last updates regarding bridging.

I'm not against the MessagePointer and TxPointer. I only try to understand why we simultaneously need (MessagePointer and MessageId) or (UtxoId and TxPointer).

@pixelcircuits
Copy link
Contributor Author

My understanding is that the "pointers" are supposed to make the fault proofs easier (potentially single step fault proofs). It lets someone directly challenge where the proposed block is claiming its data is from (as opposed to having to challenge for this data from the proposer first). However, even if we can prove fault in a single step in this case, proving fault in VM execution will almost certainly have to be proposer/challenger interactive. So you're right, I'm not sure what we're really gaining...

IMO, we should be optimizing for the optimistic case where faults proofs will never even be initiated. So I'd vote we eventually remove the pointers altogether and save on data. Embracing the optimistic case at the cost of increased fault proof complexity seems to be the winning OR design currently.

MessagePointer was added for several reasons. The first being to just put it on par with how InputCoins have a pointer. The second was to help facilitate fault proofs since the messageId is something the contracts don't know about and is actually computed off-chain. Again, we could just add an interactive fault proof step where the proposer is challenged for this data (technically anyone can respond with the correct data on behalf of the proposer).

I'd vote we drop the MessagePointer on this spec change and eventually drop TxPointer on InputCoins too.

@xgreenx
Copy link
Contributor

xgreenx commented Feb 21, 2023

I also vote for removing it if we can achieve all functionality that we need without it=) Do we need to wait for comments from @Voxelot and @adlerjohn before doing that?

| `predicateData` | `byte[]` | Predicate input data (parameters). |
| name | type | description |
|-----------------------|----------------------------------------|--------------------------------------------------------------|
| `messageID` | `byte[32]` | The messageID as described [here](../id/utxo.md#message-id). |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it was here before, but why must we duplicate the message_id field if we can compute it based on the other fields?

image

I have only one assumption: because we want to put it into the memory of the vm to be able to read it, instead of each time calculation.

But does this optimization worth 32 bytes per input?

@adlerjohn @Voxelot

Copy link
Member

@Voxelot Voxelot Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it does seem like we could omit the message id on inputs, since we have to re-compute the hash to validate it before spending anyways.

I'm also not sure how fraud-proofs would verify the messages. Since we can't lookup old events / logs, unless we put the message id into L1 contract storage I'm not sure which fields are actually needed. cc @pixelcircuits

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we may end up putting the messageIds into storage for simplicity, but it's actually possible to prove emitted events given a starting block hash (you navigate down the chain and provide the necessary merkle proofs and hash preimages as necessary to show the log that occurred). I know Optimism intends to use this approach with Bedrock (essential complicate the fault proving, but optimizing for the optimistic case). This can also potentially be done in a zkProof via something like Herodotus (their main focus is to provide proofs of other chain history, but it could be used to prove Ethereum history on Ethereum itself). It would actually be really cool to use zkProofs like Herodotus to prove output messages in our Fuel chain instead of having a list of output messages always copied into the block header

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pixelcircuits So, what is final conclusion?=) Do we need to keep the message id in the transaction, or it is okay to compute it during fraud proofs because we have access to all fields?

@pixelcircuits
Copy link
Contributor Author

I know it was here before, but why must we duplicate the message_id field if we can compute it based on the other fields?

I think I agree. It's kinda funny because earlier in the comments of this PR, the idea of dropping the nonce on InputMessage was being thrown around. This suggestion is similar but we drop the messageId instead and use the nonce as more of the raw identifier.

I'm also starting to think that we should update MessageOutput too. Right now it just has an amount and recipient field. The amount field is there so that base asset can essentially be burned by the protocol before being withdrawn from the bridge. The recipient field was added because we needed something about the output to get filled so it can only be SMO'd once (since a message with an amount of 0 is valid). I'm thinking we should replace recipient with nonce since that's probably more useful (although it's a bit odd to have it zeroed out because it is technically knowable at time of signing).

I'm also confused about where the actual output message data is getting stored/logged. It's not on the OutputMessage and it's not on the MessageOut receipt. I'm also not sure how these 'OutputMessage`s could survive a regenesis since they're not part of the initial json file.

@Voxelot
Copy link
Member

Voxelot commented Feb 24, 2023

I'm thinking we should replace recipient with nonce since that's probably more useful (although it's a bit odd to have it zeroed out because it is technically knowable at time of signing).

This also seems redundant, as it's storing data that should be derived from the existence of the output on the tx. If all we need is an output for the purpose of burning coins and all the data really comes from the receipt anyways, then we maybe we could reduce the output to only contain an amount? That way we can easily verify sum of inputs >= sum of outputs, and we don't include data unrelated to block validity? Or do we need some malleable (unsigned) field that can completely commit to the message output receipt for fraud proving (i.e. message id)?

@pixelcircuits
Copy link
Contributor Author

If we only have amount, then we can probably tweak the SMO opcode to not even need to touch an OutputMessage if the amount is 0. Are there any problems with this? We would have a MessageOut receipt with no corresponding OutputMessage.

Also, where is the message data stored? The receipt only has a hash of the data. It feels kinda weird for the message data to not be returned after executing a transaction and instead we have to make a separate graphQL query to actually get it. But maybe not since a user will always have to make a separate graphQL to get the message proof anyway.

@xgreenx
Copy link
Contributor

xgreenx commented Feb 26, 2023

Actually, I'm not sure why we need OutputMessage because we can't spend it on the Fuel chain.

image

And we don't process it in the fuel-core, right now. Maybe we should start to check that receipts contain at least the same count of MessageOut as the count of MessageOutput and that fields are the same(but for fields, it is partially useless because the user didn't sign them, and it is a double-check of the fuel-vm execution).

image

When we added this output, it participated in the sum_output calculation:

image

But now, we don't do that(because the user does not sign it, I think), and this output seems partially useless to me. It only helps on the UI side to show that transaction has withdrawals.

image

It may be worth considering removing this output. @adlerjohn

Voxelot
Voxelot previously approved these changes Feb 27, 2023
@Voxelot Voxelot merged commit 433ab79 into master Feb 27, 2023
@Voxelot Voxelot deleted the 439-fuel-improvement-proposal-retryable-input-messages branch February 27, 2023 17:28
xgreenx added a commit that referenced this pull request Mar 13, 2023
This follow-up PR removes the `MessageId` from the receipt. Previously
we removed it from the `Input` in the
#443
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fuel Improvement Proposal: Retryable Input Messages
6 participants