Move retry config out of reporter so its accessible by other clients #78

gusin13 · 2022-10-01T19:29:50Z

Retry config is part of Reporter as of now due to which Babylon and BTC client can't access config params. This PR moves Retry in common config which will be accessible by all clients.

SebastianElvis · 2022-10-04T05:01:12Z

While this PR is still WIP, one comment is to make retry config (and other potential ones in the future) a part of BaseConfig, rather than having a new RetryConfig. The BaseConfig was reserved exactly for configs that might be shared among modules.

SebastianElvis

LGTM! Finally we can merge all redundant configs in different modules to CommonConfig 👍

sample-vigilante-docker.yml

reporter/reporter.go

vitsalis

Nice work! Added some comments related to refactoring (and some others that are not entirely related to this PR, we can create another one to address those or do that here, your call):

babylonclient/client.go

babylonclient/query.go

btcclient/client_block_subscriber.go

vitsalis · 2022-10-05T12:48:40Z

config/common.go

+// CommonConfig defines the server's basic configuration
+type CommonConfig struct {
+	// Backoff interval for the first retry.
+	RetrySleepTime string `mapstructure:"retry-sleep-time"`


Some configuration entries are parseable time strings, while others are name-denomination (e.g. resend-interval-seconds). We should decide on the format that we want to use and stick to that for everything.

Right, we can make it consistent. Should I change all of them to strings? As this involves submitter also, I will probably do this in a separate PR. cc @gitferry

Sure. Is the format that we want to stick with the one that you have here or the name-denomination one?

I recall there was a suggestion by @aakoshh on one of @gitferry 's PRs that we use the name-denomination one but can't find the comment.

found Aakoshh's comment , this one. Also, I noticed for Babylon we are using string format here, so if we change the format we will need to change in BBN as well.

No problem with me and ok with this happening on another PR or here. In this PR we should make sure that we use the final version.

config/reporter.go

gusin13 · 2022-10-08T21:24:44Z

@vitsalis I have added suggested fixes in this PR now, feel free to review it again. Here is the summary of issues and fixes.

Central validation - Validation will be done only once when the config is loaded, no need to do it per client. Fixed here
Pass explicitly config attributes - Fixed here
Use a consistent format for time-based attributes -
- I noticed something cool about the Viper library we use to parse configs, time durations can be directly loaded from yaml so no need to pass it as strings. Fixed here.
- I have made a separate ticket to refactor this in the submitter. BM-221
Missing validation for netparams - Fixed here

vitsalis

Very nice work, much cleaner! Some very very minor comments, but I'm ok with most of them not happening.

config/babylon.go

vitsalis · 2022-10-10T06:50:49Z

config/babylon.go

-			return err
-		}
+	if cfg.BlockTimeout < 0 {
+		return errors.New("block-timeout can't be negative")


BlockTimeout is not being used anywhere in the code but it exists in the config so I think it's fine to keep it 0sec as of now.

config/bitcoin.go

vitsalis · 2022-10-10T06:52:25Z

config/bitcoin.go

@@ -46,7 +51,7 @@ func DefaultBTCConfig() BTCConfig {
 		WalletCAFile:      defaultBtcWalletCAFile,
 		WalletLockTime:    10,
 		TxFee:             feeAmount,
-		NetParams:         "simnet",
+		NetParams:         types.BtcSimnet.String(),


netparams/bitcoin.go

reporter/btc_handlers.go

vitsalis · 2022-10-10T06:55:32Z

reporter/reporter.go

-		return nil, err
-	}
-
+func New(cfg *config.ReporterConfig, btcClient *btcclient.Client, babylonClient *babylonclient.Client, retrySleepTime, maxRetrySleepTime time.Duration) (*Reporter, error) {


Maybe split this line since it's getting too long.

vitsalis · 2022-10-10T06:55:50Z

sample-vigilante.yml

@@ -27,7 +28,7 @@ babylon:
  key-directory: $TESTNET_PATH/node0/babylond
  debug: true
  timeout: 20s
-  block-timeout: ""
+  block-timeout: ~


Is this a placeholder for an empty string?

block-timeout is a time duration, so it can't be a string. ~ is null value so by default while parsing it will parse it as 0 sec

vitsalis · 2022-10-10T06:56:17Z

types/utils.go

@@ -5,6 +5,18 @@ import (
 	"github.com/btcsuite/btcd/wire"
 )

+type SupportedBtcNetwork string


vitsalis · 2022-10-10T06:58:18Z

types/utils.go

@@ -17,3 +29,13 @@ func GetWrappedTxs(msg *wire.MsgBlock) []*btcutil.Tx {

 	return btcTxs
 }
+
+func GetValidNetParams() map[string]bool {


Since this function is only used for an existence check, why do we abstract this existence check behind a function that returns a map and this map is being used to check for inclusion? No problem with it if it matches your style.

Co-authored-by: Vitalis Salis <[email protected]>

gusin13 added 3 commits October 3, 2022 23:05

move retry policy out of reporter config

49199ac

add retry policy in yaml

0a9e1be

utilize retry policy config in retry usages

011bdce

gusin13 force-pushed the BM-199-move-retry-config-out-of-reporter-so-its-accessible-by-other-clients branch from 04e571e to 011bdce Compare October 4, 2022 03:21

gusin13 added 2 commits October 3, 2022 23:30

fix log statements

25a55c0

Fix comments

a00dd62

gusin13 added 4 commits October 4, 2022 16:58

move retry to common config

3a7d30d

fix config access

f2d4421

fix config in bbn client

dab57f5

fix nil ptr issue in btcclient due to commoncfg

a6f556a

gusin13 marked this pull request as ready for review October 4, 2022 21:51

gusin13 requested review from SebastianElvis, vitsalis, aakoshh, gitferry and fishermanymc October 4, 2022 21:55

SebastianElvis approved these changes Oct 4, 2022

View reviewed changes

sample-vigilante-docker.yml Outdated Show resolved Hide resolved

reporter/reporter.go Outdated Show resolved Hide resolved

gusin13 added 3 commits October 5, 2022 00:53

change base to common in docker file

0d7d25e

change func sig

18a3547

fix caller

84a8af5

vitsalis reviewed Oct 5, 2022

View reviewed changes

gusin13 and others added 8 commits October 7, 2022 00:20

Refactor common config (#82)

64e35d2

Fix netparams (#83)

5a990b0

add comments

64b6684

add comments

2f7d613

fix validation func

1757da8

yaml can ingest time durations directly

aa9f3f6

fix config attrs

83a2408

remove unused vars

879f867

gusin13 added 2 commits October 8, 2022 15:31

remove redundant spacing

cd32ba3

insert no val for block timeout

cf0b80f

gusin13 requested a review from vitsalis October 8, 2022 21:26

vitsalis approved these changes Oct 10, 2022

View reviewed changes

gusin13 and others added 4 commits October 11, 2022 16:38

update error description

58e2c0d

Co-authored-by: Vitalis Salis <[email protected]>

change error description for timeout

e3b05bb

add signet back to valid net params

1353bf6

split long signature

d628033

gusin13 merged commit e8d94c6 into main Oct 11, 2022

gusin13 deleted the BM-199-move-retry-config-out-of-reporter-so-its-accessible-by-other-clients branch October 11, 2022 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move retry config out of reporter so its accessible by other clients #78

Move retry config out of reporter so its accessible by other clients #78

gusin13 commented Oct 1, 2022 •

edited

Loading

SebastianElvis commented Oct 4, 2022

SebastianElvis left a comment

vitsalis left a comment

vitsalis Oct 5, 2022

gusin13 Oct 6, 2022

vitsalis Oct 6, 2022

vitsalis Oct 6, 2022

gusin13 Oct 6, 2022 •

edited

Loading

vitsalis Oct 7, 2022

gusin13 commented Oct 8, 2022 •

edited

Loading

vitsalis left a comment

vitsalis Oct 10, 2022

gusin13 Oct 11, 2022

vitsalis Oct 10, 2022

vitsalis Oct 10, 2022

gusin13 Oct 11, 2022

vitsalis Oct 10, 2022

gusin13 Oct 11, 2022

vitsalis Oct 10, 2022

vitsalis Oct 10, 2022

Move retry config out of reporter so its accessible by other clients #78

Move retry config out of reporter so its accessible by other clients #78

Conversation

gusin13 commented Oct 1, 2022 • edited Loading

SebastianElvis commented Oct 4, 2022

SebastianElvis left a comment

Choose a reason for hiding this comment

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

gusin13 Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 commented Oct 8, 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

gusin13 commented Oct 1, 2022 •

edited

Loading

gusin13 Oct 6, 2022 •

edited

Loading

gusin13 commented Oct 8, 2022 •

edited

Loading