Create block filters before poll interval

[rodion-lim-partior]

Fixes: #151

Raising this PR to allow for block filters to be created on init instead of after the block polling interval.

[EnriqueL8]

Thanks for the PR @rodion-lim-partior - @Chengxuan could you take a look at this one?

[Chengxuan]

@rodion-lim-partior Thanks for spotting #151 !

The solution proposed is likely to make the problem happen less frequently as long as no transactions are submitted before the new block filter is created on the node.

However, there is still a race condition. I think a better fix here is to ensure the checkStartedLocked function doesn't return until a block filter is established.

Please also add a test to catch future regressions.

[rodion-lim-partior]

@Chengxuan, noticed the following when I tried to fix the race condition:

1. When we add a consumer to the block listener, we acquire a lock
2. Call checkStartedLocked, which starts the block listener loop in a separate goroutine
3. Establish block height next, which requires acquiring a lock to write to block listener, this is blocked till checkStartedLocked has finished
4. We then create a block filter. If I block checkStartedLocked until the filter is created, that ends up in a deadlock, since checkStartedLocked is required to finish to release the lock before establishing block height can proceed

Is there a reason why we acquire a lock before calling checkStartedLocked? Would there be any issues if we shift it downwards?

func (bl *blockListener) addConsumer(c *blockUpdateConsumer) {
	bl.mux.Lock()
	defer bl.mux.Unlock()
	bl.checkStartedLocked()
	bl.consumers[*c.id] = c
}

[Chengxuan]

@rodion-lim-partior It feels the problem is within the scope of checkStartedLocked function, so the solution shouldn't need to touch the global lock that is used to protect shared resources for concurrency.

The key problem to solve is synchronization between the block listener loop separate go routine and the main go routine that created that separate go routine.

Using a channel instead of a lock for go routine synchronization would be more idiomatic.

[rodion-lim-partior]

@Chengxuan, can you help to take a look at the current commit if this was what you referred to?

[Chengxuan]

@rodion-lim-partior Changes look great! I pointed out one possible issue in the code, you should be able to produce that error case in a test.

Once tests are added, I'm happy to take another look.

Thank you for the work!

[EnriqueL8]

Thank @Chengxuan and @rodion-lim-partior for the collaboration on this one! Looking forward to seeing some tests

[rodionlim]

@Chengxuan, the commit I pushed causes a deadlock in my local environment due to the reasons in my previous comments. I will have to shift the global locks downwards to resolve the deadlocks. From local testing, shifting the locks down does not seem to break the existing implementation. Let me know if that works, I will implement the tests and refactor based on your comments after.

[Chengxuan]

Hi @rodion-lim-partior I think you have a good understanding of the code now. So please feel free to try out different options and write tests. Please bear in mind the logic must ensure only 1 listener loop go routine can be created. Also, feel free to split global mutex to be resource specific if needed.

[rodionlim]

@Chengxuan, got it. Will do some refactoring of the code and come back with the tests for another round of review. Thanks for the pointers!

[rodion-lim-partior]

@Chengxuan @EnriqueL8, apologies for the delay. I refactored the code and fixed some of the race conditions along the way. Added the tests as well for the edge case and the happy case.

Some observations (correct me if I'm wrong):

1. There are two functions addConsumer and getHighestBlock which used to lock checkStartedLocked itself, leading to behavior which is difficult to reason about and unintentionally cause certain logic to occur in a particular sequence (described in a previous comment)
2. There is a small chance that a consumer is not added to the block listener (because the thread is slow etc.), causing mock getFilterChanges calls in the tests to skip the dispatch call, resulting in failed tests. This has now been fixed as well
3. Now that we wait for block filters to be established, many of the tests in event_actions.go and event_listener.go need to mock the RPC calls to create the block filters and listen on changes, this has also been corrected.

[EnriqueL8]

Looking good - a few thoughts

[Chengxuan]

@rodionlim logic looks good to me 👍 , some minor comments to clean up bits that are not necessary.

[rodion-lim-partior]

@Chengxuan, pushed a commit to clean up the code, left with one pending comment that needs a review

[Chengxuan]

@rodion-lim-partior thanks. For one pending comment that needs a review I assume that's referring to the removal of ctx parameter in blockTillBlockFilterEstablished that you are reviewing and addressing.

If there are something you'd like me to review, please let me know.

[rodion-lim-partior]

@EnriqueL8, yep that's right. Rest of your review comments have been addressed. The pending one that needs a review, I need @Chengxuan to take a look. Removing the context will cause test failures.

[Chengxuan]

@rodion-lim-partior I looked at the code closer and understood the need to have the context, here is the proposed change: partior-3p#1

6c15c1a has conflict with the proposal, so you'll need to take a look and resolve that.

Otherwise, that PR is ready to go in.

[Chengxuan]

Hi @rodion-lim-partior I'm planning to do a release to pick up another bug fix tomorrow. I'd like to get this PR in as well, please take a look at partior-3p#1 and merge it if you are happy with it. I've sorted out the merge conflicts in that PR as well

[rodion-lim-partior]

@Chengxuan, merged those changes in. Corrected some of the lint failures as well when you run make test. Seems to be working fine on my local environment after rebasing your changes in

[Chengxuan]

Awesome. Thanks for your contribution @rodion-lim-partior 👍

[EnriqueL8]

Thanks both! Loved the discussion on this PR

[rodion-lim-partior]

Cheers guys.