Add initial implementation of tx formatter #96

KonradStaniec · 2022-08-17T15:37:53Z

Initial implementation of formatter with only initial Api

Encode - which takes all data necessary to create our checkpoint and splits into two byte arrays, attaches appropriate headers, and caluclates sha256 of first half
GetCheckpointData - which acts as decode, it takes expected tag, half number, version, and return data

GetCheckpointData can act as filter of of incoming transaction i.e if it do not return error, it indicates that bytes could be valid babylon tx (i.e has correct length and correct header)

Implementation is naive performance wise as it usses append to allocate byte arrays but it can be improved at any time.

Also There is only one test checking happy case, more can be added in subsequent prs.

@gitferry @SebastianElvis as you are working on submitter and reporter, would you need any additional apis ?

SebastianElvis

Fantastic! Encoding/decoding is always tricky to get right. Apart from the below comments about implementation details, one general comment is that could we consider merging this package to types/ or btccheckpoint/types/, where most BTC/checkpoint stuff is located at?

SebastianElvis · 2022-08-18T00:42:25Z

btctxformatter/formatter.go

+	"errors"
+)
+
+type BabylonTag string


Could we define BabylonTag as []byte directly then give TestTag and MainTag the corresponding values? so that we don't need to mess up with the string-byte conversions.

So the reasons those are strings are:

In golang slices and arrays are not constants, but string are. I would prefer for this tag to be constants, so no one can change their values. There are probably some ways around it but it would add more boiler plate code.

Those tag values are under this package control so we can make sure they are more valid utf8 string, so the byte conversions will not be messed up.

Strings are more readable than bytes.
wdyt ?

Agree, especially the part that we want the tag to be constant

SebastianElvis · 2022-08-18T00:45:54Z

btctxformatter/formatter.go

+package btctxformatter
+
+import (
+	"crypto/sha256"


Would it be better to use https://github.com/tendermint/tendermint/blob/main/crypto/tmhash/hash.go so that we base most of the crypto implementations on Tendermint/Cosmos?

My goal here was to make this as dependency free as possible (all used deps are standard go library) so it could be easier to move it separatly publishible package or go module, so one can bring only this dependency to some submitter/reporter implementation without bringing whole cosmos sdk along side.

SebastianElvis · 2022-08-18T00:47:46Z

btctxformatter/formatter.go

+	return bytes
+}
+
+func encodeFirstTx(


Suggested change

func encodeFirstTx(

func encodeFirst(

This function encodes an OP_RETURN entry rather than the entire tx, so probably we don't need to mention Tx. Or if you prefer long function name we may consider encodeFirstOpReturnEntry

SebastianElvis · 2022-08-18T00:49:05Z

btctxformatter/formatter.go

+	epoch uint64,
+	lastCommitHash []byte,
+	bitMap []byte,
+	submitterAddress []byte,


Suggested change

submitterAddress []byte,

submitterAddress sdk.AccAddress,

since we have the concrete type at the first hand when using this function.

ditto, I would rather avoid on depending on sdk types here.

SebastianElvis · 2022-08-18T00:50:22Z

btctxformatter/formatter.go

+	return serializedBytes
+}
+
+func getCheckSum(firstTxBytes []byte) []byte {


Suggested change

func getCheckSum(firstTxBytes []byte) []byte {

func getCheckSum(firstBytes []byte) []byte {

ditto

SebastianElvis · 2022-08-18T00:56:31Z

btctxformatter/formatter.go

+	data []byte,
+) ([]byte, error) {
+
+	if partNumber > 1 {


I remember x/btccheckpoint defined expectedProofs = 2. Perhaps moving this constant here and using it for this check?

good idea, ultimatly btccheckpoint will dependt on formater for getting checkpoint data without headers and validating sha.

SebastianElvis · 2022-08-18T01:03:21Z

btctxformatter/formatter.go

+
+	// At this point this is probable babylon data, strip the header and return data
+	// to the caller
+	dataWithoutHeader := data[4:]


Why are we stripping the header here? I thought the vigilant reporter will forward the header bytes as well.

reporter will forward whole transactions (not only op return data) as this neccessary to properly validate proofs of inclusion.

Header i.e tag + version + half number are stripped here as:

we are calculating sha256 only over application level data and not headers, so for formatter user to properly validate sha256 headers need to be stripped.

this way headers become internal library, so if we add there another byte or bytes later, it won't leak to other parts of codebase

I was under the impression that the header is also a part of the input for the checksum. In fact I think it should be. This rules out the scenarios that a malicious guy submits/reports two txs that have different headers while their bodies constitute a valid checkpoint.

For example, checksum of the IPv4 protocol takes the entire header as input, and checksum of Cosmos address also takes the customised prefix as input, where the checksum is supported via bech32 format.

This rules out the scenarios that a malicious guy submits/reports two txs that have different headers while their bodies constitute a valid checkpoint

Currently the only way this could happen is if someone would attach header of second part to to valid data of first part. Not only such thing would cost attacker btc, but it would also fail validation as first and second part have different lengths, so we would be expecting something that should have 62bytes but it would have 77bytes.

Am I missing something ?

I agree that network protocols are checksumming headers, but there the purpose of checksum is a bit different, they want to check if some bit did not become randomly corrupted during transfer. Here we do not worry about that, we just want to ease up job of reporter to match 2 halfs of the checkpoint.

Nothing here, for exemple, protects against some malicious guy sending bogus second part which will match first part, this will make reporter to submit invalid checkpoints to babylon (and therefore loss him many) but it also cost attacker to submit those bogus parts on btc, so we assume we are fine.

SebastianElvis · 2022-08-18T01:06:08Z

btctxformatter/formatter_test.go

+	return bytes
+}
+
+func TestEncodeMainCheckpointData(t *testing.T) {


Fuzzing tests would be quite useful here, where we can automate the randNBytes thing above

Totally agree, I intend to add fuzzing in subsequent pr's, it this pr I wanted to have only initial api to not block work on submitter and reporter

SebastianElvis

Thanks for the clarification and addressing the comments! There is no blocker from my side, except the suggestion on including the header to the checksum. No strong objection here though.

gitferry · 2022-08-19T06:19:24Z

btctxformatter/formatter.go

+	// Each checkpoint is composed of two parts
+	NumberOfParts = 2
+
+	hashLength = 10


This is for linkage, right? Better to have a description.

gitferry · 2022-08-19T06:20:57Z

btctxformatter/formatter.go

+
+	BlsSigLength = 48
+
+	// 8 bytes are for 64bit unsigned epoch number


I thought we have agreed the epoch number is 4 bytes? See here.

yup, but I saw that epoch number in checkpoint is uin64 - https://github.com/babylonchain/babylon/blob/main/proto/babylon/checkpointing/checkpoint.proto#L14, and I would avoid downcasting to uint32. So the question is wether epoch number should be uint32 or uint64, at checkpoint level, formater part should match this definition.

gitferry · 2022-08-19T06:23:12Z

btctxformatter/formatter.go

+	return data
+}
+
+func u64ToBEBytes(u uint64) []byte {


If we use 4 bytes for epoch, we should use uint32, right?

gitferry · 2022-08-19T06:27:46Z

btctxformatter/formatter.go

+	return bytes
+}
+
+func encodeFirstOpRetrun(


Do we need to include linkage hash here?

we only add linkage hash to the second op return. (as currently stand there is even no space in the first part fo the linkage hash)

gitferry

Sorry for the late reply. Very good job! Thanks for providing this formatter. Just left some comments about some inconsistencies with the previous meeting. Please have a check

aakoshh · 2022-08-22T22:05:05Z

btctxformatter/formatter.go

+	if len(blsSig) != BlsSigLength {
+		return nil, nil, errors.New("BlsSig should have 48 bytes")
+	}


Duplicated check.

* slashing tx validation * fix * fix * fix * fix * fix check * fix tests and checks * fix * fix checks * fix data types * fix * update e2e tests * fix comments * fix comments * fix comments * fix comments * change to slashing rate * fix comments and checks * fix checks * hardcode slashing rate to 100% to pass tests * fix docstring * more comments * add slashing rate check in checktransactions * change staker addr to change addr * pass change addr in msgcreatedelegation * change address in msgbtcundelegate * error handling for change addr * verbose errors * fix fee checks and validation for slashing tx on rounded slashing rate * more comments * fix docstring * more comments in fee checks * remove staking output info in msgundelegate * remove receiver func Validate * fix tests and include change address in MsgCreateBTCDelegation request * fix tests and pass change address in MsgBTCUndelegate * fix naming and remove dead code * fix e2e tests - pass change addr in types.MsgCreateBTCDelegation * fix imports * fix import issue * fix imports * fix imports * fix * fix args in tx * fix imports * change address in undelegation and delegation creation * keep Validate func * add more comments * fix proto comments * fix slashing rate validation * fix validation * more comments * fix checks in validation and don't depend on change address * remove change address proto * remove change address usages * fix receiver func * remove todos * fix args * fix func sigs * fix imports * fix defs * fix imports * fix formatting * change to ceil(round up) * lock time 0 * fix checks * move fee checks * fix doc string * convert to btcutil.amount * dust relay tx fee * fix dust fn * fix mod * add todo * fix docstring

Add initial implementation of tx formatter

95666f5

KonradStaniec requested review from gitferry and SebastianElvis August 17, 2022 15:37

SebastianElvis reviewed Aug 18, 2022

View reviewed changes

Add additional api methods

183c5ec

KonradStaniec force-pushed the initial-formatter branch from 290435a to 183c5ec Compare August 18, 2022 15:38

SebastianElvis approved these changes Aug 19, 2022

View reviewed changes

gitferry reviewed Aug 19, 2022

View reviewed changes

KonradStaniec merged commit b3344e1 into main Aug 19, 2022

KonradStaniec deleted the initial-formatter branch August 19, 2022 08:20

aakoshh reviewed Aug 22, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial implementation of tx formatter #96

Add initial implementation of tx formatter #96

KonradStaniec commented Aug 17, 2022

SebastianElvis left a comment

SebastianElvis Aug 18, 2022

KonradStaniec Aug 18, 2022

SebastianElvis Aug 19, 2022

SebastianElvis Aug 18, 2022

KonradStaniec Aug 18, 2022

SebastianElvis Aug 18, 2022

SebastianElvis Aug 18, 2022

KonradStaniec Aug 18, 2022

SebastianElvis Aug 18, 2022

SebastianElvis Aug 18, 2022

KonradStaniec Aug 18, 2022

SebastianElvis Aug 18, 2022

KonradStaniec Aug 18, 2022

SebastianElvis Aug 19, 2022

KonradStaniec Aug 19, 2022

SebastianElvis Aug 18, 2022

KonradStaniec Aug 18, 2022

SebastianElvis left a comment

gitferry Aug 19, 2022

gitferry Aug 19, 2022

KonradStaniec Aug 19, 2022

gitferry Aug 19, 2022

gitferry Aug 19, 2022

KonradStaniec Aug 19, 2022

gitferry left a comment •

edited

Loading

aakoshh Aug 22, 2022

	func getCheckSum(firstTxBytes []byte) []byte {
	func getCheckSum(firstBytes []byte) []byte {


		BlsSigLength = 48

		// 8 bytes are for 64bit unsigned epoch number

Add initial implementation of tx formatter #96

Add initial implementation of tx formatter #96

Conversation

KonradStaniec commented Aug 17, 2022

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry left a comment •

edited

Loading