Add parsing correct format in btccheckpoint #102

KonradStaniec · 2022-08-22T15:47:30Z

Add parsing op return data in btccheckpoint according to format defined in txformatter (i.e requiring headers and proper linking)
add tools necessary to generate proper btc transactions (first step toward fuzzing). It was necessary as there are no valid babylon opreturns on any btc chain, therefore we need to generate our own txs for testing.

aakoshh

Looks great, you put a lot of effort in it! Looks like I missed the previous PR that added the formatter :-/

I only submitted comments where it wasn't obvious what the code was going to do when I first saw it. Maybe some extra comments will help future readers.

aakoshh · 2022-08-22T21:34:52Z

app/app.go

+			// TODO take from config
+			chaincfg.MainNetParams.PowLimit,


Great, it's almost configurable now! 🏁 @SebastianElvis tests already fail with this error.

yep making this thingy configurable is next in line :)

aakoshh · 2022-08-22T21:36:21Z

testutil/datagen/btc_transaction.go

+	lowFee = btcutil.Amount(1)
+)
+
+func standardCoinbaseScript(blockHeight int32, extraNonce uint64) ([]byte, error) {


Do you have a link for why this is the standard? What is extra nonce?

this the the thing which btcd use when adding coinbase transaction during mining (without the btcd specific coinbase flag CoinbaseFlags = "/P2SH/btcd/"). I will add comment from code base which look like:

// standardCoinbaseScript returns a standard script suitable for use as the // signature script of the coinbase transaction of a new block. In particular, // it starts with the block height that is required by version 2 blocks

Not sure though what the extra nonce is necessary for.

aakoshh · 2022-08-22T21:36:43Z

testutil/datagen/btc_transaction.go

+// opReturnScript returns a provably-pruneable OP_RETURN script with the
+// provided data.


What does it mean to be provably pruneable?

this is how, op_return which will not pollute unspent transaction outputs set, should look like.

aakoshh · 2022-08-22T21:37:28Z

testutil/datagen/btc_transaction.go

+	return script
+}
+
+func solveBlock(header *wire.BlockHeader) bool {


This method could do with a short docstring. What does it mean to solve a block, and what does the return value mean? Only after reading it fully did I see that it modifies the input.

aakoshh · 2022-08-22T21:47:05Z

testutil/datagen/btc_transaction.go

+	// TODO this will be deterministic without seed but for now it is not that
+	// important
+	idx := rand.Uint32()


You mean non-deterministic?

actually deterministic, as if we do not call rand.Seed(time.now) or something similar then it will always return the same random number.

Thanks, I learn something new every day about Go :)

aakoshh · 2022-08-22T21:48:28Z

testutil/datagen/btc_transaction.go

+}
+
+func createSpendTx(spend *spendableOut, fee btcutil.Amount) *wire.MsgTx {
+	spendTx := wire.NewMsgTx(1)


1 like a magic number. I see in the docs it's a version; maybe it could be moved next to opTrueScript?

aakoshh · 2022-08-22T21:52:07Z

testutil/datagen/btc_transaction.go

+	spendTx.AddTxOut(wire.NewTxOut(int64(spend.amount-fee),
+		opTrueScript))


What does this achieve exactly?

each output need to have some script, and from what I understand this is simplest valid script. We could probably leave it as empty byte array, but that way we closer mirror btc.

SebastianElvis

Thanks, especially the part that makes PoWLimit configurable! Some minor comments and no blocker from my side.

SebastianElvis · 2022-08-23T10:47:31Z

app/app.go

@@ -358,6 +359,8 @@ func NewBabylonApp(
 			// from some global config
 			6,
 			10,
+			// TODO take from config
+			chaincfg.MainNetParams.PowLimit,


Do you think making this a module parameter is better? Like EpochInterval in https://github.com/babylonchain/babylon/blob/main/proto/babylon/epoching/v1/params.proto.

tbh I am on the fence here. There are two things against having it as module param:

this is used it btclightclient and btccheckpoint, so i would need to be defined in two modules and kept consisten between them. (or defined in one and shared, not sure this is possible though)

we are validating pow before including tx in mempool i.e in ValidateBasic which does not have access to genesis. (or at least I am not aware how to make it aware of genesis)

Taking both into account I was thinking of making this a global configurable constant similar to how addresss prefixes are global constants which are accesible from every place, even from ValidateBasic . Downside of this, that it will always need to corrsedpond to btc light client btc genesis header, but I think I can live with it at this moment.

SebastianElvis · 2022-08-23T10:50:02Z

testutil/datagen/btc_transaction.go

+	return script
+}
+
+func solveBlock(header *wire.BlockHeader) bool {


Will we try to solve a block when testing Babylon? 😨

added docstring that this should be headers with low diff (like from simnet) as otherwise this would take too much time. It necessary as in both btclightclient and btccheckpoint we always validate proof of work of provided headers.

aakoshh · 2022-08-23T14:47:58Z

One thing I wanted to mention but forgot is that this check is no longer correct:

babylon/x/btccheckpoint/keeper/msg_server.go

Line 52 in 84d6a71

isAncestor, err := m.k.IsAncestor(ctx, &fh, &sh)

Originally when we wanted the two transactions to be connected via UTxO, it made sense that there's the first part, spent into the second, so the block the first one is in must be an ancestor of the second, for them to always be finalized together.

Now, however, we don't need this linking, the transactions can go in any order into BTC, as long as they are on the same fork. Say the 1st half has too low fee and only the 2nd half gets included in the ledger. Maybe then the submitter will do it again with a different transaction, same content, but later.

So the correct check is now isAncestor(first, second) || isAncestor(second, first).

KonradStaniec · 2022-08-24T15:52:23Z

Now, however, we don't need this linking, the transactions can go in any order into BTC, as long as they are on the same fork. Say the 1st half has too low fee and only the 2nd half gets included in the ledger. Maybe then the submitter will do it again with a different transaction, same content, but later.

So the correct check is now isAncestor(first, second) || isAncestor(second, first).

Sure makes sense, I will update it to it now.

Although I wonder if we cannot just remove isAncestor and use MainChainDepth for everything. i.e if at least one part if submission is not on main chain, we do not really care on what fork it is, checkpoint will be in unstable state. And when both are on main chain, then everything is fine as they are on the same fork. I will think about it a bit, and update it if necessary in next pr.

aakoshh · 2022-08-25T01:38:29Z

That sounds correct as well. At least if they are on different forks, they can't both have valid results from the main chain depth method 👍

…102)

KonradStaniec added 2 commits August 22, 2022 17:46

Add btc transaciton generator

fcf21fd

Add using proper parsing in btccheckpoint

84d6a71

KonradStaniec requested review from aakoshh and SebastianElvis August 22, 2022 16:40

KonradStaniec marked this pull request as ready for review August 22, 2022 16:41

aakoshh approved these changes Aug 22, 2022

View reviewed changes

SebastianElvis approved these changes Aug 23, 2022

View reviewed changes

Pr comments

6184dde

KonradStaniec merged commit feab466 into main Aug 24, 2022

KonradStaniec deleted the use-correct-format-in-btccheckpoint branch August 24, 2022 16:17

aakoshh mentioned this pull request Sep 21, 2022

fix: Allow consecutive epoch SPV proofs contained in the same BTC block #152

Closed

vitsalis pushed a commit that referenced this pull request Jan 21, 2024

btcstaking: filter out top N BTC validators as active validator set (#…

dac26d8

…102)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parsing correct format in btccheckpoint #102

Add parsing correct format in btccheckpoint #102

KonradStaniec commented Aug 22, 2022 •

edited

Loading

aakoshh left a comment

aakoshh Aug 22, 2022

KonradStaniec Aug 24, 2022

aakoshh Aug 22, 2022

KonradStaniec Aug 24, 2022

aakoshh Aug 22, 2022

KonradStaniec Aug 24, 2022 •

edited

Loading

aakoshh Aug 22, 2022

aakoshh Aug 22, 2022

KonradStaniec Aug 24, 2022

aakoshh Aug 25, 2022

aakoshh Aug 22, 2022

aakoshh Aug 22, 2022

KonradStaniec Aug 24, 2022

SebastianElvis left a comment

SebastianElvis Aug 23, 2022

KonradStaniec Aug 24, 2022

SebastianElvis Aug 23, 2022

KonradStaniec Aug 24, 2022

aakoshh commented Aug 23, 2022

KonradStaniec commented Aug 24, 2022

aakoshh commented Aug 25, 2022

		// opReturnScript returns a provably-pruneable OP_RETURN script with the
		// provided data.

		spendTx.AddTxOut(wire.NewTxOut(int64(spend.amount-fee),
		opTrueScript))

Add parsing correct format in btccheckpoint #102

Add parsing correct format in btccheckpoint #102

Conversation

KonradStaniec commented Aug 22, 2022 • edited Loading

aakoshh left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KonradStaniec Aug 24, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis left a 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

Choose a reason for hiding this comment

aakoshh commented Aug 23, 2022

KonradStaniec commented Aug 24, 2022

aakoshh commented Aug 25, 2022

KonradStaniec commented Aug 22, 2022 •

edited

Loading

KonradStaniec Aug 24, 2022 •

edited

Loading