Skip to content
This repository has been archived by the owner on Jul 1, 2021. It is now read-only.

Add Eth1Monitor to monitor and manage deposit data #1233

Merged
merged 29 commits into from
Nov 14, 2019

Conversation

mhchia
Copy link
Contributor

@mhchia mhchia commented Oct 25, 2019

What was wrong?

Fixes #709 .

How was it fixed?

Add Eth1Monitor, which keeps polling new logs from the deposit contract and saves them. When a new block latest_block polled through web3, it fetchs the block block_confirmed with block number latest_block.number - num_blocks_confirmed and parses the logs inside. We can be less concerned about forks in eth1 chain with a large enough num_blocks_confirmed selected. Thanks to @NIC619 for coming up with the strategy.

Eth1Monitor is run as a p2p.trio_service.Service, and is tested with lahja.trio.endpoint.TrioEndpoint.

lahja APIs

  • get_eth1_data(distance: int, eth1_voting_period_start_timestamp: Timestamp) -> Eth1Data through the event GetEth1DataRequest.
  • get_deposit(deposit_count: int, deposit_index: int) -> Deposit through the event GetDepositRequest.

To-Do

  • Change Eth1Monitor._deposit_data to a DB?
  • Refine the tests with pytest-trio tricks.

Cute Animal Picture

put a cute animal picture link inside the parentheses


@pytest.fixture("session")
def contract_json():
_contract_json = '{"abi": [{"name": "DepositEvent", "inputs": [{"type": "bytes", "name": "pubkey", "indexed": false}, {"type": "bytes", "name": "withdrawal_credentials", "indexed": false}, {"type": "bytes", "name": "amount", "indexed": false}, {"type": "bytes", "name": "signature", "indexed": false}, {"type": "bytes", "name": "index", "indexed": false}], "anonymous": false, "type": "event"}, {"outputs": [], "inputs": [], "constant": false, "payable": false, "type": "constructor"}, {"name": "get_deposit_root", "outputs": [{"type": "bytes32", "name": "out"}], "inputs": [], "constant": true, "payable": false, "type": "function", "gas": 95389}, {"name": "get_deposit_count", "outputs": [{"type": "bytes", "name": "out"}], "inputs": [], "constant": true, "payable": false, "type": "function", "gas": 17683}, {"name": "deposit", "outputs": [], "inputs": [{"type": "bytes", "name": "pubkey"}, {"type": "bytes", "name": "withdrawal_credentials"}, {"type": "bytes", "name": "signature"}, {"type": "bytes32", "name": "deposit_data_root"}], "constant": false, "payable": true, "type": "function", "gas": 1754607}], "bytecode": "0x740100000000000000000000000000000000000000006020526f7fffffffffffffffffffffffffffffff6040527fffffffffffffffffffffffffffffffff8000000000000000000000000000000060605274012a05f1fffffffffffffffffffffffffdabf41c006080527ffffffffffffffffffffffffed5fa0e000000000000000000000000000000000060a052341561009857600080fd5b6101406000601f818352015b600061014051602081106100b757600080fd5b600260c052602060c020015460208261016001015260208101905061014051602081106100e357600080fd5b600260c052602060c020015460208261016001015260208101905080610160526101609050602060c0825160208401600060025af161012157600080fd5b60c0519050606051600161014051018060405190131561014057600080fd5b809190121561014e57600080fd5b6020811061015b57600080fd5b600260c052602060c02001555b81516001018083528114156100a4575b50506111d656600035601c52740100000000000000000000000000000000000000006020526f7fffffffffffffffffffffffffffffff6040527fffffffffffffffffffffffffffffffff8000000000000000000000000000000060605274012a05f1fffffffffffffffffffffffffdabf41c006080527ffffffffffffffffffffffffed5fa0e000000000000000000000000000000000060a052600015610265575b6101605261014052600061018052610140516101a0526101c060006008818352015b61018051600860008112156100da578060000360020a82046100e1565b8060020a82025b905090506101805260ff6101a051166101e052610180516101e0516101805101101561010c57600080fd5b6101e0516101805101610180526101a0517ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff86000811215610155578060000360020a820461015c565b8060020a82025b905090506101a0525b81516001018083528114156100bd575b50506018600860208206610200016020828401111561019357600080fd5b60208061022082610180600060046015f15050818152809050905090508051602001806102c0828460006004600a8704601201f16101d057600080fd5b50506103206102c0516020818352015b60206103205111156101f15761020d565b6000610320516102e001535b81516001018083528114156101e0575b505060206102a05260406102c0510160206001820306601f8201039050610280525b6000610280511115156102415761025d565b602061028051036102a00151602061028051036102805261022f565b610160515650005b63c5f2892f60005114156104f757341561027e57600080fd5b6000610140526101405161016052600154610180526101a060006020818352015b60016001610180511614156103205760006101a051602081106102c157600080fd5b600060c052602060c02001546020826102400101526020810190506101605160208261024001015260208101905080610240526102409050602060c0825160208401600060025af161031257600080fd5b60c05190506101605261038e565b6000610160516020826101c00101526020810190506101a0516020811061034657600080fd5b600260c052602060c02001546020826101c0010152602081019050806101c0526101c09050602060c0825160208401600060025af161038457600080fd5b60c0519050610160525b610180600261039c57600080fd5b60028151048152505b815160010180835281141561029f575b505060006101605160208261046001015260208101905061014051610160516101805163806732896102e05260015461030052610300516006580161009b565b506103605260006103c0525b6103605160206001820306601f82010390506103c0511015156104235761043c565b6103c05161038001526103c0516020016103c052610401565b61018052610160526101405261036060088060208461046001018260208501600060046012f150508051820191505060006018602082066103e0016020828401111561048757600080fd5b60208061040082610140600060046015f150508181528090509050905060188060208461046001018260208501600060046014f150508051820191505080610460526104609050602060c0825160208401600060025af16104e757600080fd5b60c051905060005260206000f350005b63621fd13060005114156105f857341561051057600080fd5b63806732896101405260015461016052610160516006580161009b565b506101c0526000610220525b6101c05160206001820306601f82010390506102205110151561055b57610574565b610220516101e001526102205160200161022052610539565b6101c0805160200180610280828460006004600a8704601201f161059757600080fd5b50506102e0610280516020818352015b60206102e05111156105b8576105d4565b60006102e0516102a001535b81516001018083528114156105a7575b50506020610260526040610280510160206001820306601f8201039050610260f350005b6322895118600051141561105157605060043560040161014037603060043560040135111561062657600080fd5b60406024356004016101c037602060243560040135111561064657600080fd5b608060443560040161022037606060443560040135111561066657600080fd5b63ffffffff6001541061067857600080fd5b633b9aca006102e0526102e05161068e57600080fd5b6102e05134046102c052633b9aca006102c05110156106ac57600080fd5b603061014051146106bc57600080fd5b60206101c051146106cc57600080fd5b606061022051146106dc57600080fd5b610140610360525b61036051516020610360510161036052610360610360511015610706576106e4565b6380673289610380526102c0516103a0526103a0516006580161009b565b50610400526000610460525b6104005160206001820306601f8201039050610460511015156107525761076b565b6104605161042001526104605160200161046052610730565b610340610360525b610360515260206103605103610360526101406103605110151561079657610773565b610400805160200180610300828460006004600a8704601201f16107b957600080fd5b5050610140610480525b610480515160206104805101610480526104806104805110156107e5576107c3565b63806732896104a0526001546104c0526104c0516006580161009b565b50610520526000610580525b6105205160206001820306601f82010390506105805110151561083057610849565b610580516105400152610580516020016105805261080e565b610460610480525b610480515260206104805103610480526101406104805110151561087457610851565b6105208051602001806105a0828460006004600a8704601201f161089757600080fd5b505060a06106205261062051610660526101408051602001806106205161066001828460006004600a8704601201f16108cf57600080fd5b5050610600610620516106600151610240818352015b6102406106005111156108f757610918565b600061060051610620516106800101535b81516001018083528114156108e5575b5050602061062051610660015160206001820306601f82010390506106205101016106205261062051610680526101c08051602001806106205161066001828460006004600a8704601201f161096d57600080fd5b5050610600610620516106600151610240818352015b610240610600511115610995576109b6565b600061060051610620516106800101535b8151600101808352811415610983575b5050602061062051610660015160206001820306601f820103905061062051010161062052610620516106a0526103008051602001806106205161066001828460006004600a8704601201f1610a0b57600080fd5b5050610600610620516106600151610240818352015b610240610600511115610a3357610a54565b600061060051610620516106800101535b8151600101808352811415610a21575b5050602061062051610660015160206001820306601f820103905061062051010161062052610620516106c0526102208051602001806106205161066001828460006004600a8704601201f1610aa957600080fd5b5050610600610620516106600151610240818352015b610240610600511115610ad157610af2565b600061060051610620516106800101535b8151600101808352811415610abf575b5050602061062051610660015160206001820306601f820103905061062051010161062052610620516106e0526105a08051602001806106205161066001828460006004600a8704601201f1610b4757600080fd5b5050610600610620516106600151610240818352015b610240610600511115610b6f57610b90565b600061060051610620516106800101535b8151600101808352811415610b5d575b5050602061062051610660015160206001820306601f8201039050610620510101610620527f649bbc62d0e31342afea4e5cd82d4049e7e1ee912fc0889aa790803be39038c561062051610660a160006107005260006101406030806020846107c001018260208501600060046016f150508051820191505060006010602082066107400160208284011115610c2557600080fd5b60208061076082610700600060046015f15050818152809050905090506010806020846107c001018260208501600060046013f1505080518201915050806107c0526107c09050602060c0825160208401600060025af1610c8557600080fd5b60c0519050610720526000600060406020820661086001610220518284011115610cae57600080fd5b606080610880826020602088068803016102200160006004601bf1505081815280905090509050602060c0825160208401600060025af1610cee57600080fd5b60c0519050602082610a600101526020810190506000604060206020820661092001610220518284011115610d2257600080fd5b606080610940826020602088068803016102200160006004601bf15050818152809050905090506020806020846109e001018260208501600060046015f1505080518201915050610700516020826109e0010152602081019050806109e0526109e09050602060c0825160208401600060025af1610d9f57600080fd5b60c0519050602082610a6001015260208101905080610a6052610a609050602060c0825160208401600060025af1610dd657600080fd5b60c0519050610840526000600061072051602082610b000101526020810190506101c0602080602084610b0001018260208501600060046015f150508051820191505080610b0052610b009050602060c0825160208401600060025af1610e3c57600080fd5b60c0519050602082610c800101526020810190506000610300600880602084610c0001018260208501600060046012f15050805182019150506000601860208206610b800160208284011115610e9157600080fd5b602080610ba082610700600060046015f1505081815280905090509050601880602084610c0001018260208501600060046014f150508051820191505061084051602082610c0001015260208101905080610c0052610c009050602060c0825160208401600060025af1610f0457600080fd5b60c0519050602082610c8001015260208101905080610c8052610c809050602060c0825160208401600060025af1610f3b57600080fd5b60c0519050610ae052606435610ae05114610f5557600080fd5b6001805460018254011015610f6957600080fd5b6001815401815550600154610d0052610d2060006020818352015b60016001610d0051161415610fb957610ae051610d205160208110610fa857600080fd5b600060c052602060c020015561104d565b6000610d205160208110610fcc57600080fd5b600060c052602060c0200154602082610d40010152602081019050610ae051602082610d4001015260208101905080610d4052610d409050602060c0825160208401600060025af161101d57600080fd5b60c0519050610ae052610d00600261103457600080fd5b60028151048152505b8151600101808352811415610f84575b5050005b60006000fd5b61017f6111d60361017f60003961017f6111d6036000f3"}' # noqa: E501
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@njgheorghita can you package this up as an ethpm package? Deposit contract for eth2.0 seems like a nice asset to have laying around.

Copy link
Contributor

@njgheorghita njgheorghita Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Package is available here or in its raw format. Do you think it would be useful to create an eth2 registry and publish it there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Published to a registry seems like a good thing. Maybe most valuable thing would be to make a gist that explains how to install the CLI and then how to use that to install the contracts locally to disk. This would be aimed at the eth 2.x client devs and showing them how to access the contract assets via ethpm tooling.

self._deposit_contract = w3.eth.contract(
address=contract_address, abi=contract_abi
)
self._log_filter = self._deposit_contract.events.DepositEvent.createFilter(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to 👎 using createFilter. I'd advocate for using a getLogs based approach instead and manually managing the fromBlock/toBlock. This removes dependency on statefulness on the RPC-node in favor of making "stateless" requests and managing the state locall (the state being which block ranges we've already queried)

Copy link
Contributor Author

@mhchia mhchia Oct 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. May I understand that we should also avoid using block filters, and maintain the queried blockhashs on our side like LogHandler in py-evm?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhchia yeah, roughly that. Basically, I don't want to trust the RPC node we're connected to to properly maintain the filter (and in the event that they forget/lose it, we get left in an awkward state of needing to re-install it but with a new fromBlock, which is effectively the same amount of work to keep track of as just manually maintain block ranges ourselves)

@mhchia mhchia marked this pull request as ready for review November 8, 2019 09:09
@mhchia mhchia requested a review from NIC619 November 8, 2019 09:12
@mhchia
Copy link
Contributor Author

mhchia commented Nov 8, 2019

IMO it's more ready now. Though there are 800+ lines, the main logics are in eth1_monitor.py. I can separate them into smaller PRs if it is more helpful for review.

Further optimization can be done later, including storing deposit_data in the database, caching for trees, etc. Big thanks to @NIC619 for clarifying a lot of the requirements and helping out to carve the design.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

everything seems well-factored so while there are a couple things going on here, it is pretty straightforward to pull them apart....

the main thing here is tuning num_blocks_confirmed so that we avoid re-orgs but still run far ahead enough of how far we may advance per eth1 voting period so that we never block deposit generation on having to suddenly read a bunch of contract logs...

this parameter will likely be network dependent but it seems ok for us to do.

the alternative here is that we try to detect re-orgs and update our local state accordingly -- while this would be the safer approach, it will complicate the code. we could make an issue to track "improving the eth1 monitor resistance to re-orgs"

i left some minor feedback and a suggestion to make the fetching of Eth1Data more stateless to consider. some of the comments are just noting things we will want to do in the future -- we can move ahead w/ merging this if we move those observations to issues to track them moving forward.

Comment on lines 20 to 21
def test_deploy(w3, registration_contract):
pass

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this doing here?

Comment on lines 231 to 232
event = self._deposit_contract.events.DepositEvent
event_abi = event._get_event_abi()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these can be read once during __init__ and cached for future use, correct?

Comment on lines 207 to 212
if parent_block_number not in self._block_number_to_accumulated_deposit_count:
self._block_number_to_accumulated_deposit_count[block.number] = 0
else:
self._block_number_to_accumulated_deposit_count[
block.number
] = self._block_number_to_accumulated_deposit_count[parent_block_number]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an alternative:

self._block_number_to_accumulated_deposit_count[block.number] = self._block_number_to_accumulated_deposit_count.get(parent_block_number, 0)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although also see below -- we may be able to remove this...?

Comment on lines 67 to 75
def _make_deposit_tree_and_root(
list_deposit_data: Sequence[DepositData]
) -> Tuple[MerkleTree, Hash32]:
deposit_data_leaves = [data.hash_tree_root for data in list_deposit_data]
length_mix_in = len(list_deposit_data).to_bytes(32, byteorder="little")
tree = calc_merkle_tree_from_leaves(deposit_data_leaves)
tree_root = get_root(tree)
tree_root_with_mix_in = hash_eth2(tree_root + length_mix_in)
return tree, tree_root_with_mix_in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will almost certainly want to cache tree construction (with some nice bound to avoid memory exhaustion)

Comment on lines 333 to 359
accumulated_deposit_count = self._block_number_to_accumulated_deposit_count[
target_block_number
]
if accumulated_deposit_count == 0:
raise Eth1MonitorValidationError(
f"failed to make `Eth1Data`: `deposit_count = 0` at block #{target_block_number}"
)
_, deposit_root = _make_deposit_tree_and_root(
self._deposit_data[:accumulated_deposit_count]
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option is to get the root and count from the contract once we have determined target_block_number. we could also query via web3.py in this function to get the block_hash, rather than maintaining it ourselves.

assuming this is readily available to us w/ web3.py (i believe it is) then i'd think it is in fact safer in the case that we have a bug in our code. having a bug in this code implies Deposit creation would fail but Eth1Data creation could succeed. this could be relevant to a validator in the case that there have been no new additions to the deposit tree but we do come to consensus on a new block hash.

in this case, we can remove the code maintaining the self._block_number_to_hash and the self._block_number_to_accumulated_deposit_count it seems...

what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading at the time of request like this also makes this part of this component insensitive to re-orgs because you are just following the chain according to our web3 handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also query via web3.py in this function to get the block_hash, rather than maintaining it ourselves.

Do you mean call with block_identifier? That looks pretty promising. If it works well, indeed we can remove self._block_number_to_accumulated_deposit_count. I'm trying that now. Big thanks for the suggestions!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we need to go thru call...

once we have the right block number, we can just call this: https://web3py.readthedocs.io/en/stable/web3.eth.html#web3.eth.Eth.getBlock

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the thing you linked is interesting.... seems like we can get the contract state at a certain block that way -- i'll let you resolve the details but i would first just considering the overall approach here to just reading the state on-demand. like i said above, i think this is the "easier to get correct, more often" approach :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • @mhchia am I correct that this PR assumes that the eth1 chain is (kind of) finalized after num_blocks_confirmed blocks?
  • I recalled that in order to fix reorg, we have to know which block number is the reorg point so we can query the deposit data on the new canonical chain. So we still need to maintain something like _block_number_to_hash to know where should it be?

Copy link
Contributor Author

@mhchia mhchia Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ralexstokes

Thanks for the suggestion!

I am removing _block_number_to_hash because we can get the hash of a block at the number through web3.

For _block_number_to_accumulated_deposit_count, currently, I see two ways I can get the accumulated deposit count:

  1. Query the deposit contract through web3, by call(block_identifier=target_block_number).
  2. Get every block before and including the block target_block_number through getBlock, parse the transactions inside and get the resulting logs, and then aggregate them.

Is either the way what you are talking about? If not, could you elaborate more on this?
Now I lean more to do it through (1) i.e. call(block_identifier=target_block_number) since it's simple and explicit.

Thank you 🙏


_event_bus: EndpointAPI

# TODO: Store deposit data in DB?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, whatever state we end up keeping in this component, we will definitely want to persist to some database to avoid having to re-read the chain every time we boot

`eth1_block.number - distance`. Therefore, we can return `Eth1Data` according to the
information of this block.
"""
eth1_voting_period_start_block_number = self._get_closest_eth1_voting_period_start_block(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably want to cache these start block numbers for a given eth1_voting_period_start_timestamp... again a re-org means we have to invalidate everything...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that the cache is not hit frequently given that the queried timestamps are easily different? For example, they have some small differences and are mapped to the same start_block_number, but they are different keys to _get_closest_eth1_voting_period_start_block.

Copy link
Contributor

@NIC619 NIC619 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Mostly just nits.

trinity/components/eth2/eth1_monitor/eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
tests-trio/eth1-monitor/test_eth1_monitor.py Outdated Show resolved Hide resolved
@@ -65,6 +65,7 @@
"pytest-randomly==3.0.0",
# only for eth2
"ruamel.yaml==0.15.98",
"eth-tester==0.2.0b3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that eth-tester is binding the certain py-evm version, so adding this dependency means we will have to update and use eth-tester of the corresponding Trinity PYEVM_DEPENDENCY in the future. That is to say, to prevent CI from crashing, when we bump py-evm version in Trinity, we will have to bump eth-tester as well.

trinity/components/eth2/eth1_monitor/eth1_monitor.py Outdated Show resolved Hide resolved
trinity/components/eth2/eth1_monitor/eth1_monitor.py Outdated Show resolved Hide resolved
trinity/components/eth2/eth1_monitor/eth1_monitor.py Outdated Show resolved Hide resolved

# TODO: Store deposit data in DB?
# Deposit data parsed from the logs we received. The order is from the oldest to the latest.
_deposit_data: List[DepositData]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite it's better to be stored with DB, it seems better to handle the sequence with Tuple when building the Merkle tree.

Comment on lines 333 to 359
accumulated_deposit_count = self._block_number_to_accumulated_deposit_count[
target_block_number
]
if accumulated_deposit_count == 0:
raise Eth1MonitorValidationError(
f"failed to make `Eth1Data`: `deposit_count = 0` at block #{target_block_number}"
)
_, deposit_root = _make_deposit_tree_and_root(
self._deposit_data[:accumulated_deposit_count]
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • @mhchia am I correct that this PR assumes that the eth1 chain is (kind of) finalized after num_blocks_confirmed blocks?
  • I recalled that in order to fix reorg, we have to know which block number is the reorg point so we can query the deposit data on the new canonical chain. So we still need to maintain something like _block_number_to_hash to know where should it be?

@mhchia
Copy link
Contributor Author

mhchia commented Nov 12, 2019

@hwwhww

am I correct that this PR assumes that the eth1 chain is (kind of) finalized after num_blocks_confirmed blocks?

Yes, it is what we assume.

I recalled that in order to fix reorg, we have to know which block number is the reorg point so we can query the deposit data on the new canonical chain. So we still need to maintain something like _block_number_to_hash to know where should it be?

I still use _block_number_to_hash just for caching the blocks we handled, and in the worst case even we are delayed num_blocks_confirmed blocks and there is a fork happening, we can find it out by learning the fact _block_number_to_hash is not consistent with the record in eth1 node.
If we want to recover from a re-org, we need to know the block number where reorg happened. We need to keep all recent blockhashes to find out the fork point, like this. However, does it seem too late for Eth1Monitor to recover from a fork after num_blocks_confirmed? It indicates "from a fork happening and before we detecting it Eth1Monitor are returning false Eth1Data". It seems to me that waiting for a proper num_blocks_confirmed is easier. Still, if it is deemed necessary I can add the fork recovery mechanism.

mhchia and others added 19 commits November 13, 2019 14:32
Add subscribe function in Eth1Monitor service

Change to new design: temporary commit
Query logs `blocks_delayed_to_query_logs` blocks later, to avoid forks.
And remove unused exceptions.
Because `Eth1Monitor` implementation is bond with the contract, it seems
it's better to let them stay together.
Since we assume there is no fork with a proper `num_blocks_confirmed`
set, we shouldn't be worried about hashes.
Co-Authored-By: NIC Lin <twedusuck@gmail.com>
Co-Authored-By: NIC Lin <twedusuck@gmail.com>
Co-Authored-By: NIC Lin <twedusuck@gmail.com>
- Make graphs more tidy.
- Fix unclear comments.
- Fix variable names.
- Remove useless tests.
- Move `make_deposit_proof` and `make_deposit_tree_and_root` to
`validator.py`.
- Refactor `create_mock_deposits_and_root` through `make_deposit_proof`
and `make_deposit_tree_and_root`.
- Change typing.
Previously, we calculate the accumulated deposit count through
maintaining a dict from `block_number` to `accumulated_deposit_count`.
This data corrupts when there is a fork. The solution is to directly
call `get_deposit_count` to the deposit contract at the `block_number`.
This is supported by `web3.py`.
@mhchia
Copy link
Contributor Author

mhchia commented Nov 13, 2019

@hwwhww @ralexstokes @NIC619

Really appreciate your reviews!

After your reviews, I've changed several things according to the feedback:

  • Cleaned up(including several nits, rephrasing comments, and cleaning leftover code).
  • Removed _block_number_to_accumulated_deposit_count and get it through calling get_deposit_count of the deposit contract.
  • Handled errors when processing requests and propagate them back to the requester through lahja.
  • Reused make_deposit_proof and make_deposit_tree_and_root, and refactor create_mock_deposits_and_root.

There are things that haven't been done but I lean more towards putting them in a separate PR or issue:

  • Put _deposit_data in DB.
  • Cache the trees according to deposit_count.

Please add if there is anything missing 🙏, and it will be great to have your second review on the changes. I am starting on another PR to solve the issue "Put _deposit_data in DB".

@mhchia mhchia mentioned this pull request Nov 13, 2019
2 tasks
# mainchain forks.
# We always get a `block` and parse the logs from it, where
# `block.number <= latest_block.number - _num_blocks_confirmed`.
_num_blocks_confirmed: BlockNumber
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think num_blocks_confirmed is not BlockNumber, i.e., 5 blocks != block 5. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. I've changed that. Thank you for the review.

trinity/components/eth2/eth1_monitor/exceptions.py Outdated Show resolved Hide resolved
trinity/components/eth2/eth1_monitor/exceptions.py Outdated Show resolved Hide resolved
trinity/components/eth2/eth1_monitor/exceptions.py Outdated Show resolved Hide resolved
mhchia and others added 4 commits November 14, 2019 14:36
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
Co-Authored-By: Hsiao-Wei Wang <hwwang156@gmail.com>
@mhchia
Copy link
Contributor Author

mhchia commented Nov 14, 2019

I'm merging this and proceed TODOs in #1270 .

@mhchia mhchia merged commit d3af47d into ethereum:master Nov 14, 2019
@mhchia mhchia deleted the feature/add-eth1-monitor branch November 14, 2019 08:56
@cburgdorf cburgdorf mentioned this pull request Nov 14, 2019
2 tasks
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.

Add eth1 service to monitor eth1 deposit contract
6 participants