Store lastBlockSeen value #1581

bitwiseguy · 2023-08-23T01:44:39Z

This is a precursor to adding initialization logic to the chainservice to allow it to process events from blocks since the node last started. Currently, our node always assumes we are starting for the first time, therefore it doesn't look back in time to see if it's missed anything.

This PR makes the following changes:

Adds store methods to get/set the last block processed. Also adds unit tests for these new methods.
Adds code to the engine to call store.SetLastBlockSeen every time an on-chain event is received from chainservice

netlify · 2023-08-23T01:44:43Z

✅ Deploy Preview for nitro-gui canceled.

Name	Link
🔨 Latest commit	`5d4a616`
🔍 Latest deploy log	https://app.netlify.com/sites/nitro-gui/deploys/64eded4a15bee400084bb044

netlify · 2023-08-23T01:44:43Z

👷 Deploy Preview for nitrodocs processing.

Name	Link
🔨 Latest commit	`5d4a616`
🔍 Latest deploy log	https://app.netlify.com/sites/nitrodocs/deploys/64eded4afb42270008bd1c22

netlify · 2023-08-23T01:44:43Z

✅ Deploy Preview for nitro-storybook canceled.

Name	Link
🔨 Latest commit	`5d4a616`
🔍 Latest deploy log	https://app.netlify.com/sites/nitro-storybook/deploys/64eded4ac063ca00080f3e6f

node/engine/store/memstore.go

bitwiseguy · 2023-08-23T20:39:53Z

node/engine/engine.go

@@ -189,6 +189,8 @@ func (e *Engine) run(ctx context.Context) {
 		case pr := <-e.PaymentRequestsFromAPI:
 			res, err = e.handlePaymentRequest(pr)
 		case chainEvent := <-e.fromChain:
+			err = e.store.SetLastBlockProcessed(chainEvent.BlockNum())


In this branch this is the only time we call store.SetLastBlockProcessed. We should improve this so that we are updating the lastBlockProcessed even when no events are emitted by the NitroAdjudicator contractor. This optimization would reduce the amount of processing the chainservice has to do when it is initialized and checks for any events that were emitted since the node was last online.

Shouldn't the chainservice have a pretty smart way of checking historical events, though? If we are filtering our queries using the NitroAdjudicator address https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getlogs , we could potentially get away with only changing lastBlockProcessed when the adjudicator emits an event.

Yes I think we could get away with only changing lastBlockSeen when the adjudicator emits an event. However, I was trying to think of a way to reduce processing time during this scenario:

** nitro node is running and listening for chain events and new blocks ***

Block 100 mined on-chain

node detects adjudicator event (AE1) in block 100

Block 101 mined on-chain

Block 102 mined on-chain

node processes AE1

node updates lastBlockSeen to 100

Block 103-20,000 mined on-chain, but no adjudicator events are emitted

node is turned off

Block 20,001-30,000 mined on-chain

node is restarted and begins init routines

node looks through blocks from lastBlockSeen (100) to current block (30,000) for adjudicator events

The query in step [11] to search through many blocks seems wasteful since our nitro node was actually running up until block 20,000 was mined, but lastBlockSeen was not updated because no adjudicator events were included in any of those blocks. Instead of searching through the block range 100-20,000, we should just be able to search through the range 20,000-30,000 since those were the only blocks mined while the node was offline.

@geoknee do you think it is worth trying to account for the situation described above?

That's a good point, I didn't think about that scenario!

One thing to note is that ethereum makes use of bloom filters for logs(aka events) on the block header. This means it's very efficient to check if a block contains an event by checking the logsBloom on the block header. Instead of having to parse through the a block you can just check your event topics against the bloom filter to know if the events are in the block. This means it's relatively efficient to query for events over a large range of blocks. The JSON-RPC API allows you to specify a from and to block so it's pretty easy to query a large range of blocks for event's we'd be interested in.

Based on that I think we can avoid worrying about this scenario too much for now. If it's something we start noticing we can revisit and implement a solution.

That's good info on the bloom filters. I will not worry about this scenario for now but I made this issue to remind us to revisit if we see any related problems in the future: #1600

lalexgap · 2023-08-23T23:34:30Z

node/engine/engine.go

@@ -189,6 +189,8 @@ func (e *Engine) run(ctx context.Context) {
 		case pr := <-e.PaymentRequestsFromAPI:
 			res, err = e.handlePaymentRequest(pr)
 		case chainEvent := <-e.fromChain:
+			err = e.store.SetLastBlockProcessed(chainEvent.BlockNum())


Since we're setting lastBlockProcessed when the engine receives an event with that block number, it means lastBlockProcessed gets set when the engine handles the first event in a block, not when all events in a block are handled.

For example lets say we have block which contains two events: 1)Deposited 0x0, 0x0AAA... Block Num:55 2)Deposited 0x0, 0xBBBB...Block Num:55

Let's say our engine receives the first event and sets lastBlockProcessed=55. Now let's say the engine crashes. Based on the lastBlockProcessed it has no way of knowing that we never handled the second event.

This is probably not the end of the world since we can probably just replay all the events from lastBlockProcessed and assume any events we already handled will be ignored. Although we may want to consider renaming lastBlockProcessed to something like lastBlockSeen

Alternatively we could also consider storing the block number information per channel, possibly as part of the work done in this PR

This is a good point, I'm in favour of:

using lastBlockSeen terminology

replaying all logs from lastBlockSeen

I think the logic for block numbers per channel is there partly to protect against events being reordered. If we are sure that cannot happen, we may not need to worry. It's occurring to me also that your example of multiple events in one block may be problematic for the logic on the Channel class:

go-nitro/channel/channel.go

Lines 325 to 328 in dc1d694

func (c *Channel) UpdateWithChainEvent(event chainservice.Event) (*Channel, error) {

if event.BlockNum() < c.OnChain.LatestBlockNumber {

return c, nil // ignore stale information TODO: is this reorg safe?

}

The block number is not sufficient to order all events, as you point out. So if Alice deposits 5 and then Bob deposits 5 into the same channel in the same block... getting Bob's deposit first and then Alice's deposit can leave the Channel with an incorrect view of the holdings of the channel.

I will rename lastBlockProcessed --> lastBlockSeen. If we already have protection in the Channel class against processing the same event twice, then we should be able to process logs from lastBlockSeen (inclusive) to current block when the node is initialized.

I am planning to add the chainservice init function that scans for old logs using the lastBlockSeen value for the lower bound of the query as part of a follow-up PR.

If we already have protection in the Channel class against processing the same event twice

I think we need an ADR for this - a well-thought-through pattern which involves some "contract" between a chain service and the Channel class, such that we don't get any bugs around stale or incorrect information.

Example: the Channel class promises to have an idempotent function for accepting events from the chain service, and the chain service promises to pass events in order.

☝️ I'm not 100% sure about this example though. It seems like we might want to avoid (re)playing a ChallengeRegistered event if the challenge has long since expired or been cleared.

Not a blocker for merging this PR, because I think in mose conceivable patterns we will want to store lastBlockNumberSeen.

geoknee

Direction of this PR looks good, but we have some open comment threads which it would be good to settle 👍

node/engine/store/durablestore.go

geoknee

Still a few small things to tidy up, but this LGTM.

node/engine/store/memstore.go

node/engine/store/store.go

bitwiseguy added 3 commits August 22, 2023 20:51

Store last processed block number

07cdfe5

Add TestGetLastBlockProcessed

d3d0abd

Add TestGetLastBlockProcessedDurableStore

0a35258

geoknee reviewed Aug 23, 2023

View reviewed changes

node/engine/store/memstore.go Outdated Show resolved Hide resolved

Change lastBlockProcessed to uint64 in memstore instead of map

ca4f034

bitwiseguy marked this pull request as ready for review August 23, 2023 14:06

bitwiseguy marked this pull request as draft August 23, 2023 16:00

lalexgap mentioned this pull request Aug 23, 2023

WIP: Checking lint failure #1582

Closed

bitwiseguy added 2 commits August 23, 2023 13:23

Merge branch 'main' into store-last-block

9c633fa

Use GenerateTempStoreFolder in TestGetLastBlockProcessedDurableStore

dfd117c

bitwiseguy mentioned this pull request Aug 23, 2023

Refactor init functions to separate node from rpc server #1584

Merged

bitwiseguy marked this pull request as ready for review August 23, 2023 20:36

bitwiseguy commented Aug 23, 2023

View reviewed changes

lalexgap reviewed Aug 23, 2023

View reviewed changes

geoknee suggested changes Aug 24, 2023

View reviewed changes

Rename lastBlockProcessed to lastBlockSeen

fb0bac2

bitwiseguy force-pushed the store-last-block branch from 83053bf to fb0bac2 Compare August 28, 2023 14:47

bitwiseguy changed the title ~~Store last processed block number~~ Store lastBlockSeen value Aug 28, 2023

lalexgap reviewed Aug 28, 2023

View reviewed changes

node/engine/store/durablestore.go Outdated Show resolved Hide resolved

lalexgap reviewed Aug 28, 2023

View reviewed changes

node/engine/store/durablestore.go Outdated Show resolved Hide resolved

lalexgap approved these changes Aug 28, 2023

View reviewed changes

Use SetLastBlockNumSeen instead of SetLastBlockSeen

1185ea1

bitwiseguy mentioned this pull request Aug 28, 2023

Optimize chainservice init function #1600

Closed

bitwiseguy requested a review from geoknee August 28, 2023 18:31

geoknee approved these changes Aug 29, 2023

View reviewed changes

node/engine/store/memstore.go Outdated Show resolved Hide resolved

node/engine/store/store.go Outdated Show resolved Hide resolved

geoknee mentioned this pull request Aug 29, 2023

Make design doc / ADR for restart-capable event tracking #1604

Closed

Consistenly use lastBlockNumSeen instead of lastBlockSeen

5d4a616

bitwiseguy merged commit 7da3527 into main Aug 29, 2023

bitwiseguy deleted the store-last-block branch August 29, 2023 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store lastBlockSeen value #1581

Store lastBlockSeen value #1581

bitwiseguy commented Aug 23, 2023 •

edited

Loading

netlify bot commented Aug 23, 2023 •

edited

Loading

netlify bot commented Aug 23, 2023 •

edited

Loading

netlify bot commented Aug 23, 2023 •

edited

Loading

bitwiseguy Aug 23, 2023

geoknee Aug 24, 2023

bitwiseguy Aug 28, 2023 •

edited

Loading

lalexgap Aug 28, 2023

bitwiseguy Aug 28, 2023

lalexgap Aug 23, 2023 •

edited

Loading

geoknee Aug 24, 2023

geoknee Aug 25, 2023

bitwiseguy Aug 28, 2023

geoknee Aug 29, 2023

geoknee left a comment

geoknee left a comment

	func (c Channel) UpdateWithChainEvent(event chainservice.Event) (Channel, error) {
	if event.BlockNum() < c.OnChain.LatestBlockNumber {
	return c, nil // ignore stale information TODO: is this reorg safe?
	}

Store lastBlockSeen value #1581

Store lastBlockSeen value #1581

Conversation

bitwiseguy commented Aug 23, 2023 • edited Loading

netlify bot commented Aug 23, 2023 • edited Loading

✅ Deploy Preview for nitro-gui canceled.

netlify bot commented Aug 23, 2023 • edited Loading

👷 Deploy Preview for nitrodocs processing.

netlify bot commented Aug 23, 2023 • edited Loading

✅ Deploy Preview for nitro-storybook canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwiseguy Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalexgap Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoknee left a comment

Choose a reason for hiding this comment

geoknee left a comment

Choose a reason for hiding this comment

bitwiseguy commented Aug 23, 2023 •

edited

Loading

netlify bot commented Aug 23, 2023 •

edited

Loading

netlify bot commented Aug 23, 2023 •

edited

Loading

netlify bot commented Aug 23, 2023 •

edited

Loading

bitwiseguy Aug 28, 2023 •

edited

Loading

lalexgap Aug 23, 2023 •

edited

Loading