Integration tests, unit tests, key import, github workflow - all-in-one changeset #11

aakselrod · 2022-12-23T22:52:19Z

Vault package cleanup

The first commit adds exported error variables to the vault package and uses them to replace ad-hoc error messages.

The next commit breaks up the paths function into multiple functions that can be used for unit tests in future commits.

The following commit removes redundant CreateOperation entries from the paths.

Backend tests

The next commit adds initial tests to the vault plugin backend, bringing the test coverage of the package over 50%. It doesn't exercise everything as it's difficult without deterministic test support, which will be enabled by key import in a later commit.

Key import

The following commit changes the seed length from 32 to 16 to match LND in preparation for key import support.

The commit after that moves definitions from paths.go to keys.go and backend.go, where they are used.

The next commit factors out a new function, newNode(), from createNode(). The function will be used in a later commit to import a node.

The following commit implements key import. The following is added:

import of seedphrase with optional passphrase from existing LND node backup
backend tests ensuring that import works correctly
deterministic tests that use imported keys to test signing and ECDH, bringing test coverage over 80%

This fixes https://github.com/bottlepay/lndsigner/issues/3.

Keyring tests

The following commit refactors the keyring package by adding some logging statements and changing ad-hoc errors to exported error variables in preparation for tests added in a later commit.

The next commit adds unit tests to the keyring package and brings test coverage over 70% for the package. I've added #17 to follow up on additional improvements that can be made here.

Integration tests

The commit after that fixes a minor typo in a string displayed on startup.

The final commit adds integration tests. They demonstrate end-to-end functioning of the system, including:

the vault plugin running in a dev vault
lndsignerd instances backed by the vault
a real lightning network of real watch-only lnd nodes running against a real regtest bitcoind, using the signers to send money on-chain and off-chain
a new job in the github workflow to run integration and unit tests with every PR push or merge to master (can we change this branch name to main?)
Coverage measurement for unit tests (integration test coverage to come after golang 1.20 release)
Dockerized tests to make it easy to install binary dependencies for integration tests

I hope that this changeset gets us close to deploying in a testnet infrastructure with blind signing support. I've created the following issues to follow up on this PR: #12, #13, #14, #15, #16, #17, #18, and #19.

joostjager

A lot of awesome additions in this PR, great work. Commit structure has room for improvement.

vault/paths.go

vault/backend.go

lndsigner_test.go

vault/backend.go

aakselrod · 2023-01-20T01:42:20Z

I'm still working on addressing the comments but have improved commit structure in a couple of cases.

aakselrod · 2023-01-24T01:44:00Z

More progress, but not all there yet.

aakselrod · 2023-01-28T02:10:55Z

Almost there now, just need to dockerize the itests, create a docker build (#12), and add linters (#18).

aakselrod · 2023-02-01T03:20:13Z

Integration tests are dockerized, and I think I'm going to need @Jawshua's help to build an image and store it in a registry. I'll work on linting (#18) and unix sockets (#19) tomorrow.

itest/lndsigner_test.go

joostjager

This is again really good. You've picked up all the remarks so well, I am impressed.

Commit structure is nice and clean now too. I don't think there is much to be done on this PR.

vault/backend_test.go

itest/lndsigner_test.go

joostjager · 2023-02-01T13:03:34Z

itest/lndsigner_test.go

+	lncliPath      string
+	bitcoincliPath string
+
+	lndMtx sync.RWMutex


Do you really need this mutex for anything other than the initial creation? It seems that test parallelization happens mostly after lnds has already been accessed, and also lnds is static at that point?

The tests still give me a race condition because these accesses happen from different goroutines without being gated by a mutex, even if it's technically not possible that it happens at the same time.

I've fixed this and eliminated the mutex. I'm gating API calls with a channel after watching the log file, and managing the whole command lifecycle in a separate goroutine inside waitProc.

Looks like passing t into waitProc is a bad idea. Will fix.

So the error was caused by calling t.Logf with a saved t all along. Fixed in https://github.com/bottlepay/lndsigner/compare/b1998d79e511c3bf6ceecdf829d711b6323fe512..b03297c3e6a00d67a20495218d804f98dc721d86 by replacing zap's test logger with a development logger that doesn't rely on a *testing.T.

However, this is a good sign to stop lnd and bitcoind via RPC instead of OS signal.

I've replaced the OS interrupt signal to lnd and bitcoind with a lncli stop and bitcoin-cli stop, which still ends up with the processes being killed as the main test exits, and blurting out a warning. I silenced it on signal: killed errors now, so if the daemons are too slow to shut down after the tests, we don't need to hear about it.

So the error was caused by calling t.Logf with a saved t all along.

What was the root cause of this failing? t being used in a goroutine? I think that should be possible.

itest/lndsigner_test.go

lndsigner_test.go

joostjager · 2023-02-06T19:24:07Z

You can un-request review and re-request when ready ready 😄

aakselrod · 2023-02-06T19:45:20Z

OK, now really ready for review. It was ready before, but I thought I'd try to sneak in a fix for the Unix socket issue (#19) before you got to it, and ran into a new security check that git does when building in the container, so had to fix the Dockerfile too just when you were taking a look. Sorry about that!

itest/gen_protos.sh

jwelsh-nydig · 2023-02-06T19:54:53Z

itest/itest_lndharness_test.go

+		"--remotesigner.enable",
+		"--remotesigner.rpchost="+signerAddr,
+		"--remotesigner.tlscertpath=./testdata/tls.cert",
+		"--remotesigner.macaroonpath=./testdata/signer.custom.macaroon",


nitpick: we could generate TLS and macroons dynamically

I've added TLS key/cert generation in e2b734c. If you're good with this, I'll squash it into the previous commit (adding integration tests). Now I'm going to look at generating a dummy macaroon without using the macaroon libraries.

OK, I've also added dynamic macaroon generation using just manual byte slice ops in b9af6b2. If you're good with this, I'll squash it into the main integration test commit as well.

lndsigner.go

joostjager

LGTM. I must say that I didn't do another full pass because of the size of the change set. For the future, I think that smaller PRs that only do a single thing are better for review efficiency. Also things like refactorings can be spun out pretty quickly, and get reviewed and merged.

joostjager · 2023-02-07T11:52:09Z

README.md

 - [x] sign messages for network announcements
 - [x] derive shared keys for peer connections
 - [x] sign PSBTs for on-chain transactions, channel openings/closes, HTLC updates, etc.
+- [ ] run itests
+- [ ] do automated builds
+- [ ] do reproducible builds


Maybe this todo list can be a link to the repo issues with a filter by label?

Opened #25 to do this in a separate PR so as not to delay this one any longer.

joostjager · 2023-02-07T11:56:29Z

Makefile

+
+# docker-test-all runs unit and integration tests in a docker container, then
+# removes the container.
+docker-test-all: docker


make docker-test-all still fails for me at b9af6b2

go: downloading github.com/decred/dcrd/crypto/blake256 v1.0.0 go: downloading github.com/mitchellh/reflectwalk v1.0.0 go: downloading github.com/hashicorp/go-immutable-radix v1.3.1 go: downloading golang.org/x/text v0.3.7 error obtaining VCS status: exit status 128 Use -buildvcs=false to disable VCS stamping. error obtaining VCS status: exit status 128 Use -buildvcs=false to disable VCS stamping. make: *** [Makefile:50: test-all] Error 1

joostjager · 2023-02-07T12:02:05Z

itest/itest_lndharness_test.go

@@ -366,3 +370,28 @@ func mustGenCertPair(t *testing.T, certFile, keyFile string) {

 	require.NoError(t, os.WriteFile(keyFile, keyBuf.Bytes(), 0600))
 }
+
+func mustGenMacaroon(t *testing.T, macPath string) {


I think you can squash the itest commits to a single one.

Done. ✔️

aakselrod · 2023-02-07T18:15:57Z

@joostjager @jwelsh-nydig Thank you for the reviews! Can one of you please give me an approval to merge?

joostjager · 2023-02-08T07:34:39Z

@aakselrod it's not very important, but still interested in #11 (comment)

aakselrod force-pushed the integration-tests branch 2 times, most recently from a7c7b87 to 4a7c989 Compare December 24, 2022 11:47

aakselrod changed the title ~~WIP: Integration tests~~ Integration tests, unit tests, key import - all-in-one changeset Dec 24, 2022

aakselrod changed the title ~~Integration tests, unit tests, key import - all-in-one changeset~~ Integration tests, unit tests, key import, github workflow - all-in-one changeset Dec 24, 2022

aakselrod marked this pull request as ready for review December 24, 2022 12:11

aakselrod requested a review from joostjager January 3, 2023 20:42

joostjager reviewed Jan 4, 2023

View reviewed changes

aakselrod force-pushed the integration-tests branch from 69781dd to 386191f Compare January 20, 2023 01:36

aakselrod force-pushed the integration-tests branch 2 times, most recently from 4d6e9a5 to 1975a62 Compare January 24, 2023 01:43

aakselrod marked this pull request as draft January 24, 2023 01:49

aakselrod force-pushed the integration-tests branch 4 times, most recently from c8b2d67 to f6a2c88 Compare January 28, 2023 02:04

aakselrod force-pushed the integration-tests branch 3 times, most recently from 4be1e26 to 427bd17 Compare February 1, 2023 03:02

aakselrod marked this pull request as ready for review February 1, 2023 03:15

aakselrod requested review from joostjager and Jawshua February 1, 2023 03:15

aakselrod commented Feb 1, 2023

View reviewed changes

itest/lndsigner_test.go Outdated Show resolved Hide resolved

joostjager reviewed Feb 1, 2023

View reviewed changes

aakselrod force-pushed the integration-tests branch 3 times, most recently from d2193be to 4355bbd Compare February 1, 2023 20:14

aakselrod force-pushed the integration-tests branch from 29e80e9 to c8d4e21 Compare February 6, 2023 19:36

aakselrod removed request for joostjager and Jawshua February 6, 2023 19:37

aakselrod requested a review from joostjager February 6, 2023 19:45

jwelsh-nydig reviewed Feb 6, 2023

View reviewed changes

itest/gen_protos.sh Outdated Show resolved Hide resolved

jwelsh-nydig reviewed Feb 6, 2023

View reviewed changes

lndsigner.go Outdated Show resolved Hide resolved

aakselrod added 3 commits February 6, 2023 11:59

keyring: add package error variables, improve logs

050a894

keyring: add unit tests

2104eb6

main: typo fix and opportunistic upgrade to structured logging

6e5567b

aakselrod force-pushed the integration-tests branch from c8d4e21 to 2471acc Compare February 6, 2023 21:10

aakselrod requested a review from jwelsh-nydig February 6, 2023 21:56

joostjager reviewed Feb 7, 2023

View reviewed changes

lndsigner: add integration tests

96f91e4

aakselrod force-pushed the integration-tests branch from b9af6b2 to 96f91e4 Compare February 7, 2023 17:51

This was referenced Feb 7, 2023

GitHub: run tests #10

Closed

Keyring: add tests #9

Closed

Vault plugin backend tests #8

Closed

vault: implement key import #7

Closed

joostjager approved these changes Feb 7, 2023

View reviewed changes

aakselrod merged commit 48c49fb into master Feb 7, 2023

aakselrod deleted the integration-tests branch February 13, 2023 20:15

This was referenced Apr 5, 2023

Audit private key material zeroing to ensure no copies stay in memory NYDIG/lndsigner#17

Closed

Consider changing integration tests to use the GRPC interface instead of lncli NYDIG/lndsigner#19

Closed

This was referenced May 11, 2023

Audit private key material zeroing to ensure no copies stay in memory NYDIG-OSS/lndsigner#11

Open

Consider changing integration tests to use the GRPC interface instead of lncli NYDIG-OSS/lndsigner#13

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration tests, unit tests, key import, github workflow - all-in-one changeset #11

Integration tests, unit tests, key import, github workflow - all-in-one changeset #11

aakselrod commented Dec 23, 2022 •

edited

Loading

joostjager left a comment

aakselrod commented Jan 20, 2023

aakselrod commented Jan 24, 2023

aakselrod commented Jan 28, 2023 •

edited

Loading

aakselrod commented Feb 1, 2023 •

edited

Loading

joostjager left a comment

joostjager Feb 1, 2023

aakselrod Feb 1, 2023

aakselrod Feb 3, 2023 •

edited

Loading

aakselrod Feb 4, 2023

aakselrod Feb 4, 2023

aakselrod Feb 4, 2023

joostjager Feb 7, 2023

joostjager commented Feb 6, 2023

aakselrod commented Feb 6, 2023

jwelsh-nydig Feb 6, 2023

aakselrod Feb 6, 2023 •

edited

Loading

aakselrod Feb 6, 2023

jwelsh-nydig Feb 7, 2023

joostjager left a comment

joostjager Feb 7, 2023

aakselrod Feb 7, 2023

joostjager Feb 7, 2023

joostjager Feb 7, 2023

aakselrod Feb 7, 2023

aakselrod commented Feb 7, 2023

joostjager commented Feb 8, 2023

Integration tests, unit tests, key import, github workflow - all-in-one changeset #11

Integration tests, unit tests, key import, github workflow - all-in-one changeset #11

Conversation

aakselrod commented Dec 23, 2022 • edited Loading

Vault package cleanup

Backend tests

Key import

Keyring tests

Integration tests

joostjager left a comment

Choose a reason for hiding this comment

aakselrod commented Jan 20, 2023

aakselrod commented Jan 24, 2023

aakselrod commented Jan 28, 2023 • edited Loading

aakselrod commented Feb 1, 2023 • edited Loading

joostjager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aakselrod Feb 3, 2023 • 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

joostjager commented Feb 6, 2023

aakselrod commented Feb 6, 2023

Choose a reason for hiding this comment

aakselrod Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager 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

aakselrod commented Feb 7, 2023

joostjager commented Feb 8, 2023

aakselrod commented Dec 23, 2022 •

edited

Loading

aakselrod commented Jan 28, 2023 •

edited

Loading

aakselrod commented Feb 1, 2023 •

edited

Loading

aakselrod Feb 3, 2023 •

edited

Loading

aakselrod Feb 6, 2023 •

edited

Loading