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

Add transaction index to Env #1077

Merged
merged 2 commits into from
Sep 10, 2021
Merged

Add transaction index to Env #1077

merged 2 commits into from
Sep 10, 2021

Conversation

webmaster128
Copy link
Member

This came up in chat recently. With the transaction index we have a global tx identifier (block height, transaction index) that avoids exposing transaction hash.

Exposing block hash and transaction hash is not desired since people will use those for poor RNGs.

@ethanfrey
Copy link
Member

ethanfrey commented Sep 7, 2021

The problem is that I think the tx index is not exposed in Cosmos SDK.
I do not see it listed here: https://github.com/cosmos/cosmos-sdk/blob/v0.42.9/types/context.go

Maybe @alpe knows more, but we need to make sure it is feasible to support without forking the SDK. sigh

btw, I like the idea... if it is possible. It would help differentiating between multiple tx in the same block when ordering.

@assafmo
Copy link
Contributor

assafmo commented Sep 7, 2021

I love it!

A few notes:

  1. I feel like index is part of the block, as it's the total gas that was used in the current block up until now.
  2. Why is TransactionInfo an Option? When are exec or init called not inside a transaction?

IMO index can just be another field in BlockInfo.

@webmaster128
Copy link
Member Author

The problem is that I think the tx index is not exposed in Cosmos SDK.
I do not see it listed here: https://github.com/cosmos/cosmos-sdk/blob/v0.42.9/types/context.go

Maybe @alpe knows more, but we need to make sure it is feasible to support without forking the SDK. sigh

Looking at https://github.com/CosmWasm/wasmd/blob/v0.18.0/proto/cosmwasm/wasm/v1/types.proto#L123-L131 and https://github.com/CosmWasm/wasmd/blob/v0.18.0/x/wasm/types/types.go#L206-L222 it seems you are right and we do not get a simple counter.

@webmaster128
Copy link
Member Author

Why is TransactionInfo an Option? When are exec or init called not inside a transaction?

Messages do not have to be executed as part of a transaction. They can for example be executed in BeginBlocker or EndBlocker.

@ethanfrey
Copy link
Member

ethanfrey commented Sep 7, 2021

The problem is that I think the tx index is not exposed in Cosmos SDK.
I do not see it listed here: https://github.com/cosmos/cosmos-sdk/blob/v0.42.9/types/context.go
Maybe @alpe knows more, but we need to make sure it is feasible to support without forking the SDK. sigh

Looking at https://github.com/CosmWasm/wasmd/blob/v0.18.0/proto/cosmwasm/wasm/v1/types.proto#L123-L131 and https://github.com/CosmWasm/wasmd/blob/v0.18.0/x/wasm/types/types.go#L206-L222 it seems you are right and we do not get a simple counter.

Good eye. And the use of "GasConsumed" as a monotonic counter is actually quite useful. It also works when 3 messages are called in part of the same transaction. It is actually a quite clever way to order the various calls, and I could see the usage already in eg. snapshotting for voting contracts (with only block height, this is a bit course)

However, it may not provide @assafmo with what he wants to uniquely identify the transaction in the block.

@webmaster128
Copy link
Member Author

Yeah, the gas meter is nice for ordering but not helpful when used together with external tooling. E.g. it would be nice to be able to find a transation (10985258, 0) or (10985258, 3) by simply looking at a tool like this: https://www.mintscan.io/cosmos/blocks/id/10985258

@assafmo
Copy link
Contributor

assafmo commented Sep 8, 2021

However, it may not provide @assafmo with what he wants to uniquely identify the transaction in the block.

@ethanfrey that's even better, we can distinguish between contract calls. I'm not really sure if that's necessary but it's pretty cool.

@alpe
Copy link
Contributor

alpe commented Sep 8, 2021

🤔 I have not found any information about the use case this patch should handle. The "gas based position" would just be an incrementing counter with random steps. Can you not achieve the same in the contract itself with a counter per block?
A submessage sent to the same contract would have a higher gas index while it is in the same TX.
If a TX index is what we really need then I would investigate and try to make this happen.

@webmaster128
Copy link
Member Author

I have not found any information about the use case this patch should handle.

The original motivation is somewhere in a private chat, sorry. The contract should have the option of accurately describing when an event happened. A proposed solution was to expose the transaction hash. If we do that, we have to expose block hash as well for consistentcy. And we know once we expose those hashes, people use them for bad RNGs.

Can you not achieve the same in the contract itself with a counter per block?

The contract can only know how often per block height it was called. This can solve some of the problem but makes it very hard to use in combination with other tooling. Something like the 3rd contract call on height 6484654 barely allows you to find the transaction in a explorer or other client. It also requires costy storage writes.

If a TX index is what we really need then I would investigate and try to make this happen.

It would be amazing if we could implement that.

@webmaster128
Copy link
Member Author

The wasmd counter part is CosmWasm/wasmd#601. Alex says he can make it happen 🧙

@reuvenpo
Copy link
Contributor

reuvenpo commented Sep 9, 2021

Please note that a simple counter that says "this is the third time the contract was called in this block" is problematic for the following reasons:

  • It doesn't let us easily find which tx the call was included in (main motivation)
  • In complex callback chains a contract might be called multiple times in the same tx. This can happen either when contracts call back between each other, or when a contract is called multiple times with different arguments (e.g. sending CW-20 to multiple recipients). Do we want to distinguish those multiple calls? A contract counter would help here but it feels like an incomplete solution. The issue is that if a contract's counter says "5", that still might be in the first tx of the block. CC @assafmo
  • Do queries increment the counter? Should they?

@assafmo
Copy link
Contributor

assafmo commented Sep 9, 2021

Also, I think you should not be concerned by misusing txhash for creating random numbers. Professional devs wouldn't do it, and if a dev is unaware of how problematic it is to get random numbers in blockchain then nothing will help them. 🤷

@webmaster128
Copy link
Member Author

Also, I think you should not be concerned by misusing txhash for creating random numbers. Professional devs wouldn't do it, and if a dev is unaware of how problematic it is to get random numbers in blockchain then nothing will help them. 🤷

I was thinking in that direction as well alredy.

However, for the sake of consistency we should either expose both block hash and tx hash or none of the two.

@assafmo
Copy link
Contributor

assafmo commented Sep 9, 2021

For the sake of consistency we should either expose both block hash and tx hash or none of the two.

Then I vote for exposing both of them.

Call counter - I think it'll just be confusing and ugly in x/wasmd.
Block gas index - Might be interesting, but I don't see the use case for it.

@reuvenpo
Copy link
Contributor

reuvenpo commented Sep 9, 2021

Block gas index - Might be interesting, but I don't see the use case for it.

I was gonna say that it's useful for when the same contract gets called twice in the same TX - then you can imagine a scenario where a contract sends funds to multiple accounts in one tx from its own (or someone else's) balance. But then if all events are registered in an internal event log, even if they are all registered with the same txhash, then you can still see the order of events inside the tx just by the order in which events were registered.

@assafmo
Copy link
Contributor

assafmo commented Sep 9, 2021

Okay, I don't think that's very interesting, but I get it. Also, you can have multiple messages in the same tx, so is the counter resets for every message? I'd argue that you have to add more context for that (E.g. {"msg":0, "call":3}), but again I don't think that's very interesting and the code for it in x/wasmd will be very ugly.

@reuvenpo
Copy link
Contributor

reuvenpo commented Sep 9, 2021

Yeah, I agree with that.
So... what interface do we want, then?
block time + block number + block hash + tx hash?

@alpe
Copy link
Contributor

alpe commented Sep 9, 2021

I am getting confused. The original discussion was about the TX index in Env. Is this still relevant or does this not work for you? A TX can have multiple messages though.

@webmaster128
Copy link
Member Author

block hash + tx hash is a different story and I think we should keep it out of here. Maybe we do it, maybe not. Let's go with tx index for now.

@webmaster128
Copy link
Member Author

Feel free to continue the block hash/tx hash discussion in #1086.

@webmaster128 webmaster128 marked this pull request as ready for review September 10, 2021 12:39
@webmaster128 webmaster128 added this to the 1.0.0 milestone Sep 10, 2021
@webmaster128 webmaster128 merged commit 17976f1 into main Sep 10, 2021
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.

5 participants