vechain / web3-gear

Proxy Thor's RESTful API to Eth JSON-RPC, to support Remix, Truffle and more.
MIT License
30 stars 17 forks source link

Fix issue #6 #7

Closed plainerman closed 6 years ago

plainerman commented 6 years ago

Fix eth_getFilterChanges to only include new hashes.

uldaman commented 6 years ago

I did this before, but it will lead to a problem. When the client sends a transaction, it will probably be packaged very quickly, then when the client call eth_getFilterChanges, it won't get the block just packed, so I include the current block in the method. Maybe it can be like this:

    def __init__(self, client):
        super(BlockFilter, self).__init__()
        current = client.get_block_number()
        self.current = current - 1 if current > 0 else 0
        self.client = client
plainerman commented 6 years ago

Could you elaborate a bit on this issue and why your proposed code could fix this issue? Especially:

  1. Does only the first block cause an issue? Because in the constructor you would only include the previous block before subscribing.
  2. Why does subscribing to the filter before sending the transaction does not fix this problem?
uldaman commented 6 years ago

You are right, we can solve the problem by subscribing to the filter before sending the transaction. But in Truffle, It seems to be sending the transaction first, so I found this problem.

I will go to see the source code of Truffle carefully.

plainerman commented 6 years ago

But why would this be an issue with vechain but not with ethereum in general? Are the vechain blocks so much faster?

uldaman commented 6 years ago

I am using vechain's --on-demand mode, it immediately packs after receiving a transaction. So if I send a transaction first, then subscribe, I will not get the result of the transaction.

The correct way is to subscribe first, then send the transaction. But I can't assume that the user will do this. So every time I return to the current block (this is also wrong, I think it should be only the first time to return to the current block.).

plainerman commented 6 years ago

Including the first block will only delay the problem. If two blocks are mined nearly instantly (or network delays ...), the transaction won't be included either.

uldaman commented 6 years ago

Yes. So this is the Truffle own problem, when I run truffle test:

INFO - gear.rpc[line:178] - received rpc request - eth_sendTransaction
INFO - gear.rpc[line:229] - received rpc request - eth_newBlockFilter
INFO - gear.rpc[line:241] - received rpc request - eth_getFilterChanges
INFO - gear.rpc[line:201] - received rpc request - eth_getTransactionReceipt
INFO - gear.rpc[line:149] - received rpc request - eth_getCode
INFO - gear.rpc[line:235] - received rpc request - eth_uninstallFilter

Should be subscribed first...

I will think again about any better solution. If you have it, please let me know.

plainerman commented 6 years ago

We could store all transactions from sendTransactionand if a new blockfilter is created, we begin with the block that contained the transaction(s) (if any). Then delete the stored transactions.

This would ensure that:

  1. Transactions are included
  2. Send empty changes if subscribed without a sent transaction
uldaman commented 6 years ago

When creating a filter, there are no parameters, so I don't know which transaction to match.

plainerman commented 6 years ago

Just all transactions? If multiple transactions are sent simultaneously, we cannot guarantee it.

plainerman commented 6 years ago

Or we could map them by the id they send with? This sounds like a good idea.

uldaman commented 6 years ago

The problem is that new blockfilter does not have any additional information, so I can't know which block to begin with.

plainerman commented 6 years ago

Every application should send a unique id with every request. We could then map each eth_sendTransactionwith the id. When a new blockfilter is requested with the same id we include the previous transaction-block matched by the id.

uldaman commented 6 years ago

What you mean is to modify the interface like eth_newBlockFilter(client_id)?

plainerman commented 6 years ago

The web3-gear internal interface, yes. This would not change the user application nor the communication with thor.

plainerman commented 6 years ago

Changing client.py.new_block_filter() ->client.py.new_block_filter(clientId). Because if you look at the Ethereum RPC interface, the id is already sent with every request.

uldaman commented 6 years ago

But If do not change the user interface, how do I get the client_id?

I look at the eth rpc interface, did not see id:

eth_newBlockFilter

Creates a filter in the node, to notify when a new block arrives. To check if the state has changed, call eth_getFilterChanges.

Parameters

None

Returns

QUANTITY - A filter id.
plainerman commented 6 years ago

Look down to the examples listed under every command, there should always be a field "id" added.

New block filter for example:

// Request
curl -X POST --data '{"jsonrpc":"2.0","method":"eth_newBlockFilter","params":[],"id":73}'

// Result
{
  "id":1,
  "jsonrpc":  "2.0",
  "result": "0x1" // 1
}
uldaman commented 6 years ago

This id is not client id, it is to make the response consistent with the request. You can see json-rpc-what-is-the-id-for.

plainerman commented 6 years ago

Coul you test it in truffle if it is the same for a group of requests (i.e. same id for sendTranscationand newBlockFilter)?

uldaman commented 6 years ago

This id can't be obtained in the rpc method, it is used internally by json rpc.

Within the rpc method, only params can be obtained, you can try it.

plainerman commented 6 years ago

I see the problem. I would say we implement the hack with the constructor including the previous block? Maybe with a flag to disable it?

plainerman commented 6 years ago

Or create an issue at truffle itself.

uldaman commented 6 years ago

But this not be an issue with ethereum, because when ethereum receives a transaction, there will be pow, so the transaction will not be packaged immediately.

uldaman commented 6 years ago

I think the current solution is to apply your changes first, but move block num forward one when constructing the BlockFilter.