Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] CI for Ensuring Deterministic Transaction Generation and Stability #590

Open
OliverOffing opened this issue Aug 7, 2024 · 6 comments

Comments

@OliverOffing
Copy link

Following the recent chat on X.com about 'Dark Skippy' - the sneaky exfiltration of master secret seeds by dodgy signing devices - I'm wondering if there's any interest in a Continuous Integration (CI) job. This job would use seedsigner's code to sign a transaction, and check that the signature doesn't change when a Pull Request is merged.

The idea here is to help prevent attacks where malware might be snuck into the codebase through a series of seemingly harmless pull requests. These could look like small refactors or new features, but once they're all merged, they might do something we don't want.

I'd be more than willing to assist in the implementation of this feature, but I wanted to gauge interest and ensure I'm not overlooking any potential concerns or complications.

What do you think?

@kdmukai
Copy link
Contributor

kdmukai commented Aug 8, 2024

Thanks to @dbast we already have CI running our test suite on each PR update. And we have this test:

https://github.com/SeedSigner/seedsigner/blob/dev/tests/test_decodepsbtqr.py#L216

That test wasn't really meant to be a rigorous signature checker, but since the expected result is hard-coded, it's effectively serving the purpose you describe above.

However, it would be better to write an explicit standalone test (in tests/test_seed.py?). Even better if the test data uses a test vector from, say, Bitcoin Core's test suite.

@jdlcdl
Copy link

jdlcdl commented Aug 8, 2024

Could even call it something like "test_deterministic_signatures_never_change.py" so that we'd all be up-in-arms on any pr that deletes lines from the test.

@kdmukai
Copy link
Contributor

kdmukai commented Aug 9, 2024

Also note that such a test also belongs in embit which relays the actual signing/nonce generation to secp256k1. I took a quick look through the embit test suite and didn't find a relevant test, though I easily could have missed it.

I think it makes sense to have redundant tests in BOTH SeedSigner and embit.

So if no such test exists in embit, whoever takes this on for SeedSigner should also PR a test into embit.

@dbast
Copy link
Contributor

dbast commented Aug 9, 2024

this also makes the CI results on Github more trustable #568 as plain action versions are git tags and thus are mutable = can influence the workflow outcome if manipulated....

(another step on top would be installing dependencies during CI via poetry lock files instead of requirement*.txt files, to validate the sha256 of all installed dependencies during CI runs)

@kdmukai
Copy link
Contributor

kdmukai commented Aug 13, 2024

(another step on top would be installing dependencies during CI via poetry lock files instead of requirement*.txt files, to validate the sha256 of all installed dependencies during CI runs)

I don't have any experience with Poetry but am interested to learn more.

Its (optional) compatibility with our existing requirements.txt seems, at a minimum, an easy, obvious win. Dunno how much it makes sense to keep a toe in both worlds or to just rip the bandaid off and do a full switch. But @dbast let's pull this out into its own convo / PR.

https://python-poetry.org/docs/cli/#export

@OliverOffing
Copy link
Author

Alternatively, there's uv. I've been experimenting with it and having good results. It generates a requirements.txt file with fixed versions for all dependencies, which personally seems to be the solution with the most compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants