Skip to content
This repository has been archived by the owner on Apr 23, 2024. It is now read-only.

Fix issue #6 #7

Merged
merged 3 commits into from
Jul 17, 2018
Merged

Fix issue #6 #7

merged 3 commits into from
Jul 17, 2018

Conversation

plainerman
Copy link
Contributor

Fix eth_getFilterChanges to only include new hashes.

@plainerman plainerman changed the title Close issue #6 Fix issue #6 Jul 14, 2018
@uldaman
Copy link
Contributor

uldaman commented Jul 16, 2018

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

@uldaman uldaman requested review from uldaman and removed request for uldaman July 16, 2018 05:56
@uldaman uldaman closed this in 91d312e Jul 16, 2018
@uldaman uldaman reopened this Jul 16, 2018
@uldaman uldaman mentioned this pull request Jul 16, 2018
@plainerman
Copy link
Contributor Author

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
Copy link
Contributor

uldaman commented Jul 16, 2018

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
Copy link
Contributor Author

plainerman commented Jul 16, 2018

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

@uldaman
Copy link
Contributor

uldaman commented Jul 16, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

uldaman commented Jul 16, 2018

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
Copy link
Contributor Author

plainerman commented Jul 16, 2018

We could store all transactions from sendTransaction and 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
Copy link
Contributor

uldaman commented Jul 16, 2018

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

@plainerman
Copy link
Contributor Author

plainerman commented Jul 16, 2018

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

@plainerman
Copy link
Contributor Author

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

@uldaman
Copy link
Contributor

uldaman commented Jul 16, 2018

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

@plainerman
Copy link
Contributor Author

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

@uldaman
Copy link
Contributor

uldaman commented Jul 16, 2018

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

@plainerman
Copy link
Contributor Author

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

@plainerman
Copy link
Contributor Author

plainerman commented Jul 16, 2018

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
Copy link
Contributor

uldaman commented Jul 16, 2018

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
Copy link
Contributor Author

plainerman commented Jul 16, 2018

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
Copy link
Contributor

uldaman commented Jul 16, 2018

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
Copy link
Contributor Author

plainerman commented Jul 16, 2018

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

@uldaman
Copy link
Contributor

uldaman commented Jul 16, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Or create an issue at truffle itself.

@uldaman
Copy link
Contributor

uldaman commented Jul 16, 2018

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
Copy link
Contributor

uldaman commented Jul 16, 2018

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

@uldaman uldaman merged commit a3d97f1 into vechain:master Jul 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants