Features/async walletnotify #266

phillamy · 2022-05-10T23:19:00Z

First commit usng MQTT to send events from core to Proxy. Event are filtrer t send watching wallets tx only.

Next: remove code in confirmation and use data received in sub message

phillamy · 2022-05-16T18:55:37Z

I added coreutils in Dockerfile to use the most recent version of base64.

The date command changed breaking this. -D Option is not an option anymore:
inserted_ts=$(date -d "$(echo "${watching}" | cut -d '|' -f2)" -D '%Y-%m-%d %H:%M:%S' +"%s")

Line replaced by (in manage_missed_conf.sh):
inserted_ts=$(date -d "$(echo "${watching}" | cut -d '|' -f2)" +%s)

phillamy · 2022-05-16T19:05:18Z

Core will filter out transactions based on the wallet and will only send trx concerning the "watching" wallets. Core sends the trx details to the broker since it had to do a "gettransaction" to identify the wallet. The proxy will not have to do this anymore

phillamy · 2022-05-17T01:13:29Z

I had to build a new core image called "async-blocknotify". It brings a script that gets called by walletnotify=...

In setup.sh, you can see BITCOIN_VERSION="async-blocknotify"

Kexkey · 2022-06-08T15:42:09Z

I had to build a new core image called "async-blocknotify". It brings a script that gets called by walletnotify=...

In setup.sh, you can see BITCOIN_VERSION="async-blocknotify"

I don't understand why you had to build a new Bitcoin Core image? The templates' bitcoin/bitcoin.conf file has the new walletnotify instruction, no?

Kexkey · 2022-06-08T15:58:45Z

I had to build a new core image called "async-blocknotify". It brings a script that gets called by walletnotify=...
In setup.sh, you can see BITCOIN_VERSION="async-blocknotify"

I don't understand why you had to build a new Bitcoin Core image? The templates' bitcoin/bitcoin.conf file has the new walletnotify instruction, no?

Ah it's probably to include the mosquitto package to be able to publish the message...

Kexkey · 2022-06-10T15:43:02Z

cyphernodeconf_docker/templates/bitcoin/walletnotify.sh

+    tx=$(bitcoin-cli -rpcwallet=$wallet gettransaction $txid true)
+
+    if [ -n "$tx" ]; then
+      echo "[walletnotify-$pid] Found [$txid] in wallet [$wallet]"


Insert something like tx=$(echo $tx | jq -Mc) here to remove unnecessary json formatting

Kexkey · 2022-06-10T15:48:15Z

cyphernodeconf_docker/templates/bitcoin/walletnotify.sh

+    if [ -n "$tx" ]; then
+      echo "[walletnotify-$pid] Found [$txid] in wallet [$wallet]"
+      echo "[walletnotify-$pid] mosquitto_pub -h broker -t confirmation -m \"$tx\" "
+      mosquitto_pub -h broker -t confirmation -m $(echo $tx | base64 -w 0)


I would rename the topic bitcoin_watching_walletnotify to make it clear that it's triggered by Bitcoin Core's walletnotify on a watching wallet. Especially since in confirmation(), we publish something on tx_confirmation topic after making sure the related address is still being watched by CN.

Kexkey · 2022-06-10T15:49:08Z

cyphernodeconf_docker/templates/bitcoin/walletnotify.sh

+#!/bin/sh
+
+walletnotify(){
+  local pid=$(cut -d' ' -f4 < /proc/self/stat)


You can use $$ to get the PID

Kexkey · 2022-06-10T15:53:15Z

dist/setup.sh

@@ -885,7 +891,7 @@ PROXYCRON_VERSION="v0.9.0-dev"
 OTSCLIENT_VERSION="v0.9.0-dev"
 PYCOIN_VERSION="v0.9.0-dev"
 CYPHERAPPS_VERSION="dev"
-BITCOIN_VERSION="v22.0"
+BITCOIN_VERSION="async-blocknotify"


Ok I'll publish v22.0-mosquitto to Docker's hub, we can use it until I publish v23 that will include mosquitto by default, and will be used for CN v0.9.0.

The default walletnotify.sh in the image will be a no-op

Kexkey · 2022-06-10T15:54:37Z

proxy_docker/app/script/bitcoin_node_walletnotify.sh

+
+  while true  # Keep an infinite loop to reconnect when connection lost/broker unavailable
+  do
+    mosquitto_sub -h broker -t confirmation | while read -r message


topic name here as well

Kexkey · 2022-06-10T16:06:19Z

proxy_docker/app/script/confirmation.sh

 confirmation() {
  (
  flock -x 201

  trace "Entering confirmation()..."

-  local returncode
-  local txid=${1}
+  local tx_details=$(echo ${1} | base64 -d)


Ok, we got tx_details needed to have wallet-specific information about the tx and we got tx_raw_details needed to get some other information not found in gettransaction but found in getrawtransaction.

Here, we are receiving tx_details (result of gettransaction) but a few lines below we are calling getrawtransaction to get the rest of the details (into tx_raw_details).

I would like that we call getrawtransaction on the Bitcoin Core container's side in the walletnotify script and include the result in the published message. So we would have something like {"gettransaction":{...},"getrawtransaction":{...}} in the base64 message.

...instead of calling get_rawtransaction from the proxy.

Ohhh wait a minute. We can use gettransaction txid true true on the node to get everything we need, including the rawtransaction info (in the decoded field)! Let's use it in walletnotify.sh and publish the result as message!

NIce, I did not see the third argument on chainquery.com, it's not up to date (second time !!). I'll add this !!

Kexkey · 2022-06-10T16:10:57Z

proxy_docker/app/script/confirmation.sh

-      tx_blockhash="$(echo "${tx_details}" | jq -r '.result.blockhash')"
-      tx_blockheight=$(get_block_info ${tx_blockhash} | jq '.result.height')
+      tx_blockhash="$(echo "${tx_details}" | jq -r '.blockhash')"
+      tx_blockheight=$(get_block_info ${tx_blockhash} | jq '.height')


We calling the Bitcoin Core container here to get block height... Do you think it's a good idea to do this stuff in the walletnotify.sh script in the Bitcoin container and include it in the message? Instead of calling it here from the proxy...

Hey we have the blockheight and blockhash fields in gettransaction's result! It's not even needed anymore to call getblockinfo...

Kexkey · 2022-06-10T16:12:07Z

proxy_docker/app/script/confirmation.sh

    trace "[confirmation] tx_blockhash=${tx_blockhash}"
    if [ "${tx_blockhash}" = "null" ]; then
      trace "[confirmation] probably being called by executecallbacks without any confirmations since the last time we checked"
    else
-      local tx_blockheight=$(get_block_info "${tx_blockhash}" | jq '.result.height')
-      local tx_blocktime=$(echo "${tx_details}" | jq '.result.blocktime')
+      local tx_blockheight=$(get_block_info "${tx_blockhash}" | jq '.height')


Same as above, we would already have this info in the published message...

Let's get the info from the gettransaction result!

Done - no more calls to core

Kexkey · 2022-06-10T16:22:49Z

proxy_docker/app/script/manage_missed_conf.sh

@@ -111,7 +111,8 @@ manage_missed_conf() {

      latesttxid=$(echo "${received_address}" | jq -r ".txids | last")
      trace "[manage_missed_conf] latesttxid=${latesttxid}"
-      data='{"method":"gettransaction","params":["'${latesttxid}'"]}'
+      data="{\"method\":\"gettransaction\",\"params\":[\"${latesttxid}\",true]}"


Interesting, watching01.dat and xpubwatching01.dat are not created as watch-only wallets, so the "true" arg wasn't needed... But I think it's a good idea to add it, just in case. After all, this is a watching wallet...

Kexkey · 2022-06-10T16:24:07Z

proxy_docker/app/script/manage_missed_conf.sh

@@ -126,7 +127,7 @@ manage_missed_conf() {
        trace "[manage_missed_conf] Broadcast or mined after watch, we missed it!"
        # We skip the callbacks because do_callbacks is called right after in
        # requesthandler.executecallbacks (where we're from)
-        confirmation "${latesttxid}" "true"
+        confirmation $(echo "${tx}" | jq .result | base64 -w 0) "true"


Use jq -Mc to remove formatting

…tnotify

first draft - tx details sent in publish

7e2a3e2

phillamy closed this May 10, 2022

phillamy deleted the features/async-walletnotify branch May 10, 2022 23:43

phillamy restored the features/async-walletnotify branch May 10, 2022 23:46

phillamy reopened this May 10, 2022

phillamy changed the base branch from master to dev May 10, 2022 23:53

some more work

047f4b3

phillamy added 2 commits May 19, 2022 11:35

Added the PID in logging

30363a9

used walletnotify in replacement of conf

cb2fd59

Kexkey requested changes Jun 10, 2022

View reviewed changes

phillamy added 3 commits June 14, 2022 12:30

PR review - change requests

39b83fe

Removed /conf

7e99917

removed /conf from gatekeeper

114ea5c

phillamy marked this pull request as ready for review June 14, 2022 16:41

phillamy added 2 commits June 14, 2022 12:47

removed /conf from yaml

16fb9a2

Merge remote-tracking branch 'upstream/dev' into features/async-walle…

d21159e

…tnotify

Kexkey merged commit bcc0a36 into SatoshiPortal:dev Jun 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features/async walletnotify #266

Features/async walletnotify #266

phillamy commented May 10, 2022

phillamy commented May 16, 2022 •

edited

Loading

phillamy commented May 16, 2022

phillamy commented May 17, 2022

Kexkey commented Jun 8, 2022

Kexkey commented Jun 8, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

phillamy Jun 11, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

phillamy Jun 14, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

phillamy Jun 14, 2022

Kexkey Jun 10, 2022

Kexkey Jun 10, 2022

phillamy Jun 14, 2022

Features/async walletnotify #266

Features/async walletnotify #266

Conversation

phillamy commented May 10, 2022

phillamy commented May 16, 2022 • edited Loading

phillamy commented May 16, 2022

phillamy commented May 17, 2022

Kexkey commented Jun 8, 2022

Kexkey commented Jun 8, 2022

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

phillamy commented May 16, 2022 •

edited

Loading