Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: batch process bug #39

Merged
merged 13 commits into from
Nov 7, 2024
Merged

fix: batch process bug #39

merged 13 commits into from
Nov 7, 2024

Conversation

parketh
Copy link
Contributor

@parketh parketh commented Nov 6, 2024

Summary

This PR fixes a bug introduced in the batch processing refactor in #35.

here, we incorrectly assume that the returned results channel is ordered by height. since block fetching is now async, this channel is actually unordered and needs to first be sorted.

// Extract blocks and find last consecutive finalized block
var finalizedBlocks []*types.Block
var lastFinalizedHeight uint64
for block := range results {
if block == nil {
break
}
finalizedBlocks = append(finalizedBlocks, block)
lastFinalizedHeight = block.BlockHeight
}

the impact of this change was that not all finalized blocks were inserted to the db, even though the latest finalized height advances correctly

besides the fix, we also update FG to:

  • allow it to recover from local state after crashes / restarts
  • add more debugging logs
  • add a new env var in config.toml to set the log level at runtime

Test plan

make test

This bug wasn't caught by previous tests because it doesn't fall under scope of the unit tests (we mock DB inserts to keep the DB implementation generic). It would have been caught by e2e tests, but we deferred this task in favour of testing on the live network.

In the future, we should:

  • add e2e test cases to prevent code regression
  • add more debugging logs so we can catch bugs more easily in live testing

config/config.go Outdated
@@ -19,6 +19,7 @@ type Config struct {
GRPCListener string `long:"grpc-listener" description:"host:port to listen for gRPC connections"`
HTTPListener string `long:"http-listener" description:"host:port to listen for HTTP connections"`
BitcoinDisableTLS bool `long:"bitcoin-disable-tls" description:"disable TLS for RPC connections"`
DebugLogLevel bool `long:"debug-log-level" description:"set log level to debug (true) or info (false)"`
Copy link
Member

Choose a reason for hiding this comment

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

can we use a log level enum for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense - fixed!

@parketh parketh requested a review from bap2pecs November 7, 2024 11:46
@parketh parketh merged commit 8bdd24c into main Nov 7, 2024
@parketh parketh deleted the fix/batch-process-bug branch November 7, 2024 11:56
@parketh parketh restored the fix/batch-process-bug branch November 7, 2024 11:56
@parketh parketh deleted the fix/batch-process-bug branch November 7, 2024 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants