-
Notifications
You must be signed in to change notification settings - Fork 170
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
datagen: add datagen functions for testing reporter #190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Some suggestions:
testutil/datagen/btc_blockchain.go
Outdated
"github.com/btcsuite/btcd/wire" | ||
) | ||
|
||
func GenRandomBlock(numBabylonTxs int, prevHash *chainhash.Hash) (*wire.MsgBlock, *btctxformatter.RawBtcCheckpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments would be appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I suggest that this function is called GenRandomBtcdBlock
to comply with the GenRandomBtcdHeader
function.
testutil/datagen/btc_blockchain.go
Outdated
randomTxs[1] = GenRandomTx() | ||
rawCkpt = nil | ||
} else { | ||
randomTxs = []*wire.MsgTx{GenRandomTx(), GenRandomTx()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about start with
randomTxs = []*wire.MsgTx{GenRandomTx(), GenRandomTx()}
rawCkpt = nil
And update both in the first if
condition and update only randomTxs[1]
in the second if
condition. Would simplify this.
testutil/datagen/btc_blockchain.go
Outdated
header.MerkleRoot = merkleRoot | ||
header.Bits = workBits | ||
if prevHash == nil { | ||
header.PrevBlock = chainhash.DoubleHashH(GenRandomByteArray(10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GenRandomBtcdHeader()
function already creates a random PrevBlock
testutil/datagen/btc_blockchain.go
Outdated
return block, rawCkpt | ||
} | ||
|
||
func GenRandomBlockchainWithBabylonTx(n uint64, partialPercentage float32, fullPercentage float32) ([]*wire.MsgBlock, int, []*btctxformatter.RawBtcCheckpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func GenRandomBlockchainWithBabylonTx(n uint64, partialPercentage float32, fullPercentage float32) ([]*wire.MsgBlock, int, []*btctxformatter.RawBtcCheckpoint) { | |
func GenRandomBtcdBlockchainWithBabylonTx(n uint64, partialPercentage float32, fullPercentage float32) ([]*wire.MsgBlock, int, []*btctxformatter.RawBtcCheckpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, comments would be appreciated.
testutil/datagen/btc_blockchain.go
Outdated
for i := uint64(1); i < n; i++ { | ||
var msgBlock *wire.MsgBlock | ||
prevHash := blocks[len(blocks)-1].BlockHash() | ||
if rand.Float32() < partialPercentage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the partialPercentage
and fullPercentage
do not properly demonstrate what they are used for. Maybe use oneTxThreshold
and twoTxThreshold
, with the latter being a full threshold instead of having to be added to partialPercentage
and a check that the second one is bigger than the first one.
testutil/datagen/btc_transaction.go
Outdated
TxIn: []*wire.TxIn{ | ||
{ | ||
PreviousOutPoint: wire.OutPoint{ | ||
Hash: chainhash.HashH(GenRandomByteArray(10)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also use: chainhash.NewHashFromStr(GenRandomHexStr(32))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewHashFromStr
returns two values. Created a new function to this end.
testutil/datagen/btc_transaction.go
Outdated
rawBTCCkpt := GetRandomRawBtcCheckpoint() | ||
// encode raw checkpoint to two halves | ||
firstHalf, secondHalf, err := btctxformatter.EncodeCheckpointData( | ||
btctxformatter.TestTag(48), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a random value instead of 48
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not straightforward to do. Vigilante and Babylon need to use a consistent tag. So if we want to use a random tag, then we need to feed vigilante and this function with the same tag when testing vigilante. Left a TODO on this.
// encode raw checkpoint to two halves | ||
firstHalf, secondHalf, err := btctxformatter.EncodeCheckpointData( | ||
btctxformatter.TestTag(48), | ||
btctxformatter.CurrentVersion, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a historical record of older versions? Might be useful to test with other ones as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep it would be good to have. At least we need to remember the last version we are using, so that we don't repeat ourselves with inconsistent tag format.
testutil/datagen/btc_transaction.go
Outdated
// fake a raw checkpoint | ||
rawBTCCkpt := GetRandomRawBtcCheckpoint() | ||
// encode raw checkpoint to two halves | ||
firstHalf, secondHalf, err := btctxformatter.EncodeCheckpointData( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use GenRandomBabylonTxPair()
here and get only one of the answers depending on the random choice in 403?
if err != nil { | ||
panic(err) | ||
} | ||
idx := rand.Intn(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use datagen.OneInN(2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even better, if we use the GenRandomBabylonTxPair()
, we can use idx
as the index for the result. Then we wouldn't have to copy paste the same code under both if/else
conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 👍
Note that the link for the vigilante |
This PR migrates datagen functions in vigilante to Babylon repo. These datagen functions are used for testing the vigilante reporter, and might also be useful for testing Babylon functionalities.
The code is mostly identical to https://github.com/babylonchain/vigilante/blob/main/testutil/datagen/reporter.go, except that the code reuses some of the existing private functions in Babylon's datagen module.