reporter: improving resilience #69
Conversation
| client.Client = rpcClient | ||
| return client, nil | ||
| } | ||
|
|
||
| func (c *Client) SubscribeBlocksByWebSocket() { | ||
| if err := c.NotifyBlocks(); err != nil { |
There was a problem hiding this comment.
If we are getting error here , do we want to retry to improve resilience?
There was a problem hiding this comment.
Agree. Added a function MustSubscribeBlocksByWebSocket and made the reporter to use it 👍
| @@ -8,6 +8,21 @@ import ( | |||
| "github.com/btcsuite/btcd/wire" | |||
| ) | |||
|
|
|||
| // GetBestBlock provides similar functionality with the btcd.rpcclient.GetBestBlock function | |||
| // We implement this, because this function is only provided by btcd. | |||
| func (c *Client) GetBestBlock() (*chainhash.Hash, uint64, error) { | |||
There was a problem hiding this comment.
correct me if wrong, this is to support bitcoind? So vigilante can switch easily b/w different clients right?
There was a problem hiding this comment.
also can you explain if bitcoind is a go implementation of bitcoin core just like btcd, if yes I couldn't find the go package
There was a problem hiding this comment.
Yep. The reason of implementing this again rather than using the one provided by rpcclient is that the provided one uses APIs that are not supported by bitcoind. https://github.com/btcsuite/btcd/blob/a27738721a1ecdae504fb4738846401544a53e4d/rpcclient/extensions.go#L189-L192
There was a problem hiding this comment.
also can you explain if bitcoind is a go implementation of bitcoin core just like btcd, if yes I couldn't find the go package
It's https://github.com/bitcoin/bitcoin. This is the earliest and most widely used Bitcoin implementation, and is written in C++.
babylonclient/query.go
Outdated
| @@ -69,6 +72,22 @@ func (c *Client) QueryBTCCheckpointParams() (*btcctypes.Params, error) { | |||
| return &resp.Params, nil | |||
| } | |||
|
|
|||
| func (c *Client) MustQueryBTCCheckpointParams() *btcctypes.Params { | |||
| var params *btcctypes.Params | |||
| err := types.Retry(1*time.Second, 1*time.Minute, func() error { // TODO parameterise | |||
There was a problem hiding this comment.
Why not use the same retrying parameters that are used elsewhere?
There was a problem hiding this comment.
Since the parameters are attached to reporter's config, that Babylon client does not have access. Updated the TODO on making the retry parameters universal for all packages.
There was a problem hiding this comment.
Awesome, thanks! A github issue might help keep track as well.
There was a problem hiding this comment.
Good idea! Created #70
btcclient/client.go
Outdated
|
|
||
| func (c *Client) MustSubscribeBlocksByWebSocket() { | ||
| err := types.Retry(1*time.Second, 1*time.Minute, func() error { // TODO parameterise | ||
| return c.NotifyBlocks() |
There was a problem hiding this comment.
We could use the SubscribeBlocksByWebSocket here.
| @@ -24,6 +24,12 @@ func (r *Reporter) Init() { | |||
|
|
|||
| /* ensure BTC has catched up with BBN header chain */ | |||
|
|
|||
| // Find the base height | |||
| _, bbnBaseHeight, err = r.babylonClient.QueryBaseHeader() | |||
There was a problem hiding this comment.
Maybe we can make this a MustQueryBaseHeader.
There was a problem hiding this comment.
Yeah @gusin13 has a ticket https://babylon-chain.atlassian.net/browse/BM-183 dedicated for replacing panics with Must-style functions.
| @@ -75,13 +83,9 @@ func (r *Reporter) Init() { | |||
| } | |||
| log.Debugf("BTC cache size: %d", tempBTCCache.Size()) | |||
|
|
|||
| /* Initial consistency check: whether the `max(bbn_tip_height - confirmation_depth, bbn_base_height)`-th block is same */ | |||
| r.btcClient.MustSubscribeBlocksByWebSocket() | |||
There was a problem hiding this comment.
We want to ensure that the block subscription starts right after snapshotting the BTC chain (which is done by initBTCCache), so that the subscribed blocks and snapshotted blocks do not have overlap. Otherwise, if we subscribe too early, then they will have overlap, leading to duplicated header/ckpt submissions.
Added some justification on this.
Fixes BM-186
This PR implements some measures to improve the reporter's resilience.
It is separated from #52. The PR #52 will be dedicated for implementing the bitcoind-compatible BTC client.