feature/add dockerfile · Pull Request #1 · cryptogarageinc/electrs

ghost · 2020-12-18T00:54:57Z

https://github.com/cryptogarageinc/utxomanager/issues/573 に対するPRです

https://github.com/cryptogarageinc/utxomanager/pull/578 より移動しました。

タグ命名規則

以下の形式で実装した

${直近のタグ名}-${コミットハッシュ8桁}

脆弱性検出結果

trivy による検出結果

cryptogarageinc/electrs:v0.4.2-0f4d9309 (alpine 3.12.3)
=======================================================
Total: 0 (UNKNOWN: 0, LOW: 0, MEDIUM: 0, HIGH: 0, CRITICAL: 0)

残り作業

docker hub に push できることを確認したら Github Actions の対象ブランチを修正する
脆弱性が解消されていることを確認し、エビデンスを貼る
GitHub Secret の値を一時的なものから適切なものに変更してもらう

Using a new --electrum-public-hosts argument for specifying the public hosts where the server is reachable (as a JSON dictionary).

The limit applies if the utxo set of an address exceeds the limit at any point in history. Set to 500 by default, can be configured with --utxos-limit.

To be used with bitcoind's blocknotify functionality, with something like: blocknotify=pkill -USR1 electrs

Adds the `server.add_peer` and `server.peers.subscribe` RPC commands to support discovery of other servers, with a mechanism for running scheduled health checks and verifying the servers availability.

Enabled when --electrum-public-hosts is set, disabled otherwise.

…etrying them *forever*

… unspecified IP address types

dgyoshi

LGTM

ayuri-kn · 2020-12-24T05:56:28Z

+on:
+  push:
+    branches:
+      - new-index


@cgonoda
これ、改めてちゃんとしたbranch切りませんか。。？？
CC: @k-matsuzawa

これ、改めてちゃんとしたbranch切りませんか。。？？

賛成です。 new-index はBlockStream側のブランチとして扱っておいた方が、混同しなくて済むと思います。
それこそdevelop でもいいと思うので。（見たら使ってなかった）

master / main ？
でも良いかもしれませんね。(mainはまだシックリこないのですが、、)

こちら、私はとくに意見ないです…

コメントありがとうございます。

@k-matsuzawa @cgonoda
v0.4.2 からmasterブランチを作成しました！
https://github.com/cryptogarageinc/electrs/tree/master

ありがとうございます。
では、on.push.branches: master としておきます。

f46b6ad で対応しました。

ayuri-kn · 2020-12-24T05:57:12Z

+    branches:
+      - new-index
+    tags:
+      - '*'


@cgonoda
v始まり。とかにしておかなくて大丈夫です？
（どのtagもimage buildしちゃいそうですが、、）

私の思いとしては、どのtagもimage buildしちゃっていい気がしていました。

実際に環境で使いたいもの（の命名ルール）とそうでないもの。は、明確に分けておいた方が良いかなと思いました。

v0.41.2.zzzzzzzz latest

のimageがあったときに、「これ、どっち使えば良いんだろう。。？？」となりはしないかと思いまして。。

「これ、どっち使えば良いんだろう。。？？」

https://github.com/cryptogarageinc/electrs/pull/1/files/e39ba46c0b5f1045e4c6b4acf17819ca4ef96885#diff-3414847e2ad632333f775cabb810f0dc0df61a570365df34750a08b00912fe82R42-R43

あれ、こっちの話ですか?

いえ、imageタグの命名ルールの話ですね。
今の設定だと、下記状況になりうる。と認識しているのですが、認識誤っているでしょうか。。？

特にimageとして利用する想定ではないタグを切りたい。と思ったときも、この指定だとimageが出来てしまう（誰かが使ってしまう可能性がある

imageタグ自体の命名規則がなく、様々なimageが作成出来てしまう
※極端な話、new-imageというtagを切ると、そのimageが出来てしまう。。

ちょっと毛色が違いますが、他リポジトリではimage(binary)作成の契機をこんな感じで制御していたりします。
https://github.com/cryptogarageinc/settlenetio-v2-traderApp/blob/25d7c51a618fb6326ca7b43a071247f0df4f750a/.github/workflows/release-electron-mac-prod.yml#L7-L8

ご参考まで

あー、命名ルールというか、image 作成の契機がガバガバだと、作りたくないときにも作っちゃうという話ですね。
たしかにそうですね。とりあえず on.push.tags: 'v*' にしておきます。

ちなみに、命名ルールそのものについては何かコメントありますか?
https://github.com/cryptogarageinc/electrs/pull/1/files/e39ba46c0b5f1045e4c6b4acf17819ca4ef96885#diff-3414847e2ad632333f775cabb810f0dc0df61a570365df34750a08b00912fe82R42-R43

お願いします！
後半はこちらにコメントつけました。
https://github.com/cryptogarageinc/electrs/pull/1/files#r549197094

f46b6ad で対応しました。

ayuri-kn · 2020-12-24T05:59:46Z

+
+jobs:
+  build_and_push:
+    runs-on: ubuntu-latest


latestがどう。。という話ってありませんでしたっけ。。
CI workerの指定ってどうなんでしょうね。。？？（どっちが良いか。の答えには自分はたどり着いていないのですが、、）

現在はubuntu-18.04がlatestですけど、間もなくubuntu-20.04に切り替わりますね。。
行うことはあまりないのでlatestでも影響はほぼないと思いますが、できればversion固定しておいた方が面倒事は少ないと思います。

うぉ、そうでした! latest排除します。

f46b6ad で対応しました。

ayuri-kn · 2020-12-24T06:02:37Z

それはそうと、これってimage scannerで発見した脆弱性対応が起点ではなかったでしょうか？
https://github.com/cryptogarageinc/settlenetio-support/issues/5

Dockerhub管理だとすると、scannerは別途実装しなきゃいけないですね。。
であれば、CIにtrivyを入れておく。というのはいかがでしょう？

https://github.com/aquasecurity/trivy-action
https://qiita.com/homines22/items/7aa3cb0a16e56e41ca38

ghost · 2020-12-24T06:11:18Z

それはそうと、これってimage scannerで発見した脆弱性対応が起点ではなかったでしょうか？
cryptogarageinc/settlenetio-support#5

Dockerhub管理だとすると、scannerは別途実装しなきゃいけないですね。。
であれば、CIにtrivyを入れておく。というのはいかがでしょう？

https://github.com/aquasecurity/trivy-action
https://qiita.com/homines22/items/7aa3cb0a16e56e41ca38

trivy いれてみようと思います!

ayuri-kn · 2020-12-28T02:49:28Z

+          context: .
+          push: true
+          tags: |
+            ${{ github.repository }}:latest


https://github.com/cryptogarageinc/electrs/pull/1/files#r549195571

命名ルールそのものについては何かコメントありますか?

latestって作っておいた方が良いんでしょうか。（これは素朴な質問なので、答えは自分はあまり持っていないです。。）
publicには合ったほうが親切なんでしょうけれど、僕らしか使わないといえば使わない。。

という位で、これ自体はみなさんのご意見伺いたいな。位の気持ちです。

「一番新しいのどれだっけ…」
「(日付で見てもいいけど) "latest"と同じ DIGEST を持ってるこれか…」
とかいうシーンがないかなぁーとは思っています。

なるほど！ではこのままで大丈夫です！

The base branch was changed.

…einc/electrs into feature/add-dockerfile

ghost · 2020-12-28T06:35:22Z

f46b6ad で trivy のgithub action を追加しました。

ayuri-kn · 2021-01-04T09:37:07Z

@cgonoda
branchの向き先を変更お願いできますか？
new-index --> master

https://crypto-garage.slack.com/archives/CD43F2YUT/p1609123086050600

k-matsuzawa

utACK

dgyoshi

utACK

ayuri-kn · 2021-01-06T02:08:53Z

@dgyoshi @k-matsuzawa @cgonoda
これ、、現在CGで使われてるとされていた、 b18f82c --> https://github.com/cryptogarageinc/electrs/releases/tag/v0.4.2
で、masterブランチを作成していたのですが、 new-index ブランチ（Blockstream側の）のHEADをマージしてしまったように見えています。。

これでも特に問題はないでしょうか？？

k-matsuzawa · 2021-01-06T02:19:42Z

レビュー時にさっと全体を見てみましたが、大きな影響がありそうな変更は入っていないと思います。（new-indexブランチ自体はblockstreamのblock explorerで運用しているはずなので、ある程度の動作は担保できているはず）
ですのでdev/test環境でしばらく（半月位？）使って問題なければ（使用している各種モジュールに影響がなければ）Updateしても大丈夫と思っています。

ayuri-kn · 2021-01-06T02:34:53Z

なるほど。
ある意味いい機会なので、最新アップデートに追随しておいた方が良いだろう。ということですね。
承知しました！

ayuri-kn · 2021-01-06T02:36:13Z

だとすると、とりあえず、これでタグ切っておいた方が良いですね。。

ayuri-kn · 2021-01-06T02:42:33Z

+            ${{ github.repository }}:latest
+            ${{ github.repository }}:${{ steps.tagname.outputs.tagname }}-${{ steps.short-sha.outputs.sha8 }}
+
+      - name: Run Trivy vulnerability scanner


@cgonoda @dgyoshi
これ、今更指摘で恐縮ですが、build --> run scanner --> push の順序かなとおもったのですが、いかがでしょうか？
（というのは、脆弱性が見つかったものをrepositoryにpushして良いのかな？と思ったりしたので。。）

たしかに、その順序がいいですね。
ただ、このPRはマージしてしまったので、別PRで作業させてくださいー。

shesek added 30 commits April 21, 2020 20:41

Use newly added AssetId::from_slice()

93b116e

Display a more useful error message

6c53234

Electrum: Implement server.features RPC method

9b06f2b

Using a new --electrum-public-hosts argument for specifying the public hosts where the server is reachable (as a JSON dictionary).

Electrum: Make the banner configurable via --electrum-banner

0d5624b

Electrum: Implement dummy server.add_peer method

c594051

Change dummy server.add_peer to return true

441d4cb

Add a maximum pre-address utxos limit

b42fa4f

The limit applies if the utxo set of an address exceeds the limit at any point in history. Set to 500 by default, can be configured with --utxos-limit.

Increase the default --electrum-txs-limit to 500

8b63283

Support triggering a real-time sync using SIGUSR1

9629e7a

To be used with bitcoind's blocknotify functionality, with something like: blocknotify=pkill -USR1 electrs

Add some missing Prometheus instrumentations

b840198

Expose underlying error messages instead of a generic error

995c93a

Initiate "never" signal only once

61d1579

Remove unused code and outdated tests

b6a4d46

Move electrum into a submodule

d2cdae0

Implement Electrum server discovery

8306b2f

Adds the `server.add_peer` and `server.peers.subscribe` RPC commands to support discovery of other servers, with a mechanism for running scheduled health checks and verifying the servers availability.

Make electrum server discovery optional at runtime

d8f783b

Enabled when --electrum-public-hosts is set, disabled otherwise.

Minor comments/messages/formatting changes

af76d8f

Give unavailable default servers some more leniency, but don't keep r…

b3385f3

…etrying them *forever*

Refactor DiscoveryManager

eaaff0c

Rename health_check -> job

3ece2ca

Avoid listing ourselves

57e9f6c

Initialize our ServerFeatures once and share it

d6486e7

electrum: Announce ourselves to the servers we're connecting to

ad921b6

electrum: Track the number of connected clients with Prometheus

8fb06c4

electrum: Don't error in add_peer when there are no new entries

3cac784

electrum: Reject add_peer requests with loopback, local, multicast or…

4674d98

… unspecified IP address types

Shortem /mempool/recent TTL for 5 seconds

89c5954

Reset the client count to 0 on startup

e9ba1e3

docs: Update README with new CLI options

bebdcfe

docs: Update current index storage requirements

1c85ae4

ghost requested review from ayuri-kn, chiba-cg and dgyoshi December 24, 2020 03:58

ghost marked this pull request as ready for review December 24, 2020 03:59

dgyoshi previously approved these changes Dec 24, 2020

View reviewed changes

ayuri-kn reviewed Dec 24, 2020

View reviewed changes

ayuri-kn requested a review from k-matsuzawa December 24, 2020 06:00

ayuri-kn reviewed Dec 28, 2020

View reviewed changes

ghost changed the base branch from new-index to master December 28, 2020 06:21

ghost changed the base branch from master to new-index December 28, 2020 06:22

onoda-cg added 2 commits December 28, 2020 15:29

レビューコメントに対応

f46b6ad

Merge branch 'feature/add-dockerfile' of ssh://github.com/cryptogarag…

3071465

…einc/electrs into feature/add-dockerfile

動作確認用のブランチ指定を削除

86d08cd

ghost requested review from ayuri-kn and dgyoshi December 28, 2020 06:50

ghost changed the base branch from new-index to master January 4, 2021 09:39

k-matsuzawa approved these changes Jan 5, 2021

View reviewed changes

dgyoshi approved these changes Jan 5, 2021

View reviewed changes

ghost merged commit 9c1623b into master Jan 5, 2021

ghost deleted the feature/add-dockerfile branch January 5, 2021 05:21

ayuri-kn reviewed Jan 6, 2021

View reviewed changes

This pull request was closed.

Conversation

ghost commented Dec 18, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

タグ命名規則

脆弱性検出結果

残り作業

Uh oh!

dgyoshi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayuri-kn Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayuri-kn commented Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 28, 2020

Uh oh!

ayuri-kn commented Jan 4, 2021

Uh oh!

k-matsuzawa left a comment

Choose a reason for hiding this comment

Uh oh!

dgyoshi left a comment

Choose a reason for hiding this comment

Uh oh!

ayuri-kn commented Jan 6, 2021

Uh oh!

k-matsuzawa commented Jan 6, 2021

Uh oh!

ayuri-kn commented Jan 6, 2021

Uh oh!

ayuri-kn commented Jan 6, 2021

Uh oh!

ghost commented Dec 18, 2020 •

edited by ghost

Loading

ayuri-kn Dec 28, 2020 •

edited

Loading

ayuri-kn commented Dec 24, 2020 •

edited

Loading