search

hashgraph / hedera-services

Crypto, token, consensus, file, and smart contract services for the Hedera public ledger
Apache License 2.0
320 stars 140 forks source link

ContractCallLocalQuery does not handle long return values effectively #3759

Closed shemnon closed 2 years ago

shemnon commented 2 years ago

Description

When submitting a ContractCallLocalQuery against a contract that will return (via return or revert) more than 32 bytes the query will fail with INSUFFICIENT_TX_FEE unless an explicit setQueryPayment call is made via the SDK.

The root of the problem comes in the query cost estimation logic. A fake query is created that has a response length of 32 btytes, set via the dynamic system property contracts.localCall.estRetBytes. Any contract call with an answer of more bytes will cost more than the cost estimate.

The end user can explicitly set a query payment, but that will often result in overpayment and occasionally insufficient fees depending on the movement of the exchange rage.

A quick fix is to change the system property contracts.localCall.estRetBytes to a larger number, but not too large as it impacts all users.

A longer term fix would be to allow the user to suggest how long the response will be (most ABIs will be able to handle this). We could re-use the deprecated maxResultSize to set the estimate if provided, and fall back on the system property.

There is a potential DOS vector, in that if the query has insufficient fees the fees that are offered are not charged to the user despite the fact the local query was executed in its entirety before the error was thrown. The offered query fee should always be consumed when an error occurs after execution of the query.

Steps to reproduce

See internal slack discussions, searching for contracts.localCall.estRetBytes

Additional context

discord discussion - https://discord.com/channels/373889138199494658/616725732650909710/1004411362656661637

Hedera network

mainnet, testnet, previewnet, other

Version

0.28.x, 0.27x, 0.26x

Operating system

No response

nickpoorman commented 2 years ago

Looping in @tinker-michaelj so that we can begin the discussion about a longer-term solution to this problem. In speaking with @shemnon, for an immediate fix we will have DevOps change the dynamic property on the nodes.

dalvizu commented 2 years ago

This was addressed in https://github.com/swirlds/infrastructure/issues/2922