reporter: setup zmq block subscription #92

gusin13 · 2022-10-24T03:14:41Z

Fixes

https://babylon-chain.atlassian.net/browse/BM-208

Description

This PR adds zmq-based block subscription support in the reporter.

Setup

Download bitcoin core - https://bitcoin.org/en/wallets/desktop/mac/bitcoincore/
Add following in bitcoin.conf

regtest=1
server=1
rpcuser=rpcuser
rpcpassword=rpcpass
zmqpubrawblock=tcp://127.0.0.1:29000
zmqpubrawtx=tcp://127.0.0.1:29000
zmqpubhashtx=tcp://127.0.0.1:29000
zmqpubhashblock=tcp://127.0.0.1:29000
zmqpubsequence=tcp://127.0.0.1:29000

Run bitcoin-cli getzmqnotifications in terminal to confirm if notifications are enabled

Run the reporter
Generate new block bitcoin-cli -generate 1

ZMQ will handle this notification in reporter.

SebastianElvis

Thanks for figuring out all the ZMQ stuff, which looks challenging! Left some comments on the overall design.

SebastianElvis · 2022-10-30T22:23:42Z

sample-vigilante.yml

+  no-client-tls: true
  ca-file: $TESTNET_PATH/bitcoin/rpc.cert
-  endpoint: localhost:18556
+  endpoint: localhost:18443


Why do we change these parameters?

this is the port required for bitcoind regtest

SebastianElvis · 2022-10-30T22:24:20Z

sample-vigilante.yml

  tx-fee: 2500
  wallet-endpoint: localhost:18554
  wallet-password: walletpass
  wallet-name: default
  wallet-lock-time: 10
  wallet-ca-file: $TESTNET_PATH/bitcoin/rpc-wallet.cert
-  net-params: simnet
+  net-params: regtest


Maybe worth adding some comments about the supported net parameters of Btcd and bitcoind.

SebastianElvis · 2022-10-30T22:24:45Z

types/blockevent.go

+
+	TransactionRemoved
+
+	TransactionAdded


Are we interested in these events?

nope, we are not using it anywhere, I added it because these are returned from the sequence socket.

Then I would say we ignore these events, since they are not related to blocks while our event type is called BlockEvent.

sample-vigilante.yml

SebastianElvis · 2022-10-30T22:28:26Z

btcclient/client_block_subscriber.go

+			HTTPPostMode: true,
+			User:         cfg.Username,
+			Pass:         cfg.Password,
+			DisableTLS:   cfg.DisableClientTLS,


Is this because bitcoind does not support TLS?

right, it doesn't support TLS.

SebastianElvis · 2022-10-30T22:34:00Z

btcclient/client_block_subscriber.go

+		panic(err)
+	}
+
+	c.ZmqSequenceMsgChan = ch


Could we filter BlockConnected and BlockDisconnected events from ZmqSequenceMsgChan, and forward these events to BlockEventChan in a goroutine, so that we can omit the public ZmqSequenceMsgChan channel in Client?

I think in that case ZmqSequenceMsgChan would still need to be a public channel as we process events in different package here ?

If we use a unified channel, then the two different handlers can be merged, right?

I see, so I think you mean we don't need ZmqSequenceMsgChan, we should directly push messages to BlockEventChan from zmq library here. Yes that can be done 👍

Yep, I feel like this can simplify the design a little bit

I was refraining to use bbn/vigilante code inside zmq package as I wanted to keep it separate but I agree above suggestion will simplify stuff, will fix it soon.

Note

As the zmq socket just gives us block hash, to make the BlockEvent , I will need to fetch header/height using rpc client , so need to pass rpc client info in the zmq lib as well.

Right, for modularity I think we can make events like TransactionRemoved and ZmqSequenceMsgChan to be a part of this Zmq library, while making BTC client to only consume block-related events.

SebastianElvis · 2022-10-30T22:37:12Z

config/bitcoin.go

@@ -32,6 +35,17 @@ func (cfg *BTCConfig) Validate() error {
 	if _, ok := types.GetValidNetParams()[cfg.NetParams]; !ok {
 		return errors.New("invalid net params")
 	}
+
+	if cfg.EnableZmq {
+		if cfg.ZmqPubAddress == "" {


Probably a TODO on regex check of IP address

SebastianElvis · 2022-10-30T22:37:52Z

reporter/block_handler.go

 func (r *Reporter) blockEventHandler() {
 	defer r.wg.Done()
 	quit := r.quitChan()

 	for {
 		select {
-		case event := <-r.btcClient.BlockEventChan:
+		case event, open := <-r.btcClient.BlockEventChan:


SebastianElvis · 2022-10-30T22:38:53Z

reporter/block_handler.go

@@ -23,6 +31,56 @@ func (r *Reporter) blockEventHandler() {
 	}
 }

+// zmqSequenceMessageHandler handles sequence messages from the ZMQ sequence socket.
+func (r *Reporter) zmqSequenceMessageHandler() {


This can be replaced by the redirection approach suggested above

SebastianElvis · 2022-10-30T22:41:22Z

zmq/client.go

@@ -0,0 +1,115 @@
+package zmq


Great work on the ZMQ library, which is a tough one! Since only btcclient will use this zmq library, can we move zmq/ to btcclient/zmq? In addition, IIRC some of the code is from an open-source codebase, so perhaps we can attach URL on the origin of these code here.

I have added a reference to open source code at start of the package but couldn't move zmq inside btcclient due to cyclic import issues.

SebastianElvis

Perfect, thanks for figuring these Zmq things out!

SebastianElvis · 2022-11-02T00:29:48Z

btcclient/client_block_subscriber.go

-	}
-	if backend != rpcclient.Btcd {
-		return nil, fmt.Errorf("NewWithBlockSubscriber is only compatible with Btcd")
+		// ensure we are using bitcoind as Bitcoin node, as zmq is only supported by bitcoind


These checks between L50-L56 can happen before L45, right?

SebastianElvis · 2022-11-02T00:43:26Z

zmq/subscribe.go

+	c.blockEventChan <- types.NewBlockEvent(event, ib.Height, ib.Header)
+}
+
+func (c *Client) getBlockByHash(blockHash *chainhash.Hash) (*types.IndexedBlock, *wire.MsgBlock, error) {


This function looks like the cause why the Zmq client needs the pointer to the BTC client. It would be good if we avoid them referencing each other. Is it possible to

remove the height from BlockEvent and remove this function and the pointer to the BTC client, or

pass BlockEvent with zero height when using Zmq?

since the height in BlockEvent is not used anyway. This also might allow us to put zmq/ under btcclient/ as well.

ZMQ only needs BTC client to query BTC. Instead pointing to BTC client , I inserted rpc client inside zmq client so it can query directly here

in this PR I inserted rpc client in the zmq client here

I was asking the possibility to avoid the zmq client to query BTC client. There might be some dependency issues when BTC client and Zmq client are referencing each other. From a modularity point of view the Zmq client should not refer to BTC client as well. The ideal case would be that Zmq client pushes messages to the BlockEvent channel without querying BTC client.

The ideal case would be that Zmq client pushes messages to the BlockEvent channel without querying BTC client.

Sure this is possible.

Is it possible to

remove the height from BlockEvent and remove this function and the pointer to the BTC client, or

pass BlockEvent with zero height when using Zmq?

I think this might not be work as zmq socket just provides block hash, it doesn't give us header or height so omitting height won't be suffice. We should rather change BlockEvent to accept just hash as both bitcoind and btcd provides hash on notification arrival. We can then query BTC when this block is handled in blockEventHandler

added TODO for this.

Yeah I would also prefer removing the height from BlockEvent. Let's do it in subsequent PRs. 👍

Co-authored-by: Runchao Han <[email protected]>

vitsalis

Great work! Minor comments

vitsalis · 2022-11-02T10:48:52Z

btcclient/client_block_subscriber.go

+			HTTPPostMode: true,
+			User:         cfg.Username,
+			Pass:         cfg.Password,
+			DisableTLS:   cfg.DisableClientTLS,


When bitcoind is used, DisableClientTLS should always be true, right?

yup, will fix it in next PR.

vitsalis · 2022-11-02T10:50:32Z

btcclient/client_block_subscriber.go

+		if err != nil {
+			return nil, fmt.Errorf("failed to get BTC backend: %v", err)
+		}
+		if backend != rpcclient.BitcoindPost19 {


What is BitcoindPost19?

These are enums given by the rpc client library

BTCD is used for btcd

BitcoindPost19 is used for bitcoind

Check here

vitsalis · 2022-11-02T10:53:37Z

btcclient/client_block_subscriber.go

+	case types.WebsocketMode:
+		notificationHandlers := rpcclient.NotificationHandlers{
+			OnFilteredBlockConnected: func(height int32, header *wire.BlockHeader, txs []*btcutil.Tx) {
+				log.Debugf("Block %v at height %d has been connected at time %v", header.BlockHash(), height, header.Timestamp)


These debugging logs only appear on a btcd connection. To be more consistent, maybe we could add those in the consumer channel?

vitsalis · 2022-11-02T10:58:01Z

config/bitcoin.go

+	WalletName        string                          `mapstructure:"wallet-name"`
+	WalletCAFile      string                          `mapstructure:"wallet-ca-file"`
+	WalletLockTime    int64                           `mapstructure:"wallet-lock-time"` // time duration in which the wallet remains unlocked, in seconds
+	TxFee             btcutil.Amount                  `mapstructure:"tx-fee"`           // BTC tx fee, in BTC


This is in Satoshi

right, I will fix it in the next PR.

gusin13 force-pushed the BM-208-investigate-zmq-for-bitcoind branch from 8c372ab to 5afdeec Compare October 29, 2022 17:35

gusin13 changed the title ~~Setup ZMQ in reporter~~ Setup ZMQ subscription in Reporter Oct 30, 2022

gusin13 changed the title ~~Setup ZMQ subscription in Reporter~~ Setup ZMQ based block handling in Reporter Oct 30, 2022

gusin13 changed the title ~~Setup ZMQ based block handling in Reporter~~ Setup zmq based block handling in reporter. Oct 30, 2022

gusin13 changed the title ~~Setup zmq based block handling in reporter.~~ reporter: setup zmq block subscription. Oct 30, 2022

gusin13 changed the title ~~reporter: setup zmq block subscription.~~ reporter: setup zmq block subscription Oct 30, 2022

gusin13 marked this pull request as ready for review October 30, 2022 02:25

gusin13 requested review from SebastianElvis, vitsalis, gitferry, fishermanymc and KonradStaniec October 30, 2022 02:26

SebastianElvis reviewed Oct 30, 2022

View reviewed changes

SebastianElvis approved these changes Nov 2, 2022

View reviewed changes

gusin13 added 16 commits November 1, 2022 22:18

lib setup

104f3b8

lib setup

0ef93fd

spin up go routine to ingest zmq seq

2f9cc12

print more info

d3941d5

push zmq skeleton code

4883c15

refactor zmq

62c4a2c

add config back in client

7b79adc

rename package

c58066d

pass pointer to zmq chan

e88c8db

ingest zmq seq messages

b7e9dc4

go fmt files

ca625a0

change vigilante config

01068bc

install deps in circle ci

aec7689

add sudo in command

b319c8c

list pkg

b96721d

fix yaml

d05bf63

gusin13 and others added 26 commits November 1, 2022 22:25

subscribe blocks via zmq in bootstrap

896b9dc

fix spell

139780c

mod tidy

bd26c72

rename all receivers client

87be082

remove empty comment

042b59c

handle closed channel properly

e9b393b

add func comments

62bfc61

add logs

e947bdd

change log string

d25c712

no need to send to channel, directly process blocks

d7aa00d

update validation rules

e90951b

fix typo

629e887

add regtest params

adb533d

add validation rules

732d434

improve comment string

54bddcd

zmq specific config

685cd12

rename to subscription-mode

441437c

Co-authored-by: Runchao Han <[email protected]>

set valid sub modes, pass it through config

f122811

remove unwanted events

00c89cf

refactor zmq lib and pub directly to BlockEventChan (#97)

d5f9db1

ensure proper backend

fbf5f37

add comments in yaml

e341666

add more comments

60dc3ec

move lines

a8d6312

add todo

927d116

fix pkg

d53144e

gusin13 force-pushed the BM-208-investigate-zmq-for-bitcoind branch from 815f460 to d53144e Compare November 2, 2022 02:37

gusin13 merged commit f743696 into main Nov 2, 2022

gusin13 deleted the BM-208-investigate-zmq-for-bitcoind branch November 2, 2022 02:40

vitsalis reviewed Nov 2, 2022

View reviewed changes

reporter: setup zmq block subscription #92

reporter: setup zmq block subscription #92

Conversation

gusin13 commented Oct 24, 2022 • edited Loading

Fixes

Description

Setup

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

gusin13 Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Oct 31, 2022 • edited Loading

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 31, 2022 • edited Loading

Choose a reason for hiding this comment

SebastianElvis Nov 1, 2022 • edited Loading

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 Nov 2, 2022 • edited Loading

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

gusin13 Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 commented Oct 24, 2022 •

edited

Loading

gusin13 Nov 1, 2022 •

edited

Loading

gusin13 Oct 31, 2022 •

edited

Loading

gusin13 Oct 31, 2022 •

edited

Loading

SebastianElvis Nov 1, 2022 •

edited

Loading

gusin13 Nov 2, 2022 •

edited

Loading

gusin13 Nov 2, 2022 •

edited

Loading

gusin13 Nov 2, 2022 •

edited

Loading