neo-project / neo

NEO Smart Economy
MIT License
3.47k stars 1.03k forks source link

Ledger.GetTransaction() can't get current transaction #2568

Open RichardBelsum opened 3 years ago

RichardBelsum commented 3 years ago

I used method Ledger.GetTransaction() in the smart contract, and I expect it to return true when pre-execution, and false when I actually execution it.

Contract code

public static bool t1()
{
    return Ledger.GetTransaction(((Transaction)Runtime.ScriptContainer).Hash) == null;
}

Invokefunction(pre-execution) request

{
    "jsonrpc": "2.0",
    "id": 1,
    "method": "invokefunction",
    "params": [
        "0xd2448d18052d5732f7da35d5d31e6ef47e664508",
        "t1",
        [],
        [
            {
                "account": "0x4578060c29f4c03f1e16c84312429d991952c94c",
                "scopes": "CalledByEntry",
                "allowedcontracts": [],
                "allowedgroups": []
            }
        ]
    ]
}

response

{
    "jsonrpc": "2.0",
    "id": 1,
    "result": {
        "script": "wh8MAnQxDBQIRWZ+9G4e09U12vcyVy0FGI1E0kFifVtS",
        "state": "HALT",
        "gasconsumed": "4718760",
        "exception": null,
        "stack": [
            {
                "type": "Boolean",
                "value": true
            }
        ],
        "tx": "ANaI8RGoAEgAAAAAAPjrEQAAAAAAlhoAAAFMyVIZmZ1CEkPIFh4/wPQpDAZ4RQEAIcIfDAJ0MQwUCEVmfvRuHtPVNdr3MlctBRiNRNJBYn1bUgFCDEBB6QBbnLMUY/Vw4MDHdRClOeYy02quh/S3f7PNq5pOud8EkCWIVq+4w39RRbbr5wCpWGhc5IiG2eQRSHFANngqKAwhAnuOhIl9zf58vffm17ad227dYyPL5zLqaN0rPUSoDoDwQVbnsyc="
    }
}

Then I sent the transaction and got the transaction log: (actually execution)

{
    "jsonrpc": "2.0",
    "id": 1,
    "result": {
        "txid": "0x5490875a4a161f457de2de8553af13b089fd36e8092b7fe199d85c13651f5ea0",
        "executions": [
            {
                "trigger": "Application",
                "vmstate": "HALT",
                "exception": null,
                "gasconsumed": "4718760",
                "stack": [
                    {
                        "type": "Boolean",
                        "value": true
                    }
                ],
                "notifications": []
            }
        ]
    }
}

So, Ledger.GetTransaction can't get current transaction?

RichardBelsum commented 3 years ago

I can get the transaction in the previous blocks, such as:

public static bool t2(UInt256 tx)
{
    return Ledger.GetTransaction(tx) == null;
}

image

roman-khimov commented 3 years ago

So, Ledger.GetTransaction can't get current transaction?

Yes. As well as any other transaction from the same block, they're not traceable. https://github.com/neo-project/neo/blob/a4d2eddbee049bbe3d5d9552e11181d294af929e/src/neo/SmartContract/Native/LedgerContract.cs#L54-L59 Which actually allows for some interesting optimizations.

RichardBelsum commented 3 years ago

First, the modification of the Prefix_CurrentBlock value is done in the postPersist phase.

image

Then CurrentIndex actually gets the Index of the previous block image

Then, IsTraceableBlock return false. image

Then, GetTransactionForContract return null. image

Jim8y commented 3 years ago

I think you can find the answer in your code, you were asking your transaction from the Ledger while your transaction was not in the Ledger, it was still persisting.

RichardBelsum commented 3 years ago

So, it's by design, not bug?

Jim8y commented 3 years ago

So, it's by design, not bug?

At least you should not expect to get persisting transactions in a Ledger. The Ledger only contains persisted transactions.

shargon commented 3 years ago

I think you can find the answer in your code, you were asking your transaction from the Ledger while your transaction was not in the Ledger, it was still persisting.

Agree

erikzhang commented 3 years ago
Transaction current_tx = (Transaction)Runtime.ScriptContainer;
RichardBelsum commented 3 years ago

There may be some misunderstanding. I want to use GetTransaction() to determine whether the currently executing smart contract is a pre-execution or actually execution.

RichardBelsum commented 3 years ago

Because the paths of pre-execution and actual execution may not match (1), and now I can't use Gas.Refuel. So I want to increase the pre-execution fee by determining whether the contract is pre-executed or not.

1: e.g.

var index = Storage.Get(Storage.CurrentContainer,"index");

if(index % 2 == 0)
{
    //Some of the codes that consume GAS
}

Storage.Put(Storage.CurrentContainer,"index", index + 1);
RichardBelsum commented 3 years ago

Any updates? I think it's a bug, do you agree?

Jim8y commented 3 years ago

Any updates? I think it's a bug, do you agree?

Why do you think it is a bug?

RichardBelsum commented 3 years ago

Any updates? I think it's a bug, do you agree?

Why do you think it is a bug?

As long as the transaction is on the chain, it should be traceable

Qiao-Jin commented 3 years ago

Because the paths of pre-execution and actual execution may not match

The execution path of actual execution surely can be different from pre-execution and cannot be controlled, depending on balance, constrained condition within script logic, etc. I'm not clear what's its relationship to the feature you want.

RichardBelsum commented 3 years ago

I want to determine in the contract whether the code is being pre-executed, and if so, add some code to increase the fee to avoid the actual execution fee being insufficient @Qiao-Jin

e.g.

if(Ledger.GetTransaction(((Transaction)Runtime.ScriptContainer).Hash) == null)
{
    //only pre-execution
    for(int i = 0; i < 10; i++)
        Storage.Put(Storage.CurrentContext, "temp", "abc");
}

var index = Storage.Get(Storage.CurrentContainer,"index");
if(index % 2 == 0)
{
    //Some of the codes that consume GAS
}
Storage.Put(Storage.CurrentContainer,"index", index + 1);
Jim8y commented 3 years ago
roman-khimov commented 3 years ago

You can add some GAS to system fee when you're creating a transaction, contract itself shouldn't even try to do estimations of this kind. And it shouldn't differentiate between on-chain and test executions in general, otherwise what's use of test execution? It's not just about GAS, I'd also like to see exactly what happens when I do something with the contract, if the contract decides to change its behavior on-chain I'd say this contract is cheating me.

RichardBelsum commented 3 years ago

When you in pre-execution, you are always the first transaction, which is not the case when you actually execute. The person in front of you will modify the storage, causing you to have a different execution path and different fees.

I can write code to add systemfee, but normal users can only call the contract with a wallet(like NeoLine/O3), they don't support adding systemfee

RichardBelsum commented 3 years ago

I could write it that way before, but now I need a new way to replace Gas.Refuel.

Gas.Refuel((Transaction)Runtime.ScriptContainer).Sender, 1000000);

var index = Storage.Get(Storage.CurrentContainer,"index");
if(index % 2 == 0)
{
    //Some of the codes that consume GAS
}
Storage.Put(Storage.CurrentContainer,"index", index + 1);
roman-khimov commented 3 years ago

When you in pre-execution, you are always the first transaction, which is not the case when you actually execute. The person in front of you will modify the storage, causing you to have a different execution path and different fees.

True, but

Think of it the other way around, if your contract always burns additional GAS (not needed for its logic) then users always pay this additional GAS for nothing. They might be upset by this too.

But there was a proposal to return unspent system fee to senders which could technically allow to always safely add some GAS (#1536), maybe that could be something to consider long-term. While for now we have what we have.

RichardBelsum commented 3 years ago

Assuming that pre-execution requires 2GAS and the actual execution has a 20% chance of requiring 2.01GAS, I would rather add 0.01 GAS to each transaction, otherwise I would lose 2GAS because the contract execution would fail due to sufficient fees.

The wallet(Neo-cli/Neo3-GUI/NeoLine/O3/Neon) used by the user can't add systemfee, so I can only add systemfee to the pre-execution of the contract.

Jim8y commented 3 years ago

Assuming that pre-execution requires 2GAS and the actual execution has a 20% chance of requiring 2.01GAS, I would rather add 0.01 GAS to each transaction, otherwise I would lose 2GAS because the contract execution would fail due to sufficient fees.

The wallet(Neo-cli/Neo3-GUI/NeoLine/O3/Neon) used by the user can't add systemfee, so I can only add systemfee to the pre-execution of the contract.

Then you can open an issue for that. I agree with you that wallet should allow users to pay more gas.

erikzhang commented 3 years ago

Add a maxExecFee field for each method in ABI or manifest?

RichardBelsum commented 3 years ago

Add a maxExecFee field for each method in ABI or manifest?

However, the committee can dynamically adjust the fee factor. If so the developer will have to update the contract after each fee change.

roman-khimov commented 3 years ago

Add a maxExecFee field for each method in ABI or manifest?

I doubt it's viable. What is the max cost of GAS transfer?

RichardBelsum commented 3 years ago

Add a maxExecFee field for each method in ABI or manifest?

Such a function, no maxExecFee too.

public static bool Airdrop(BigInteger amount)
{
    for (int i = 0; i < amount; i++)
    {
        ……
        Mint(tokenId, tokenState);
    }
    return true;
}