chore(btcclient): use notifier interface #20

Lazar955 · 2024-08-23T08:24:05Z

Removes usage of btc client with subscription to zmq
Removes proprietary client implementation of zmq subscription as we now rely on lnd's

References issue

RafilxTenfen · 2024-08-23T18:02:10Z

Can we remove

Line 111 in 998cee9

func (c *Client) DumpPrivKey(address btcutil.Address) (*btcutil.WIF, error) {

func (c *Client) DumpPrivKey(address btcutil.Address) (*btcutil.WIF, error) {
	return c.Client.DumpPrivKey(address)
}

Lazar955 · 2024-08-23T18:19:22Z

Can we remove

vigilante/btcclient/client_wallet.go

Line 111 in 998cee9

func (c *Client) DumpPrivKey(address btcutil.Address) (*btcutil.WIF, error) {
func (c *Client) DumpPrivKey(address btcutil.Address) (*btcutil.WIF, error) {
	return c.Client.DumpPrivKey(address)
}

Yep, I've opened an issue to cleanup btc client and btc wallet interaces, they can be merged. So will be done in a separate PR

RafilxTenfen

LGTM, tested the vigilante with bitcoind on deployments/local and was able to have healthy BTC delegations again 🎉

gitferry

Nice work! Generally, different vigilante programs share a lot of code. So one point of the refactoring is to extract common parts into a libary (e.g., lnd notifier) and reduce duplicate code as much as possible. This PR is a good step heading this direction. We can continue this in future prs

gitferry · 2024-08-24T02:12:21Z

cmd/vigilante/cmd/monitor.go

+			if err != nil {
+				panic(fmt.Errorf("failed to get BTC net params: %w", err))
+			}
+			btcCfg := btcclient.CfgToBtcNodeBackendConfig(cfg.BTC, "")


rawCert is needed for btcd, which we wanted to remove, correct?

Yep, there's a new issue to remove btcd from repo (previously merged PR regarding that was to use it in e2e)

gitferry · 2024-08-24T02:15:08Z

cmd/vigilante/cmd/reporter.go

+			// create the chain notifier
+			btcParams, err := netparams.GetBTCParams(cfg.BTC.NetParams)
+			if err != nil {
+				panic(fmt.Errorf("failed to get BTC net params: %w", err))
+			}
+			btcCfg := btcclient.CfgToBtcNodeBackendConfig(cfg.BTC, "")
+			btcNotifier, err := btcclient.NewNodeBackend(btcCfg, btcParams, &btcclient.EmptyHintCache{})
+			if err != nil {
+				panic(fmt.Errorf("failed to initialize notifier: %w", err))
+			}


This chunk is duplicated across several programs. Maybe we can wrap it into a general constructor?

gitferry · 2024-08-24T02:31:52Z

monitor/btcscanner/block_handler.go

+	if err := bs.btcNotifier.Start(); err != nil {
+		bs.logger.Errorf("Failed starting notifier")
+		return
+	}
+
+	blockNotifier, err := bs.btcNotifier.RegisterBlockEpochNtfn(nil)
+	if err != nil {
+		bs.logger.Errorf("Failed registering block epoch notifier")
+		return
+	}
+
+	defer blockNotifier.Cancel()


Maybe it's better to ducouple the start of the notifier and the block handler by passing the block channel to the handler. See

vigilante/btcstaking-tracker/unbondingwatcher/unbondingwatcher.go

Line 120 in d73b523

go uw.handleNewBlocks(blockEventNotifier)

Yup I would start notifier in start function

gitferry · 2024-08-24T02:33:34Z

reporter/block_handler.go

+	if err := r.btcNotifier.Start(); err != nil {
+		r.logger.Errorf("Failed starting notifier")
+		return
+	}
+
+	blockNotifier, err := r.btcNotifier.RegisterBlockEpochNtfn(nil)
+	if err != nil {
+		r.logger.Errorf("Failed registering block epoch notifier")
+		return
+	}
+
+	defer blockNotifier.Cancel()


gitferry · 2024-08-24T04:07:58Z

reporter/block_handler.go

+			tip := r.btcCache.Tip()
+
+			// Determine if a reorg happened to we know which flow to continue
+			// if the new block has the same height but a different hash.
+			reorg := false
+			if tip != nil {
+				if epoch.Height < tip.Height ||
+					(epoch.Height == tip.Height && epoch.BlockHeader.BlockHash() != tip.Header.BlockHash()) {
+					reorg = true
+				}
+			}
+
 			var errorRequiringBootstrap error
-			if event.EventType == types.BlockConnected {
-				errorRequiringBootstrap = r.handleConnectedBlocks(event)
-			} else if event.EventType == types.BlockDisconnected {
-				errorRequiringBootstrap = r.handleDisconnectedBlocks(event)
+			if !reorg {
+				errorRequiringBootstrap = r.handleConnectedBlocks(epoch.Height, epoch.BlockHeader)
+			} else {
+				errorRequiringBootstrap = r.handleDisconnectedBlocks(epoch.BlockHeader)


It seems that we do the same chaining check (from L35 to L45) in both handleDisconnectedBlocks and handleDisconnectedBlocks. I believe we can combine the logic in one handleNewBlock() method. Basically, if the new block cannot connect to the cache tip, we bootstrap. Otherwise, we add it to the cache, no needing of separate logic

oh that this good idea 👍

My initial thought was that we could, translate new block events into connected / disconnected block events i.e
If we have the chain:
H1 -> H2 -> H3 , now the the reorg happens and the chain becomes H1 - H2' - H3', and now we receive H3' then we can:

Find the common ancestor between forks i.e H1

Calculate that H2 and H3 were removed, so send to disconnection events starting from the top

Now send connection events for H2 and H3

Bootstrapping after reorg is a bit brutal i.e it is costly operation, but given that re-orgs are pretty rare imo it is possible route to take and if we encounter some problems with it we can always go for more fancy approach.

refactor notifier init merge reporter block handles

gitferry

Much cleaner! One last concern is whether the btc scanner can output a canonical chain in expected sequence after bootstraping and event handler switch. It would be good to have a test to cover the cases where bootstraping -> block handler, block handler -> reorg happens -> bootstraping -> block handler. Currently we only have test for the bootstraping case. The tests can be done in this or separate pr

gitferry · 2024-08-26T14:08:45Z

cmd/vigilante/cmd/btcstaking_tracker.go

-			btcNotifier, err := btcclient.NewNodeBackend(btcCfg, btcParams, &btcclient.EmptyHintCache{})
-			if err != nil {
-				panic(fmt.Errorf("failed to create btc chain notifier: %w", err))
+				panic(err)


Suggested change

panic(err)

panic(fmt.Errorf("failed to create btc chain notifier: %w", err))

errors are already wrapped in the NewNodeBackendWithParams

gitferry · 2024-08-26T14:23:44Z

monitor/btcscanner/block_handler.go

+		// failing to request the block, which means a bug
+		panic(err)


Suggested change

// failing to request the block, which means a bug

panic(err)

// failing to request the block

panic(fmt.Errorf("failed to request block by hash: %s", blockHash.String))

This could also be network issue

good spot, typo in suggestion

gitferry · 2024-08-26T14:44:45Z

monitor/btcscanner/btc_scanner.go

 	go bs.Bootstrap()

-	bs.BtcClient.MustSubscribeBlocks()
-
 	bs.Started.Store(true)
 	bs.logger.Info("the BTC scanner is started")

+	if err := bs.btcNotifier.Start(); err != nil {
+		bs.logger.Errorf("Failed starting notifier")
+		return
+	}
+
+	blockNotifier, err := bs.btcNotifier.RegisterBlockEpochNtfn(nil)
+	if err != nil {
+		bs.logger.Errorf("Failed registering block epoch notifier")
+		return
+	}
+
 	// start handling new blocks
 	bs.wg.Add(1)
-	go bs.blockEventHandler()
+	go bs.blockEventHandler(blockNotifier)


I was worried about possible racing issue between the Bootstrap and blockEventHandler threads. They both produce confirmed blocks to the same channel, so they need to be sequencial. I can think of two options:

keep them as two separate go routines and use the Synced controller to ensure blockEventHandler happens after Bootstrap

combine them into a single goroutine and when creating the notifier

I'm in favor of option (2) in which we probably can remove the Synced controller. Nevertheless, after bootstrapping, we should be able to get the best known block and put it in btcNotifier.RegisterBlockEpochNtfn(bestKnownBlock), so that we don't duplicate the chain tip

gitferry · 2024-08-26T14:49:45Z

reporter/reporter.go

+		return
+	}
+
+	blockNotifier, err := r.btcNotifier.RegisterBlockEpochNtfn(nil)


Can we get the best known block after bootstrapping and set it as the parameter? Otherwise, the notifier will immediately get the chain tip which could be duplicate with the one processed during bootstrap

gitferry

Thanks for all the changes. Lgtm! It would be nice to have comprehensive tests about boostraping and normal block events but can be done in a separate pr

gitferry · 2024-08-27T12:58:33Z

monitor/btcscanner/block_handler.go

+	blockHash := header.BlockHash()
+	ib, _, err := bs.BtcClient.GetBlockByHash(&blockHash)
+	if err != nil {
+		// failing to request the block, which means a bug


Suggested change

// failing to request the block, which means a bug

// failing to request the block

Lazar955 · 2024-08-27T13:21:37Z

@gitferry

Thanks, created a new issue for that

Lazar955 added 10 commits August 22, 2024 13:22

reporter uses notifier

3f0bb19

monitor uses notifier

1991b21

use btc client without subscription

55c846d

remove zmq impl

eaa9900

update mocks

8575dd1

cleanup unused code

998cee9

cleanup

376bc3a

fix test

93f04e3

build flag

3d19173

rm commented out code

e3eb8b7

Lazar955 requested a review from KonradStaniec August 23, 2024 13:09

Lazar955 marked this pull request as ready for review August 23, 2024 13:09

Lazar955 requested review from gitferry and RafilxTenfen August 23, 2024 13:09

RafilxTenfen approved these changes Aug 24, 2024

View reviewed changes

gitferry reviewed Aug 24, 2024

View reviewed changes

Lazar955 added 3 commits August 26, 2024 13:13

pr comments

bdf4f9e

refactor notifier init merge reporter block handles

btc scanner merge handle block logic

f7335e2

rm unused code

df198b3

Lazar955 requested a review from gitferry August 26, 2024 13:40

gitferry reviewed Aug 26, 2024

View reviewed changes

combine booststrap and blockevent handle

e47d580

Lazar955 requested a review from gitferry August 27, 2024 08:40

gitferry approved these changes Aug 27, 2024

View reviewed changes

Lazar955 merged commit e992231 into dev Aug 27, 2024
8 checks passed

Lazar955 deleted the lazar/use-notifier branch August 27, 2024 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(btcclient): use notifier interface #20

chore(btcclient): use notifier interface #20

Lazar955 commented Aug 23, 2024

RafilxTenfen commented Aug 23, 2024

Lazar955 commented Aug 23, 2024

RafilxTenfen left a comment

gitferry left a comment

gitferry Aug 24, 2024

Lazar955 Aug 26, 2024

gitferry Aug 24, 2024

gitferry Aug 24, 2024

KonradStaniec Aug 26, 2024

gitferry Aug 24, 2024

gitferry Aug 24, 2024

KonradStaniec Aug 26, 2024

gitferry left a comment

gitferry Aug 26, 2024

Lazar955 Aug 27, 2024

gitferry Aug 26, 2024

gitferry Aug 26, 2024

Lazar955 Aug 27, 2024

gitferry Aug 26, 2024

gitferry Aug 26, 2024

gitferry left a comment

gitferry Aug 27, 2024

Lazar955 commented Aug 27, 2024 •

edited

Loading

	panic(err)
	panic(fmt.Errorf("failed to create btc chain notifier: %w", err))

		// failing to request the block, which means a bug
		panic(err)

	// failing to request the block, which means a bug
	// failing to request the block

chore(btcclient): use notifier interface #20

chore(btcclient): use notifier interface #20

Conversation

Lazar955 commented Aug 23, 2024

RafilxTenfen commented Aug 23, 2024

Lazar955 commented Aug 23, 2024

RafilxTenfen left a comment

Choose a reason for hiding this comment

gitferry 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

gitferry 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

gitferry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lazar955 commented Aug 27, 2024 • edited Loading

Lazar955 commented Aug 27, 2024 •

edited

Loading