Use L1 block number for Ticket Parameters and Round Initialization

[leszko]

What does this pull request do? Explain your changes. (required)
Use L1 block numbers during Round Initialization and in Ticket Params.

Important comments:

• The L1 block numbers are not stored in DB and therefore need to be enriched with L1 Block Number using Arbitrum RPC API; an alternative approach would be to store L1 Block Numbers in DB:
  • it would mean fewer calls to Aribtrum RPC API
  • it would be a backward-incompatible change
  • I think enriching blocks on-the-fly is good enough
• TimeWatcher now has a function LastSeenL1Block(), but I found it does not need to have LastSeenBlock() anymore
• Having now both blocks and l1Blocks in the code can be super-confusing, I tried to rename all the necessary parts of the code, so that block always refers to L2 Block, however, there are still a places for which we may need an additional renaming in the future:
  • TicketParams
  • lp_rpc.proto
  • Eth contracts or/and contract bindings
• There is one place where we could think about optimizing more, which is to try to fetch L1 Number while fetching logs in this part), but it doesn't seem to be an issue at the moment.

Specific updates (required)

• Add L1 Block Number while fetching blocks by Block Watcher from ETH API
• Use L1 Block Number in Time Watcher (and propagate it to Ticket Sender and Recipient)
• Rename all block into l1Block when it refers to L1 Blocks
• Refactor Block Watcher Client (reuse ETH API code)

How did you test each of these updates (required)

Tests on Arbitrum:

1. Added debug logs to Round Initializer to check that it uses L1 block numbers
2. Add debug logs to ticket params to check that that sender and receiver use L1 block numbers for ticket params

Does this pull request close any open issues?

fix #2220

Checklist:

• Read the contribution guide
• make runs successfully
• All tests in ./test.sh pass
• README and other documentation updated
• Pending changelog updated

[yondonfu]

Just a few minor requests.

The L1 block numbers are not stored in DB and therefore need to be enriched with L1 Block Number using Arbitrum RPC API; an alternative approach would be to store L1 Block Numbers in DB:

I was slightly concerned about this because the # of RPC requests from the node is already going to be higher when connected to Arbitrum Rinkeby/mainnet due to the higher frequency of blocks. If the request count increases significantly that can affect the costs of using certain ETH JSON-RPC providers - for example, Infura's pricing tiers are based on the # of RPC requests sent per day. However, I see that extra RPC requests are only added by enriching the MiniHeaders with the L1 block number when:

• We batch request logs when backfilling and there would be an additional RPC request per log. However, since the # of logs will be determined by the # of relevant logs found with the topics that the node cares about (this is what FilterLogs does by setting specific topics as parameters), if there are very few logs that the node cares about there would be very few additional RPC requests. I suspect this will be common right now
• In buildCanonicalChain(). I don't completely grok the # of additional RPC requests that would be required there. Would be good to get a sense of this

Generally, I think your point that this may be okay for now is reasonable, but I do think we should get a sense of the additional RPC requests for that last point that result from this enrichment process.

[leszko]

Good points @yondonfu!

I made one improvement in terms of the number of RPC calls here 1382f59 I realized that we actually don't need to add L1 Block Number to every block we process, it's enough to add it into the block with the highest number. It makes the code a little more tricky, but on the other hand, we minimize the number of RPC calls we make. Let me know your thoughts.

With regards to your concerns.

• We batch request logs when backfilling and there would be an additional RPC request per log. However, since the # of logs will be determined by the # of relevant logs found with the topics that the node cares about (this is what FilterLogs does by setting specific topics as parameters), if there are very few logs that the node cares about there would be very few additional RPC requests. I suspect this will be common right now

Right, especially after this change 1382f59, I think this part is not an issue.

• In buildCanonicalChain(). I don't completely grok the # of additional RPC requests that would be required there. Would be good to get a sense of this

My understanding is that when the next block number is the child of the current block, then buildCanonicalChain() does not execute recursively and finishes here. Maybe I miss something, but I actually never encountered a case when it would reach the point of enrichWithL1BlockNumber().

[yondonfu]

it's enough to add it into the block with the highest number. It makes the code a little more tricky, but on the other hand, we minimize the number of RPC calls we make.

Because the block with the highest number is the latest block that we've received from polling and all other blocks we should've seen already? So, I think this should actually generally be true when using Arbitrum since re-orgs [1] shouldn't be a thing while Arbitrum is using a sequencer so we wouldn't expect buildCanonicalChain() to return a slice of events of length greater than 1 (i.e. should just contain an event for the latest block). Thus, I think your solution works for Arbitrum and since that is the focus and reason for fetching L1 block numbers in the first place. A easy way to still maintain compatibility with L1 may be to set the L1 block number on all the events in enrichL1() using the L1 block number fetched for the newest block since if the node is connected to L1 each event should have BlockNumber == L1BlockNumber.

[1] i.e. When certain blocks that have already occurred are discarded and new blocks take their place - transactions in the previous blocks generally end up eventually being included in the new blocks. This happens from time to time on L1 Ethereum.

Maybe I miss something, but I actually never encountered a case when it would reach the point of enrichWithL1BlockNumber().

See above point about re-orgs being a case to handle (which is why buildCanonicalChain() is implemented in the first place) on L1 Ethereum, but not being a problem with Arbitrum (at least that I am aware of).

[leszko]

[1] i.e. When certain blocks that have already occurred are discarded and new blocks take their place - transactions in the previous blocks generally end up eventually being included in the new blocks. This happens from time to time on L1 Ethereum.

Ahh, I missed this information. So, I start to understand the idea behind buildCanonicalChain() and the reason why I never actually encountered its recursive call. @yondonfu thanks for the explanation!

it's enough to add it into the block with the highest number. It makes the code a little more tricky, but on the other hand, we minimize the number of RPC calls we make.

Because the block with the highest number is the latest block that we've received from polling and all other blocks we should've seen already?

Because we're not interested in the L1 block number of all other blocks. Timewatcher is the only entity that actually reads the L1 block number and Timewatcher is only interested in the highest L1 block number as it checks:

last := tw.LastSeenL1Block()
new := event.BlockHeader.L1BlockNumber
if new != nil && (last == nil || last.Cmp(new) != 0) {
	tw.setLastSeenL1Block(new)
	tw.l1BlockSubFeed.Send(new)
}

It's the only place in the whole codebase where we read the L1 Block Number from the event. So, I think it's safe to enrich L1 only the event with the highest block number. Obviously assuming that The highest L2 Block number contains the highest L1 Block Number. But I understand it's the case, because of this sequencer thingy.

A easy way to still maintain compatibility with L1 may be to set the L1 block number on all the events in enrichL1() using the L1 block number fetched for the newest block since if the node is connected to L1 each event should have BlockNumber == L1BlockNumber.

I think we don't need to set L1 Block Number on all events. It's already compatible with L1, since the block with the highest number is enriched with L1 Block Number (which is equal to Block Number in case of running on L1).

[yondonfu]

Obviously assuming that The highest L2 Block number contains the highest L1 Block Number. But I understand it's the case, because of this sequencer thingy.

Should be true.

I think we don't need to set L1 Block Number on all events. It's already compatible with L1, since the block with the highest number is enriched with L1 Block Number (which is equal to Block Number in case of running on L1).

Hm let me try to walk through a scenario that I think might happen when connected to a L1 provider:

• The TimeWatcher receives a slice of events of length > 1 in handleBlockEvents() and calls handleBlockNum() on each event
• The slice of events might have been published by the BlockWatcher because of an internal call to buildCanonicalChain() which needed to handle a re-org. For example, the highest block number might have been 10, but due to re-org, instead of the next block polling returning a slice with an event for block number 11 it returns a slice with events for block numbers 8, 9, 10 and 11 because blocks 8, 9 and 10 were removed and re-added in the re-org
• handleBlockNum() would be called for each of these blocks including block 11 and publish the block number to its own feed
• As a result, subscribers to the TimeWatcher block number feed would receive blocks 8, 9, 10 and 11 and not just block 11
• If the other blocks don't have their L1BlockNumber fields set then TimeWatcher would return nil for LastSeenL1BlockNum() if that method is called before block 11 is processed by the TimeWatcher.

I'm not sure if existing synchronization prevents the last case from happening - thoughts?

[yondonfu]

I'm not sure if existing synchronization prevents the last case from happening - thoughts?

Discussed offline. The check in https://github.com/leszko/go-livepeer/blob/1382f59ad78aa7f819ba84207d1d1ad4c90e5e6b/eth/watchers/timewatcher.go#L209 will ensure that handleBlockNum() will only publish a block number to the TimeWatcher block number feed if the header has a non-nil L1BlockNumber field. So, in the example from the previous comment, blocks 8, 9 and 10 would not publish anything to the feed and only block 11 would publish something to feed because block 11 has a non-nil L1BlockNumber. Until block 11 is processed, LastSeenL1BlockNum() would return the last known L1 block number which would be whatever the L1 block number was for block 10 before it was re-orged and after it is processed it would return the next L1 block number for block 11. So, the concern I mentioned shouldn't be an issue.

[yondonfu]

LGTM after cleaning up commit history & resolving conflicts.