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

reporter: refactor reporter, datagen and fuzz tests on block handlers #95

Merged
merged 9 commits into from
Nov 2, 2022

Conversation

SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Oct 28, 2022

Partially fixes BM-156

This PR aims at implementing fuzz tests for reporter's ProcessHeaders function. Along the way, it does some refactoring for mocking the reporter. Specifically, this PR

  • replaces the concrete fields of Babylon/BTC clients with their interfaces in Reporter
  • creates a function GenRandomBlockchainWithBabylonTx that generates a blockchain in which blocks may contain some Babylon txs
  • adds a fuzz test on ProcessHeaders, where given a block that may or may not be duplicated on Babylon BTCLightclient, check whether the reporter does the deduplication correctly or not.
  • added a fuzz tests on ProcessCheckpoints, where given a block that may or may not contain a pair of Babylon txs, check whether the reporter extracts correct checkpoint segments and matches checkpoint segments correctly.

Copy link
Member

@vitsalis vitsalis 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! Some comments about testing more cases:

return mockBTCClient, mockBabylonClient, reporter
}

// FuzzProcessHeaders is a fuzz tests for ProcessHeaders()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// FuzzProcessHeaders is a fuzz tests for ProcessHeaders()
// FuzzProcessHeaders fuzz tests ProcessHeaders()

}

// FuzzProcessHeaders is a fuzz tests for ProcessHeaders()
// - Data: a number of random blocks, with or without Babylon txs
Copy link
Member

Choose a reason for hiding this comment

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

This test case seems to test the submission of only one block. We should also test this with multiple blocks. To make this realistic, once a block does not exist then all of its descendants won't exist as well. So we can do:

  1. Randomly identify the total number of blocks and generate them.
  2. Randomly identify the number of those blocks that exist on Babylon. Can be [0, N], where N is the total number of blocks. The first set of headers up to this number exist on Babylon, the rest do not.
  3. Verify that only the required headers were submitted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done

})
}

// FuzzProcessCheckpoints is a fuzz tests for ProcessCheckpoints()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// FuzzProcessCheckpoints is a fuzz tests for ProcessCheckpoints()
// FuzzProcessCheckpoints fuzz tests ProcessCheckpoints()

mockBabylonClient.EXPECT().MustInsertBTCSpvProof(gomock.Any()).Return(&sdk.TxResponse{Code: 0}).AnyTimes()

containsCkpt := datagen.OneInN(2)
containsCkpt = true
Copy link
Member

Choose a reason for hiding this comment

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

This is always true?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, it was from a debugging attempt. Good catch!

}

// FuzzProcessCheckpoints is a fuzz tests for ProcessCheckpoints()
// - Data: a number of random blocks, with or without Babylon txs
Copy link
Member

Choose a reason for hiding this comment

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

This tests only a single block.


containsCkpt := datagen.OneInN(2)
containsCkpt = true
block, _ := vdatagen.GenRandomBlock(containsCkpt, nil)
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this only contains one half of the checkpoint? Maybe we should test for this case as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done

@@ -68,6 +68,40 @@ func GenRandomTx() *wire.MsgTx {
return tx
}

func GenRandomBabylonTxPair() ([]*wire.MsgTx, *btctxformatter.RawBtcCheckpoint) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@SebastianElvis SebastianElvis changed the title reporter: refactor reporter, datagen and fuzz test on ProcessHeaders reporter: refactor reporter, datagen and fuzz tests on block handlers Nov 2, 2022
@SebastianElvis SebastianElvis merged commit a8c3500 into main Nov 2, 2022
@SebastianElvis SebastianElvis deleted the btc-handler-test branch November 2, 2022 00:26
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