feat: submitter/send sealed checkpoints to BTC #37

gitferry · 2022-09-13T14:45:56Z

This is a replacement for #16. This PR aims to poll sealed checkpoints, convert them into two BTC transactions and send them to BTC. The PR has been tested on the simnet of BTC.

The submitter requires both btcd and btcwallet running in the backend. This PR adds a btc rpc client and connects to btcwallet to perform wallet-related commands. To test the code, a wallet needs to be created beforehand and there should be sufficient unspent txs.

…er/sent-tx-to-btc

vitsalis

Nice work! Some comments related to the config and BTC transactions:

btcclient/client.go

config/submitter.go

sample-vigilante.yml

vitsalis · 2022-09-14T00:06:22Z

config/submitter.go

 	return SubmitterConfig{
-		NetParams: "simnet",
+		NetParams:        "simnet",
+		TxFee:            amount,


Is this the BTC tx fee?

Yes. I'm adding a comment here. Thanks!

vitsalis · 2022-09-14T00:26:26Z

submitter/ckpt_relayer.go

+		return err
+	}
+
+	utxo1, utxo2, err := s.getTopTwoUTXOs()


We could also use a single UTXO to create the first transaction and use the UTXO created by it as an input for the second transaction. Overall, we could use this UTXO chaining trick, with the first transaction having limited fees and the second transaction having double the amount of fees to make the transactions be included in the same block. I can understand that this might be too much to implement right now, so let's add an investigation task for that in the backlog.

I'm not sure the second tx will pass the mempool check because the input the second tx consumes is not confirmed.

It would typically pass that check because the input is already in the mempool. At least bitcoin core does that iirc.

Worth investigating at a later point imo.

I see. I think you are right. I marked it as a TODO. Will investigate soon. Thanks!

Better to create a Github issue imo, since it might get lost here.

It seems like a bit of an arbitrary requirement to need two UTxOs ready to be spent 🤔
I see how it simplifies the logic here, e.g. after a restart the vigilante doesn't need to recognise it has sent one, but not the other, but still, it would be good to eventually be more flexible on this.

vitsalis · 2022-09-14T00:32:02Z

submitter/ckpt_relayer.go

+	if utxos[0].Amount < s.Cfg.TxFee.ToBTC() {
+		return nil, nil, errors.New("insufficient fees")
+	}
+
+	if utxos[1].Amount < s.Cfg.TxFee.ToBTC() {
+		return nil, nil, errors.New("insufficient fees")
+	}


What would happen if the UTXO has exactly the amount for the fees? Then that would mean that the transaction would have a single output containing the OP_RETURN. However, there is a minimum amount of "dust" that should be contained in the outputs of transactions in order for them to be considered by the network.

See this and this.

Never thought of that. Thanks! So, the condition should be utxos[0].Amount < s.Cfg.TxFee.ToBTC() - dust, right? Would that be a config attribute of BTC?

Yep something like that. I can't find the exact reference where the minimum output is implemented, but I found this: https://www.oreilly.com/library/view/mastering-bitcoin/9781491902639/ch08.html#tx_verification

Thanks! I'm not expecting this issue in the near future. I marked it here as a TODO.

Better to create a Github issue about that. It might get lost in the code.

However, I think we should implement that if there's time since it might come to haunt us during the demo.

vitsalis · 2022-09-14T00:33:38Z

submitter/ckpt_relayer.go

+	if err != nil {
+		return nil, err
+	}
+	amount, err := btcutil.NewAmount(50)


This is a hardcoded amount that the UTXO contains. I suppose it is there because of the coinbase transactions that the simnet creates. However, we should get the actual amount that the UTXO contains here and also perform dust checks.

Nice catch!

vitsalis · 2022-09-14T00:34:31Z

submitter/ckpt_relayer.go

+	}
+}
+
+func (s *Submitter) SubmitCkpt(ckpt *ckpttypes.RawCheckpointWithMeta) error {


Some logs here and the below methods would be useful.

…er/sent-tx-to-btc

gitferry · 2022-09-14T02:30:57Z

Thanks, @vitsalis for your review. I have fixed some of them and marked the rest as TODOs since they are not expected to be urgent. Please see the updates. Thanks!

sample-vigilante.yml

vitsalis

Looks good, although I would suggest that we resolve the bug related to orphaned transactions that was identified during the call before merging.

submitter/ckpt_relayer.go

vitsalis · 2022-09-14T09:05:13Z

submitter/ckpt_relayer.go

+		return nil, nil, errors.New("insufficient fees")
+	}
+
+	log.Debugf("Found two unspent transactions, tx1: %v, tx2: %v", utxos[0].TxID, utxos[1].TxID)


During the call, we discussed that someone might use UTXOs that stem from coinbase transactions and might not be available for 100 blocks. It would be good to check that here.

How long before a UTxO is spendable, when it's not a coinbase one?
I saw that @fishermanymc considers epoch lengths of 30 minutes. In that case a submitter might not even have their previous UTxO change confirmed and spendable again?

Other UTXOs are spendable immediately. That is even if they are not included in a block and are included in the mempool of a node. For example, that's how the chaining UTXOs for including both transactions in the same block works: One submits a transacation with very low fees that is nonetheless included in the mempool and then submits a transaction with more than double the fees that spends the UTXO created by the first transaction.

And will the wallet list the new UTxO immediately after sending the transaction to the mempool?

Say I have UTxO1 and UTxO2 at the moment. I see a checkpoint, I spend the two to send TX1 and TX2 to the mempool with the checkpoint in the OP_RETURN fields, and also give myself the change in UTxO3 and UTxO4. Say there are ~9 minutes remaining to the next BTC block, and the polling frequency is the default 60 seconds.

Will the submitter in 1 minute spend UTxO3 and UTxO4 to send TX3 and TX4 with the same checkpoint? Or will the wallet not list these UTxOs until they are included in the next BTC block in 8 minutes time? (It would still spend another two UTxOs if it has them).

If they can be spent, the submitter will submit 9 times in the next BTC block.

And will the wallet list the new UTxO immediately after sending the transaction to the mempool?

Maybe not immediately but almost. The wallet needs some time (less than 1 second) to update the db.

BTC simnet produces blocks in 30 seconds, right? If we move to testnet or mainnet, the decualt polling-interval would be much longer than 60 seconds, similar to epoch length, giving sufficient time for previous checkpoints' status changed to Submitted or Confirmed.

If you will have 1 hour epochs, and the submitters poll once per hour as well, then a submitter could keep missing the end of the epoch by a few minutes, and not submit the checkpoint for almost another hour 😕 Also if someone else gets in at the right time, they can get all the rewards because by the time my submitter does another poll, their submission is stable on BTC.

Ideally the client would subscribe to the event stream from Babylon and submit the checkpoint exactly once when it's notified about it going Sealed in the emitted events of a block. The query would only be used during bootstrapping.

Using the just the poll interval to ensure both that 1) you submit the checkpoint ASAP and 2) you don't submit too often or else you waste your BTC, is awkward at best.

In any case, I have said it enough times now I think :)

Totally agree. Thanks for sharing your thoughts! Polling is a backup plan and subscription would be ideal.

aakoshh

Looks great!

But I think it's missing deduplication logic. Left like this it will spend your UTxOs in a jiffy.

btcclient/tls.go

aakoshh · 2022-09-14T12:55:20Z

config/babylon.go

+		Timeout:          "20s",
+		OutputFormat:     "json",
+		SignModeStr:      "direct",
+		SubmitterAddress: "bbn1v6k7k9s8md3k29cu9runasstq5zaa0lpznk27w",


How do you know this default will be recognised by Babylon? Worth adding a comment I think.

It's just a placeholder showing the format of the submitter address and it's needed for building BTC txs. I guess this will not be recognized by Babylon during the demo. Will add a comment

aakoshh · 2022-09-14T13:00:40Z

submitter/ckpt_poller.go

+			log.Infof("Found %d sealed raw checkpoints", len(sealedRawCkpts))
+			for _, ckpt := range sealedRawCkpts {
+				s.rawCkptChan <- ckpt
+			}


Deduplication? There will be only one sealed checkpoint per epoch. Once you see it, it's not going to be replaced by a different one, so you can just increment an epoch counter, maybe? Although in theory it is possible for epoch N+1 to be sealed while epoch N is not, so maybe that's not the best mechanism. Still, there's no reason to keep sending it every few seconds.

Are you saying that by incrementing an epoch counter ec, the submitter only needs to submit checkpoints with epoch number that is higher than ec? Maybe it's not safe because the sent checkpoints might be lost due to some reason and we need to re-send them if their status is still Sealed. The point is, the submitter is stateless, it periodically sends Sealed checkpoints. And the status of the checkpoint is determined by the Babylon node.

The point is, the submitter is stateless, it periodically sends Sealed checkpoints.

Well, that might sound good for a reporter but the submitter has to pay BTC fees, so it is in its economic interest to send once, and only once. It will take on average 5 minutes until the next BTC block is created where this transaction can be included, and it might take more if the fees are too low and Bitcoin is congested. Meanwhile the checkpoint will stay sealed, but that doesn't mean the submitters have to keep sending. They might have to cancel their previous transactions and re-submit with higher fees, but resending will just waste their money.

So I think that saying the submitter is stateless is false, its state is at least the UTxOs it has, which are changed every time it submits something, and wilfully ignoring it only hurts them.

I totally agree with you. An efficient submitter should be stateful in the sense that it should remember the checkpoints that it has submitted and increases fees if the corresponding txs do not appear on BTC within a certain period.

The current implementation of the submitter follows a best-effort manner without considering economic interest. Given that the submitting frequency, which is roughly the same as the epoch length, is fairly long (1 hour or so). A submitted checkpoint will probably have become Submitted or Confirmed before the next submitting happens, so I would say that re-sending checkpoints for the same epoch happens rarely (only when severe congestion happens). In my recent experiments, submitting a checkpoint is less than 1 USD for inclusion in 10 minutes. So, I think it would be good for the demo.

I created an issue #48 here as a TODO for implementing a smarter submitter.

That sounds like you mean the polling will happen once per hour, but the default is 60 seconds.

Right, but that's for BTC simnet in which blocks are produced in 30 seconds or less, right?

Maybe the polling interval can be based on the BTC interval, rather than the epoch interval.

aakoshh · 2022-09-14T13:06:11Z

submitter/ckpt_relayer.go

+		return err
+	}
+
+	utxo1, utxo2, err := s.getTopTwoUTXOs()


It seems like a bit of an arbitrary requirement to need two UTxOs ready to be spent 🤔
I see how it simplifies the logic here, e.g. after a restart the vigilante doesn't need to recognise it has sent one, but not the other, but still, it would be good to eventually be more flexible on this.

aakoshh · 2022-09-14T13:07:53Z

submitter/ckpt_relayer.go

+	tx2, err := s.buildTxWithData(*utxo2, data2)
+	if err != nil {
+		return err
+	}


If you think this can fail, then maybe build before you send the first to not spend funds in futility?

It seems like a bit of an arbitrary requirement to need two UTxOs ready to be spent 🤔
I see how it simplifies the logic here, e.g. after a restart the vigilante doesn't need to recognise it has sent one, but not the other, but still, it would be good to eventually be more flexible on this.

Thanks for pointing this out. Yeah, it's not an elegant way to do that. Maybe we can chain the two txs together with input. I reported this in the issue #43

If you think this can fail, then maybe build before you send the first to not spend funds in futility?

You are right, the sending should start when both txs are successfully built.

aakoshh · 2022-09-14T13:09:51Z

submitter/ckpt_relayer.go

+		return err
+	}
+
+	log.Debugf("Sent two txs to BTC for checkpointing epoch %v, first txid: %v, second txid: %v", ckpt.Ckpt.EpochNum, txid1.String(), txid2.String())


I think this log should be on the INFO level, not DEBUG. This is where we spent our money and it lists the important transactions to monitor. By contrast the log you will see every few seconds is INFO, which could arguably be DEBUG.

aakoshh · 2022-09-14T13:11:43Z

submitter/ckpt_relayer.go

+
+	// sort utxos by amount in the descending order and pick the first one as input
+	sort.Slice(utxos, func(i, j int) bool {
+		return utxos[i].Spendable && utxos[i].Amount > utxos[j].Amount


Can the slice have unspendable in it? Is that when it's unspent but unconfirmed?

I guess ListUnspent will not return unspendable txs. Adding that just to make sure it's spendable but I think it's safe to remove that condition.

How long before a UTxO is spendable, when it's not a coinbase one?
I saw that @fishermanymc considers epoch lengths of 30 minutes. In that case a submitter might not even have their previous UTxO change confirmed and spendable again?

In my experience, the UTXO is spendable immediately. That's why chaining two txs by input could work.

aakoshh · 2022-09-14T13:13:50Z

submitter/ckpt_relayer.go

+		return nil, nil, errors.New("insufficient fees")
+	}
+
+	log.Debugf("Found two unspent transactions, tx1: %v, tx2: %v", utxos[0].TxID, utxos[1].TxID)


How long before a UTxO is spendable, when it's not a coinbase one?
I saw that @fishermanymc considers epoch lengths of 30 minutes. In that case a submitter might not even have their previous UTxO change confirmed and spendable again?

gitferry · 2022-09-14T13:17:49Z

Thanks, @vitsalis for your comments. The latest commit fixed #40. The bug is caused by the hardcoded vout index (should not be fixed 0) when building tx. The commit also introduced PollingFrequency as a config attribute for the submitter.

gitferry · 2022-09-14T13:57:09Z

Thanks @aakoshh for your review. I addressed/replied them. Please see the updates. thanks!

aakoshh · 2022-09-15T09:39:18Z

config/babylon.go

+		Timeout:          "20s",
+		OutputFormat:     "json",
+		SignModeStr:      "direct",
+		SubmitterAddress: "bbn1v6k7k9s8md3k29cu9runasstq5zaa0lpznk27w", // this is currently a placeholder, will not recognized by Babylon


In that case maybe the DefaultBabylonConfig could have this as a parameter, to make sure it's not forgotten. The example could be on the CLI arg or in the config file as a docstring. Or the Babylon devnet/local testnet config could give some funds to this account in its genesis file.

But at least the comment helps warn about it 👍

Thanks for the advice! I marked it as issue #49 as a reminder.

aakoshh

Great, thanks for the updates!

I maintain my reservation about the deduplication, but you since you seem to resist it for now, I don't want to block you.

aakoshh · 2022-09-15T10:32:12Z

config/submitter.go

-	NetParams string `mapstructure:"netparams"` // should be mainnet|testnet|simnet
+	NetParams        string `mapstructure:"netparams"`   // should be mainnet|testnet|simnet
+	BufferSize       uint   `mapstructure:"buffer-size"` // buffer for raw checkpoints
+	PollingFrequency uint   `mapstructure:"polling-frequency"`


"frequency" is probably not the right word here. The unit of frequency is normally 1/s, so a default of 60 would mean 60 times per second.

Maybe "polling-interval", and if it's numeric, unlike the Timeout in the Babylon Config which is a string like "20s", then it should also mention what unit it's in, like "polling-interval-seconds".

Sounds good. Thanks. Will change this in the next PR.

gitferry added 7 commits September 8, 2022 11:40

add ckpt handlers

790c1bb

added ckpt handler

1d87382

temp

e34548e

Merge branch 'main' of github.com:babylonchain/vigilante into submitt…

918443f

…er/sent-tx-to-btc

integrate wallet

e834def

Merge branch 'main' of github.com:babylonchain/vigilante into submitt…

02ec4a6

…er/sent-tx-to-btc

submitter mvp

bdf4695

gitferry requested review from vitsalis and aakoshh September 13, 2022 14:46

vitsalis reviewed Sep 14, 2022

View reviewed changes

gitferry mentioned this pull request Sep 14, 2022

feat: submitter/implement raw checkpoint handler #16

Closed

gitferry added 2 commits September 14, 2022 10:27

move wallet-related config to btc config

50c2a8d

Merge branch 'main' of github.com:babylonchain/vigilante into submitt…

576d1b1

…er/sent-tx-to-btc

vitsalis reviewed Sep 14, 2022

View reviewed changes

sample-vigilante.yml Show resolved Hide resolved

vitsalis reviewed Sep 14, 2022

View reviewed changes

fixed a bug and introduce polling frequency

732ccc2

aakoshh reviewed Sep 14, 2022

View reviewed changes

minor

7b55118

add support for loading wallet

bc48954

aakoshh reviewed Sep 15, 2022

View reviewed changes

aakoshh approved these changes Sep 15, 2022

View reviewed changes

gitferry merged commit 6256628 into main Sep 15, 2022

gitferry deleted the submitter/sent-tx-to-btc branch September 15, 2022 10:18

aakoshh reviewed Sep 15, 2022

View reviewed changes

gitferry mentioned this pull request Sep 19, 2022

Submitter: failed to send BTC txs due to referencing orphan txs #40

Closed

gitferry mentioned this pull request Feb 7, 2023

Submitter: finding smarter algorithm for retrieving unspent transactions #43

Closed

feat: submitter/send sealed checkpoints to BTC #37

feat: submitter/send sealed checkpoints to BTC #37

Conversation

gitferry commented Sep 13, 2022 • edited Loading

vitsalis 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

Choose a reason for hiding this comment

gitferry commented Sep 14, 2022

vitsalis 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

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

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

gitferry commented Sep 14, 2022

gitferry commented Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

gitferry commented Sep 13, 2022 •

edited

Loading

gitferry commented Sep 14, 2022 •

edited

Loading