Repository navigation
test: run the integration tests against a Tapyrus node - #30
Conversation
be0c193 to
3c91d69
Compare
azuchi
left a comment
There was a problem hiding this comment.
マージを妨げる問題は見つかりませんでした。今回は手元で実際に Tapyrus ノードを起動して確認しています。
| 検証項目 | 結果 |
|---|---|
npm run integration(tapyrus/tapyrusd:v0.7.2、新規チェーン) |
39 件パス |
| 同じチェーンのまま 2 回目 | 39 件パス |
| ノードを起動せずに実行 | ハングせず即座に失敗、メッセージも明確 |
./scripts/check.sh |
3,138 件パス、カバレッジは全指標で 90% 超(分岐 91.97%) |
| 実行後の作業ツリー | 差分なし |
一番お願いしたいのは、統合テストのワークフローをプルリクエストでも走らせることです(integration.yml のインラインコメントに理由を書きました)。それ以外は、テストの検出力とドキュメントの不整合についての非ブロッキングな指摘です。
diff に現れない箇所で 2 点あります。
ルートの README.md の例一覧が古い(93〜109 行目)
統合テストへのリンク一覧が、この PR で削除・改名したテストを指したままです。「Support the retrieval of transactions for an address (3rd party blockchain)」と「Create a BIP49, bitcoin testnet, …」は削除され、「broadcast via 3PBP」は「broadcast to a node」に改名されています。coloredcoins.spec.ts の例も一覧にありません。
PR 本文が後続コミットに追従していない
- 「Psbt は彩色スクリプトを finalize できないので、transfer と burn は独自の finalizer を
finalizeInputに渡す」とありますが、実装は Pstt に移行済みで、組み込みの finalizer をfinalizeAllInputs()で使っています。 - 対応表の
unspents→listunspentはRegtestUtilsに存在しません。 - テスト件数は 3,140 ではなく 3,138 でした。
| on: | ||
| workflow_dispatch: | ||
| push: | ||
| branches: [master] |
There was a problem hiding this comment.
プルリクエストでも走るようにしてください。 pull_request をトリガーに加えるのがよいと思います。
on:
workflow_dispatch:
pull_request:
push:
branches: [master]理由は次のとおりです。
- 検出したい不具合は、マージ前に見つけたい種類のものです。 README にあるとおり、統合テストは「ユニットテストは通るがノードが拒否する」誤りを捕まえる唯一の手段です。このシリーズで直してきた outpoint の hashMalFix(fix: refer to the hashMalFix txid in outpoints and fix coloured input handling #26)や colorId の検証(fix: validate the type byte of colour identifiers #28)がまさにその類いで、master に入ってから赤くなるのでは、原因のコミットを特定して revert する手間が増えます。
- この PR 自身が、CI 上で一度も統合テストを実行できていません。 現在のトリガーでは master への push か手動実行でしか動かないので、ワークフローの YAML が正しいかどうかもマージするまで分かりません。
- 「無関係な理由でプルリクエストが赤くなる」懸念は、ワークフローが分かれていることで解消できます。
ci.ymlとは別のチェックとして表示されるので、ユニットテストの結果は汚れません。ノードの起動失敗で止めたくなければ、ブランチ保護の required checks に含めなければ済みます。 - コストは小さいです。 手元ではノードは起動から 2 秒で RPC に応答し、39 件のテストは約 1 秒で終わりました。シークレットも不要なので、fork からのプルリクエストでもそのまま動きます。
冒頭のコメント(3〜5 行目)も、あわせて書き換えが必要です。
| @@ -258,7 +255,7 @@ describe('bitcoinjs-lib (transactions w/ CLTV)', () => { | |||
| await regtestUtils.broadcast(tx.toHex()).catch(err => { | |||
There was a problem hiding this comment.
拒否を期待するテストですが、検証が .catch(...) の中にしかないので、ブロードキャストが成功した場合は何も assert せずに合格します。期限前の CLTV 償還がノードに受理されるような退行が起きても、このテストは緑のままです。csv.spec.ts:231 の non-BIP68-final も同じ形です。
この PR で正規表現を書き換えた行なので、coloredcoins.spec.ts の NFT のテストと同じ形に揃えるのがよいと思います。
await assert.rejects(
regtestUtils.broadcast(tx.toHex()),
/non-final \(code 64\)/,
);There was a problem hiding this comment.
31a5480 で修正してます。cltv.spec.ts および、同じ形の csv.spec.ts(non-BIP68-final)を変更してます。
どちらも assert.rejects(regtestUtils.broadcast(tx.toHex()), /…/) にしたので、ブロードキャストが成功するとテストが失敗します。
| Buffer.from(childNode.privateKey!), | ||
| { network: regtest }, | ||
| ); | ||
| pstt.signInput(0, childKeyPair); |
There was a problem hiding this comment.
コメントにあるとおり Pstt には signInputHD がないため、導出済みの子鍵を ECPair に包み直して signInput しています。bip32Derivation は入力に記録されるだけで署名時には参照されないので、パスやフィンガープリントが誤っていてもこのテストは通ります。
つまり検証しているのは「HD で導出した鍵による通常の署名」で、HD 署名そのものではありません。テスト名の「using HD」と PR 本文の「HD signing is still covered」は実態と合わないので、テスト名を実態に合わせる(例: w/ a P2PKH input whose key is derived with BIP32)か、Pstt に HD 署名の手段を用意してから検証するかのどちらかがよいと思います。後者は別 PR で構いません。
There was a problem hiding this comment.
31a5480 で修正しました。
1 つ目の案を採用し、テスト名を w/ a P2PKH input whose key is derived with BIP32 に変えました。
| private async ensureFunds(value: number): Promise<void> { | ||
| const needed = (value + 1e6) / 1e8; | ||
| while ((await this.rpc<number>('getbalance')) < needed) { | ||
| await this.mine(1); |
There was a problem hiding this comment.
このループには上限がありません。README は既存ノードでの実行を案内していますが、ウォレットの残高が増えないノード(ウォレットが無効、別のアドレスに採掘される、など)では無限ループになり、原因を示さないまま mocha のタイムアウトで落ちます。試行回数に上限を設けて、超えたら「N ブロック採掘しても残高が X に届かない」というエラーにすると、原因が分かります。
手元の新規チェーンでは全 39 件が約 1 秒で終わったので、速度面の問題は確認していません。
There was a problem hiding this comment.
31a5480 で直しました。ensureFunds は最大 500 ブロックまでmineするようにしました。それでも残高が足りなければ、現在の残高と必要額を示し、考えられる原因(Blocks may be paying an address the node wallet does not hold, or the wallet may be disabled.)を添えたエラーで止まります。
| }), | ||
| }); | ||
| } catch (err) { | ||
| throw new Error( |
There was a problem hiding this comment.
fetch の例外をすべて「Cannot reach a Tapyrus node … Start one with docker compose」に変換しているので、AbortSignal.timeout によるタイムアウトも未起動として報告されます。ノードは動いているが RPC が 20 秒を超えた、という場合に原因を取り違えます。err.name === 'TimeoutError' を分けて、メソッド名とタイムアウト値を出すのがよいと思います。
なお未起動時の挙動は手元で確認しました。ハングせず即座に失敗し、メッセージも明確でした。
There was a problem hiding this comment.
31a5480 で直しました。TimeoutError は別に扱い、<method> did not answer within 20000ms. The node at <url> is reachable but not responding in time. と報告するようにしてます。それ以外の失敗は、従来どおり「Cannot reach a Tapyrus node」のメッセージです。
| # in test/integration/_regtest.ts opens this chain by design. | ||
| services: | ||
| tapyrusd: | ||
| image: tapyrus/tapyrusd:v0.7.2 |
There was a problem hiding this comment.
起動待ちのロジックが integration.yml の curl ループにしかなく、README の手順(up -d の直後に npm run integration)では、ノードが Loading block index... を返している間にテストが始まる可能性があります。ここに healthcheck を定義して docker compose up -d --wait を使うと、待機がローカルと CI で一本化され、ワークフロー側のループも不要になります。
healthcheck:
test: ['CMD', 'tapyrus-cli', '-conf=/etc/tapyrus/tapyrus.conf', 'getblockcount']
interval: 1s
timeout: 3s
retries: 60手元では起動から 2 秒で応答したので、実害が出る場面は限られます。
There was a problem hiding this comment.
4564efc で修正しました。
docker-compose.integration.yml にhealthcheckの定義を追加して、ci側では--wait で待機するようにしました。
| assert.strictEqual(burned.outs[0].token, 'TPC'); | ||
| }); | ||
|
|
||
| it('can hold a token in a CP2SH output', async () => { |
There was a problem hiding this comment.
このテストは CP2SH の出力にトークンを保持するところまでで、そこから使用していません。#26 で直した CP2SH の署名経路を実ノードで確かめられる好機なので、保持したトークンを別のアドレスへ送るところまで含めると、テストの価値が上がると思います。Pstt の finalizer が CP2SH に対応していない場合は、その旨をコメントに残すだけでも構いません。
There was a problem hiding this comment.
31a5480 で対応しました。テスト名は can hold a token in a CP2SH output, and spend it に変更してます。また、保持したトークンを、手数料を払う TPC の入力とあわせて、CP2PKH のアドレスへ送るようにしてます。
| run: npm run lint:tests | ||
| - name: Run unit test | ||
| run: npm run unit | ||
| # npm run unit builds first, so src/ and types/ are up to date here |
There was a problem hiding this comment.
このコメントは npm run unit を前提にしていますが、そのステップはこの PR で削除されています。git diff --exit-code src types の前提になるビルドは、いまは npm test の中の npm run build が担っているので、コメントをそれに合わせて更新してください。
| docker compose -f docker-compose.integration.yml down | ||
| ``` | ||
|
|
||
| The node listens on `127.0.0.1:12382`. If nothing is listening there, every |
There was a problem hiding this comment.
「every test fails」とありますが、ノードを起動せずに実行したところ、ノードを使わない 12 件(addresses、bip32、blocks の各 spec と「can create a 1-to-1 Transaction」)は成功し、残りが失敗しました。「ノードを必要とするテストは」のような書き方が正確です。
6ce7f2a to
a87b2a4
Compare
README.mdを 4564efc で修正しました。 |
|
更新分を確認しました。前回の指摘はすべて対応されており、内容面ではマージ可能です。CI の統合テストのジョブがプルリクエストで実際に走って 39 件パスしていること、手元でも新しい compose 定義(healthcheck + マージ順についてお願いです。 ベースの #29 は、#28 がリベースマージされて別 SHA になった影響で master と競合しており( |
a87b2a4 to
591b689
Compare
45bb88b to
a818ffd
Compare
Proposed body for PR #30
This is the current body with the minimum edits needed to match the code and to answer the review. Every changed passage is marked with a
<!-- changed -->comment and can be found by searching for it; remove the comments before pasting.Changes from the current body:
csv.spec.tsfinalizer sentence correctedunspentsrow removed (no such method).faucetnow stops after a bounded number of blocks. Timeout is reported separately from an unreachable nodePsbtfinalizer paragraph replaced: the tests usePsttnowintegration.ymlalso runs on pull requests, and waits through a compose healthcheckup -d --waitSummary
test/integration/pointed athttps://regtest.bitbank.cc/1, the Bitcoinregtest service bitcoinjs runs, and
addresses.spec.tsalso queriedblockchain.info. Nothing in it could pass, and since the SegWit removal(#23) it did not even compile:
which is why
npm teststopped atbuild:testswith exit code 2.This replaces the harness with a small JSON-RPC client for tapyrus-core, drops
what only made sense on Bitcoin, and adds coverage for coloured coins.
The commits are grouped by purpose below.
1. Drop the SegWit and Bitcoin cases
Eight tests in
transactions.spec.ts(P2WPKH, P2WSH, P2SH(P2WSH) and theirnonWitnessUtxovariants), four address cases, the BIP49 derivation, the P2WSHand P2SH(P2WSH) rounds in
payments.spec.tsalong withp2wpkhitself, theP2WSH case in
csv.spec.ts, and theblockchain.infolookup. The HD test waskept and moved to P2PKH.
Pstthas nosignInputHD, so that test wraps the derived child key in anECPairand callssignInput.bip32Derivationis only recorded on the input,so the test checks an ordinary signature made with a BIP32-derived key, not HD
signing itself. It is named accordingly.
csv.spec.tssigns withPsttand builds the final script by hand, and itsP2WSH case is gone.
Two things here are not SegWit but were wrong all the same:
reverseBuffer(tx.getHash()). Sincefix: refer to the hashMalFix txid in outpoints and fix coloured input handling #26 that is not what an outpoint or the node refers to. Now
tx.getId().blocks.spec.tsparsed a Bitcoin SegWit coinbase, which throwsTransaction has unexpected dataafter fix: remove SegWit code paths that Tapyrus does not have #23. It now uses the coinbase oftestnet block
896574be…de2a, and additionally asserts that Tapyrus repeatsthe height in the coinbase input's outpoint index.
1-to-1 Transactionexample embedded a Bitcoin previous transaction withversion = 2. Under the Tapyrus rules its outpoint no longer matches, so itwas rebuilt at
features = 1and the expected hex regenerated.2. Talk to a node
tapyrusjs-clientis gone. ItsRegtestUtilsspeaks the HTTP API of bitcoinjs'regtest-server, not tapyrus-core's JSON-RPC, so pointingAPIURLat a Tapyrusnode would not have worked. It was also pinned as
git+ssh://git@github.com/…, which makesnpm cifail wherever there is no SSHkey.
_regtest.tsnow calls the node directly:broadcastsendrawtransactionminegeneratetoaddress nblocks address privkeyheightgetblockcountfetchgetrawtransaction txid truefaucetsendtoaddressthen a blockfaucetComplexfundrawtransaction→signrawtransactionwithwallet→sendrawtransactionfaucetlocates its own output by scanning the transaction, because theaddresses under test are not in the node's wallet.
If the wallet cannot pay, it mines at most 500 blocks and then fails with an
error saying so, instead of looping until mocha's timeout.
Starting the node is deliberately not the test suite's job. Mixing the two makes
it hard to tell which one broke, and it rules out running against a node you
already have.
docker-compose.integration.yml,test/integration/tapyrus.confand
test/integration/README.mdcover the setup instead, and the tests fail witha message naming the command rather than hanging:
An RPC that runs past its timeout is reported as such, with the method name and
the timeout, not as an unreachable node.
The genesis block and the signer WIF are tapyrus-core's own dev mode example
from
doc/docker_image.md.README.mdsays so and explains why a key has to bein the repository at all: Tapyrus has no proof of work, so
generatetoaddresstakes the aggregate key as an argument.
nobuild:coveragewas limited to'test/*.js', sonpm testruns the unittests only.
npm run integrationis the way to run these.3. Coloured coins
coloredcoins.spec.tsis new: it issues a token all three ways, transfers it,burns it, and holds one in a CP2SH output. These are the paths a Bitcoin derived
library has no reason to get right, and until now nothing exercised them.
The NFT case also checks that the node refuses an amount other than 1
(
bad-txns-nft-amount).The CP2SH case also spends the held token to a CP2PKH address, so the node
checks the CP2SH signing path.
Every coloured transaction carries a TPC input for the fee; without one
tapyrus-core answers
bad-txns-token-without-fee.The transactions are built with
Pstt. Its built-in Input Finalizer alreadyhandles the coloured scripts, so the tests call
finalizeAllInputs()and needno finalizer of their own.
4. CI
With
test/integration/out ofnobuild:coverage,npm testpasses, soci.ymlnow runs it instead oflintandunit. That addsformat:ciand the90% coverage threshold to what a pull request has to pass, and CI checks what
CONTRIBUTING.mdasks contributors to run.lint:testsis not part ofnpm testand stays a step of its own.scripts/check.shruns the same two..github/workflows/integration.ymlruns onworkflow_dispatch, on pullrequests, and on a push to
master. It shows up as a check of its own next toci.yml, so a node that fails to start does not turn the unit test result red.Keeping it out of the required checks also keeps it from blocking a merge. The
bugs it catches, such as the outpoint hash in #26 and the colour id checks in
#28, are better found before merging than after.
It uses the same compose file the README gives and waits with
docker compose up -d --wait, which relies on a healthcheck in that file(
tapyrus-cli getblockcount). tapyrus-core answers every RPC error with HTTP500, the
Loading block indexit returns while warming up included, so areachable port is not a ready node. The same healthcheck covers the local run in
the README. The workflow prints the node log if anything fails.
Affected areas
test/integration/,package.json,.github/workflows/andscripts/check.sh. No library code.The compose file binds the RPC port to
127.0.0.1. The node holds nothingworth stealing, but
generatetoaddressandsendtoaddressare oneuser:passaway, and there is no reason to offer those to whatever network the development
machine is on.
How to verify
Unit tests, format and lint, the same as CI:
npm ci ./scripts/check.sh # 3,159 passing, coverage above 90%Integration tests:
docker compose downthrows the chain away, so each run starts at the sameheight with an empty wallet. No test needs that — each one funds a newly
generated key, so its colour identifiers are new every time — but a chain that
keeps growing makes a failure harder to read, and the image ignores
GENESIS_BLOCK_WITH_SIGonce a genesis file exists, so a kept volume wouldoutlive a change to the genesis block.
What was checked without a node
The issuance, transfer, burn and CP2SH transactions were built offline and
inspected:
featuresis 1, the coloured outputs classify ascoloredpubkeyhashandcoloredscripthash, and the fee comes out at theintended 10,000 satoshi. Acceptance by a node is what the workflow above is for.
Base branch
Stacked on
feat/color_identifier, whosecoloridentifiermodule the colouredcoin tests use. Please merge that first; GitHub retargets this PR at
masterwhen it does.